Fix #899 cached copy results in inconsistent key

pkg/commands/copy.go

@@ -121,24 +121,7 @@ func (c *CopyCommand) String() string {
 }
 
 func (c *CopyCommand) FilesUsedFromContext(config *v1.Config, buildArgs *dockerfile.BuildArgs) ([]string, error) {
-	// We don't use the context if we're performing a copy --from.
-	if c.cmd.From != "" {
-		return nil, nil
-	}
-
-	replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
-	srcs, _, err := util.ResolveEnvAndWildcards(c.cmd.SourcesAndDest, c.buildcontext, replacementEnvs)
-	if err != nil {
-		return nil, err
-	}
-
-	files := []string{}
-	for _, src := range srcs {
-		fullPath := filepath.Join(c.buildcontext, src)
-		files = append(files, fullPath)
-	}
-	logrus.Infof("Using files from context: %v", files)
-	return files, nil
+	return copyCmdFilesUsedFromContext(config, buildArgs, c.cmd, c.buildcontext)
 }
 
 func (c *CopyCommand) MetadataOnly() bool {
@@ -157,9 +140,10 @@ func (c *CopyCommand) ShouldCacheOutput() bool {
 func (c *CopyCommand) CacheCommand(img v1.Image) DockerCommand {
 
 	return &CachingCopyCommand{
-		img:       img,
-		cmd:       c.cmd,
-		extractFn: util.ExtractFile,
+		img:          img,
+		cmd:          c.cmd,
+		buildcontext: c.buildcontext,
+		extractFn:    util.ExtractFile,
 	}
 }
 
@@ -172,6 +156,7 @@ type CachingCopyCommand struct {
 	img            v1.Image
 	extractedFiles []string
 	cmd            *instructions.CopyCommand
+	buildcontext   string
 	extractFn      util.ExtractFunction
 }
 
@@ -192,6 +177,10 @@ func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *docke
 	return nil
 }
 
+func (cr *CachingCopyCommand) FilesUsedFromContext(config *v1.Config, buildArgs *dockerfile.BuildArgs) ([]string, error) {
+	return copyCmdFilesUsedFromContext(config, buildArgs, cr.cmd, cr.buildcontext)
+}
+
 func (cr *CachingCopyCommand) FilesToSnapshot() []string {
 	return cr.extractedFiles
 }
@@ -224,3 +213,32 @@ func resolveIfSymlink(destPath string) (string, error) {
 	}
 	return destPath, nil
 }
