From ee2249b3d58267b7d34009b91f9c536b969bc4fd Mon Sep 17 00:00:00 2001
From: Dawei Ma <madawei2699@gmail.com>
Date: Fri, 31 Dec 2021 01:51:00 +0800
Subject: [PATCH] fix: ARG/ENV used in script does not invalidate build cache
 (#1688) (#1693)

---
 pkg/executor/build.go      |  8 +++++
 pkg/executor/build_test.go | 60 +++++++++++++++++++++++++++++---------
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/pkg/executor/build.go b/pkg/executor/build.go
index 1f2b3aee1c..053521d80d 100644
--- a/pkg/executor/build.go
+++ b/pkg/executor/build.go
@@ -185,6 +185,14 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
 	if err != nil {
 		return compositeKey, err
 	}
+	// Use the special argument "|#" at the start of the args array. This will
+	// 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)))
+		compositeKey.AddKey(replacementEnvs...)
+	}
 	// Add the next command to the cache key.
 	compositeKey.AddKey(resolvedCmd)
 	if copyCmd, ok := commands.CastAbstractCopyCommand(command); ok == true {
diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go
index 4f59f1b425..48e25bc063 100644
--- a/pkg/executor/build_test.go
+++ b/pkg/executor/build_test.go
@@ -597,35 +597,35 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 		shdEqual    bool
 	}{
 		{
-			description: "cache key for same command, different buildargs, args not used in command",
+			description: "cache key for same command with same build args",
 			cmd1: newStageContext(
-				"RUN echo const > test",
+				"RUN echo $ARG > test",
 				map[string]string{"ARG": "foo"},
-				[]string{"ENV=foo1"},
+				[]string{},
 			),
 			cmd2: newStageContext(
-				"RUN echo const > test",
-				map[string]string{"ARG": "bar"},
-				[]string{"ENV=bar1"},
+				"RUN echo $ARG > test",
+				map[string]string{"ARG": "foo"},
+				[]string{},
 			),
 			shdEqual: true,
 		},
 		{
-			description: "cache key for same command with same build args",
+			description: "cache key for same command with same env and args",
 			cmd1: newStageContext(
-				"RUN echo $ARG > test",
+				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
-				[]string{},
+				[]string{"ENV=same"},
 			),
 			cmd2: newStageContext(
-				"RUN echo $ARG > test",
+				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
-				[]string{},
+				[]string{"ENV=same"},
 			),
 			shdEqual: true,
 		},
 		{
-			description: "cache key for same command with same env",
+			description: "cache key for same command with same env but different args",
 			cmd1: newStageContext(
 				"RUN echo $ENV > test",
 				map[string]string{"ARG": "foo"},
@@ -636,7 +636,35 @@ func Test_stageBuilder_populateCompositeKey(t *testing.T) {
 				map[string]string{"ARG": "bar"},
 				[]string{"ENV=same"},
 			),
-			shdEqual: true,
+		},
+		{
+			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"},
+			),
+		},
+		{
+			description: "cache key for same command, different buildargs, args used in script",
+			// test.sh
+			// #!/bin/sh
+			// echo ${ARG}
+			cmd1: newStageContext(
+				"RUN ./test.sh",
+				map[string]string{"ARG": "foo"},
+				[]string{"ENV=foo1"},
+			),
+			cmd2: newStageContext(
+				"RUN ./test.sh",
+				map[string]string{"ARG": "bar"},
+				[]string{"ENV=bar1"},
+			),
 		},
 		{
 			description: "cache key for same command with a build arg values",
@@ -1116,6 +1144,8 @@ RUN foobar
 		func() testcase {
 			dir, _ := tempDirAndFile(t)
 			ch := NewCompositeCache("")
+			ch.AddKey("|1")
+			ch.AddKey("test=value")
 			ch.AddKey("RUN foobar")
 			hash, err := ch.Hash()
 			if err != nil {
@@ -1151,6 +1181,8 @@ RUN foobar
 			dir, _ := tempDirAndFile(t)
 
 			ch := NewCompositeCache("")
+			ch.AddKey("|1")
+			ch.AddKey("arg=value")
 			ch.AddKey("RUN value")
 			hash, err := ch.Hash()
 			if err != nil {
@@ -1193,6 +1225,8 @@ RUN foobar
 			}
 
 			ch2 := NewCompositeCache("")
+			ch2.AddKey("|1")
+			ch2.AddKey("arg=anotherValue")
 			ch2.AddKey("RUN anotherValue")
 			hash2, err := ch2.Hash()
 			if err != nil {