diff --git a/cmd/bashbrew/docker.go b/cmd/bashbrew/docker.go index 596d06c4..b71c66e7 100644 --- a/cmd/bashbrew/docker.go +++ b/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) } diff --git a/pkg/dockerfile/parse.go b/pkg/dockerfile/parse.go index cba566c8..1249daaa 100644 --- a/pkg/dockerfile/parse.go +++ b/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() } diff --git a/pkg/dockerfile/parse_test.go b/pkg/dockerfile/parse_test.go index e4cd31c9..03a5201f 100644 --- a/pkg/dockerfile/parse_test.go +++ b/pkg/dockerfile/parse_test.go @@ -12,23 +12,27 @@ func TestParse(t *testing.T) { name string dockerfile string metadata dockerfile.Metadata + wantErr bool }{ { dockerfile: `FROM scratch`, metadata: dockerfile.Metadata{ - Froms: []string{"scratch"}, + Froms: []string{"scratch"}, + Parents: []dockerfile.Parent{{From: "scratch", Kind: "FROM"}}, }, }, { dockerfile: `from bash`, metadata: dockerfile.Metadata{ - Froms: []string{"bash:latest"}, + Froms: []string{"bash:latest"}, + Parents: []dockerfile.Parent{{From: "bash:latest", Kind: "FROM"}}, }, }, { dockerfile: `fRoM bash:5`, metadata: dockerfile.Metadata{ - Froms: []string{"bash:5"}, + Froms: []string{"bash:5"}, + Parents: []dockerfile.Parent{{From: "bash:5", Kind: "FROM"}}, }, }, { @@ -48,6 +52,10 @@ func TestParse(t *testing.T) { `, metadata: dockerfile.Metadata{ Froms: []string{"scratch", "bash:latest"}, + Parents: []dockerfile.Parent{ + {From: "scratch", Kind: "FROM"}, + {From: "bash:latest", Kind: "FROM"}, + }, }, }, { @@ -63,16 +71,21 @@ func TestParse(t *testing.T) { COPY --from=bar / / COPY --from=foo2 / / COPY --chown=1234:5678 /foo /bar + COPY --from=hello-world /hello /usr/local/bin/ `, metadata: dockerfile.Metadata{ - StageFroms: []string{"bash:latest", "busybox:uclibc", "bash:5", "bash:latest", "scratch"}, - StageNames: []string{"foo", "bar", "foo2"}, - StageNameFroms: map[string]string{ - "foo": "bash:latest", - "bar": "bash:5", - "foo2": "bash:latest", + Froms: []string{"bash:latest", "busybox:uclibc", "bash:5", "bash:latest", "scratch", "bash:latest", "bash:5", "bash:latest", "hello-world:latest"}, + Parents: []dockerfile.Parent{ + // one per FROM (including "foo2", resolved to "foo"'s underlying image) + {From: "bash:latest", Kind: "FROM"}, + {From: "busybox:uclibc", Kind: "FROM"}, + {From: "bash:5", Kind: "FROM"}, + {From: "bash:latest", Kind: "FROM"}, + {From: "scratch", Kind: "FROM"}, + // COPY --from=foo/bar/foo2 all reference local stages, already represented above -- no Parent + // COPY --chown=... has no --from= at all + {From: "hello-world:latest", Kind: "COPY"}, }, - Froms: []string{"bash:latest", "busybox:uclibc", "bash:5", "bash:latest", "scratch", "bash:latest", "bash:5", "bash:latest"}, }, }, { @@ -111,6 +124,10 @@ func TestParse(t *testing.T) { `, metadata: dockerfile.Metadata{ Froms: []string{"scratch", "scratch"}, + Parents: []dockerfile.Parent{ + {From: "scratch", Kind: "FROM"}, + {From: "scratch", Kind: "FROM"}, + }, }, }, { @@ -127,8 +144,14 @@ func TestParse(t *testing.T) { RUN --mount=type=bind,from=2 cat /foo `, metadata: dockerfile.Metadata{ - StageFroms: []string{"bash:latest", "scratch", "scratch", "bash:latest"}, - Froms: []string{"bash:latest", "scratch", "bash:latest", "scratch", "scratch", "bash:latest", "scratch"}, + Froms: []string{"bash:latest", "scratch", "bash:latest", "scratch", "scratch", "bash:latest", "scratch"}, + Parents: []dockerfile.Parent{ + // COPY/RUN --from= all reference local stages, already represented here -- no separate Parent + {From: "bash:latest", Kind: "FROM"}, + {From: "scratch", Kind: "FROM"}, + {From: "scratch", Kind: "FROM"}, + {From: "bash:latest", Kind: "FROM"}, + }, }, }, { @@ -138,8 +161,11 @@ func TestParse(t *testing.T) { RUN --mount=type=bind,from=busybox:uclibc,target=/tmp ["/tmp/bin/sh","-euxc","echo foo > /foo"] `, metadata: dockerfile.Metadata{ - StageFroms: []string{"scratch"}, - Froms: []string{"scratch", "busybox:uclibc"}, + Froms: []string{"scratch", "busybox:uclibc"}, + Parents: []dockerfile.Parent{ + {From: "scratch", Kind: "FROM"}, + {From: "busybox:uclibc", Kind: "RUN"}, + }, }, }, { @@ -152,26 +178,54 @@ func TestParse(t *testing.T) { RUN --mount=type=bind,from=bb,target=/tmp ["/tmp/bin/sh","-euxc","echo foo > /foo"] `, metadata: dockerfile.Metadata{ - StageFroms: []string{"busybox:uclibc", "scratch"}, - StageNames: []string{"bb"}, - StageNameFroms: map[string]string{"bb": "busybox:uclibc"}, - Froms: []string{"busybox:uclibc", "scratch", "busybox:uclibc"}, + Froms: []string{"busybox:uclibc", "scratch", "busybox:uclibc"}, + Parents: []dockerfile.Parent{ + // RUN --mount=...,from=bb references a local stage, already represented here -- no separate Parent + {From: "busybox:uclibc", Kind: "FROM"}, + {From: "scratch", Kind: "FROM"}, + }, }, }, + { + name: "FROM --platform", + dockerfile: ` + FROM --platform=$BUILDPLATFORM golang AS build + RUN do some stuff + FROM --platform=$TARGETPLATFORM debian + COPY --from=build /some/binary /some/other/place + `, + metadata: dockerfile.Metadata{ + Froms: []string{"golang:latest", "debian:latest", "golang:latest"}, + Parents: []dockerfile.Parent{ + {From: "golang:latest", Platform: "$BUILDPLATFORM", Kind: "FROM"}, + // explicit "$TARGETPLATFORM" is normalized to "" -- semantically identical, no reason to make callers treat two values as equivalent + {From: "debian:latest", Kind: "FROM"}, + // COPY --from=build references a local stage, already represented above -- no separate Parent + }, + }, + }, + { + name: "FROM --platform mismatch with referenced stage", + dockerfile: ` + FROM --platform=$BUILDPLATFORM golang AS build + FROM --platform=$TARGETPLATFORM build + `, + wantErr: true, + }, } { td := td // some light normalization if td.name == "" { td.name = td.dockerfile } - if len(td.metadata.Froms) > 0 && len(td.metadata.StageFroms) == 0 { - td.metadata.StageFroms = td.metadata.Froms - } - if td.metadata.StageNameFroms == nil { - td.metadata.StageNameFroms = map[string]string{} - } t.Run(td.name, func(t *testing.T) { parsed, err := dockerfile.Parse(td.dockerfile) + if td.wantErr { + if err == nil { + t.Fatalf("expected an error, got:\n%#v", parsed) + } + return + } if err != nil { t.Fatal(err) }