remove build args from composite key and replace all build args

pkg/executor/build.go

@@ -72,7 +72,7 @@ type stageBuilder struct {
 	stageIdxToDigest map[string]string
 	snapshotter      snapShotter
 	layerCache       cache.LayerCache
-	pushCache        cachePusher
+	pushLayerToCache cachePusher
 }
 
 // newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage
@@ -115,7 +115,7 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, cross
 		layerCache: &cache.RegistryCache{
 			Opts: opts,
 		},
-		pushCache: pushLayerToCache,
+		pushLayerToCache: pushLayerToCache,
 	}
 
 	for _, cmd := range s.stage.Commands {
@@ -146,9 +146,15 @@ func initializeConfig(img partial.WithConfigFile) (*v1.ConfigFile, error) {
 	return imageConfig, nil
 }
 
-func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string, compositeKey CompositeCache) (CompositeCache, error) {
+func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, 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)
+	resolvedCmd, err := util.ResolveEnvironmentReplacement(command.String(), replacementEnvs, false)
+	if err != nil {
+		return compositeKey, err
+	}
 	// Add the next command to the cache key.
-	compositeKey.AddKey(command.String())
+	compositeKey.AddKey(resolvedCmd)
 	switch v := command.(type) {
 	case *commands.CopyCommand:
 		compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey)
@@ -201,7 +207,7 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro
 			return errors.Wrap(err, "failed to get files used from context")
 		}
 
-		compositeKey, err = s.populateCompositeKey(command, files, compositeKey)
+		compositeKey, err = s.populateCompositeKey(command, files, compositeKey, s.args, cfg.Env)
 		if err != nil {
 			return err
 		}
@@ -251,8 +257,6 @@ func (s *stageBuilder) build() error {
 		compositeKey = NewCompositeCache(s.baseImageDigest)
 	}
 
