Merge pull request #3609 from flouthoc/fix-arg-history

build: history should not contain `ARG` values. OTOH `RUN` history should contain final values.

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

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" {

tests/bud/with-arg/Dockerfile

@@ -0,0 +1,4 @@
+FROM alpine
+ARG FOO
+ENV FOO=bat
+RUN echo $FOO

tests/bud/with-arg/Dockerfile2

@@ -0,0 +1,4 @@
+FROM alpine
+ARG FOO
+ENV FOO=${FOO}
+RUN echo $FOO