Merge pull request #4511 from nalind/multiple-label

bud: Consolidate multiple synthetic LABEL instructions

docs/buildah-build.1.md

@@ -319,6 +319,7 @@ Set custom DNS search domains. Invalid if using **--dns-search** with **--networ
 Add a value (e.g. env=*value*) to the built image.  Can be used multiple times.
 If neither `=` nor a `*value*` are specified, but *env* is set in the current
 environment, the value from the current environment will be added to the image.
+The value of *env* can be overridden by ENV instructions in the Containerfile.
 To remove an environment variable from the built image, use the `--unsetenv`
 option.
 

imagebuildah/build.go

@@ -419,31 +419,32 @@ func buildDockerfilesOnce(ctx context.Context, store storage.Store, logger *logr
 		mainNode.Children = append(mainNode.Children, additionalNode.Children...)
 	}
 
-	// Check if any modifications done to labels
-	// add them to node-layer so it becomes regular
-	// layer.
-	// Reason: Docker adds label modification as
-	// last step which can be processed as regular
-	// steps and if no modification is done to layers
-	// its easier to re-use cached layers.
+	// Check if any labels were passed in via the API, and add a final line
+	// to the Dockerfile that would provide the same result.
+	// Reason: Docker adds label modification as a last step which can be
+	// processed like regular steps, and if no modification is done to
+	// layers, its easier to re-use cached layers.
 	if len(options.Labels) > 0 {
-		for _, labelSpec := range options.Labels {
+		var labelLine string
+		labels := append([]string{}, options.Labels...)
+		for _, labelSpec := range labels {
 			label := strings.SplitN(labelSpec, "=", 2)
-			labelLine := ""
 			key := label[0]
 			value := ""
 			if len(label) > 1 {
 				value = label[1]
 			}
-			// check from only empty key since docker supports empty value
+			// check only for an empty key since docker allows empty values
 			if key != "" {
-				labelLine = fmt.Sprintf("LABEL %q=%q\n", key, value)
-				additionalNode, err := imagebuilder.ParseDockerfile(strings.NewReader(labelLine))
-				if err != nil {
-					return "", nil, fmt.Errorf("while adding additional LABEL steps: %w", err)
-				}
-				mainNode.Children = append(mainNode.Children, additionalNode.Children...)
+				labelLine += fmt.Sprintf(" %q=%q", key, value)
+			}
+		}
+		if len(labelLine) > 0 {
+			additionalNode, err := imagebuilder.ParseDockerfile(strings.NewReader("LABEL" + labelLine + "\n"))
+			if err != nil {
+				return "", nil, fmt.Errorf("while adding additional LABEL step: %w", err)
 			}
+			mainNode.Children = append(mainNode.Children, additionalNode.Children...)
 		}
 	}
 

imagebuildah/executor.go

@@ -456,6 +456,10 @@ func (b *Executor) buildStage(ctx context.Context, cleanupStages map[int]*StageE
 	ib := stage.Builder
 	node := stage.Node
 	base, err := ib.From(node)
+	if err != nil {
+		logrus.Debugf("buildStage(node.Children=%#v)", node.Children)
+		return "", nil, err
+	}
 
 	// If this is the last stage, then the image that we produce at
 	// its end should be given the desired output name.
@@ -464,9 +468,30 @@ func (b *Executor) buildStage(ctx context.Context, cleanupStages map[int]*StageE
 		output = b.output
 	}
 
-	if err != nil {
-		logrus.Debugf("buildStage(node.Children=%#v)", node.Children)
-		return "", nil, err
+	// If this stage is starting out with environment variables that were
+	// passed in via our API, we should include them in the history, since
+	// they affect RUN instructions in this stage.
+	if len(b.envs) > 0 {
+		var envLine string
+		for _, envSpec := range b.envs {
+			env := strings.SplitN(envSpec, "=", 2)
+			key := env[0]
+			if len(env) > 1 {
+				value := env[1]
+				envLine += fmt.Sprintf(" %q=%q", key, value)
+			} else {
+				value := os.Getenv(key)
+				envLine += fmt.Sprintf(" %q=%q", key, value)
+			}
+		}
+		if len(envLine) > 0 {
+			additionalNode, err := imagebuilder.ParseDockerfile(strings.NewReader("ENV" + envLine + "\n"))
+			if err != nil {
+				return "", nil, fmt.Errorf("while adding additional ENV step: %w", err)
+			}
+			// make this the first instruction in the stage after its FROM instruction
+			stage.Node.Children = append(additionalNode.Children, stage.Node.Children...)
+		}
 	}
 
 	b.stagesLock.Lock()

imagebuildah/stage_executor.go

@@ -1624,17 +1624,17 @@ func (s *StageExecutor) getBuildArgsResolvedForRun() string {
 	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
+// getBuildArgs key returns the set of args which were specified during the
+// build process, formatted for inclusion in the build history
 func (s *StageExecutor) getBuildArgsKey() string {
-	var envs []string
+	var args []string
 	for key := range s.stage.Builder.Args {
 		if _, ok := s.stage.Builder.AllowedArgs[key]; ok {
-			envs = append(envs, key)
+			args = append(args, key)
 		}
 	}
-	sort.Strings(envs)
-	return strings.Join(envs, " ")
+	sort.Strings(args)
+	return strings.Join(args, " ")
 }
 
 // tagExistingImage adds names to an image already in the store
@@ -1935,25 +1935,6 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
 		spec := strings.SplitN(envSpec, "=", 2)
 		s.builder.SetEnv(spec[0], spec[1])
 	}
-	for _, envSpec := range s.executor.envs {
-		env := strings.SplitN(envSpec, "=", 2)
-		if len(env) > 1 {
-			getenv := func(name string) string {
-				for _, envvar := range s.builder.Env() {
-					val := strings.SplitN(envvar, "=", 2)
-					if len(val) == 2 && val[0] == name {
-						return val[1]
-					}
-				}
-				logrus.Errorf("error expanding variable %q: no value set in image", name)
-				return name
-			}
-			env[1] = os.Expand(env[1], getenv)
-			s.builder.SetEnv(env[0], env[1])
-		} else {
-			s.builder.SetEnv(env[0], os.Getenv(env[0]))
-		}
-	}
 	for _, envSpec := range s.executor.unsetEnvs {
 		s.builder.UnsetEnv(envSpec)
 	}

tests/bud.bats

@@ -5855,3 +5855,21 @@ _EOF
      expect_output --substring "65534"
   fi
 }
+
+@test "build-env-precedence" {
+  skip_if_no_runtime
+
+  _prefetch alpine
+
+  run_buildah build --no-cache --env E=F --env G=H --env I=J --env K=L -f ${BUDFILES}/env/Dockerfile.env-precedence ${BUDFILES}/env
+  expect_output --substring "a=b c=d E=F G=H"
+  expect_output --substring "a=b c=d E=E G=G"
+  expect_output --substring "w=x y=z I=J K=L"
+  expect_output --substring "w=x y=z I=I K=K"
+
+  run_buildah build --no-cache --layers --env E=F --env G=H --env I=J --env K=L -f ${BUDFILES}/env/Dockerfile.env-precedence ${BUDFILES}/env
+  expect_output --substring "a=b c=d E=F G=H"
+  expect_output --substring "a=b c=d E=E G=G"
+  expect_output --substring "w=x y=z I=J K=L"
+  expect_output --substring "w=x y=z I=I K=K"
+}

tests/bud/env/Dockerfile.env-precedence

@@ -0,0 +1,17 @@
+FROM alpine
+ENV a=b
+ENV c=d
+# E and G are passed in on the command-line, and we haven't overridden them yet, so the command will get the CLI values.
+RUN echo a=$a c=$c E=$E G=$G
+ENV E=E G=G
+# We just set E and G, and that will override values passed at the command line thanks to imagebuilder's handling of ENV instructions.
+RUN echo a=$a c=$c E=$E G=$G
+
+FROM 0
+ENV w=x
+ENV y=z
+# I and K are passed in on the command-line, and we haven't overridden them yet, so the command will get the CLI values.
+RUN echo w=$w y=$y I=$I K=$K
+ENV I=I K=K
+# We just set I and K, and that will override values passed at the command line thanks to imagebuilder's handling of ENV instructions.
+RUN echo w=$w y=$y I=$I K=$K

tests/conformance/conformance_test.go

@@ -2997,4 +2997,10 @@ var internalTestCases = []testCase{
 		fsSkip:            []string{"(dir):created:mtime", "(dir):created:(dir):directory:mtime"},
 		dockerUseBuildKit: true,
 	},
+
+	{
+		name:              "env-precedence",
+		contextDir:        "env/precedence",
+		dockerUseBuildKit: true,
+	},
 }

tests/conformance/testdata/env/precedence/Dockerfile

@@ -0,0 +1,6 @@
+FROM busybox
+ENV a=b
+ENV c=d
+RUN echo E=$E G=$G
+ENV E=E G=G
+RUN echo E=$E G=$G