+
+func copyCmdFilesUsedFromContext(
+	config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd *instructions.CopyCommand,
+	buildcontext string,
+) ([]string, error) {
+	// We don't use the context if we're performing a copy --from.
+	if cmd.From != "" {
+		return nil, nil
+	}
+
+	replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
+
+	srcs, _, err := util.ResolveEnvAndWildcards(
+		cmd.SourcesAndDest, buildcontext, replacementEnvs,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	files := []string{}
+	for _, src := range srcs {
+		fullPath := filepath.Join(buildcontext, src)
+		files = append(files, fullPath)
+	}
+
+	logrus.Debugf("Using files from context: %v", files)
+
+	return files, nil
+}

pkg/commands/copy_test.go

@@ -218,6 +218,8 @@ func Test_resolveIfSymlink(t *testing.T) {
 }
 
 func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
+	tempDir := setupTestTemp()
+
 	tarContent, err := prepareTarFixture([]string{"foo.txt"})
 	if err != nil {
 		t.Errorf("couldn't prepare tar fixture %v", err)
@@ -238,12 +240,19 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
 	}
 	testCases := []testCase{
 		func() testCase {
+			err = ioutil.WriteFile(filepath.Join(tempDir, "foo.txt"), []byte("meow"), 0644)
+			if err != nil {
+				t.Errorf("couldn't write tempfile %v", err)
+				t.FailNow()
+			}
+
 			c := &CachingCopyCommand{
 				img: fakeImage{
 					ImageLayers: []v1.Layer{
 						fakeLayer{TarContent: tarContent},
 					},
 				},
+				buildcontext: tempDir,
 				cmd: &instructions.CopyCommand{
 					SourcesAndDest: []string{
 						"foo.txt", "foo.txt",
@@ -339,17 +348,16 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
 						t.Errorf("Expected extracted files to include %v but did not %v", file, cFiles)
 					}
 				}
-				// CachingCopyCommand does not override BaseCommand
-				// FilesUseFromContext so this will always return an empty slice and no error
-				// This seems like it might be a bug as it results in CopyCommands and CachingCopyCommands generating different cache keys - cvgw - 2019-11-27
+
 				cmdFiles, err := c.FilesUsedFromContext(
 					config, buildArgs,
 				)
 				if err != nil {
-					t.Errorf("failed to get files used from context from command")
+					t.Errorf("failed to get files used from context from command %v", err)
 				}
-				if len(cmdFiles) != 0 {
-					t.Errorf("expected files used from context to be empty but was not")
+
+				if len(cmdFiles) != len(tc.contextFiles) {
+					t.Errorf("expected files used from context to equal %v but was %v", tc.contextFiles, cmdFiles)
 				}
 			}
 

pkg/executor/build_test.go

@@ -36,7 +36,6 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/moby/buildkit/frontend/dockerfile/instructions"
-	"github.com/sirupsen/logrus"
 )
 
 func Test_reviewConfig(t *testing.T) {
@@ -620,7 +619,7 @@ func Test_stageBuilder_build(t *testing.T) {
 
 			ch := NewCompositeCache("", "")
 			ch.AddPath(filepath)
-			logrus.SetLevel(logrus.DebugLevel)
+
 			hash, err := ch.Hash()
 			if err != nil {
 				t.Errorf("couldn't create hash %v", err)
@@ -664,7 +663,7 @@ func Test_stageBuilder_build(t *testing.T) {
 			filePath := filepath.Join(dir, filename)
 			ch := NewCompositeCache("", "")
 			ch.AddPath(filePath)
-			logrus.SetLevel(logrus.DebugLevel)
+
 			hash, err := ch.Hash()
 			if err != nil {
 				t.Errorf("couldn't create hash %v", err)
@@ -698,34 +697,103 @@ func Test_stageBuilder_build(t *testing.T) {
 			dir, filenames := tempDirAndFile(t)
 			filename := filenames[0]
 			tarContent := generateTar(t, filename)
+
 			destDir, err := ioutil.TempDir("", "baz")
 			if err != nil {
 				t.Errorf("could not create temp dir %v", err)
 			}
+
 			filePath := filepath.Join(dir, filename)
-			ch := NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename))
-			ch.AddPath(filePath)
-			logrus.SetLevel(logrus.DebugLevel)
-			logrus.Infof("test composite key %v", ch)
+
+			ch := NewCompositeCache("", fmt.Sprintf("RUN foobar"))
+
 			hash1, err := ch.Hash()
 			if err != nil {
 				t.Errorf("couldn't create hash %v", err)
 			}
+
 			ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename))
 			ch.AddPath(filePath)
-			logrus.Infof("test composite key %v", ch)
+
 			hash2, err := ch.Hash()
 			if err != nil {
 				t.Errorf("couldn't create hash %v", err)
 			}
 			ch = NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename))
 			ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename))
 			ch.AddPath(filePath)
-			logrus.Infof("test composite key %v", ch)
-			hash3, err := ch.Hash()
+
+			image := fakeImage{
+				ImageLayers: []v1.Layer{
+					fakeLayer{
+						TarContent: tarContent,
+					},
+				},
+			}
+
+			dockerFile := fmt.Sprintf(`
+FROM ubuntu:16.04
+RUN foobar
+COPY %s bar.txt
+`, filename)
+			f, _ := ioutil.TempFile("", "")
+			ioutil.WriteFile(f.Name(), []byte(dockerFile), 0755)
+			opts := &config.KanikoOptions{
+				DockerfilePath: f.Name(),
+			}
+
+			stages, err := dockerfile.Stages(opts)
+			if err != nil {
+				t.Errorf("could not parse test dockerfile")
+			}
+
+			stage := stages[0]
+
+			cmds := stage.Commands
+			return testcase{
+				description: "cached run command followed by uncached copy command result in consistent read and write hashes",
+				opts:        &config.KanikoOptions{Cache: true},
+				rootDir:     dir,
+				config:      &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}},
+				layerCache: &fakeLayerCache{
+					keySequence: []string{hash1},
+					img:         image,
+				},
+				image: image,
+				// hash1 is the read cachekey for the first layer
+				// hash2 is the read cachekey for the second layer
+				expectedCacheKeys: []string{hash1, hash2},
+				pushedCacheKeys:   []string{hash2},
+				commands:          getCommands(dir, cmds),
+			}
+		}(),
+		func() testcase {
+			dir, filenames := tempDirAndFile(t)
+			filename := filenames[0]
+			tarContent := generateTar(t, filename)
+			destDir, err := ioutil.TempDir("", "baz")
+			if err != nil {
+				t.Errorf("could not create temp dir %v", err)
+			}
+			filePath := filepath.Join(dir, filename)
+			ch := NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename))
+			ch.AddPath(filePath)
+
+			hash1, err := ch.Hash()
+			if err != nil {
+				t.Errorf("couldn't create hash %v", err)
+			}
+			ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename))
+			ch.AddPath(filePath)
+
+			hash2, err := ch.Hash()
 			if err != nil {
 				t.Errorf("couldn't create hash %v", err)
 			}
