Merge pull request #971 from tejal29/915

Fix Symlinks not being copied across stages

integration/dockerfiles/Dockerfile_test_copy_symlink

@@ -0,0 +1,6 @@
+FROM busybox as t
+RUN echo "hello" > /tmp/target
+RUN ln -s /tmp/target /tmp/link
+
+FROM scratch
+COPY --from=t /tmp/link /tmp

pkg/commands/copy.go

@@ -85,7 +85,7 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 				return err
 			}
 			c.snapshotFiles = append(c.snapshotFiles, copiedFiles...)
-		} else if fi.Mode()&os.ModeSymlink != 0 {
+		} else if util.IsSymlink(fi) {
 			// If file is a symlink, we want to create the same relative symlink
 			exclude, err := util.CopySymlink(fullPath, destPath, c.buildcontext)
 			if err != nil {

pkg/executor/build.go

@@ -23,8 +23,6 @@ import (
 	"strconv"
 	"time"
 
-	otiai10Cpy "github.com/otiai10/copy"
-
 	"github.com/google/go-containerregistry/pkg/v1/partial"
 
 	"github.com/moby/buildkit/frontend/dockerfile/instructions"
@@ -574,8 +572,8 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) {
 			return nil, err
 		}
 		for _, p := range filesToSave {
-			logrus.Infof("Saving file %s for later use.", p)
-			otiai10Cpy.Copy(p, filepath.Join(dstDir, p))
+			logrus.Infof("Saving file %s for later use", p)
+			util.CopyFileOrSymlink(p, dstDir)
 		}
 
 		// Delete the filesystem
@@ -587,16 +585,23 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) {
 	return nil, err
 }
 
+// fileToSave returns all the files matching the given pattern in deps.
+// If a file is a symlink, it also returns the target file.
 func filesToSave(deps []string) ([]string, error) {
-	allFiles := []string{}
+	srcFiles := []string{}
 	for _, src := range deps {
 		srcs, err := filepath.Glob(src)
 		if err != nil {
 			return nil, err
 		}
-		allFiles = append(allFiles, srcs...)
+		for _, f := range srcs {
+			if link, err := util.EvalSymLink(f); err == nil {
+				srcFiles = append(srcFiles, link)
+			}
+			srcFiles = append(srcFiles, f)
+		}
 	}
-	return allFiles, nil
+	return srcFiles, nil
 }
 
 func fetchExtraStages(stages []config.KanikoStage, opts *config.KanikoOptions) error {

pkg/snapshot/snapshot.go

@@ -19,6 +19,7 @@ package snapshot
 import (
 	"fmt"
 	"io/ioutil"
+	"os"
 	"path/filepath"
 	"sort"
 	"syscall"
@@ -112,7 +113,6 @@ func (s *Snapshotter) TakeSnapshotFS() (string, error) {
 	if err := writeToTar(t, filesToAdd, filesToWhiteOut); err != nil {
 		return "", err
 	}
-
 	return f.Name(), nil
 }
 
@@ -179,8 +179,13 @@ func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) {
 			return nil, nil, fmt.Errorf("could not check if file has changed %s %s", path, err)
 		}
 		if fileChanged {
-			logrus.Tracef("Adding %s to layer, because it was changed.", path)
-			filesToAdd = append(filesToAdd, path)
+			// Get target file for symlinks so the symlink is not a dead link.
+			files, err := filesWithLinks(path)
+			if err != nil {
+				return nil, nil, err
+			}
+			logrus.Tracef("Adding files %s to layer, because it was changed.", files)
+			filesToAdd = append(filesToAdd, files...)
 		}
 	}
 
@@ -188,14 +193,12 @@ func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) {
 	filesToAdd = filesWithParentDirs(filesToAdd)
 
 	sort.Strings(filesToAdd)
-
 	// Add files to the layered map
 	for _, file := range filesToAdd {
 		if err := s.l.Add(file); err != nil {
 			return nil, nil, fmt.Errorf("unable to add file %s to layered map: %s", file, err)
 		}
 	}
-
 	return filesToAdd, filesToWhiteOut, nil
 }
 