-	compositeKey.AddKey(s.opts.BuildArgs...)
-
 	// Apply optimizations to the instructions.
 	if err := s.optimize(*compositeKey, s.cf.Config); err != nil {
 		return errors.Wrap(err, "failed to optimize instructions")
@@ -309,7 +313,7 @@ func (s *stageBuilder) build() error {
 			return errors.Wrap(err, "failed to get files used from context")
 		}
 
-		*compositeKey, err = s.populateCompositeKey(command, files, *compositeKey)
+		*compositeKey, err = s.populateCompositeKey(command, files, *compositeKey, s.args, s.cf.Config.Env)
 		if err != nil {
 			return err
 		}
@@ -358,7 +362,7 @@ func (s *stageBuilder) build() error {
 			// Push layer to cache (in parallel) now along with new config file
 			if s.opts.Cache && command.ShouldCacheOutput() {
 				cacheGroup.Go(func() error {
-					return s.pushCache(s.opts, ck, tarPath, command.String())
+					return s.pushLayerToCache(s.opts, ck, tarPath, command.String())
 				})
 			}
 			if err := s.saveSnapshotToImage(command.String(), tarPath); err != nil {

pkg/executor/build_test.go

@@ -497,7 +497,8 @@ func Test_stageBuilder_optimize(t *testing.T) {
 			cf := &v1.ConfigFile{}
 			snap := fakeSnapShotter{}
 			lc := &fakeLayerCache{retrieve: tc.retrieve}
-			sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc}
+			sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc,
+				args: dockerfile.NewBuildArgs([]string{})}
 			ck := CompositeCache{}
 			file, err := ioutil.TempFile("", "foo")
 			if err != nil {
@@ -517,6 +518,130 @@ func Test_stageBuilder_optimize(t *testing.T) {
 	}
 }
 
+type stageContext struct {
+	command fmt.Stringer
+	args    *dockerfile.BuildArgs
+	env     []string
+}
+
+func newStageContext(command string, args map[string]string, env []string) stageContext {
+	dockerArgs := dockerfile.NewBuildArgs([]string{})
+	for k, v := range args {
+		dockerArgs.AddArg(k, &v)
+	}
+	return stageContext{MockDockerCommand{command: command}, dockerArgs, env}
+}
+
+func Test_stageBuilder_populateCompositeKey(t *testing.T) {
+	testCases := []struct {
+		description string
+		cmd1        stageContext
+		cmd2        stageContext
+		shdEqual    bool
+	}{
+		{
+			description: "cache key for same command, different buildargs, args not used in command",
+			cmd1: newStageContext(
+				"RUN echo const > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=foo1"},
+			),
+			cmd2: newStageContext(
+				"RUN echo const > test",
+				map[string]string{"ARG": "bar"},
+				[]string{"ENV=bar1"},
+			),
+			shdEqual: true,
+		},
+		{
+			description: "cache key for same command with same build args",
+			cmd1: newStageContext(
+				"RUN echo $ARG > test",
+				map[string]string{"ARG": "foo"},
+				[]string{},
+			),
+			cmd2: newStageContext(
+				"RUN echo $ARG > test",
+				map[string]string{"ARG": "foo"},
+				[]string{},
+			),
+			shdEqual: true,
+		},
+		{
+			description: "cache key for same command with same env",
+			cmd1: newStageContext(
+				"RUN echo $ENV > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=same"},
+			),
+			cmd2: newStageContext(
+				"RUN echo $ENV > test",
+				map[string]string{"ARG": "bar"},
+				[]string{"ENV=same"},
+			),
+			shdEqual: true,
+		},
+		{
+			description: "cache key for same command with a build arg values",
+			cmd1: newStageContext(
+				"RUN echo $ARG > test",
+				map[string]string{"ARG": "foo"},
+				[]string{},
+			),
+			cmd2: newStageContext(
+				"RUN echo $ARG > test",
+				map[string]string{"ARG": "bar"},
+				[]string{},
+			),
+		},
+		{
+			description: "cache key for same command with different env values",
+			cmd1: newStageContext(
+				"RUN echo $ENV > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+			cmd2: newStageContext(
+				"RUN echo $ENV > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=2"},
+			),
+		},
+		{
+			description: "cache key for different command same context",
+			cmd1: newStageContext(
+				"RUN echo other > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+			cmd2: newStageContext(
+				"RUN echo another > test",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=1"},
+			),
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			sb := &stageBuilder{opts: &config.KanikoOptions{SrcContext: "workspace"}}
+			ck := CompositeCache{}
+
+			ck1, err := sb.populateCompositeKey(tc.cmd1.command, []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)
+			if err != nil {
+				t.Errorf("Expected error to be nil but was %v", err)
+			}
+			key1, key2 := hashCompositeKeys(t, ck1, ck2)
+			if b := key1 == key2; b != tc.shdEqual {
+				t.Errorf("expected keys to be equal as %t but found %t", tc.shdEqual, !tc.shdEqual)
+			}
+		})
+	}
+}
+
 func Test_stageBuilder_build(t *testing.T) {
 	type testcase struct {
 		description       string
@@ -544,6 +669,7 @@ func Test_stageBuilder_build(t *testing.T) {
 				t.Errorf("couldn't create hash %v", err)
 			}
 			command := MockDockerCommand{
+				command:      "meow",
 				contextFiles: []string{filePath},
 				cacheCommand: MockCachedDockerCommand{
 					contextFiles: []string{filePath},
@@ -576,6 +702,7 @@ func Test_stageBuilder_build(t *testing.T) {
 				t.Errorf("couldn't create hash %v", err)
 			}
 			command := MockDockerCommand{
+				command:      "meow",
 				contextFiles: []string{filePath},
 				cacheCommand: MockCachedDockerCommand{
 					contextFiles: []string{filePath},
@@ -881,7 +1008,7 @@ COPY %s bar.txt
 				cf:          cf,
 				snapshotter: snap,
 				layerCache:  lc,
-				pushCache: func(_ *config.KanikoOptions, cacheKey, _, _ string) error {
+				pushLayerToCache: func(_ *config.KanikoOptions, cacheKey, _, _ string) error {
 					keys = append(keys, cacheKey)
 					return nil
 				},
@@ -993,3 +1120,15 @@ func generateTar(t *testing.T, dir string, fileNames ...string) []byte {
 	}
 	return buf.Bytes()
 }
+
+func hashCompositeKeys(t *testing.T, ck1 CompositeCache, ck2 CompositeCache) (string, string) {
+	key1, err := ck1.Hash()
+	if err != nil {
+		t.Errorf("could not hash composite key due to %s", err)
+	}
+	key2, err := ck2.Hash()
+	if err != nil {
+		t.Errorf("could not hash composite key due to %s", err)
+	}
+	return key1, key2
+}

pkg/executor/fakes.go

@@ -43,13 +43,14 @@ func (f fakeSnapShotter) TakeSnapshot(_ []string) (string, error) {
 }
 
 type MockDockerCommand struct {
+	command      string
 	contextFiles []string
 	cacheCommand commands.DockerCommand
 }
 
 func (m MockDockerCommand) ExecuteCommand(c *v1.Config, args *dockerfile.BuildArgs) error { return nil }
 func (m MockDockerCommand) String() string {
-	return "meow"
+	return m.command
 }
 func (m MockDockerCommand) FilesToSnapshot() []string {
 	return []string{"meow-snapshot-no-cache"}