+			ch = NewCompositeCache("", fmt.Sprintf("COPY %s foo.txt", filename))
+			ch.AddKey(fmt.Sprintf("COPY %s bar.txt", filename))
+			ch.AddPath(filePath)
+
 			image := fakeImage{
 				ImageLayers: []v1.Layer{
 					fakeLayer{
@@ -749,10 +817,12 @@ COPY %s bar.txt
 			if err != nil {
 				t.Errorf("could not parse test dockerfile")
 			}
+
 			stage := stages[0]
+
 			cmds := stage.Commands
 			return testcase{
-				description: "cached copy command followed by uncached copy command result in different read and write hashes",
+				description: "cached copy command followed by uncached copy command result in consistent read and write hashes",
 				opts:        &config.KanikoOptions{Cache: true},
 				rootDir:     dir,
 				config:      &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}},
@@ -764,10 +834,8 @@ COPY %s bar.txt
 				// hash1 is the read cachekey for the first layer
 				// hash2 is the read cachekey for the second layer
 				expectedCacheKeys: []string{hash1, hash2},
-				// Due to CachingCopyCommand and CopyCommand returning different values the write cache key for the second copy command will never match the read cache key
-				// hash3 is the cachekey used to write to the cache for layer 2
-				pushedCacheKeys: []string{hash3},
-				commands:        getCommands(dir, cmds),
+				pushedCacheKeys:   []string{hash2},
+				commands:          getCommands(dir, cmds),
 			}
 		}(),
 	}
@@ -848,6 +916,11 @@ func assertCacheKeys(t *testing.T, expectedCacheKeys, actualCacheKeys []string,
 	sort.Slice(actualCacheKeys, func(x, y int) bool {
 		return actualCacheKeys[x] > actualCacheKeys[y]
 	})
+
+	if len(expectedCacheKeys) != len(actualCacheKeys) {
+		t.Errorf("expected %v to equal %v", actualCacheKeys, expectedCacheKeys)
+	}
+
 	for i, key := range expectedCacheKeys {
 		if key != actualCacheKeys[i] {
 			t.Errorf("expected to %v keys %d to be %v but was %v %v", description, i, key, actualCacheKeys[i], actualCacheKeys)