From 062445509bbc0fe4ede2d4c5d0b918e85616abf2 Mon Sep 17 00:00:00 2001
From: Aditya Rajan <arajan@redhat.com>
Date: Mon, 1 Nov 2021 16:17:36 +0530
Subject: [PATCH] build: history should not contain ARG values

It seems docker never writes ARG values to build history and only writes
values with RUN statements. As a result it can utilize caching better if
assgnment of an ARG value is never used.

Instance
```Dockerfile
FROM alpine
ARG FOO
ENV FOO=bat
RUN echo $FOO
```

In above example --build-arg FOO=value has no significance as value
is always overriden by ENV.

Following PR makes sure buildah mimics a similar behaviour.

Signed-off-by: Aditya Rajan <arajan@redhat.com>
---
 imagebuildah/stage_executor.go | 49 +++++++++++++++++++++++++++++-----
 tests/bud.bats                 | 33 ++++++++++++++++++++---
 tests/bud/with-arg/Dockerfile  |  4 +++
 tests/bud/with-arg/Dockerfile2 |  4 +++
 4 files changed, 81 insertions(+), 9 deletions(-)
 create mode 100644 tests/bud/with-arg/Dockerfile
 create mode 100644 tests/bud/with-arg/Dockerfile2

diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go
index 1d1f174bed..99f829db92 100644
--- a/imagebuildah/stage_executor.go
+++ b/imagebuildah/stage_executor.go
@@ -1112,10 +1112,10 @@ func (s *StageExecutor) getCreatedBy(node *parser.Node, addedContentSummary stri
 	}
 	switch strings.ToUpper(node.Value) {
 	case "ARG":
-		buildArgs := s.getBuildArgs()
+		buildArgs := s.getBuildArgsKey()
 		return "/bin/sh -c #(nop) ARG " + buildArgs
 	case "RUN":
-		buildArgs := s.getBuildArgs()
+		buildArgs := s.getBuildArgsResolvedForRun()
 		if buildArgs != "" {
 			return "|" + strconv.Itoa(len(strings.Split(buildArgs, " "))) + " " + buildArgs + " /bin/sh -c " + node.Original[4:]
 		}
@@ -1133,10 +1133,47 @@ func (s *StageExecutor) getCreatedBy(node *parser.Node, addedContentSummary stri
 
 // getBuildArgs returns a string of the build-args specified during the build process
 // it excludes any build-args that were not used in the build process
-func (s *StageExecutor) getBuildArgs() string {
-	buildArgs := s.stage.Builder.Arguments()
-	sort.Strings(buildArgs)
-	return strings.Join(buildArgs, " ")
+// values for args are overridden by the values specified using ENV.
+// Reason: Values from ENV will always override values specified arg.
+func (s *StageExecutor) getBuildArgsResolvedForRun() string {
+	var envs []string
+	configuredEnvs := make(map[string]string)
+	dockerConfig := s.stage.Builder.Config()
+
+	for _, env := range dockerConfig.Env {
+		splitv := strings.SplitN(env, "=", 2)
+		if len(splitv) == 2 {
+			configuredEnvs[splitv[0]] = splitv[1]
+		}
+	}
+
+	for key, value := range s.stage.Builder.Args {
+		if _, ok := s.stage.Builder.AllowedArgs[key]; ok {
+			// if value was in image it will be given higher priority
+			// so please embed that into build history
+			_, inImage := configuredEnvs[key]
+			if inImage {
+				envs = append(envs, fmt.Sprintf("%s=%s", key, configuredEnvs[key]))
+			} else {
+				envs = append(envs, fmt.Sprintf("%s=%s", key, value))
+			}
+		}
+	}
+	sort.Strings(envs)
+	return strings.Join(envs, " ")
+}
+
+// getBuildArgs key returns set args are key which were specified during the build process
+// following function will be exclusively used by build history
+func (s *StageExecutor) getBuildArgsKey() string {
+	var envs []string
+	for key := range s.stage.Builder.Args {
+		if _, ok := s.stage.Builder.AllowedArgs[key]; ok {
+			envs = append(envs, key)
+		}
+	}
+	sort.Strings(envs)
+	return strings.Join(envs, " ")
 }
 
 // tagExistingImage adds names to an image already in the store
diff --git a/tests/bud.bats b/tests/bud.bats
index bb0c4efabe..289d8c2a9d 100644
--- a/tests/bud.bats
+++ b/tests/bud.bats
@@ -366,17 +366,44 @@ symlink(subdir)"
   # two more, starting at the "echo $user | base64" instruction
   run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=user=1 --layers -t test1 -f Dockerfile.build-args ${TESTSDIR}/bud/use-layers
   run_buildah images -a
-  expect_line_count 8
+  expect_line_count 7
 
   # one more, because we added a new name to the same image
   run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=user=1 --layers -t test2 -f Dockerfile.build-args ${TESTSDIR}/bud/use-layers
   run_buildah images -a
-  expect_line_count 9
+  expect_line_count 8
 
   # two more, starting at the "echo $user | base64" instruction
   run_buildah build --signature-policy ${TESTSDIR}/policy.json --layers -t test3 -f Dockerfile.build-args ${TESTSDIR}/bud/use-layers
   run_buildah images -a
-  expect_line_count 12
+  expect_line_count 11
+}
+
+
+@test "bud with --layers and --build-args: override ARG with ENV and image must be cached" {
+  _prefetch alpine
+  #when ARG is overriden by config
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=FOO=1 --layers -t args-cache -f ${TESTSDIR}/bud/with-arg/Dockerfile
+  run_buildah inspect -f '{{.FromImageID}}' args-cache
+  idbefore="$output"
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=FOO=12 --layers -t args-cache -f ${TESTSDIR}/bud/with-arg/Dockerfile
+  run_buildah inspect -f '{{.FromImageID}}' args-cache
+  expect_output --substring ${idbefore}
+  run_buildah rmi args-cache
+}
+
+@test "bud with --layers and --build-args: use raw ARG and cache should not be used" {
+  # when ARG is used as a raw value
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=FOO=1 --layers -t args-cache -f ${TESTSDIR}/bud/with-arg/Dockerfile2
+  run_buildah inspect -f '{{.FromImageID}}' args-cache
+  idbefore="$output"
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json --build-arg=FOO=12 --layers -t args-cache -f ${TESTSDIR}/bud/with-arg/Dockerfile2
+  run_buildah inspect -f '{{.FromImageID}}' args-cache
+  idafter="$output"
+  run_buildah rmi args-cache
+
+  assert "$idbefore" != "$idafter" \
+         ".Args changed so final image id should be different"
 }
 
 @test "bud with --rm flag" {
diff --git a/tests/bud/with-arg/Dockerfile b/tests/bud/with-arg/Dockerfile
new file mode 100644
index 0000000000..3f747ddcbf
--- /dev/null
+++ b/tests/bud/with-arg/Dockerfile
@@ -0,0 +1,4 @@
+FROM alpine
+ARG FOO
+ENV FOO=bat
+RUN echo $FOO
diff --git a/tests/bud/with-arg/Dockerfile2 b/tests/bud/with-arg/Dockerfile2
new file mode 100644
index 0000000000..70035cf91c
--- /dev/null
+++ b/tests/bud/with-arg/Dockerfile2
@@ -0,0 +1,4 @@
+FROM alpine
+ARG FOO
+ENV FOO=${FOO}
+RUN echo $FOO