Implement cross-platform parsing

cmd/bashbrew/docker.go

@@ -19,11 +19,16 @@ import (
 
 // this returns the "FROM" value for the last stage (which essentially determines the "base" for the final published image)
 func (r Repo) ArchLastStageFrom(arch string, entry *manifest.Manifest2822Entry) (string, error) {
-	dockerfileMeta, err := r.archDockerfileMetadata(arch, entry)
+	parents, err := r.ArchDockerfileParents(arch, entry)
 	if err != nil {
 		return "", err
 	}
-	return dockerfileMeta.StageFroms[len(dockerfileMeta.StageFroms)-1], nil
+	for i := len(parents) - 1; i >= 0; i-- {
+		if parents[i].Kind == "FROM" {
+			return parents[i].From, nil
+		}
+	}
+	return "", fmt.Errorf("no FROM found for arch %q from entry %q", arch, entry.String())
 }
 
 func (r Repo) DockerFroms(entry *manifest.Manifest2822Entry) ([]string, error) {
@@ -38,6 +43,15 @@ func (r Repo) ArchDockerFroms(arch string, entry *manifest.Manifest2822Entry) ([
 	return dockerfileMeta.Froms, nil
 }
 
+// exposes every parent reference (both "FROM" and external "COPY --from="/"RUN --mount=...,from=") tagged with "Kind" and (for "FROM") "Platform", so callers can decide for themselves which are build-pinned vs target-pinned, instead of bashbrew choosing one slice to hand back and leaving the complement as a set-subtraction exercise
+func (r Repo) ArchDockerfileParents(arch string, entry *manifest.Manifest2822Entry) ([]dockerfile.Parent, error) {
+	dockerfileMeta, err := r.archDockerfileMetadata(arch, entry)
+	if err != nil {
+		return nil, err
+	}
+	return dockerfileMeta.Parents, nil
+}
+
 func (r Repo) dockerfileMetadata(entry *manifest.Manifest2822Entry) (dockerfile.Metadata, error) {
 	return r.archDockerfileMetadata(arch, entry)
 }

pkg/dockerfile/parse.go

@@ -2,16 +2,41 @@ package dockerfile
 
 import (
 	"bufio"
+	"fmt"
 	"io"
 	"strconv"
 	"strings"
 	"unicode"
 )
 
+// internal-only bookkeeping used while scanning to resolve "FROM stage-name",
+// "COPY --from=stage-name-or-number", and "RUN --mount=...,from=..." back to
+// real image references; never exposed outside this package
+// TODO stage name/reference (e.g. "build") is available here if a future consumer needs it
+type namedStage struct {
+	from     string
+	platform string
+}
+
+// Parent represents a single parent reference in a Dockerfile: every "FROM"
+// (Kind == "FROM") and every "COPY --from="/"RUN --mount=...,from=" that
+// references an external image rather than a local stage (Kind == "COPY" or
+// "RUN"). A "--from=" referencing a local stage does not get its own Parent,
+// since the underlying image is already represented by that stage's own
+// Kind == "FROM" entry -- and, per BuildKit's own behavior, an external
+// "--from=" reference always resolves against the target platform (there is
+// no way to make it inherit a stage's platform, so Platform is always ""
+// for Kind != "FROM").
+type Parent struct {
+	From string
+	// "" (also covers an explicit "$TARGETPLATFORM", normalized away here --
+	// they're semantically identical) or "$BUILDPLATFORM"; always "" for Kind != "FROM"
+	Platform string `json:",omitempty"`
+	Kind     string // "FROM", "COPY", "RUN"
+}
+
 type Metadata struct {
-	StageFroms     []string          // every image "FROM" instruction value (or the parent stage's FROM value in the case of a named stage)
-	StageNames     []string          // the name of any named stage (in order)
-	StageNameFroms map[string]string // map of stage names to FROM values (or the parent stage's FROM value in the case of a named stage), useful for resolving stage names to FROM values
+	Parents []Parent
 
 	Froms []string // every "FROM" or "COPY --from=xxx" value (minus named and/or numbered stages in the case of "--from=")
 }
@@ -21,10 +46,26 @@ func Parse(dockerfile string) (Metadata, error) {
 }
 
 func ParseReader(dockerfile io.Reader) (Metadata, error) {
-	meta := Metadata{
-		// panic: assignment to entry in nil map
-		StageNameFroms: map[string]string{},
-		// (nil slices work fine)
+	var meta Metadata
+
+	// parsing-time-only bookkeeping (see "namedStage" above)
+	var stages []namedStage
+	namedStages := map[string]int{}
+
+	// isStage tells callers to skip adding a Parent for this --from= -- a stage reference's
+	// platform (if any, e.g. via "FROM --platform=$BUILDPLATFORM foo AS bar") is already
+	// captured on that stage's own Kind == "FROM" Parent, so recording it again here would
+	// either duplicate or (worse) misrepresent it as target-pinned
+	resolveFrom := func(from string) (resolved string, isStage bool) {
+		if i, ok := namedStages[from]; ok {
+			// see note above regarding stage names in FROM
+			return stages[i].from, true
+		} else if stageNumber, err := strconv.Atoi(from); err == nil && stageNumber < len(stages) {
+			// must be a stage number, we should resolve it too
+			return stages[stageNumber].from, true
+		}
+		// make sure to add ":latest" if it's implied
+		return latestizeRepoTag(from), false
 	}
 
 	scanner := bufio.NewScanner(dockerfile)
@@ -77,76 +118,103 @@ func ParseReader(dockerfile io.Reader) (Metadata, error) {
 
 		instruction := strings.ToUpper(fields[0])
 
-		// TODO balk at ARG / $ in from values
+		args := fields[1:]
 
 		switch instruction {
 		case "FROM":
-			from := fields[1]
-
-			if stageFrom, ok := meta.StageNameFroms[from]; ok {
-				// if this is a valid stage name, we should resolve it back to the original FROM value of that previous stage (we don't care about inter-stage dependencies for the purposes of either tag dependency calculation or tag building -- just how many there are and what external things they require)
-				from = stageFrom
+			var stage namedStage
+			var stageName string
+			explicitPlatform := "" // exactly as written: "", "$BUILDPLATFORM", or "$TARGETPLATFORM" -- kept distinct from "" (unspecified) until after stage-name resolution below, since normalizing "$TARGETPLATFORM" away too early would make an *explicit* "--platform=$TARGETPLATFORM" indistinguishable from "wrote nothing at all", silently inheriting a referenced stage's "$BUILDPLATFORM" instead of erroring on the mismatch
+
+			if platform, ok := strings.CutPrefix(args[0], "--platform="); ok {
+				explicitPlatform = platform
+				args = args[1:]
+				switch explicitPlatform {
+				case "$BUILDPLATFORM", "$TARGETPLATFORM":
+					// explicitly allowed for more efficient cross-compiling (see also condition outside the meta loop to ensure the final stage is either without platform or explicitly --platform=$TARGETPLATFORM)
+				default:
+					return meta, fmt.Errorf("FROM has unsupported --platform=%q -- any --platform must be generic or unspecified for correct dependency calculation", explicitPlatform)
+				}
+			}
+			// normalized for comparison/storage -- "" and "$TARGETPLATFORM" are semantically identical; "explicitPlatform" (unnormalized) is kept for the "was anything written at all" check below and for a more useful error message
+			normalizedPlatform := explicitPlatform
+			if normalizedPlatform == "$TARGETPLATFORM" {
+				normalizedPlatform = ""
 			}
+			stage.platform = normalizedPlatform
 
-			// make sure to add ":latest" if it's implied
-			from = latestizeRepoTag(from)
+			from := args[0]
+			args = args[1:]
 
-			meta.StageFroms = append(meta.StageFroms, from)
-			meta.Froms = append(meta.Froms, from)
+			if strings.ContainsRune(from, '$') {
+				return meta, fmt.Errorf("FROM %q contains invalid/disallowed character '$' -- explicit FROM values are required for dependency calculation", from)
+			}
 
-			if len(fields) == 4 && strings.ToUpper(fields[2]) == "AS" {
-				stageName := fields[3]
-				meta.StageNames = append(meta.StageNames, stageName)
-				meta.StageNameFroms[stageName] = from
+			if i, ok := namedStages[from]; ok {
+				// if this is a valid stage name, we should resolve it back to the original FROM value of that previous stage (we don't care about inter-stage dependencies for the purposes of either tag dependency calculation or tag building -- just how many there are and what external things they require)
+				parent := stages[i]
+				if explicitPlatform == "" {
+					// bare "FROM stage-name" makes no platform assertion of its own -- just continue whatever the referenced stage resolved to
+					stage.platform = parent.platform
+				} else if normalizedPlatform != parent.platform {
+					return meta, fmt.Errorf("FROM %q has --platform=%q but stage %q has --platform=%q", from, explicitPlatform, from, parent.platform)
+				}
+				stage.from = parent.from
+			} else {
+				// make sure to add ":latest" if it's implied
+				stage.from = latestizeRepoTag(from)
 			}
 
+			i := len(stages)
+			if len(args) == 2 && strings.ToUpper(args[0]) == "AS" {
+				stageName = args[1]
+				namedStages[stageName] = i
+			}
+			stages = append(stages, stage)
+
+			meta.Froms = append(meta.Froms, stage.from)
+			meta.Parents = append(meta.Parents, Parent{From: stage.from, Platform: stage.platform, Kind: "FROM"})
+
 		case "COPY":
-			for _, arg := range fields[1:] {
+			for _, arg := range args {
 				if !strings.HasPrefix(arg, "--") {
 					// doesn't appear to be a "flag"; time to bail!
 					break
 				}
-				if !strings.HasPrefix(arg, "--from=") {
+				from, ok := strings.CutPrefix(arg, "--from=")
+				if !ok {
 					// ignore any flags we're not interested in
 					continue
 				}
-				from := arg[len("--from="):]
-
-				if stageFrom, ok := meta.StageNameFroms[from]; ok {
-					// see note above regarding stage names in FROM
-					from = stageFrom
-				} else if stageNumber, err := strconv.Atoi(from); err == nil && stageNumber < len(meta.StageFroms) {
-					// must be a stage number, we should resolve it too
-					from = meta.StageFroms[stageNumber]
-				}
 
-				// make sure to add ":latest" if it's implied
-				from = latestizeRepoTag(from)
-
-				meta.Froms = append(meta.Froms, from)
+				resolved, isStage := resolveFrom(from)
+				meta.Froms = append(meta.Froms, resolved)
+				if !isStage {
+					meta.Parents = append(meta.Parents, Parent{From: resolved, Kind: "COPY"})
+				}
 			}
 
 		case "RUN": // TODO combine this and the above COPY-parsing code somehow sanely
-			for _, arg := range fields[1:] {
+			for _, arg := range args {
 				if !strings.HasPrefix(arg, "--") {
 					// doesn't appear to be a "flag"; time to bail!
 					break
 				}
-				if !strings.HasPrefix(arg, "--mount=") {
+				csv, ok := strings.CutPrefix(arg, "--mount=")
+				if !ok {
 					// ignore any flags we're not interested in
 					continue
 				}
-				csv := arg[len("--mount="):]
 				// TODO more correct CSV parsing
 				fields := strings.Split(csv, ",")
 				var mountType, from string
 				for _, field := range fields {
-					if strings.HasPrefix(field, "type=") {
-						mountType = field[len("type="):]
+					if val, ok := strings.CutPrefix(field, "type="); ok {
+						mountType = val
 						continue
 					}
-					if strings.HasPrefix(field, "from=") {
-						from = field[len("from="):]
+					if val, ok := strings.CutPrefix(field, "from="); ok {
+						from = val
 						continue
 					}
 				}
@@ -155,21 +223,26 @@ func ParseReader(dockerfile io.Reader) (Metadata, error) {
 					continue
 				}
 
-				if stageFrom, ok := meta.StageNameFroms[from]; ok {
-					// see note above regarding stage names in FROM
-					from = stageFrom
-				} else if stageNumber, err := strconv.Atoi(from); err == nil && stageNumber < len(meta.StageFroms) {
-					// must be a stage number, we should resolve it too
-					from = meta.StageFroms[stageNumber]
+				resolved, isStage := resolveFrom(from)
+				meta.Froms = append(meta.Froms, resolved)
+				if !isStage {
+					meta.Parents = append(meta.Parents, Parent{From: resolved, Kind: "RUN"})
 				}
-
-				// make sure to add ":latest" if it's implied
-				from = latestizeRepoTag(from)
-
-				meta.Froms = append(meta.Froms, from)
 			}
 		}
 	}
+
+	// TODO maybe we *shouldn't* support parsing a fully empty Dockerfile? 🤔 (we actively use an "empty" Dockerfile in the tests to test edge cases of continuation though that are otherwise hard to test, so it's probably ~fine)
+	if len(stages) > 0 {
+		finalStage := stages[len(stages)-1]
+		switch finalStage.platform {
+		case "":
+			// yay, all is well (note: "$TARGETPLATFORM" is normalized to "" above, so this covers both)
+		default:
+			return meta, fmt.Errorf("final stage/FROM (%q) has --platform=%q but must be unspecified or $TARGETPLATFORM", finalStage.from, finalStage.platform)
+		}
+	}
+
 	return meta, scanner.Err()
 }