Merge pull request #943 from shortstories/bugfix/copy-with-symlink

Fix #942 COPY or ADD to symlink destination breaks image

Author: cvgw

pkg/commands/copy.go

@@ -20,13 +20,13 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
+	"github.com/GoogleContainerTools/kaniko/pkg/constants"
 	"github.com/moby/buildkit/frontend/dockerfile/instructions"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
-	"github.com/GoogleContainerTools/kaniko/pkg/constants"
-
 	"github.com/GoogleContainerTools/kaniko/pkg/dockerfile"
 	"github.com/GoogleContainerTools/kaniko/pkg/util"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -59,11 +59,15 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 		if err != nil {
 			return err
 		}
+		if fi.IsDir() && !strings.HasSuffix(fullPath, string(os.PathSeparator)) {
+			fullPath += "/"
+		}
 		cwd := config.WorkingDir
 		if cwd == "" {
 			cwd = constants.RootDir
 		}
-		destPath, err := util.DestinationFilepath(src, dest, cwd)
+
+		destPath, err := util.DestinationFilepath(fullPath, dest, cwd)
 		if err != nil {
 			return err
 		}
@@ -76,11 +80,7 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 		}
 
 		if fi.IsDir() {
-			if !filepath.IsAbs(dest) {
-				// we need to add '/' to the end to indicate the destination is a directory
-				dest = filepath.Join(cwd, dest) + "/"
-			}
-			copiedFiles, err := util.CopyDir(fullPath, dest, c.buildcontext)
+			copiedFiles, err := util.CopyDir(fullPath, destPath, c.buildcontext)
 			if err != nil {
 				return err
 			}
@@ -197,21 +197,42 @@ func (cr *CachingCopyCommand) From() string {
 }
 
 func resolveIfSymlink(destPath string) (string, error) {
-	baseDir := filepath.Dir(destPath)
-	if info, err := os.Lstat(baseDir); err == nil {
-		switch mode := info.Mode(); {
-		case mode&os.ModeSymlink != 0:
-			linkPath, err := os.Readlink(baseDir)
-			if err != nil {
-				return "", errors.Wrap(err, "error reading symlink")
+	if !filepath.IsAbs(destPath) {
+		return "", errors.New("dest path must be abs")
+	}
+
+	var nonexistentPaths []string
+
+	newPath := destPath
+	for newPath != "/" {
+		_, err := os.Lstat(newPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				dir, file := filepath.Split(newPath)
+				newPath = filepath.Clean(dir)
+				nonexistentPaths = append(nonexistentPaths, file)
+				continue
+			} else {
+				return "", errors.Wrap(err, "failed to lstat")
 			}
-			absLinkPath := filepath.Join(filepath.Dir(baseDir), linkPath)
-			newPath := filepath.Join(absLinkPath, filepath.Base(destPath))
-			logrus.Tracef("Updating destination path from %v to %v due to symlink", destPath, newPath)
-			return newPath, nil
 		}
+
+		newPath, err = filepath.EvalSymlinks(newPath)
+		if err != nil {
+			return "", errors.Wrap(err, "failed to eval symlinks")
+		}
+		break
 	}
-	return destPath, nil
+
+	for i := len(nonexistentPaths) - 1; i >= 0; i-- {
+		newPath = filepath.Join(newPath, nonexistentPaths[i])
+	}
+
+	if destPath != newPath {
+		logrus.Tracef("Updating destination path from %v to %v due to symlink", destPath, newPath)
+	}
+
+	return filepath.Clean(newPath), nil
 }
 
 func copyCmdFilesUsedFromContext(

pkg/commands/copy_test.go

@@ -194,14 +194,28 @@ func Test_resolveIfSymlink(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	cases := []testCase{{destPath: thepath, expectedPath: thepath, err: nil}}
-
+	cases := []testCase{
+		{destPath: thepath, expectedPath: thepath, err: nil},
+		{destPath: "/", expectedPath: "/", err: nil},
+	}
 	baseDir = tmpDir
 	symLink := filepath.Join(baseDir, "symlink")
 	if err := os.Symlink(filepath.Base(thepath), symLink); err != nil {
 		t.Error(err)
 	}
-	cases = append(cases, testCase{filepath.Join(symLink, "foo.txt"), filepath.Join(thepath, "foo.txt"), nil})
+	cases = append(cases,
+		testCase{filepath.Join(symLink, "foo.txt"), filepath.Join(thepath, "foo.txt"), nil},
+		testCase{filepath.Join(symLink, "inner", "foo.txt"), filepath.Join(thepath, "inner", "foo.txt"), nil},
+	)
+
+	absSymlink := filepath.Join(tmpDir, "abs-symlink")
+	if err := os.Symlink(thepath, absSymlink); err != nil {
+		t.Error(err)
+	}
+	cases = append(cases,
+		testCase{filepath.Join(absSymlink, "foo.txt"), filepath.Join(thepath, "foo.txt"), nil},
+		testCase{filepath.Join(absSymlink, "inner", "foo.txt"), filepath.Join(thepath, "inner", "foo.txt"), nil},
+	)
 
 	for i, c := range cases {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {

pkg/util/command_util.go

@@ -164,20 +164,25 @@ func IsDestDir(path string) bool {
 //	If dest is a dir, copy it to /dest/relpath
 // 	If dest is a file, copy directly to dest
 // If source is a dir:
-//	Assume dest is also a dir, and copy to dest/relpath
+//	Assume dest is also a dir, and copy to dest/
 // If dest is not an absolute filepath, add /cwd to the beginning
 func DestinationFilepath(src, dest, cwd string) (string, error) {
-	if IsDestDir(dest) {
-		destPath := filepath.Join(dest, filepath.Base(src))
-		if filepath.IsAbs(dest) {
-			return destPath, nil
-		}
-		return filepath.Join(cwd, destPath), nil
+	_, srcFileName := filepath.Split(src)
+	newDest := dest
+
+	if IsDestDir(newDest) {
+		newDest = filepath.Join(newDest, srcFileName)
 	}
-	if filepath.IsAbs(dest) {
-		return dest, nil
+
+	if !filepath.IsAbs(newDest) {
+		newDest = filepath.Join(cwd, newDest)
 	}
-	return filepath.Join(cwd, dest), nil
+
+	if len(srcFileName) <= 0 && !strings.HasSuffix(newDest, "/") {
+		newDest += "/"
+	}
+
+	return newDest, nil
 }
 
 // URLDestinationFilepath gives the destination a file from a remote URL should be saved to

pkg/util/command_util_test.go

@@ -153,13 +153,13 @@ var destinationFilepathTests = []struct {
 		src:              "context/bar/",
 		cwd:              "/",
 		dest:             "pkg/",
-		expectedFilepath: "/pkg/bar",
+		expectedFilepath: "/pkg/",
 	},
 	{
 		src:              "context/bar/",
 		cwd:              "/newdir",
 		dest:             "pkg/",
-		expectedFilepath: "/newdir/pkg/bar",
+		expectedFilepath: "/newdir/pkg/",
 	},
 	{
 		src:              "./context/empty",
@@ -177,7 +177,7 @@ var destinationFilepathTests = []struct {
 		src:              "./",
 		cwd:              "/",
 		dest:             "/dir",
-		expectedFilepath: "/dir",
+		expectedFilepath: "/dir/",
 	},
 	{
 		src:              "context/foo",