image build: match pre-pulling corner case to buildah

When a containerfile contains stages with duplicate names, buildah's
behavior is weird: containers/buildah#6731

Implement the same behavior for now. If buildah adjusts the behavior,
our integration tests will catch it and we can adjust accordingly.

Signed-off-by: Adam Cmiel <acmiel@redhat.com>

Author: chmeliik

integration_tests/build_test.go

@@ -1556,7 +1556,15 @@ RUN echo "this instruction also creates an intermediate layer" > /tmp/bar.txt
 			contextDir := setupTestContext(t)
 
 			writeContainerfile(contextDir, fmt.Sprintf(`
-# Real base image used for an earlier stage
+# This stage is unused, because the second stage1 overrides it
+FROM scratch AS stage1
+
+LABEL stage1.label=this-stage-is-unused
+LABEL common.build.label=this-stage-is-unused
+LABEL common.label=this-stage-is-unused
+
+
+# Real base image
 FROM %s AS stage1
 
 LABEL stage1.label=value-gets-overriden
@@ -2054,6 +2062,14 @@ FROM scratch AS stage1
 LABEL stage1.label=label-from-stage1
 LABEL common.label=common-stage1
 
+
+# --target matches the *first* stage with a matching name, so this is skipped
+FROM base.image.does.not/exist:latest AS stage1
+
+LABEL stage1.label=unused-stage
+
+
+# (this is skipped too, the stage is unnamed)
 FROM stage1
 
 LABEL stage2.label=label-from-stage2
@@ -2256,16 +2272,25 @@ RUN echo > /dev/udp/127.0.0.1/9
 			baseImage2 := createBaseImage("base2", 2048, "scratch")
 			baseImage3 := createBaseImage("base3", 4096, "scratch")
 			realBaseImage := createBaseImage("realbase", 8192, baseImage)
+			unusedBaseImage := createBaseImage("unused", 16384, baseImage)
 
 			r := strings.NewReplacer(
 				"{baseImage1}", baseImage1,
 				"{baseImage2}", baseImage2,
 				"{baseImage3}", baseImage3,
 				"{realBaseImage}", realBaseImage,
+				"{unusedBaseImage}", unusedBaseImage,
 			)
 
 			contextDir := setupTestContext(t)
 			writeContainerfile(contextDir, r.Replace(`
+# This base image is "unused" because the second base1 overrides this stage.
+# However, buildah will build *both* of the base1 stages, so we have to pre-pull both of the images.
+FROM {unusedBaseImage} AS base1
+
+RUN echo "the unused stage WAS built"
+
+
 FROM {baseImage1} AS base1
 
 # COPY directly from an image
@@ -2312,14 +2337,17 @@ RUN cp /random-data.bin /data/realBaseImage.bin
 
 			container := setupBuildContainerWithCleanup(t, buildParams, imageRegistry)
 
-			_, _, err := runBuildWithOutput(container, buildParams)
+			stdout, _, err := runBuildWithOutput(container, buildParams)
 			// Main check: no error (would fail without pre-pulling)
 			Expect(err).ToNot(HaveOccurred())
 
-			// Secondary check: verify that the correct base was pulled for each FROM/from
+			// Verify that buildah really did build the unused stage (otherwise we wasted a pull)
+			Expect(stdout).To(ContainSubstring("the unused stage WAS built"))
+
+			// Verify that the correct base was pulled for each FROM/from
 			// by checking the sizes of the random-data files
-			stdout := runWithMountedOutputImage(container, outputRef, "cd $CONTAINER_ROOT; du -b data/*")
-			Expect(stdout).To(SatisfyAll(
+			stdout2 := runWithMountedOutputImage(container, outputRef, "cd $CONTAINER_ROOT; du -b data/*")
+			Expect(stdout2).To(SatisfyAll(
 				ContainSubstring("1024\tdata/baseImage1.bin"),
 				ContainSubstring("2048\tdata/baseImage2.bin"),
 				ContainSubstring("4096\tdata/baseImage3.bin"),

pkg/commands/build.go

@@ -1168,8 +1168,9 @@ func (c *Build) prePullBaseImages(df *dockerfile.Dockerfile) error {
 
 	var targetStage int
 	if c.Params.Target != "" {
-		if idx, ok := resolveStageRef(df.Stages, c.Params.Target); ok {
-			targetStage = idx
+		if stages, ok := findMatchingStages(df.Stages, c.Params.Target); ok {
+			// buildah's --target matches the first stage with a matching name
+			targetStage = stages[0]
 		} else {
 			return fmt.Errorf("target stage %q not found", c.Params.Target)
 		}
@@ -1203,10 +1204,12 @@ func (c *Build) collectBaseImages(df *dockerfile.Dockerfile, targetStage int) []
 	stagesToProcess := []int{}
 	stagesSeen := make(map[int]struct{})
 
-	enqueue := func(stageIdx int) {
-		if _, seen := stagesSeen[stageIdx]; !seen {
-			stagesSeen[stageIdx] = struct{}{}
-			stagesToProcess = append(stagesToProcess, stageIdx)
+	enqueue := func(stageIndexes ...int) {
+		for _, stageIdx := range stageIndexes {
+			if _, seen := stagesSeen[stageIdx]; !seen {
+				stagesSeen[stageIdx] = struct{}{}
+				stagesToProcess = append(stagesToProcess, stageIdx)
+			}
 		}
 	}
 	enqueue(targetStage)
@@ -1228,9 +1231,10 @@ func (c *Build) collectBaseImages(df *dockerfile.Dockerfile, targetStage int) []
 		precedingStages := df.Stages[:stageIdx]
 
 		for _, ref := range getFromRefsInCommands(stage) {
-			if idx, ok := resolveStageRef(precedingStages, ref); ok {
-				// ref is a stage
-				enqueue(idx)
+			if stages, ok := findMatchingStages(precedingStages, ref); ok {
+				// ref matches one or more stages
+				// buildah builds all matching stages, we have to pre-pull all the images
+				enqueue(stages...)
 			} else {
 				// ref is an image
 				// (the third option is that ref is a --build-context,
@@ -1243,16 +1247,28 @@ func (c *Build) collectBaseImages(df *dockerfile.Dockerfile, targetStage int) []
 	return slices.Sorted(maps.Keys(baseImageSet))
 }
 
-func resolveStageRef(stages []*dockerfile.Stage, ref string) (int, bool) {
+// Given a list of containerfile stages and a string ref, determine if the ref matches any stage(s).
+// If yes, return ({indexes of matching stages}, true).
+//
+// First, look for matching named stages ('FROM ... AS name').
+// If no such stage exists, the ref may be an integer index of a stage - parse it and verify bounds.
+// If ref doesn't match any named stage and isn't a valid integer index, return (nil, false).
+func findMatchingStages(stages []*dockerfile.Stage, ref string) ([]int, bool) {
+	var matchingStages []int
 	for i, stage := range stages {
 		if stage.Name != nil && *stage.Name == ref {
-			return i, true
+			matchingStages = append(matchingStages, i)
 		}
 	}
+	if len(matchingStages) > 0 {
+		return matchingStages, true
+	}
+
 	if i, err := strconv.Atoi(ref); err == nil && 0 <= i && i < len(stages) {
-		return i, true
+		return []int{i}, true
 	}
-	return -1, false
+
+	return nil, false
 }
 
 // Returns all 'from' references from a stage's commands (COPY --from and RUN --mount=from).

pkg/commands/build_test.go

@@ -1473,68 +1473,83 @@ func Test_Build_injectBuildinfo(t *testing.T) {
 	g.Expect(c.buildinfoBuildContext.Location).To(Equal(filepath.Join(c.tempWorkdir, "buildinfo")))
 }
 
-func Test_resolveStageRef(t *testing.T) {
+func Test_findMatchingStages(t *testing.T) {
 	g := NewWithT(t)
 
 	tests := []struct {
-		name        string
-		dockerfile  string
-		ref         string
-		expectIndex int
-		expectOk    bool
+		name          string
+		dockerfile    string
+		ref           string
+		expectIndexes []int
+		expectOk      bool
 	}{
 		{
 			name: "match stage by name",
 			dockerfile: strings.Join([]string{
 				"FROM golang:1.21 AS builder",
 				"FROM scratch",
 			}, "\n"),
-			ref:         "builder",
-			expectIndex: 0,
-			expectOk:    true,
+			ref:           "builder",
+			expectIndexes: []int{0},
+			expectOk:      true,
 		},
 		{
 			name: "match stage by numeric index",
 			dockerfile: strings.Join([]string{
 				"FROM golang:1.21 AS builder",
 				"FROM scratch",
 			}, "\n"),
-			ref:         "1",
-			expectIndex: 1,
-			expectOk:    true,
+			ref:           "1",
+			expectIndexes: []int{1},
+			expectOk:      true,
 		},
 		{
-			name:        "no match for unknown name",
-			dockerfile:  "FROM golang:1.21 AS builder\n",
-			ref:         "nonexistent",
-			expectIndex: -1,
-			expectOk:    false,
+			name:          "no match for unknown name",
+			dockerfile:    "FROM golang:1.21 AS builder\n",
+			ref:           "nonexistent",
+			expectIndexes: nil,
+			expectOk:      false,
 		},
 		{
-			name:        "no match for negative index",
-			dockerfile:  "FROM golang:1.21\n",
-			ref:         "-1",
-			expectIndex: -1,
-			expectOk:    false,
+			name:          "no match for negative index",
+			dockerfile:    "FROM golang:1.21\n",
+			ref:           "-1",
+			expectIndexes: nil,
+			expectOk:      false,
 		},
 		{
 			name: "no match for index out of range",
 			dockerfile: strings.Join([]string{
 				"FROM golang:1.21 AS builder",
 				"FROM scratch",
 			}, "\n"),
-			ref:         "2",
-			expectIndex: -1,
-			expectOk:    false,
+			ref:           "2",
+			expectIndexes: nil,
+			expectOk:      false,
+		},
+		{
+			name: "duplicate stage names return multiple indexes",
+			dockerfile: strings.Join([]string{
+				"FROM golang:1.21 AS builder",
+				"RUN echo first",
+				"",
+				"FROM alpine:3.18 AS builder",
+				"RUN echo second",
+				"",
+				"FROM scratch",
+			}, "\n"),
+			ref:           "builder",
+			expectIndexes: []int{0, 1},
+			expectOk:      true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			df := parseDockerfile(t, g, tt.dockerfile)
-			idx, ok := resolveStageRef(df.Stages, tt.ref)
+			indexes, ok := findMatchingStages(df.Stages, tt.ref)
 			g.Expect(ok).To(Equal(tt.expectOk), "expected ok=%v", tt.expectOk)
-			g.Expect(idx).To(Equal(tt.expectIndex), "expected index=%d", tt.expectIndex)
+			g.Expect(indexes).To(Equal(tt.expectIndexes), "expected indexes=%v", tt.expectIndexes)
 		})
 	}
 }
@@ -1715,6 +1730,21 @@ func Test_Build_collectBaseImages(t *testing.T) {
 				"golang:1.21",
 			},
 		},
+		{
+			name: "duplicate stage names: all matching stages are included",
+			dockerfile: strings.Join([]string{
+				"FROM imageA AS builder",
+				"RUN echo first",
+				"",
+				"FROM imageB AS builder",
+				"RUN echo second",
+				"",
+				"FROM scratch",
+				"COPY --from=builder /app /app",
+			}, "\n"),
+			targetStage: 2,
+			expected:    []string{"imageA", "imageB"},
+		},
 		{
 			name: "unused stage not reachable from target is excluded",
 			dockerfile: strings.Join([]string{