From 67ab55bbb747dbc6b80cef98fbe4a48ca18f0578 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 9 Jan 2023 14:41:21 -0500 Subject: [PATCH] bud: Consolidate multiple synthetic LABEL instructions We handle --label command line arguments by appending LABEL instructions to the Dockerfile contents before we parse it. Previously, we were appending a separate line for each label-value pair. Consolidate them for the sake of tools that arbitrarily limit the length of histories that they're willing to accept in images (boo!). Add a similar implementation for --env command line arguments. Previously, we'd set them in the initial configuration for each stage and also set them at commit-time, and that potentially overrode any values that were explicitly in the stage itself, and which would have affected RUN instructions. Remove the set-at-commit-time logic so that the history reflects what ends up in the image. Signed-off-by: Nalin Dahyabhai --- docs/buildah-build.1.md | 1 + imagebuildah/build.go | 33 ++++++++++--------- imagebuildah/executor.go | 31 +++++++++++++++-- imagebuildah/stage_executor.go | 31 ++++------------- tests/bud.bats | 18 ++++++++++ tests/bud/env/Dockerfile.env-precedence | 17 ++++++++++ tests/conformance/conformance_test.go | 6 ++++ .../testdata/env/precedence/Dockerfile | 6 ++++ 8 files changed, 99 insertions(+), 44 deletions(-) create mode 100644 tests/bud/env/Dockerfile.env-precedence create mode 100644 tests/conformance/testdata/env/precedence/Dockerfile diff --git a/docs/buildah-build.1.md b/docs/buildah-build.1.md index 53a3fb3254..aa1daf259d 100644 --- a/docs/buildah-build.1.md +++ b/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. diff --git a/imagebuildah/build.go b/imagebuildah/build.go index f82de7176d..ca45501c86 100644 --- a/imagebuildah/build.go +++ b/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...) } } diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go index a1886c94fa..5c9f1ef87e 100644 --- a/imagebuildah/executor.go +++ b/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() diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 0fc8b984f6..fce5dc33e6 100644 --- a/imagebuildah/stage_executor.go +++ b/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) } diff --git a/tests/bud.bats b/tests/bud.bats index 4b97ca3d98..42dabb1e56 100644 --- a/tests/bud.bats +++ b/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" +} diff --git a/tests/bud/env/Dockerfile.env-precedence b/tests/bud/env/Dockerfile.env-precedence new file mode 100644 index 0000000000..d8824b8caf --- /dev/null +++ b/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 diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 42be69f76d..8d53386843 100644 --- a/tests/conformance/conformance_test.go +++ b/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, + }, } diff --git a/tests/conformance/testdata/env/precedence/Dockerfile b/tests/conformance/testdata/env/precedence/Dockerfile new file mode 100644 index 0000000000..3d89d5ae0c --- /dev/null +++ b/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