Merge pull request #5641 from nalind/update-imagebuilder-v1.2.12

Update github.com/openshift/imagebuilder to v1.2.14

go.mod

@@ -41,7 +41,7 @@ require (
 	github.com/opencontainers/runtime-spec v1.2.0
 	github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc
 	github.com/opencontainers/selinux v1.11.0
-	github.com/openshift/imagebuilder v1.2.11
+	github.com/openshift/imagebuilder v1.2.14
 	github.com/seccomp/libseccomp-golang v0.10.0
 	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/cobra v1.8.1

go.sum

@@ -277,8 +277,8 @@ github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc h1:
 github.com/opencontainers/runtime-tools v0.9.1-0.20230914150019-408c51e934dc/go.mod h1:8tx1helyqhUC65McMm3x7HmOex8lO2/v9zPuxmKHurs=
 github.com/opencontainers/selinux v1.11.0 h1:+5Zbo97w3Lbmb3PeqQtpmTkMwsW5nRI3YaLpt7tQ7oU=
 github.com/opencontainers/selinux v1.11.0/go.mod h1:E5dMC3VPuVvVHDYmi78qvhJp8+M586T4DlDRYpFkyec=
-github.com/openshift/imagebuilder v1.2.11 h1:4EmEMyiLr7jlskS1h6V6smdcrQSGLRdcIeaXeV3F8EM=
-github.com/openshift/imagebuilder v1.2.11/go.mod h1:KkkXOyRjJlZEXWQtHNBNzVHqh4vf/0xX5cDIQ2gr+5I=
+github.com/openshift/imagebuilder v1.2.14 h1:l4gUw0KIsjZrX7otfS4WoKxzGBrxYldU3pF4+5W/ud8=
+github.com/openshift/imagebuilder v1.2.14/go.mod h1:KkkXOyRjJlZEXWQtHNBNzVHqh4vf/0xX5cDIQ2gr+5I=
 github.com/ostreedev/ostree-go v0.0.0-20210805093236-719684c64e4f h1:/UDgs8FGMqwnHagNDPGOlts35QkhAZ8by3DR7nMih7M=
 github.com/ostreedev/ostree-go v0.0.0-20210805093236-719684c64e4f/go.mod h1:J6OG6YJVEWopen4avK3VNQSnALmmjvniMmni/YFYAwc=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=

imagebuildah/build.go

@@ -38,6 +38,7 @@ import (
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/openshift/imagebuilder"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/exp/maps"
 	"golang.org/x/exp/slices"
 	"golang.org/x/sync/semaphore"
 )
@@ -204,6 +205,9 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 	if options.SystemContext == nil {
 		options.SystemContext = &types.SystemContext{}
 	}
+	if options.AdditionalBuildContexts == nil {
+		options.AdditionalBuildContexts = make(map[string]*define.AdditionalBuildContext)
+	}
 
 	if len(options.Platforms) == 0 {
 		options.Platforms = append(options.Platforms, struct{ OS, Arch, Variant string }{
@@ -213,9 +217,6 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 	}
 
 	if options.AllPlatforms {
-		if options.AdditionalBuildContexts == nil {
-			options.AdditionalBuildContexts = make(map[string]*define.AdditionalBuildContext)
-		}
 		options.Platforms, err = platformsForBaseImages(ctx, logger, paths, files, options.From, options.Args, options.AdditionalBuildContexts, options.SystemContext)
 		if err != nil {
 			return "", nil, err
@@ -251,11 +252,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 			logPrefix = "[" + platforms.Format(platformSpec) + "] "
 		}
 		// Deep copy args to prevent concurrent read/writes over Args.
-		argsCopy := make(map[string]string)
-		for key, value := range options.Args {
-			argsCopy[key] = value
-		}
-		platformOptions.Args = argsCopy
+		platformOptions.Args = maps.Clone(options.Args)
 		builds.Go(func() error {
 			loggerPerPlatform := logger
 			if platformOptions.LogFile != "" && platformOptions.LogSplitByPlatform {
@@ -395,36 +392,38 @@ func buildDockerfilesOnce(ctx context.Context, store storage.Store, logger *logr
 	// --platform was explicitly selected for this build
 	// so set correct TARGETPLATFORM in args if it is not
 	// already selected by the user.
+	builtinArgDefaults := make(map[string]string)
 	if options.SystemContext.OSChoice != "" && options.SystemContext.ArchitectureChoice != "" {
 		// os component from --platform string populates TARGETOS
 		// buildkit parity: give priority to user's `--build-arg`
-		if _, ok := options.Args["TARGETOS"]; !ok {
-			options.Args["TARGETOS"] = options.SystemContext.OSChoice
-		}
+		builtinArgDefaults["TARGETOS"] = options.SystemContext.OSChoice
 		// arch component from --platform string populates TARGETARCH
 		// buildkit parity: give priority to user's `--build-arg`
-		if _, ok := options.Args["TARGETARCH"]; !ok {
-			options.Args["TARGETARCH"] = options.SystemContext.ArchitectureChoice
-		}
+		builtinArgDefaults["TARGETARCH"] = options.SystemContext.ArchitectureChoice
 		// variant component from --platform string populates TARGETVARIANT
 		// buildkit parity: give priority to user's `--build-arg`
-		if _, ok := options.Args["TARGETVARIANT"]; !ok {
-			if options.SystemContext.VariantChoice != "" {
-				options.Args["TARGETVARIANT"] = options.SystemContext.VariantChoice
-			}
-		}
+		builtinArgDefaults["TARGETVARIANT"] = options.SystemContext.VariantChoice
 		// buildkit parity: give priority to user's `--build-arg`
-		if _, ok := options.Args["TARGETPLATFORM"]; !ok {
-			// buildkit parity: TARGETPLATFORM should be always created
-			// from SystemContext and not `TARGETOS` and `TARGETARCH` because
-			// users can always override values of `TARGETOS` and `TARGETARCH`
-			// but `TARGETPLATFORM` should be set independent of those values.
-			options.Args["TARGETPLATFORM"] = options.SystemContext.OSChoice + "/" + options.SystemContext.ArchitectureChoice
-			if options.SystemContext.VariantChoice != "" {
-				options.Args["TARGETPLATFORM"] = options.Args["TARGETPLATFORM"] + "/" + options.SystemContext.VariantChoice
-			}
+		// buildkit parity: TARGETPLATFORM should be always created
+		// from SystemContext and not `TARGETOS` and `TARGETARCH` because
+		// users can always override values of `TARGETOS` and `TARGETARCH`
+		// but `TARGETPLATFORM` should be set independent of those values.
+		builtinArgDefaults["TARGETPLATFORM"] = builtinArgDefaults["TARGETOS"] + "/" + builtinArgDefaults["TARGETARCH"]
+		if options.SystemContext.VariantChoice != "" {
+			builtinArgDefaults["TARGETPLATFORM"] += "/" + options.SystemContext.VariantChoice
+		}
+	} else {
+		// fill them in using values for the default platform
+		defaultPlatform := platforms.DefaultSpec()
+		builtinArgDefaults["TARGETOS"] = defaultPlatform.OS
+		builtinArgDefaults["TARGETVARIANT"] = defaultPlatform.Variant
+		builtinArgDefaults["TARGETARCH"] = defaultPlatform.Architecture
+		builtinArgDefaults["TARGETPLATFORM"] = defaultPlatform.OS + "/" + defaultPlatform.Architecture
+		if defaultPlatform.Variant != "" {
+			builtinArgDefaults["TARGETPLATFORM"] += "/" + defaultPlatform.Variant
 		}
 	}
+	delete(options.Args, "TARGETPLATFORM")
 
 	for i, d := range dockerfilecontents[1:] {
 		additionalNode, err := imagebuilder.ParseDockerfile(bytes.NewReader(d))
@@ -440,6 +439,9 @@ func buildDockerfilesOnce(ctx context.Context, store storage.Store, logger *logr
 		return "", nil, fmt.Errorf("creating build executor: %w", err)
 	}
 	b := imagebuilder.NewBuilder(options.Args)
+	for k, v := range builtinArgDefaults {
+		b.BuiltinArgDefaults[k] = v
+	}
 	defaultContainerConfig, err := config.Default()
 	if err != nil {
 		return "", nil, fmt.Errorf("failed to get container config: %w", err)

imagebuildah/executor.go

@@ -773,18 +773,19 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) (image
 									base = child.Next.Value
 								}
 							}
+							builtinArgs := argsMapToSlice(stage.Builder.BuiltinArgDefaults)
 							headingArgs := argsMapToSlice(stage.Builder.HeadingArgs)
 							userArgs := argsMapToSlice(stage.Builder.Args)
 							// append heading args so if --build-arg key=value is not
 							// specified but default value is set in Containerfile
 							// via `ARG key=value` so default value can be used.
-							userArgs = append(headingArgs, userArgs...)
+							userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
 							baseWithArg, err := imagebuilder.ProcessWord(base, userArgs)
 							if err != nil {
 								return "", nil, fmt.Errorf("while replacing arg variables with values for format %q: %w", base, err)
 							}
 							b.baseMap[baseWithArg] = struct{}{}
-							logrus.Debugf("base for stage %d: %q", stageIndex, base)
+							logrus.Debugf("base for stage %d: %q resolves to %q", stageIndex, base, baseWithArg)
 							// Check if selected base is not an additional
 							// build context and if base is a valid stage
 							// add it to current stage's dependency tree.
@@ -811,16 +812,18 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) (image
 							// if following ADD or COPY needs any other
 							// stage.
 							stageName := rootfs
+							builtinArgs := argsMapToSlice(stage.Builder.BuiltinArgDefaults)
 							headingArgs := argsMapToSlice(stage.Builder.HeadingArgs)
 							userArgs := argsMapToSlice(stage.Builder.Args)
 							// append heading args so if --build-arg key=value is not
 							// specified but default value is set in Containerfile
 							// via `ARG key=value` so default value can be used.
-							userArgs = append(headingArgs, userArgs...)
+							userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
 							baseWithArg, err := imagebuilder.ProcessWord(stageName, userArgs)
 							if err != nil {
 								return "", nil, fmt.Errorf("while replacing arg variables with values for format %q: %w", stageName, err)
 							}
+							logrus.Debugf("stage %d name: %q resolves to %q", stageIndex, stageName, baseWithArg)
 							stageName = baseWithArg
 							// If --from=<index> convert index to name
 							if index, err := strconv.Atoi(stageName); err == nil {

tests/bud.bats

@@ -532,12 +532,15 @@ symlink(subdir)"
 }
 
 @test "build with basename resolving default arg" {
+  run_buildah info --format '{{.host.os}}/{{.host.arch}}{{if .host.variant}}/{{.host.variant}}{{end}}'
+  myplatform="$output"
   run_buildah info --format '{{.host.arch}}'
   myarch="$output"
-  run_buildah info --format '{{.host.variant}}'
-  myvariant="$output"
 
-  run_buildah build --platform linux/$myarch/$myvariant $WITH_POLICY_JSON -t test -f $BUDFILES/base-with-arg/Containerfile
+  run_buildah build --platform ${myplatform} $WITH_POLICY_JSON -t test -f $BUDFILES/base-with-arg/Containerfile
+  expect_output --substring "This is built for $myarch"
+
+  run_buildah build                          $WITH_POLICY_JSON -t test -f $BUDFILES/base-with-arg/Containerfile
   expect_output --substring "This is built for $myarch"
 }
 

tests/bud/base-with-arg/Containerfile2

@@ -1,6 +1,7 @@
-FROM alpine as build
 ARG CUSTOM_TARGET
 
+FROM alpine as build
+
 FROM build AS platform-first
 ENV BUILT_FOR=first
 

tests/bud/multiarch/Dockerfile.no-run

@@ -1,8 +1,10 @@
-# A base image that is known to be a manifest list.
-FROM docker.io/library/alpine
-COPY Dockerfile.no-run /root/
 # A different base image that is known to be a manifest list, supporting a
 # different but partially-overlapping set of platforms.
 ARG SAFEIMAGE
+
+# A base image that is known to be a manifest list.
+FROM docker.io/library/alpine
+COPY Dockerfile.no-run /root/
+
 FROM $SAFEIMAGE
 COPY --from=0 /root/Dockerfile.no-run /root/

tests/conformance/conformance_test.go

@@ -52,6 +52,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/maps"
 	"golang.org/x/exp/slices"
 )
 
@@ -490,21 +491,21 @@ func testConformanceInternalBuild(ctx context.Context, t *testing.T, cwd string,
 	if t.Failed() {
 		t.FailNow()
 	}
-	deleteLabel := func(config map[string]interface{}, label string) {
+	deleteIdentityLabel := func(config map[string]interface{}) {
 		for _, configName := range []string{"config", "container_config"} {
 			if configStruct, ok := config[configName]; ok {
 				if configMap, ok := configStruct.(map[string]interface{}); ok {
 					if labels, ok := configMap["Labels"]; ok {
 						if labelMap, ok := labels.(map[string]interface{}); ok {
-							delete(labelMap, label)
+							delete(labelMap, buildah.BuilderIdentityAnnotation)
 						}
 					}
 				}
 			}
 		}
 	}
-	deleteLabel(originalBuildahConfig, buildah.BuilderIdentityAnnotation)
-	deleteLabel(ociBuildahConfig, buildah.BuilderIdentityAnnotation)
+	deleteIdentityLabel(originalBuildahConfig)
+	deleteIdentityLabel(ociBuildahConfig)
 
 	var originalDockerConfig, ociDockerConfig, fsDocker map[string]interface{}
 
@@ -517,6 +518,10 @@ func testConformanceInternalBuild(ctx context.Context, t *testing.T, cwd string,
 			t.FailNow()
 		}
 
+		// Some of the base images for our tests were built with buildah, too
+		deleteIdentityLabel(originalDockerConfig)
+		deleteIdentityLabel(ociDockerConfig)
+
 		miss, left, diff, same := compareJSON(originalDockerConfig, originalBuildahConfig, originalSkip)
 		if !same {
 			assert.Failf(t, "Image configurations differ as committed in Docker format", configCompareResult(miss, left, diff, "buildah"))
@@ -606,6 +611,7 @@ func buildUsingBuildah(ctx context.Context, t *testing.T, store storage.Store, t
 		ForceRmIntermediateCtrs: true,
 		CompatSetParent:         test.compatSetParent,
 		CompatVolumes:           test.compatVolumes,
+		Args:                    maps.Clone(test.buildArgs),
 	}
 	// build the image and gather output. log the output if the build part of the test failed
 	imageID, _, err := imagebuildah.BuildDockerfiles(ctx, store, options, dockerfileName)
@@ -695,6 +701,10 @@ func buildUsingDocker(ctx context.Context, t *testing.T, client *docker.Client,
 	require.NoErrorf(t, err, "archiving context directory %q", contextDir)
 	defer input.Close()
 
+	var buildArgs []docker.BuildArg
+	for k, v := range test.buildArgs {
+		buildArgs = append(buildArgs, docker.BuildArg{Name: k, Value: v})
+	}
 	// set up build options
 	output := &bytes.Buffer{}
 	options := docker.BuildImageOptions{
@@ -706,6 +716,7 @@ func buildUsingDocker(ctx context.Context, t *testing.T, client *docker.Client,
 		NoCache:             true,
 		RmTmpContainer:      true,
 		ForceRmTmpContainer: true,
+		BuildArgs:           buildArgs,
 	}
 	if test.dockerUseBuildKit || test.dockerBuilderVersion != "" {
 		if test.dockerBuilderVersion != "" {
@@ -781,7 +792,7 @@ func buildUsingImagebuilder(t *testing.T, client *docker.Client, test testCase,
 		})
 	}
 	// build the image and gather output. log the output if the build part of the test failed
-	builder := imagebuilder.NewBuilder(nil)
+	builder := imagebuilder.NewBuilder(maps.Clone(test.buildArgs))
 	node, err := imagebuilder.ParseFile(filepath.Join(contextDir, dockerfileRelativePath))
 	if err != nil {
 		assert.Nil(t, err, "error parsing Dockerfile: %v", err)
@@ -1399,6 +1410,7 @@ type testCase struct {
 	compatVolumes        types.OptionalBool        // placeholder for a value to set for the buildah compatVolumes flag
 	transientMounts      []string                  // one possible buildah-specific feature
 	fsSkip               []string                  // expected filesystem differences, typically timestamps on files or directories we create or modify during the build and don't reset
+	buildArgs            map[string]string         // build args to supply, as if --build-arg was used
 }
 
 var internalTestCases = []testCase{
@@ -3212,6 +3224,19 @@ var internalTestCases = []testCase{
 		contextDir:       "chown-volume",
 		testUsingVolumes: true,
 	},
+
+	{
+		name:              "builtins",
+		contextDir:        "builtins",
+		dockerUseBuildKit: true,
+		buildArgs:         map[string]string{"SOURCE": "source", "BUSYBOX": "busybox", "ALPINE": "alpine", "OWNERID": "0", "       SECONDBASE": "localhost/no-such-image"},
+	},
+
+	{
+		name:              "header-builtin",
+		contextDir:        "header-builtin",
+		dockerUseBuildKit: true,
+	},
 }
 
 func TestCommit(t *testing.T) {

tests/conformance/testdata/builtins/Dockerfile

@@ -0,0 +1,23 @@
+ARG ALPINE
+
+FROM busybox
+RUN echo TARGETPLATFORM=$TARGETPLATFORM | tee 0.txt
+ARG TARGETPLATFORM
+RUN echo TARGETPLATFORM=$TARGETPLATFORM | tee -a 0.txt
+RUN touch -d @0 0.txt
+
+FROM ${SECONDBASE:-busybox}
+COPY --from=0 /*.txt /
+COPY --chown=${OWNERID:-1}:${OWNERID:-1} ${SOURCE:-other}file.txt /1a.txt
+ARG OWNERID=1
+ARG SOURCE=
+COPY --chown=${OWNERID:-1}:${OWNERID:-1} ${SOURCE:-other}file.txt /1b.txt
+
+FROM ${ALPINE:-busybox}
+ARG SECONDBASE=localhost/no-such-image
+COPY --from=1 /*.txt /
+RUN cp -p /etc/nsswitch.conf /2.txt
+
+FROM ${BUSYBOX:-alpine}
+COPY --from=2 /*.txt /
+RUN cp -p /etc/nsswitch.conf /3.txt

tests/conformance/testdata/builtins/otherfile.txt

@@ -0,0 +1 @@
+other

tests/conformance/testdata/builtins/sourcefile.txt

@@ -0,0 +1 @@
+source

tests/conformance/testdata/header-builtin/Dockerfile

@@ -0,0 +1,15 @@
+# if TARGETARCH is defined, we'l pull the x86_64 image
+FROM registry.fedoraproject.org/fedora-minimal:41${TARGETARCH:+-x86_64} AS amd64
+# if TARGETARCH is defined, we'l pull the aarch64 image
+FROM registry.fedoraproject.org/fedora-minimal:41${TARGETARCH:+-aarch64} AS arm64
+
+# run "file" against both shared libraries
+FROM registry.fedoraproject.org/fedora-minimal AS native
+COPY --from=amd64 /lib64/libc.so.6 /libc-amd64
+COPY --from=arm64 /lib64/libc.so.6 /libc-arm64
+RUN microdnf -y install file && microdnf -y clean all
+RUN file /libc-* | tee /libc-types.txt && touch -d @0 /libc-types.txt
+
+# expect them to have different target architectures listed in their ELF headers
+FROM registry.fedoraproject.org/fedora-minimal
+COPY --from=native /libc-types.txt /