@@ -236,3 +239,21 @@ func filesWithParentDirs(files []string) []string {
 
 	return newFiles
 }
+
+// filesWithLinks returns the symlink and the target path if its exists.
+func filesWithLinks(path string) ([]string, error) {
+	link, err := util.GetSymLink(path)
+	if err == util.ErrNotSymLink {
+		return []string{path}, nil
+	} else if err != nil {
+		return nil, err
+	}
+	// Add symlink if it exists in the FS
+	if !filepath.IsAbs(link) {
+		link = filepath.Join(filepath.Dir(path), link)
+	}
+	if _, err := os.Stat(link); err != nil {
+		return []string{path}, nil
+	}
+	return []string{path, link}, nil
+}

pkg/snapshot/snapshot_test.go

@@ -31,7 +31,7 @@ import (
 )
 
 func TestSnapshotFSFileChange(t *testing.T) {
-	testDir, snapshotter, cleanup, err := setUpTestDir()
+	testDir, snapshotter, cleanup, err := setUpTest()
 	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")
 	defer cleanup()
 	if err != nil {
@@ -90,7 +90,7 @@ func TestSnapshotFSFileChange(t *testing.T) {
 }
 
 func TestSnapshotFSIsReproducible(t *testing.T) {
-	testDir, snapshotter, cleanup, err := setUpTestDir()
+	testDir, snapshotter, cleanup, err := setUpTest()
 	defer cleanup()
 	if err != nil {
 		t.Fatal(err)
@@ -129,7 +129,7 @@ func TestSnapshotFSIsReproducible(t *testing.T) {
 }
 
 func TestSnapshotFSChangePermissions(t *testing.T) {
-	testDir, snapshotter, cleanup, err := setUpTestDir()
+	testDir, snapshotter, cleanup, err := setUpTest()
 	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")
 	defer cleanup()
 	if err != nil {
@@ -180,7 +180,7 @@ func TestSnapshotFSChangePermissions(t *testing.T) {
 }
 
 func TestSnapshotFiles(t *testing.T) {
-	testDir, snapshotter, cleanup, err := setUpTestDir()
+	testDir, snapshotter, cleanup, err := setUpTest()
 	testDirWithoutLeadingSlash := strings.TrimLeft(testDir, "/")
 	defer cleanup()
 	if err != nil {
@@ -230,7 +230,7 @@ func TestSnapshotFiles(t *testing.T) {
 }
 
 func TestEmptySnapshotFS(t *testing.T) {
-	_, snapshotter, cleanup, err := setUpTestDir()
+	_, snapshotter, cleanup, err := setUpTest()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -253,12 +253,97 @@ func TestEmptySnapshotFS(t *testing.T) {
 	}
 }
 
-func setUpTestDir() (string, *Snapshotter, func(), error) {
-	testDir, err := ioutil.TempDir("", "")
-	if err != nil {
-		return "", nil, nil, errors.Wrap(err, "setting up temp dir")
+func TestFileWithLinks(t *testing.T) {
+
+	link := "baz/link"
+	tcs := []struct {
+		name           string
+		path           string
+		linkFileTarget string
+		expected       []string
+		shouldErr      bool
+	}{
+		{
+			name:           "given path is a symlink that points to a valid target",
+			path:           link,
+			linkFileTarget: "file",
+			expected:       []string{link, "baz/file"},
+		},
+		{
+			name:           "given path is a symlink points to non existing path",
+			path:           link,
+			linkFileTarget: "does-not-exists",
+			expected:       []string{link},
+		},
+		{
+			name:           "given path is a regular file",
+			path:           "kaniko/file",
+			linkFileTarget: "file",
+			expected:       []string{"kaniko/file"},
+		},
 	}
 
+	for _, tt := range tcs {
+		t.Run(tt.name, func(t *testing.T) {
+			testDir, cleanup, err := setUpTestDir()
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer cleanup()
+			if err := setupSymlink(testDir, link, tt.linkFileTarget); err != nil {
+				t.Fatalf("could not set up symlink due to %s", err)
+			}
+			actual, err := filesWithLinks(filepath.Join(testDir, tt.path))
+			if err != nil {
+				t.Fatalf("unexpected error %s", err)
+			}
+			sortAndCompareFilepaths(t, testDir, tt.expected, actual)
+		})
+	}
+}
+
+func setupSymlink(dir string, link string, target string) error {
+	return os.Symlink(target, filepath.Join(dir, link))
+}
+
+func sortAndCompareFilepaths(t *testing.T, testDir string, expected []string, actual []string) {
+	expectedFullPaths := make([]string, len(expected))
+	for i, file := range expected {
+		expectedFullPaths[i] = filepath.Join(testDir, file)
+	}
+	sort.Strings(expectedFullPaths)
+	sort.Strings(actual)
+	testutil.CheckDeepEqual(t, expectedFullPaths, actual)
+}
+
+func setUpTestDir() (string, func(), error) {
+	testDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		return "", nil, errors.Wrap(err, "setting up temp dir")
+	}
+	files := map[string]string{
+		"foo":         "baz1",
+		"bar/bat":     "baz2",
+		"kaniko/file": "file",
+		"baz/file":    "testfile",
+	}
+	// Set up initial files
+	if err := testutil.SetupFiles(testDir, files); err != nil {
+		return "", nil, errors.Wrap(err, "setting up file system")
+	}
+
+	cleanup := func() {
+		os.RemoveAll(testDir)
+	}
+
+	return testDir, cleanup, nil
+}
+
+func setUpTest() (string, *Snapshotter, func(), error) {
+	testDir, dirCleanUp, err := setUpTestDir()
+	if err != nil {
+		return "", nil, nil, err
+	}
 	snapshotPath, err := ioutil.TempDir("", "")
 	if err != nil {
 		return "", nil, nil, errors.Wrap(err, "setting up temp dir")
@@ -266,16 +351,6 @@ func setUpTestDir() (string, *Snapshotter, func(), error) {
 
 	snapshotPathPrefix = snapshotPath
 
-	files := map[string]string{
-		"foo":         "baz1",
-		"bar/bat":     "baz2",
-		"kaniko/file": "file",
-	}
-	// Set up initial files
-	if err := testutil.SetupFiles(testDir, files); err != nil {
-		return "", nil, nil, errors.Wrap(err, "setting up file system")
-	}
-
 	// Take the initial snapshot
 	l := NewLayeredMap(util.Hasher(), util.CacheHasher())
 	snapshotter := NewSnapshotter(l, testDir)
@@ -285,7 +360,7 @@ func setUpTestDir() (string, *Snapshotter, func(), error) {
 
 	cleanup := func() {
 		os.RemoveAll(snapshotPath)
-		os.RemoveAll(testDir)
+		dirCleanUp()
 	}
 
 	return testDir, snapshotter, cleanup, nil

pkg/util/fs_util.go

@@ -30,6 +30,8 @@ import (
 	"syscall"
 	"time"
 
+	otiai10Cpy "github.com/otiai10/copy"
+
 	"github.com/GoogleContainerTools/kaniko/pkg/constants"
 	"github.com/docker/docker/builder/dockerignore"
 	"github.com/docker/docker/pkg/fileutils"
@@ -485,18 +487,8 @@ func FilepathExists(path string) bool {
 // CreateFile creates a file at path and copies over contents from the reader
 func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid uint32) error {
 	// Create directory path if it doesn't exist
-	baseDir := filepath.Dir(path)
-	if info, err := os.Lstat(baseDir); os.IsNotExist(err) {
-		logrus.Tracef("baseDir %s for file %s does not exist. Creating.", baseDir, path)
-		if err := os.MkdirAll(baseDir, 0755); err != nil {
-			return err
-		}
-	} else {
-		switch mode := info.Mode(); {
-		case mode&os.ModeSymlink != 0:
-			logrus.Infof("destination cannot be a symlink %v", baseDir)
-			return errors.New("destination cannot be a symlink")
-		}
+	if err := createParentDirectory(path); err != nil {
+		return err
 	}
 	dest, err := os.Create(path)
 	if err != nil {
@@ -556,6 +548,7 @@ func CopyDir(src, dest, buildcontext string) ([]string, error) {
 		fullPath := filepath.Join(src, file)
 		fi, err := os.Lstat(fullPath)
 		if err != nil {
+			fmt.Println(" i am returning from here this", err)
 			return nil, err
 		}
 		if excludeFile(fullPath, buildcontext) {
@@ -573,7 +566,7 @@ func CopyDir(src, dest, buildcontext string) ([]string, error) {
 			if err := mkdirAllWithPermissions(destPath, mode, uid, gid); err != nil {
 				return nil, err
 			}
-		} else if fi.Mode()&os.ModeSymlink != 0 {
+		} else if IsSymlink(fi) {
 			// If file is a symlink, we want to create the same relative symlink
 			if _, err := CopySymlink(fullPath, destPath, buildcontext); err != nil {
 				return nil, err
@@ -604,6 +597,9 @@ func CopySymlink(src, dest, buildcontext string) (bool, error) {
 			return false, err
 		}
 	}
+	if err := createParentDirectory(dest); err != nil {
+		return false, err
+	}
 	return false, os.Symlink(link, dest)
 }
 
@@ -732,3 +728,64 @@ func CreateTargetTarfile(tarpath string) (*os.File, error) {
 	return os.Create(tarpath)
 
 }
+
+// Returns true if a file is a symlink
+func IsSymlink(fi os.FileInfo) bool {
+	return fi.Mode()&os.ModeSymlink != 0
+}
+
+var ErrNotSymLink = fmt.Errorf("not a symlink")
+
+func GetSymLink(path string) (string, error) {
+	if err := getSymlink(path); err != nil {
+		return "", err
+	}
+	return os.Readlink(path)
+}
+
+func EvalSymLink(path string) (string, error) {
+	if err := getSymlink(path); err != nil {
+		return "", err
+	}
+	return filepath.EvalSymlinks(path)
+}
+
+func getSymlink(path string) error {
+	fi, err := os.Lstat(path)
+	if err != nil {
+		return err
+	}
+	if !IsSymlink(fi) {
+		return ErrNotSymLink
+	}
+	return nil
+}
+
+// For cross stage dependencies kaniko must persist the referenced path so that it can be used in
+// the dependent stage. For symlinks we copy the target path because copying the symlink would
+// result in a dead link
+func CopyFileOrSymlink(src string, destDir string) error {
+	destFile := filepath.Join(destDir, src)
+	if fi, _ := os.Lstat(src); IsSymlink(fi) {
+		link, err := os.Readlink(src)
+		if err != nil {
+			return err
+		}
+		return os.Symlink(link, destFile)
+	}
+	return otiai10Cpy.Copy(src, destFile)
+}
+
+func createParentDirectory(path string) error {
+	baseDir := filepath.Dir(path)
+	if info, err := os.Lstat(baseDir); os.IsNotExist(err) {
+		logrus.Tracef("baseDir %s for file %s does not exist. Creating.", baseDir, path)
+		if err := os.MkdirAll(baseDir, 0755); err != nil {
+			return err
+		}
+	} else if IsSymlink(info) {
+		logrus.Infof("destination cannot be a symlink %v", baseDir)
+		return errors.New("destination cannot be a symlink")
+	}
+	return nil
+}

pkg/util/fs_util_test.go

@@ -859,6 +859,9 @@ func TestCopySymlink(t *testing.T) {
 			tc := tc
 			t.Parallel()
 			r, err := ioutil.TempDir("", "")
+			os.MkdirAll(filepath.Join(r, filepath.Dir(tc.linkTarget)), 0777)
+			tc.linkTarget = filepath.Join(r, tc.linkTarget)
+			ioutil.WriteFile(tc.linkTarget, nil, 0644)
 			if err != nil {
 				t.Fatal(err)
 			}