Fix multistage caching with COPY --from (#2559)

* Removed block on use --cache-copy-layers with multistage builds
* Removed using digest in composite key with command COPY --from
* COPY --from command uses src as file context (only changed files will be reason for change hash)
* ARG and ENV changed before COPY dont change composite key
* Add and fix some tests
* Caching work same as caching in docker buildx

Co-authored-by: Sergei Kraev <skraev@tradingview.com>

pkg/commands/base_command.go

@@ -24,6 +24,10 @@ import (
 type BaseCommand struct {
 }
 
+func (b *BaseCommand) IsArgsEnvsRequiredInCache() bool {
+	return false
+}
+
 func (b *BaseCommand) CacheCommand(v1.Image) DockerCommand {
 	return nil
 }

pkg/commands/commands.go

@@ -58,6 +58,10 @@ type DockerCommand interface {
 
 	// ShouldDetectDeletedFiles returns true if the command could delete files.
 	ShouldDetectDeletedFiles() bool
+
+	// True if need add ARGs and EVNs to composite cache string with resolved command
+	// need only for RUN instruction
+	IsArgsEnvsRequiredInCache() bool
 }
 
 func GetCommand(cmd instructions.Command, fileContext util.FileContext, useNewRun bool, cacheCopy bool, cacheRun bool) (DockerCommand, error) {

pkg/commands/copy.go

@@ -270,9 +270,8 @@ func copyCmdFilesUsedFromContext(
 	config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd *instructions.CopyCommand,
 	fileContext util.FileContext,
 ) ([]string, error) {
-	// We don't use the context if we're performing a copy --from.
 	if cmd.From != "" {
-		return nil, nil
+		fileContext = util.FileContext{Root: filepath.Join(kConfig.KanikoDir, cmd.From)}
 	}
 
 	replacementEnvs := buildArgs.ReplacementEnvs(config.Env)

pkg/commands/run.go

@@ -44,6 +44,10 @@ var (
 	userLookup = util.LookupUser
 )
 
+func (r *RunCommand) IsArgsEnvsRequiredInCache() bool {
+	return true
+}
+
 func (r *RunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
 	return runCommandInExec(config, buildArgs, r.cmd)
 }
@@ -200,6 +204,10 @@ type CachingRunCommand struct {
 	extractFn      util.ExtractFunction
 }
 
+func (cr *CachingRunCommand) IsArgsEnvsRequiredInCache() bool {
+	return true
+}
+
 func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
 	logrus.Infof("Found cached layer, extracting to filesystem")
 	var err error

pkg/commands/run_marker.go

@@ -59,6 +59,10 @@ func (r *RunMarkerCommand) ProvidesFilesToSnapshot() bool {
 	return true
 }
 
+func (r *RunMarkerCommand) IsArgsEnvsRequiredInCache() bool {
+	return true
+}
+
 // CacheCommand returns true since this command should be cached
 func (r *RunMarkerCommand) CacheCommand(img v1.Image) DockerCommand {
 

pkg/dockerfile/dockerfile.go

@@ -58,10 +58,6 @@ func ParseStages(opts *config.KanikoOptions) ([]instructions.Stage, []instructio
 		return nil, nil, errors.Wrap(err, "parsing dockerfile")
 	}
 
-	if opts.CacheCopyLayers && len(stages) >= 2 {
-		return nil, nil, errors.New("kaniko does not support caching copy layers in multistage builds")
-	}
-
 	metaArgs, err = expandNested(metaArgs, opts.BuildArgs)
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "expanding meta ARGs")

pkg/dockerfile/dockerfile_test.go

@@ -29,39 +29,6 @@ import (
 	"github.com/moby/buildkit/frontend/dockerfile/instructions"
 )
 
-func Test_ParseStages_NoMultistageWithCacheCopy(t *testing.T) {
-	dockerfile := `
-	FROM scratch as first
-	COPY testfile /
-
-	FROM scratch as second
-	COPY --from=second testfile /
-	`
-	tmpfile, err := ioutil.TempFile("", "Dockerfile.test")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	defer os.Remove(tmpfile.Name())
-
-	if _, err := tmpfile.Write([]byte(dockerfile)); err != nil {
-		t.Fatal(err)
-	}
-	if err := tmpfile.Close(); err != nil {
-		t.Fatal(err)
-	}
-
-	opts := &config.KanikoOptions{
-		DockerfilePath:  tmpfile.Name(),
-		CacheCopyLayers: true,
-	}
-
-	_, _, err = ParseStages(opts)
-	if err == nil {
-		t.Fatal("expected ParseStages to fail on MultiStage build if CacheCopyLayers=true")
-	}
-}
-
 func Test_ParseStages_ArgValueWithQuotes(t *testing.T) {
 	dockerfile := `
 	ARG IMAGE="ubuntu:16.04"

pkg/executor/build.go

@@ -198,7 +198,7 @@ func isOCILayout(path string) bool {
 	return strings.HasPrefix(path, "oci:")
 }
 
-func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string, compositeKey CompositeCache, args *dockerfile.BuildArgs, env []string) (CompositeCache, error) {
+func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, files []string, compositeKey CompositeCache, args *dockerfile.BuildArgs, env []string) (CompositeCache, error) {
 	// First replace all the environment variables or args in the command
 	replacementEnvs := args.ReplacementEnvs(env)
 	// The sort order of `replacementEnvs` is basically undefined, sort it
@@ -212,15 +212,16 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
 	// avoid conflicts with any RUN command since commands can not
 	// start with | (vertical bar). The "#" (number of build envs) is there to
 	// help ensure proper cache matches.
-	if len(replacementEnvs) > 0 {
+
-		compositeKey.AddKey(fmt.Sprintf("|%d", len(replacementEnvs)))
+	if command.IsArgsEnvsRequiredInCache() {
-		compositeKey.AddKey(replacementEnvs...)
+		if len(replacementEnvs) > 0 {
+			compositeKey.AddKey(fmt.Sprintf("|%d", len(replacementEnvs)))
+			compositeKey.AddKey(replacementEnvs...)
+		}
 	}
+
 	// Add the next command to the cache key.
 	compositeKey.AddKey(resolvedCmd)
-	if copyCmd, ok := commands.CastAbstractCopyCommand(command); ok == true {
-		compositeKey = s.populateCopyCmdCompositeKey(command, copyCmd.From(), compositeKey)
-	}
 
 	for _, f := range files {
 		if err := compositeKey.AddPath(f, s.fileContext); err != nil {
@@ -230,22 +231,6 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
 	return compositeKey, nil
 }
 
-func (s *stageBuilder) populateCopyCmdCompositeKey(command fmt.Stringer, from string, compositeKey CompositeCache) CompositeCache {
-	if from != "" {
-		digest, ok := s.stageIdxToDigest[from]
-		if ok {
-			ds := digest
-			cacheKey, ok := s.digestToCacheKey[ds]
-			if ok {
-				logrus.Debugf("Adding digest %v from previous stage to composite key for %v", ds, command.String())
-				compositeKey.AddKey(cacheKey)
-			}
-		}
-	}
-
-	return compositeKey
-}
-
 func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) error {
 	if !s.opts.Cache {
 		return nil

pkg/executor/build_test.go

@@ -654,14 +654,15 @@ func newStageContext(command string, args map[string]string, env []string) stage
 }
 
 func Test_stageBuilder_populateCompositeKey(t *testing.T) {
-	testCases := []struct {
+	type testcase struct {
 		description string
 		cmd1        stageContext
 		cmd2        stageContext
 		shdEqual    bool
-	}{
+	}
+	testCases := []testcase{
 		{
-			description: "cache key for same command with same build args",
+			description: "cache key for same command [RUN] with same build args",
 			cmd1: newStageContext(
 				"RUN echo $ARG > test",
 				map[string]string{"ARG": "foo"},
@@ -675,7 +676,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			shdEqual: true,
 		},
 		{
-			description: "cache key for same command with same env and args",
+			description: "cache key for same command [RUN] with same env and args",
 			cmd1: newStageContext(
 				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
@@ -689,7 +690,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			shdEqual: true,
 		},
 		{
-			description: "cache key for same command with same env but different args",
+			description: "cache key for same command [RUN] with same env but different args",
 			cmd1: newStageContext(
 				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
@@ -702,7 +703,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			),
 		},
 		{
-			description: "cache key for same command, different buildargs, args not used in command",
+			description: "cache key for same command [RUN], different buildargs, args not used in command",
 			cmd1: newStageContext(
 				"RUN echo const > test",
 				map[string]string{"ARG": "foo"},
@@ -715,7 +716,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			),
 		},
 		{
-			description: "cache key for same command, different buildargs, args used in script",
+			description: "cache key for same command [RUN], different buildargs, args used in script",
 			// test.sh
 			// #!/bin/sh
 			// echo ${ARG}
@@ -731,7 +732,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			),
 		},
 		{
-			description: "cache key for same command with a build arg values",
+			description: "cache key for same command [RUN] with a build arg values",
 			cmd1: newStageContext(
 				"RUN echo $ARG > test",
 				map[string]string{"ARG": "foo"},
@@ -744,7 +745,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			),
 		},
 		{
-			description: "cache key for same command with different env values",
+			description: "cache key for same command [RUN] with different env values",
 			cmd1: newStageContext(
 				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
@@ -757,7 +758,7 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 			),
 		},
 		{
-			description: "cache key for different command same context",
+			description: "cache key for different command [RUN] same context",
 			cmd1: newStageContext(
 				"RUN echo other > test",
 				map[string]string{"ARG": "foo"},
@@ -769,17 +770,105 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 				[]string{"ENV=1"},
 			),
 		},
+		func() testcase {
+			dir, files := tempDirAndFile(t)
+			file := files[0]
+			filePath := filepath.Join(dir, file)
+			return testcase{
+				description: "cache key for same command [COPY] with same args",
+				cmd1: newStageContext(
+					fmt.Sprintf("COPY %s /meow", filePath),
+					map[string]string{"ARG": "foo"},
+					[]string{"ENV=1"},
+				),
+				cmd2: newStageContext(
+					fmt.Sprintf("COPY %s /meow", filePath),
+					map[string]string{"ARG": "foo"},
+					[]string{"ENV=1"},
+				),
+				shdEqual: true,
+			}
+		}(),
+		func() testcase {
+			dir, files := tempDirAndFile(t)
+			file := files[0]
+			filePath := filepath.Join(dir, file)
+			return testcase{
+				description: "cache key for same command [COPY] with different args",
+				cmd1: newStageContext(
+					fmt.Sprintf("COPY %s /meow", filePath),
+					map[string]string{"ARG": "foo"},
+					[]string{"ENV=1"},
+				),
+				cmd2: newStageContext(
+					fmt.Sprintf("COPY %s /meow", filePath),
+					map[string]string{"ARG": "bar"},
+					[]string{"ENV=2"},
+				),
+				shdEqual: true,
+			}
+		}(),
+		{
+			description: "cache key for same command [WORKDIR] with same args",
+			cmd1: newStageContext(
+				"WORKDIR /",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+			cmd2: newStageContext(
+				"WORKDIR /",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+			shdEqual: true,
+		},
+		{
+			description: "cache key for same command [WORKDIR] with different args",
+			cmd1: newStageContext(
+				"WORKDIR /",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+			cmd2: newStageContext(
+				"WORKDIR /",
+				map[string]string{"ARG": "bar"},
+				[]string{"ENV=2"},
+			),
+			shdEqual: true,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
 			sb := &stageBuilder{fileContext: util.FileContext{Root: "workspace"}}
 			ck := CompositeCache{}
 
-			ck1, err := sb.populateCompositeKey(tc.cmd1.command, []string{}, ck, tc.cmd1.args, tc.cmd1.env)
+			instructions1, err := dockerfile.ParseCommands([]string{tc.cmd1.command.String()})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			fc1 := util.FileContext{Root: "workspace"}
+			dockerCommand1, err := commands.GetCommand(instructions1[0], fc1, false, true, true)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			instructions, err := dockerfile.ParseCommands([]string{tc.cmd2.command.String()})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			fc2 := util.FileContext{Root: "workspace"}
+			dockerCommand2, err := commands.GetCommand(instructions[0], fc2, false, true, true)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			ck1, err := sb.populateCompositeKey(dockerCommand1, []string{}, ck, tc.cmd1.args, tc.cmd1.env)
 			if err != nil {
 				t.Errorf("Expected error to be nil but was %v", err)
 			}
-			ck2, err := sb.populateCompositeKey(tc.cmd2.command, []string{}, ck, tc.cmd2.args, tc.cmd2.env)
+			ck2, err := sb.populateCompositeKey(dockerCommand2, []string{}, ck, tc.cmd2.args, tc.cmd2.env)
 			if err != nil {
 				t.Errorf("Expected error to be nil but was %v", err)
 			}
@@ -1206,6 +1295,7 @@ RUN foobar
 				cacheCommand: MockCachedDockerCommand{
 					contextFiles: []string{},
 				},
+				argToCompositeCache: true,
 			}
 
 			return testcase{
@@ -1243,6 +1333,7 @@ RUN foobar
 				cacheCommand: MockCachedDockerCommand{
 					contextFiles: []string{},
 				},
+				argToCompositeCache: true,
 			}
 
 			return testcase{
@@ -1286,6 +1377,7 @@ RUN foobar
 				cacheCommand: MockCachedDockerCommand{
 					contextFiles: []string{},
 				},
+				argToCompositeCache: true,
 			}
 
 			return testcase{
@@ -1500,9 +1592,10 @@ func Test_stageBuild_populateCompositeKeyForCopyCommand(t *testing.T) {
 		expectedCacheKey string
 	}{
 		{
-			description:      "multi-stage copy command",
+			description: "multi-stage copy command",
+			// dont use digest from previoust stage for COPY
 			command:          "COPY --from=0 foo.txt bar.txt",
-			expectedCacheKey: "COPY --from=0 foo.txt bar.txt-some-cache-key",
+			expectedCacheKey: "COPY --from=0 foo.txt bar.txt",
 		},
 		{
 			description:      "copy command",
@@ -1524,7 +1617,7 @@ func Test_stageBuild_populateCompositeKeyForCopyCommand(t *testing.T) {
 
 			for _, useCacheCommand := range []bool{false, true} {
 				t.Run(fmt.Sprintf("CacheCommand=%t", useCacheCommand), func(t *testing.T) {
-					var cmd fmt.Stringer = copyCommand
+					var cmd commands.DockerCommand = copyCommand
 					if useCacheCommand {
 						cmd = copyCommand.(*commands.CopyCommand).CacheCommand(nil)
 					}

pkg/executor/fakes.go

@@ -43,9 +43,10 @@ func (f fakeSnapShotter) TakeSnapshot(_ []string, _, _ bool) (string, error) {
 }
 
 type MockDockerCommand struct {
-	command      string
+	command             string
-	contextFiles []string
+	contextFiles        []string
-	cacheCommand commands.DockerCommand
+	cacheCommand        commands.DockerCommand
+	argToCompositeCache bool
 }
 
 func (m MockDockerCommand) ExecuteCommand(c *v1.Config, args *dockerfile.BuildArgs) error { return nil }
@@ -76,9 +77,13 @@ func (m MockDockerCommand) ShouldCacheOutput() bool {
 func (m MockDockerCommand) ShouldDetectDeletedFiles() bool {
 	return false
 }
+func (m MockDockerCommand) IsArgsEnvsRequiredInCache() bool {
+	return m.argToCompositeCache
+}
 
 type MockCachedDockerCommand struct {
-	contextFiles []string
+	contextFiles        []string
+	argToCompositeCache bool
 }
 
 func (m MockCachedDockerCommand) ExecuteCommand(c *v1.Config, args *dockerfile.BuildArgs) error {
@@ -111,6 +116,9 @@ func (m MockCachedDockerCommand) RequiresUnpackedFS() bool {
 func (m MockCachedDockerCommand) ShouldCacheOutput() bool {
 	return false
 }
+func (m MockCachedDockerCommand) IsArgsEnvsRequiredInCache() bool {
+	return m.argToCompositeCache
+}
 
 type fakeLayerCache struct {
 	retrieve     bool