From fd4620264fc0c5a00c189bf1efa3dd9ffaf9cf9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Fern=C3=A1ndez?= Date: Wed, 4 Nov 2026 17:10:57 -0300 Subject: [PATCH 1/7] fix(docker): pre-pull the Dockerfile syntax frontend image (#150) * fix(docker): pre-pull the Dockerfile syntax frontend image BuildImage builds through ImageBuild with no BuildKit session, and BuildKit before v0.31 (Docker Engine before 29.6) refuses registry access without one, even for anonymous public pulls: a bearer challenge with no session fails with "no active sessions". FROM images were already pre-pulled through the classic pull API for this reason; a `# syntax=` frontend was not, so any user Dockerfile naming one failed before parsing (#148). Pre-pull the frontend too: the BUILDKIT_SYNTAX build arg, else the syntax parser directive, read as BuildKit's parser.DetectSyntax reads it (leading directive lines only, # or // comments, after an optional BOM and shebang; the image is the value's first word). Verified against a Docker 29.5.0 daemon (containerd image store): the new integration test fails there with the reported error without this change and passes with it. Docker 29.6+ resolves anonymously without a session, so it passes there either way. Fixes #148. Co-Authored-By: Claude Opus 5.5 * fix(docker): pre-pull a frontend named by a JSON syntax document BuildKit's parser.DetectSyntax falls back, after the # and // directive forms, to a JSON object with a string "syntax" member, the input of custom frontends that take JSON, and uses the whole value as the frontend image. syntaxFrontend left that form out, so its frontend was not pre-pulled and hit the session-less resolution failure on engines before 29.6 (DAP review finding 3891 on #150). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: bilby91 <2201079+bilby91@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 8 +++ runtime/docker/build.go | 101 ++++++++++++++++++++++++++++++--- runtime/docker/build_test.go | 37 ++++++++++++ test/integration/build_test.go | 35 ++++++++++++ 4 files changed, 172 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68adca8..292fd04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 discovered `devcontainer.json`'s directory for local features and the compose working directory. Previously they always assumed `.devcontainer/`, which was wrong for a root `.devcontainer.json`. +- **runtime/docker** — a user Dockerfile that names a frontend with + `# syntax=` (or the `BUILDKIT_SYNTAX` build arg) no longer fails with + `failed to resolve source metadata for docker.io/docker/dockerfile:1: + no active sessions` on Docker Engine before 29.6. `BuildImage` builds + without a BuildKit session, and BuildKit before v0.31 refuses registry + access without one. The frontend image is now pre-pulled through the + classic pull API, as `FROM` images already were. The directive is read + the way BuildKit's parser reads it. (#148) ## [0.5.3] - 2026-10-28 diff --git a/runtime/docker/build.go b/runtime/docker/build.go index 3daf3d9..a679666 100644 --- a/runtime/docker/build.go +++ b/runtime/docker/build.go @@ -2,6 +2,7 @@ package docker import ( "archive/tar" + "bytes" "context" "encoding/json" "fmt" @@ -9,8 +10,10 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "strings" "time" + "unicode" "github.com/moby/moby/api/types/build" "github.com/moby/moby/client" @@ -258,20 +261,32 @@ func tarDirectory(dir string, w io.Writer) error { } // prePullBaseImages reads the Dockerfile at contextPath/dockerfile, -// extracts FROM image references (resolving simple $ARG substitutions -// against ARG defaults in the file and the spec's BuildArgs -// overrides), and pulls each that isn't already in the local image -// store. Best-effort: any pull error is silently ignored — the -// subsequent ImageBuild surfaces a clearer "failed to resolve" error -// if the image really can't be obtained, and images that exist only -// locally (e.g. dc-go-base-*) correctly skip the pull attempt because -// ImageInspect finds them. +// extracts its frontend image (a `# syntax=` directive or the +// BUILDKIT_SYNTAX build arg) and FROM image references (resolving +// simple $ARG substitutions against ARG defaults in the file and the +// spec's BuildArgs overrides), and pulls each that isn't already in +// the local image store. The frontend needs it as much as FROM +// images: BuildKit resolves it from the registry before parsing the +// Dockerfile, under the same session requirement. Best-effort: any +// pull error is silently ignored — the subsequent ImageBuild surfaces +// a clearer "failed to resolve" error if the image really can't be +// obtained, and images that exist only locally (e.g. dc-go-base-*) +// correctly skip the pull attempt because ImageInspect finds them. +// +// BuildKit releases since v0.31 (Docker Engine 29.6) fall back to +// anonymous registry access when no session is attached, so this only +// changes the outcome on older engines; there it is what makes public +// images resolvable at all. func prePullBaseImages(ctx context.Context, r *Runtime, contextPath, dockerfile string, buildArgs map[string]string, events chan<- runtime.BuildEvent) { df, err := os.ReadFile(filepath.Join(contextPath, dockerfile)) if err != nil { return } - for _, ref := range extractBaseImages(string(df), buildArgs) { + refs := extractBaseImages(string(df), buildArgs) + if frontend := syntaxFrontend(df, buildArgs); frontend != "" { + refs = append([]string{frontend}, refs...) + } + for _, ref := range refs { if _, err := r.api.ImageInspect(ctx, ref); err == nil { continue } @@ -358,6 +373,74 @@ func extractBaseImages(content string, buildArgs map[string]string) []string { return out } +// syntaxFrontend returns the frontend image BuildKit will run for a +// Dockerfile, or "" for the built-in one. The BUILDKIT_SYNTAX build +// arg wins over the file; otherwise the `syntax` parser directive is +// read the way BuildKit's parser.DetectSyntax reads it: +// +// - after an optional UTF-8 BOM and a leading `#!` line; +// - only from the leading run of directive lines, each `#` (or `//`) +// at column 0 followed by `key = value` with key one of syntax, +// escape, check (case-insensitive) — the first other line ends the +// run; +// - the image is the value's first word (the rest is frontend +// arguments); +// - failing both comment forms, a file that is a JSON object with a +// string "syntax" member names the image with the whole value (the +// input format of custom frontends that take JSON). +func syntaxFrontend(content []byte, buildArgs map[string]string) string { + if v, ok := buildArgs["BUILDKIT_SYNTAX"]; ok { + return firstWord(v) + } + content = bytes.TrimPrefix(content, []byte{0xEF, 0xBB, 0xBF}) + if first, rest, _ := bytes.Cut(content, []byte("\n")); bytes.HasPrefix(first, []byte("#!")) { + content = rest + } + for _, comment := range []string{"#", "//"} { + if v, ok := directive(content, comment, "syntax"); ok { + return firstWord(v) + } + } + var doc map[string]any + if json.Unmarshal(content, &doc) == nil { + if v, ok := doc["syntax"].(string); ok { + return v + } + } + return "" +} + +// directiveLine matches the body of a parser directive line. +var directiveLine = regexp.MustCompile(`^([a-zA-Z][a-zA-Z0-9]*)\s*=\s*(.+?)\s*$`) + +// directive returns the value of the parser directive key in content's +// leading directive lines written with the given comment prefix. +func directive(content []byte, comment, key string) (string, bool) { + for _, line := range strings.Split(string(content), "\n") { + body, ok := strings.CutPrefix(strings.TrimSuffix(line, "\r"), comment) + if !ok { + return "", false + } + m := directiveLine.FindStringSubmatch(strings.TrimLeftFunc(body, unicode.IsSpace)) + if m == nil { + return "", false + } + switch name := strings.ToLower(m[1]); name { + case key: + return m[2], true + case "syntax", "escape", "check": + default: + return "", false + } + } + return "", false +} + +func firstWord(s string) string { + w, _, _ := strings.Cut(strings.TrimSpace(s), " ") + return w +} + // indexFoldWord returns the byte index of the first occurrence of // `word` in s as a standalone whitespace-delimited token, matching // case-insensitively. Returns -1 if not found. diff --git a/runtime/docker/build_test.go b/runtime/docker/build_test.go index 3b24f41..f5c9899 100644 --- a/runtime/docker/build_test.go +++ b/runtime/docker/build_test.go @@ -203,3 +203,40 @@ func TestSubstituteArgs(t *testing.T) { } } } + +func TestSyntaxFrontend(t *testing.T) { + cases := []struct { + name string + dockerfile string + buildArgs map[string]string + want string + }{ + {"none", "FROM alpine:3.20\n", nil, ""}, + {"directive", "# syntax=docker/dockerfile:1\nFROM alpine:3.20\n", nil, "docker/dockerfile:1"}, + {"spaced and uppercase key", "# SYNTAX = docker/dockerfile:1.7-labs \nFROM alpine\n", nil, "docker/dockerfile:1.7-labs"}, + {"frontend arguments", "# syntax=registry.example.com/fe:1 --opt=x\nFROM alpine\n", nil, "registry.example.com/fe:1"}, + {"after other directives", "# escape=`\n# check=skip=all\n# syntax=docker/dockerfile:1\nFROM alpine\n", nil, "docker/dockerfile:1"}, + {"after shebang", "#!/usr/bin/env buildctl\n# syntax=docker/dockerfile:1\nFROM alpine\n", nil, "docker/dockerfile:1"}, + {"after BOM", "\xef\xbb\xbf# syntax=docker/dockerfile:1\nFROM alpine\n", nil, "docker/dockerfile:1"}, + {"CRLF", "# syntax=docker/dockerfile:1\r\nFROM alpine\r\n", nil, "docker/dockerfile:1"}, + {"c-style comment", "// syntax=docker/dockerfile:1\nFROM alpine\n", nil, "docker/dockerfile:1"}, + {"after a plain comment", "# build the app\n# syntax=docker/dockerfile:1\nFROM alpine\n", nil, ""}, + {"after a blank line", "\n# syntax=docker/dockerfile:1\nFROM alpine\n", nil, ""}, + {"indented", " # syntax=docker/dockerfile:1\nFROM alpine\n", nil, ""}, + {"after an instruction", "FROM alpine\n# syntax=docker/dockerfile:1\n", nil, ""}, + {"build arg wins", "# syntax=docker/dockerfile:1\nFROM alpine\n", map[string]string{"BUILDKIT_SYNTAX": " docker/dockerfile:1.9 --x"}, "docker/dockerfile:1.9"}, + {"build arg without directive", "FROM alpine\n", map[string]string{"BUILDKIT_SYNTAX": "docker/dockerfile:1"}, "docker/dockerfile:1"}, + {"json", "{\"syntax\": \"registry.example.com/fe:2\", \"stages\": []}\n", nil, "registry.example.com/fe:2"}, + {"json after shebang", "#!/usr/bin/env x\n{\"syntax\": \"registry.example.com/fe:2\"}", nil, "registry.example.com/fe:2"}, + {"json without syntax", "{\"stages\": []}", nil, ""}, + {"json non-string syntax", "{\"syntax\": 1}", nil, ""}, + {"comment directive wins over json", "# syntax=docker/dockerfile:1\n{\"syntax\": \"x\"}", nil, "docker/dockerfile:1"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := syntaxFrontend([]byte(tc.dockerfile), tc.buildArgs); got != tc.want { + t.Errorf("syntaxFrontend = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/test/integration/build_test.go b/test/integration/build_test.go index d809f05..30f2564 100644 --- a/test/integration/build_test.go +++ b/test/integration/build_test.go @@ -149,6 +149,41 @@ func TestEngineBuild_BuildSource_NoFeatures(t *testing.T) { t.Cleanup(func() { _ = rt.RemoveImage(context.Background(), tag) }) } +// TestEngineBuild_BuildSource_SyntaxDirective: a user Dockerfile that +// names a frontend image with `# syntax=` and uses syntax only that +// frontend parses (COPY --chmod, RUN --mount). On Docker Engine +// before 29.6, BuildKit cannot resolve the frontend from the registry +// without a session (#148); the engine pre-pulls it. +func TestEngineBuild_BuildSource_SyntaxDirective(t *testing.T) { + if testing.Short() { + t.Skip() + } + + eng, rt := newEngine(t) + defer rt.Close() + + dir := t.TempDir() + mustWrite(t, filepath.Join(dir, "Dockerfile"), "# syntax=docker/dockerfile:1\n"+ + "FROM "+testImage+"\n"+ + "COPY --chmod=0755 hello.sh /usr/local/bin/hello\n"+ + "RUN --mount=type=tmpfs,target=/scratch hello > /etc/marker\n") + mustWrite(t, filepath.Join(dir, "hello.sh"), "#!/bin/sh\necho syntax-directive\n") + mustWrite(t, filepath.Join(dir, ".devcontainer", "devcontainer.json"), + `{"build": {"dockerfile": "Dockerfile", "context": ".."}}`) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + const tag = "dc-go-build-test-syntax:v1" + if _, err := eng.Build(ctx, devcontainer.BuildOptions{ + LocalWorkspaceFolder: dir, + ImageName: tag, + }); err != nil { + t.Fatalf("Build: %v", err) + } + t.Cleanup(func() { _ = rt.RemoveImage(context.Background(), tag) }) +} + // TestEngineBuild_ComposeRefused: compose sources are refused with a // clear error rather than silently doing nothing. func TestEngineBuild_ComposeRefused(t *testing.T) { From ceaba2070fa6783221a83bdebf3a142470b3e066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Fern=C3=A1ndez?= Date: Wed, 4 Nov 2026 17:32:42 -0300 Subject: [PATCH 2/7] fix(features): enforce lockfile pins on baked features and empty configs (#153) * fix(features): enforce lockfile pins on baked features and empty configs Two findings from the review of #146: - A feature the base image's label satisfied was skipped and its existing lock entry copied, so frozen mode accepted a base image built from another artifact, or one the lockfile had no entry for. The label already records each feature's resolvedRef, but ParseMetadataLabel dropped it (and so did carrying base entries into a rebuilt label). Keep it on FeatureMetadata, read the lockfile before matching the label, and accept a baked feature only when its artifact is the pinned one or, with no pin, a recorded one, which is then locked. Anything else is fetched at its pin and installed. LockfileDisabled keeps the label-only match. - layerFeatures returned before the lockfile when no features were declared, so removing the last feature left its entry behind and frozen mode accepted it. Reconcile an existing lockfile then too: default and upgrade empty it, frozen reports the stale entries. Without a lockfile nothing is created, as with the reference CLI. The pre-baked hot path is unchanged for images this library built, whose labels have recorded resolvedRef since v0.1.0. Co-Authored-By: Claude Opus 5.5 * fix(features): re-resolve baked features in lockfile upgrade mode acceptsBaked found no pin in LockfileUpgrade (pin ignores the lockfile there) and so accepted any recorded baked artifact, which commit then locked: an upgrade could replace a newer pin with the older artifact the base image carries (DAP review finding 3902 on #153). Upgrade mode now accepts no baked feature, so each reference is resolved afresh. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: bilby91 <2201079+bilby91@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 9 ++- config/resolved.go | 6 ++ design/features.md | 25 ++++++-- engine_test.go | 2 +- feature/dockerfile_test.go | 27 +++++++++ feature/metadata.go | 4 +- lockfile.go | 53 ++++++++++++++-- lockfile_test.go | 120 ++++++++++++++++++++++++++++++++----- up.go | 19 +++--- 9 files changed, 230 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 292fd04..84a8f53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (mutually exclusive), named as in the reference CLI. - The file is byte-compatible with the CLI's, and a `.devcontainer.json` gets a `.devcontainer-lock.json`. Local features are not recorded. - - Features baked into the base image keep their existing entry. + - A feature baked into the base image is used only if the artifact its + `devcontainer.metadata` label entry records is the locked one (or, + with no pin, is recorded at all, and is then locked); otherwise it + is fetched at its pin and installed. `LockfileUpgrade` always + re-resolves baked features. The label's `resolvedRef` now + survives parsing and rebuilds on top of the image. + - Removing the last feature empties the lockfile; frozen mode reports + the stale entries. - Compose reattach does not consult the lockfile. - See design/features.md §8.2. diff --git a/config/resolved.go b/config/resolved.go index a5f91e1..8c50052 100644 --- a/config/resolved.go +++ b/config/resolved.go @@ -257,6 +257,12 @@ type FeatureMetadata struct { DocumentationURL string LicenseURL string + // ResolvedRef is the pinned artifact a label entry records for the + // feature it installed (ResolvedFeature.ResolvedRef at build time). + // Label-only, and empty when the image was built by a tool that does + // not record it. + ResolvedRef string + Options map[string]FeatureOption // Mergeable surface (devpod parity). Empty / nil means "unset". diff --git a/design/features.md b/design/features.md index 3085114..3d4dcc2 100644 --- a/design/features.md +++ b/design/features.md @@ -369,7 +369,9 @@ sha256. So the lockfile needs no store change: - **HTTPS pin:** the URL is fetched as before and its digest is compared with `integrity` (`*feature.FeatureIntegrityError` on mismatch). - **Write:** after the fetch loop and before the build, as the CLI does. - The lockfile is regenerated from scratch, so stale entries drop. It is + The lockfile is regenerated from scratch, so stale entries drop, + including when the last feature is removed (frozen mode reports them; + a config without features and without a lockfile gets none). It is compared by content, not bytes, and written only when entries changed. Decisions: @@ -382,10 +384,23 @@ Decisions: fixtures). The one divergence is key order for keys where bytewise order and `localeCompare` disagree. Comparing by content keeps either tool from rewriting the other's file over it. -2. **Features baked into the base image keep their existing entry.** - They are not fetched, so they produce no new entry. Dropping theirs - would churn the file on every pre-baked run and always fail frozen - mode. +2. **A baked feature stands in for a fetch only when the lockfile + accepts its artifact.** The base image's label records each + feature's `resolvedRef` (this library has written it since v0.1.0), + and the label match additionally requires that artifact to be the + pinned one, or, with no pin, a known one that is then locked. A + baked feature of a different or unrecorded artifact is fetched at + its pin and installed over the base image, so frozen mode never + accepts a base image the lockfile does not describe. An accepted + feature keeps its pin rather than being re-locked from the label, + which does not record `dependsOn`. Upgrade mode accepts no baked + feature: it resolves every reference afresh, and locking the baked + artifact could replace a newer pin with an older one. With the + lockfile disabled the label match alone decides, as before. + + Labels written by other tools record no artifact. The reference + CLI's never matched here anyway: their `id` is the full reference, + not the bare id the match compares. 3. **Compose reattach neither reads nor writes the lockfile, and the lockfile is not part of the compose inputs hash.** Reattach resolves nothing, matching the CLI, which does not re-resolve features when it diff --git a/engine_test.go b/engine_test.go index e3cb445..dff7878 100644 --- a/engine_test.go +++ b/engine_test.go @@ -557,7 +557,7 @@ func TestUp_PrebakedImageSkipsFeatureFetch(t *testing.T) { ID: "sha256:base", Tags: []string{"alpine:3.20"}, Labels: map[string]string{ - "devcontainer.metadata": `[{"id":"pre-baked-feature","version":"1.5.0"}]`, + "devcontainer.metadata": `[{"id":"pre-baked-feature","version":"1.5.0","resolvedRef":"ghcr.io/x/pre-baked-feature@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}]`, }, } eng, _ := New(EngineOptions{Runtime: rt}) diff --git a/feature/dockerfile_test.go b/feature/dockerfile_test.go index 6f12f13..0d04f84 100644 --- a/feature/dockerfile_test.go +++ b/feature/dockerfile_test.go @@ -226,6 +226,33 @@ func TestParseMetadataLabel_RoundTrip(t *testing.T) { } } +// TestParseMetadataLabel_ResolvedRefSurvivesRebuild: the artifact a +// label entry records is what the lockfile checks a baked feature +// against, so it must survive parsing and being carried into the label +// of an image built on top. +func TestParseMetadataLabel_ResolvedRefSurvivesRebuild(t *testing.T) { + const ref = "ghcr.io/x/git@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + base, err := ParseMetadataLabel(`[{"id":"git","version":"1.0","resolvedRef":"` + ref + `"}]`) + if err != nil { + t.Fatal(err) + } + if base[0].ResolvedRef != ref { + t.Fatalf("parsed ResolvedRef = %q, want %q", base[0].ResolvedRef, ref) + } + raw, err := buildMetadataLabel(BuildPlan{BaseImage: "base", BaseImageMetadata: base, + Features: []config.ResolvedFeature{fixtureFeature(t, "node", "20")}}) + if err != nil { + t.Fatal(err) + } + rebuilt, err := ParseMetadataLabel(string(raw)) + if err != nil { + t.Fatal(err) + } + if rebuilt[0].ID != "git" || rebuilt[0].ResolvedRef != ref { + t.Errorf("carried entry = %+v, want git at %s", rebuilt[0], ref) + } +} + func TestParseMetadataLabel_PreservesFinalEntryFields(t *testing.T) { // Reference behavior: the final no-ID resolved-config entry carries // remoteUser/containerUser. ParseMetadataLabel must round-trip them diff --git a/feature/metadata.go b/feature/metadata.go index 2d6fd0c..db8484d 100644 --- a/feature/metadata.go +++ b/feature/metadata.go @@ -100,6 +100,7 @@ func metadataFromFeatureMetadata(m config.FeatureMetadata) metadataEntry { out := metadataEntry{ ID: m.ID, Version: m.Version, + ResolvedRef: m.ResolvedRef, RemoteUser: m.RemoteUser, ContainerUser: m.ContainerUser, UserEnvProbe: string(m.UserEnvProbe), @@ -205,7 +206,7 @@ func ParseMetadataLabel(label string) ([]config.FeatureMetadata, error) { if err := json.Unmarshal([]byte(label), &single); err != nil { return nil, err } - return []config.FeatureMetadata{{ID: single.ID, Version: single.Version}}, nil + return []config.FeatureMetadata{{ID: single.ID, Version: single.Version, ResolvedRef: single.ResolvedRef}}, nil } var entries []metadataEntry if err := json.Unmarshal([]byte(label), &entries); err != nil { @@ -226,6 +227,7 @@ func (e metadataEntry) toFeatureMetadata() (config.FeatureMetadata, error) { out := config.FeatureMetadata{ ID: e.ID, Version: e.Version, + ResolvedRef: e.ResolvedRef, RemoteUser: e.RemoteUser, ContainerUser: e.ContainerUser, UserEnvProbe: config.UserEnvProbe(e.UserEnvProbe), diff --git a/lockfile.go b/lockfile.go index bd9e3bf..de6d933 100644 --- a/lockfile.go +++ b/lockfile.go @@ -65,22 +65,45 @@ func (l *featureLock) pin(f config.ResolvedFeature) (feature.LockedFeature, bool return l.existing.Entry(f.Ref) } +// acceptsBaked reports whether the base image's installation of f, +// recorded by the label entry baked, may stand in for fetching f. With +// the lockfile disabled the label match alone decides. Otherwise the +// baked artifact must be the one the lockfile pins, or, with no pin, +// one the label records, so that it can be locked. Upgrade mode accepts +// none: it resolves every reference afresh, and locking the baked +// artifact could replace a newer pin with an older one. A baked feature +// that is not accepted is fetched and installed like any other; the +// label records no artifact when another tool built the image. +func (l *featureLock) acceptsBaked(f config.ResolvedFeature, baked config.FeatureMetadata) bool { + switch l.mode { + case LockfileDisabled: + return true + case LockfileUpgrade: + return false + } + if baked.ResolvedRef == "" { + return false + } + pin, pinned := l.pin(f) + return !pinned || feature.CheckIntegrity(f, feature.Fetched{ResolvedRef: baked.ResolvedRef}, pin) == nil +} + // commit records what features resolved to: written in default and // upgrade modes when it differs from the existing lockfile, compared // in frozen mode. // -// Features the base image already carries were not fetched, so there -// is nothing new to record for them: their existing entry is kept. +// A feature the base image carries keeps its pin, which acceptsBaked +// checked it against; the label does not record dependsOn, so an entry +// rebuilt from it could differ. Without a pin it is locked from the +// artifact the label records. func (l *featureLock) commit(features []config.ResolvedFeature) error { if l.mode == LockfileDisabled { return nil } next := &feature.Lockfile{Features: map[string]feature.LockedFeature{}} for _, f := range features { - if f.AlreadyInstalled { - if entry, ok := l.existing.Entry(f.Ref); ok { - next.Features[f.Ref] = entry - } + if pin, pinned := l.pin(f); f.AlreadyInstalled && pinned { + next.Features[f.Ref] = pin continue } if entry, ok := feature.Lock(f); ok { @@ -95,3 +118,21 @@ func (l *featureLock) commit(features []config.ResolvedFeature) error { } return feature.WriteLockfile(l.path, next) } + +// commitNoFeatures reconciles the lockfile of a configuration that +// declares no features. Nothing is pinned, but entries left by removed +// features are stale: default and upgrade modes empty the lockfile and +// frozen mode reports them. Without a lockfile there is nothing to do +// in any mode, as with the reference CLI. +func commitNoFeatures(mode LockfileMode, configPath string) error { + if mode == LockfileDisabled { + return nil + } + l := &featureLock{mode: mode, path: feature.LockfilePath(configPath)} + existing, err := feature.ReadLockfile(l.path) + if err != nil || existing == nil { + return err + } + l.existing = existing + return l.commit(nil) +} diff --git a/lockfile_test.go b/lockfile_test.go index b6f24b6..5dfe078 100644 --- a/lockfile_test.go +++ b/lockfile_test.go @@ -244,26 +244,118 @@ func TestLockfile_HTTPSIntegrityMismatchFails(t *testing.T) { } } -// TestLockfile_KeepsEntryForFeatureBakedIntoBaseImage: a feature the -// base image's metadata label satisfies is not fetched, so its entry -// carries over rather than being dropped (which frozen mode would -// then reject on every run). -func TestLockfile_KeepsEntryForFeatureBakedIntoBaseImage(t *testing.T) { - f := newLockFixture(t, `{"ghcr.io/x/a:1":{},"ghcr.io/x/b:1":{}}`) - f.rt.imagesByRef["alpine:3.20"] = &runtime.ImageDetails{Labels: map[string]string{ - feature.MetadataLabel: `[{"id":"a","version":"1.0.0"}]`, +// TestLockfile_BakedFeature: a feature the base image's metadata label +// carries stands in for a fetch only when the lockfile accepts the +// artifact the label records: the pinned one, or with no pin a known +// one, which is then locked. Anything else is fetched (at the pin). +func TestLockfile_BakedFeature(t *testing.T) { + baked := func(resolvedRef string) string { + return `[{"id":"a","version":"1.0.0","resolvedRef":"` + resolvedRef + `"}]` + } + pinned := &feature.Lockfile{Features: map[string]feature.LockedFeature{ + "ghcr.io/x/a:1": ociEntry("ghcr.io/x/a", lockDigestOld), }} - locked := &feature.Lockfile{Features: map[string]feature.LockedFeature{ + cases := []struct { + name string + label string + lock *feature.Lockfile + mode LockfileMode + wantFetched []string + wantErr bool + wantLock *feature.Lockfile // nil: not checked + }{ + { + name: "pinned artifact", label: baked("ghcr.io/x/a@" + lockDigestOld), lock: pinned, + mode: LockfileFrozen, wantLock: pinned, + }, + { + name: "other artifact", label: baked("ghcr.io/x/a@" + lockDigestNew), lock: pinned, + mode: LockfileFrozen, wantFetched: []string{"ghcr.io/x/a@" + lockDigestOld}, wantLock: pinned, + }, + { + name: "unrecorded artifact", label: `[{"id":"a","version":"1.0.0"}]`, lock: pinned, + mode: LockfileFrozen, wantFetched: []string{"ghcr.io/x/a@" + lockDigestOld}, wantLock: pinned, + }, + { + name: "no pin locks the baked artifact", label: baked("ghcr.io/x/a@" + lockDigestOld), + mode: LockfileDefault, wantLock: pinned, + }, + { + name: "no pin in frozen mode", label: baked("ghcr.io/x/a@" + lockDigestOld), + lock: &feature.Lockfile{Features: map[string]feature.LockedFeature{}}, mode: LockfileFrozen, wantErr: true, + }, + { + name: "disabled trusts the label", label: `[{"id":"a","version":"1.0.0"}]`, mode: LockfileDisabled, + }, + { + // The tag now resolves to lockDigestNew; an older baked + // artifact must not be locked in its place. + name: "upgrade resolves afresh", label: baked("ghcr.io/x/a@" + lockDigestOld), + lock: &feature.Lockfile{Features: map[string]feature.LockedFeature{ + "ghcr.io/x/a:1": ociEntry("ghcr.io/x/a", lockDigestNew), + }}, + mode: LockfileUpgrade, wantFetched: []string{"ghcr.io/x/a:1"}, + wantLock: &feature.Lockfile{Features: map[string]feature.LockedFeature{ + "ghcr.io/x/a:1": ociEntry("ghcr.io/x/a", lockDigestNew), + }}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newLockFixture(t, `{"ghcr.io/x/a:1":{}}`) + f.rt.imagesByRef["alpine:3.20"] = &runtime.ImageDetails{Labels: map[string]string{feature.MetadataLabel: tc.label}} + if tc.lock != nil { + f.writeLock(tc.lock) + } + err := f.build(tc.mode) + var me *feature.LockfileMismatchError + if tc.wantErr != errors.As(err, &me) || (!tc.wantErr && err != nil) { + t.Fatalf("build: %v, want mismatch error %v", err, tc.wantErr) + } + if !reflect.DeepEqual(f.store.fetched, tc.wantFetched) { + t.Errorf("fetched %v, want %v", f.store.fetched, tc.wantFetched) + } + if tc.wantLock != nil { + if got := f.readLock(); !got.Equal(tc.wantLock) { + t.Errorf("lockfile differs in %v", got.Changed(tc.wantLock)) + } + } + }) + } +} + +// TestLockfile_NoFeatures: removing the last feature leaves entries +// that are stale. Default mode empties the lockfile and frozen mode +// reports them; without a lockfile none is created. +func TestLockfile_NoFeatures(t *testing.T) { + stale := &feature.Lockfile{Features: map[string]feature.LockedFeature{ "ghcr.io/x/a:1": ociEntry("ghcr.io/x/a", lockDigestOld), - "ghcr.io/x/b:1": ociEntry("ghcr.io/x/b", lockDigestOld), }} - f.writeLock(locked) - if err := f.build(LockfileFrozen); err != nil { + f := newLockFixture(t, `{}`) + f.writeLock(stale) + var me *feature.LockfileMismatchError + if err := f.build(LockfileFrozen); !errors.As(err, &me) || !reflect.DeepEqual(me.Changed, []string{"ghcr.io/x/a:1"}) { + t.Fatalf("frozen: got %v, want a mismatch naming ghcr.io/x/a:1", err) + } + if err := f.build(LockfileDefault); err != nil { t.Fatal(err) } - if want := []string{"ghcr.io/x/b@" + lockDigestOld}; !reflect.DeepEqual(f.store.fetched, want) { - t.Errorf("fetched %v, want only b", f.store.fetched) + if got := f.readLock(); got == nil || len(got.Features) != 0 { + t.Errorf("default: lockfile = %+v, want no entries", got) + } + if err := f.build(LockfileFrozen); err != nil { + t.Errorf("frozen after default emptied the lockfile: %v", err) + } + + f = newLockFixture(t, `{}`) + for _, mode := range []LockfileMode{LockfileDefault, LockfileFrozen} { + if err := f.build(mode); err != nil { + t.Errorf("%q without a lockfile: %v", mode, err) + } + } + if _, err := os.Stat(f.lockPath); !errors.Is(err, os.ErrNotExist) { + t.Errorf("a lockfile was created for a config without features: %v", err) } } diff --git a/up.go b/up.go index 2d1a588..51210f1 100644 --- a/up.go +++ b/up.go @@ -397,7 +397,10 @@ func (e *Engine) createFresh(ctx context.Context, cfg *config.ResolvedConfig, op // markAlreadyInstalled flips AlreadyInstalled on each cfg.Features // entry whose request is satisfied by an entry in the base image's // devcontainer.metadata label. The match strategy (permissive vs -// strict) follows EngineOptions.StrictFeatureVersionMatch. +// strict) follows EngineOptions.StrictFeatureVersionMatch, and the +// feature lockfile must accept the baked artifact (see +// featureLock.acceptsBaked); a feature it rejects is fetched and +// installed like any other. // // Pre-baked-image matching only applies to OCI features: the bare id // comparison requires extracting a basename from the request ref, @@ -405,7 +408,7 @@ func (e *Engine) createFresh(ctx context.Context, cfg *config.ResolvedConfig, op // HTTPS URLs or local paths whose disk-side id may differ from the // ref's last path component. Local / HTTPS features always go through // fetch (cheap for local; HTTPS fetch is what gives us the id anyway). -func (e *Engine) markAlreadyInstalled(cfg *config.ResolvedConfig, baked []config.FeatureMetadata) { +func (e *Engine) markAlreadyInstalled(cfg *config.ResolvedConfig, baked []config.FeatureMetadata, lock *featureLock) { mode := feature.MatchPermissive if e.opts.StrictFeatureVersionMatch { mode = feature.MatchStrict @@ -426,11 +429,12 @@ func (e *Engine) markAlreadyInstalled(cfg *config.ResolvedConfig, baked []config continue } for _, b := range baked { - if feature.Matches(b, req, mode) { + if feature.Matches(b, req, mode) && lock.acceptsBaked(*f, b) { f.AlreadyInstalled = true // Preserve baked metadata so the regenerated label can // include it without re-fetching. f.Metadata = b + f.ResolvedRef = b.ResolvedRef break } } @@ -1273,26 +1277,27 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, var baseMeta []config.FeatureMetadata if details, err := e.runtime.InspectImage(ctx, baseImage); err == nil && details != nil { if label := details.Labels[feature.MetadataLabel]; label != "" { - parsed, err := feature.ParseMetadataLabel(label) - if err == nil { + if parsed, err := feature.ParseMetadataLabel(label); err == nil { baseMeta = parsed - e.markAlreadyInstalled(cfg, parsed) } } } if len(cfg.Features) == 0 { - return baseImage, baseMeta, nil + return baseImage, baseMeta, commitNoFeatures(opts.Lockfile, opts.ConfigPath) } // Local features are relative to devcontainer.json; Up and Build // always set ConfigPath. configDir := filepath.Dir(opts.ConfigPath) + // The lockfile decides which baked features stand in for a fetch, + // so it is read before the label is matched. lock, err := loadFeatureLock(opts.Lockfile, opts.ConfigPath) if err != nil { return "", nil, err } + e.markAlreadyInstalled(cfg, baseMeta, lock) // Emit a Skipped event for features the base-image label satisfied // (markAlreadyInstalled has already flipped their AlreadyInstalled From 09a116a9b601180f5296e08c3005617094b69913 Mon Sep 17 00:00:00 2001 From: bilby91 <2201079+bilby91@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:18:26 +0000 Subject: [PATCH 3/7] feat(features): install features reachable only through dependsOn dependsOn only ordered features that devcontainer.json declared anyway: an undeclared dependency was silently skipped, so a feature relying on one installed without it (#147). The spec makes dependsOn a hard dependency, installed recursively with the options it names. - layerFeatures' fetch loop appends each fetched feature's dependsOn requirements and fetches them in turn: lockfile pinning, option validation and defaults apply as for declared features, and they are locked under their dependsOn key as the reference CLI does. - Spec equality decides duplicates: same content (digest) and same options once defaults apply. A repeat of a key with the same options is skipped before fetching, which also ends cycles; the same content under another reference is dropped after. One feature with different options is installed once per set, and topoSort maps an id to all its entries, pointing dependsOn edges at the one with the requested options. - A dependency the base image's label satisfies is not fetched or installed again: bakedFeature, factored from markAlreadyInstalled, includes the lockfile's check of the artifact the label records. Its own dependencies are still followed, as are a declared baked feature's, since an image built before this change lacks them. Labels now record each feature's dependsOn ({} when none); for a label written before that, the baked feature's metadata is fetched at its recorded artifact to find them. - A dependency dropped as a duplicate under another reference is still locked under its own key. - The compose inputs hash also covers a local dependency's content, which the declaring feature's content does not; projects without one hash as before. - OCI references are lowercased before parsing, as the CLI's getRef does: published dependsOn keys such as ghcr.io/codspace/dependson/A failed to parse. The integration test builds the CLI's own lockfile-dependson fixture in frozen mode against its expected lockfile. Closes #147. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 27 ++ README.md | 4 +- config/resolve.go | 8 +- config/resolved.go | 5 +- depends_on_test.go | 372 ++++++++++++++++++++++++++++ design/features.md | 91 ++++++- engine_test.go | 2 +- feature/disk_store.go | 2 +- feature/dockerfile_test.go | 43 ++++ feature/graph.go | 84 +++++-- feature/https.go | 4 +- feature/lockfile.go | 2 +- feature/lockfile_test.go | 1 + feature/metadata.go | 20 ++ feature/oci.go | 11 +- feature/parse.go | 4 +- lockfile_test.go | 6 +- test/integration/depends_on_test.go | 82 ++++++ up.go | 163 ++++++++++-- up_compose_reattach.go | 36 +++ up_compose_reattach_test.go | 19 ++ 21 files changed, 917 insertions(+), 69 deletions(-) create mode 100644 depends_on_test.go create mode 100644 test/integration/depends_on_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 84a8f53..41e558d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 access without one. The frontend image is now pre-pulled through the classic pull API, as `FROM` images already were. The directive is read the way BuildKit's parser reads it. (#148) +- **features** — features that another feature's `dependsOn` requires + are now installed even when `devcontainer.json` does not declare them, + as the spec requires. (#147) + - Previously `dependsOn` only ordered features that were declared + anyway. A feature that relied on an undeclared one installed without + it. + - Dependencies are fetched recursively, get their options validated + and defaults applied, and are locked and pinned in + `devcontainer-lock.json` under their `dependsOn` key, as the + reference CLI does. + - Duplicates are collapsed by spec equality: same content and same + options once defaults apply. One feature requested with different + options is installed once per set of options. + - Declared features that resolve to the same content with the same + options are now installed once rather than twice. + - A dependency the base image already carries (at the artifact the + lockfile accepts) is not installed again, but its own dependencies + are still followed. Image labels now record each feature's + `dependsOn`; for a label written before that, the baked feature's + metadata is fetched at its recorded artifact to find them. + - A dependency dropped as a duplicate of another reference is still + locked under its own key. + - A local dependency's content is part of the compose inputs hash. + - See design/features.md §5.1. +- **features** — OCI feature references are lowercased before they are + parsed, as the reference CLI does. References with uppercase + characters, such as `ghcr.io/codspace/dependson/A:2`, failed to parse. ## [0.5.3] - 2026-10-28 diff --git a/README.md b/README.md index d3f1c54..9d0a80b 100644 --- a/README.md +++ b/README.md @@ -70,9 +70,9 @@ field/behavior the library covers. Legend: ✅ acted on · ⚠️ parsed but not | Field | Status | Notes | | --- | --- | --- | | `features` (OCI / HTTPS / local) | ✅ | DAG ordering, options validation, content-addressed cache (local-first for digests and exact `X.Y.Z` versions; floating tags fall back to the last resolution when the registry is unreachable) | -| `dependsOn` / `installsAfter` / `overrideFeatureInstallOrder` | ✅ | | +| `dependsOn` / `installsAfter` / `overrideFeatureInstallOrder` | ✅ | `dependsOn` features are installed whether declared or not, deduplicated by content and options per the spec | | Pre-baked-image short-circuit | ✅ | `devcontainer.metadata` label read on Up | -| `devcontainer-lock.json` | ✅ | Written and pinned by default (`UpOptions.Lockfile` / `BuildOptions.Lockfile`: frozen, upgrade, disabled), CLI-compatible format. Records declared features only: features reachable only through `dependsOn` are not installed | +| `devcontainer-lock.json` | ✅ | Written and pinned by default (`UpOptions.Lockfile` / `BuildOptions.Lockfile`: frozen, upgrade, disabled), CLI-compatible format, including features pulled in through `dependsOn` | **Lifecycle** diff --git a/config/resolve.go b/config/resolve.go index 0bc7363..ff44fae 100644 --- a/config/resolve.go +++ b/config/resolve.go @@ -227,7 +227,7 @@ func buildPartialFeatures(rawFeatures map[string]json.RawMessage, overrideOrder feature: ResolvedFeature{ Ref: ref, Options: opts, - SourceKind: classifyFeatureRef(ref), + SourceKind: ClassifyFeatureRef(ref), }, }) } @@ -289,12 +289,12 @@ func parseFeatureOptions(raw json.RawMessage, ref, source string) (map[string]an }} } -// classifyFeatureRef maps a features-map key to its source kind based -// on the ref shape. Matches the spec's rules: +// ClassifyFeatureRef maps a features-map or dependsOn key to its source +// kind based on the ref shape. Matches the spec's rules: // - "./..." or "../..." or absolute path → Local // - "https://..." → HTTPS // - everything else → OCI (the canonical "ghcr.io/..." form) -func classifyFeatureRef(ref string) FeatureSourceKind { +func ClassifyFeatureRef(ref string) FeatureSourceKind { switch { case strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") || strings.HasPrefix(ref, "/"): return FeatureSourceLocal diff --git a/config/resolved.go b/config/resolved.go index 8c50052..a84d67f 100644 --- a/config/resolved.go +++ b/config/resolved.go @@ -248,7 +248,10 @@ const ( // // ID/Version/Name/Description/DocumentationURL/LicenseURL/Options/ // InstallsAfter/DependsOn are feature-only — they have no merge semantics -// and are ignored when the layer originates from a label entry. +// and are ignored when the layer originates from a label entry. A label +// entry's DependsOn says which dependencies the image was built with: +// non-nil (possibly empty) when the label records them, nil when it was +// written before they were recorded. type FeatureMetadata struct { ID string Version string diff --git a/depends_on_test.go b/depends_on_test.go new file mode 100644 index 0000000..60565dc --- /dev/null +++ b/depends_on_test.go @@ -0,0 +1,372 @@ +package devcontainer + +import ( + "context" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/crunchloop/devcontainer/config" + "github.com/crunchloop/devcontainer/feature" + "github.com/crunchloop/devcontainer/runtime" +) + +// depStore is a feature.Store serving OCI features from a table keyed +// by repository. A tag resolves to the entry's digest; repo@digest is +// served when it is that digest. +type depStore struct { + dir string + features map[string]depEntry // repo → entry + fetched []string +} + +type depEntry struct { + digest string + meta config.FeatureMetadata +} + +func (s *depStore) Fetch(_ context.Context, ref string, _ config.FeatureSourceKind) (feature.Fetched, error) { + s.fetched = append(s.fetched, ref) + repo := ref + if i := strings.LastIndex(repo, "@"); i >= 0 { + repo = repo[:i] + } else if i := strings.LastIndex(repo, ":"); i > strings.LastIndex(repo, "/") { + repo = repo[:i] + } + e, ok := s.features[repo] + if !ok { + return feature.Fetched{}, errors.New("not found: " + ref) + } + return feature.Fetched{Dir: s.dir, ResolvedRef: repo + "@" + e.digest, Metadata: e.meta}, nil +} + +func digestOf(c byte) string { return "sha256:" + strings.Repeat(string(c), 64) } + +// meta returns feature metadata with id, a "flavor" option defaulting +// to "plain", and the given dependsOn. +func meta(id string, dependsOn map[string]map[string]any) config.FeatureMetadata { + return config.FeatureMetadata{ + ID: id, + Version: "1.0.0", + Options: map[string]config.FeatureOption{"flavor": {Type: "string", Default: "plain"}}, + DependsOn: dependsOn, + } +} + +type depFixture struct { + t *testing.T + eng *Engine + rt *fakeRuntime + store *depStore + ws string +} + +func newDepFixture(t *testing.T, features string, table map[string]depEntry) *depFixture { + t.Helper() + ws := writeImageDevcontainer(t, `{"image":"alpine:3.20","features":`+features+`}`) + rt := newFakeRuntime() + store := &depStore{dir: t.TempDir(), features: table} + eng, err := New(EngineOptions{Runtime: rt, FeatureStore: store}) + if err != nil { + t.Fatal(err) + } + return &depFixture{t: t, eng: eng, rt: rt, store: store, ws: ws} +} + +// layer resolves the workspace and runs layerFeatures, returning the +// config it fetched and ordered features into. The fake runtime cannot +// build, so a run that got that far ends in ErrNotImplemented, which is +// swallowed. +func (f *depFixture) layer(mode LockfileMode) (*config.ResolvedConfig, error) { + f.t.Helper() + configPath := filepath.Join(f.ws, ".devcontainer", "devcontainer.json") + cfg, err := Resolve(context.Background(), ResolveOptions{LocalWorkspaceFolder: f.ws, ConfigPath: configPath}) + if err != nil { + f.t.Fatal(err) + } + opts := UpOptions{LocalWorkspaceFolder: f.ws, ConfigPath: configPath, Lockfile: mode} + opts.bus = newEventBus(f.eng.emitter, nil) + defer opts.bus.Close() + _, _, err = f.eng.layerFeatures(context.Background(), cfg, "alpine:3.20", opts) + if errors.Is(err, runtime.ErrNotImplemented) { + err = nil + } + return cfg, err +} + +// installed renders cfg.Features in install order as ref or +// ref{flavor} when installed with a non-default flavor; skipped ones +// are suffixed "(baked)". +func installed(cfg *config.ResolvedConfig) []string { + var out []string + for _, f := range cfg.Features { + s := f.Ref + if fl, _ := f.Options["flavor"].(string); fl != "" && fl != "plain" { + s += "{" + fl + "}" + } + if f.AlreadyInstalled { + s += "(baked)" + } + out = append(out, s) + } + return out +} + +func TestDependsOn_PullsInChainInDependencyOrder(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", map[string]map[string]any{"ghcr.io/x/c:1": {}})}, + "ghcr.io/x/c": {digestOf('c'), meta("c", nil)}, + }) + cfg, err := f.layer(LockfileDefault) + if err != nil { + t.Fatal(err) + } + if got, want := installed(cfg), []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1", "ghcr.io/x/a:1"}; !reflect.DeepEqual(got, want) { + t.Errorf("installed %v, want %v", got, want) + } + // Pulled-in features get spec defaults like declared ones. + if fl := cfg.Features[0].Options["flavor"]; fl != "plain" { + t.Errorf("pulled-in feature options = %v, want defaults applied", cfg.Features[0].Options) + } +} + +func TestDependsOn_DiamondInstallsSharedDependencyOnce(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}, "ghcr.io/x/c:1": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", map[string]map[string]any{"ghcr.io/x/d:1": {}})}, + "ghcr.io/x/c": {digestOf('c'), meta("c", map[string]map[string]any{"ghcr.io/x/d:1": {}})}, + "ghcr.io/x/d": {digestOf('d'), meta("d", nil)}, + }) + cfg, err := f.layer(LockfileDefault) + if err != nil { + t.Fatal(err) + } + if got, want := installed(cfg), []string{"ghcr.io/x/d:1", "ghcr.io/x/b:1", "ghcr.io/x/c:1", "ghcr.io/x/a:1"}; !reflect.DeepEqual(got, want) { + t.Errorf("installed %v, want %v", got, want) + } + if n := strings.Count(strings.Join(f.store.fetched, " "), "ghcr.io/x/d"); n != 1 { + t.Errorf("fetched d %d times: %v", n, f.store.fetched) + } +} + +// TestDependsOn_DeclaredFeatureSatisfiesDependency covers spec +// equality: a dependency is a declared feature when both are the same +// content with the same options once defaults apply, however the +// reference is spelled, and a different feature (installed again) +// when the options differ. +func TestDependsOn_DeclaredFeatureSatisfiesDependency(t *testing.T) { + table := func(depOpts map[string]any, depKey string) map[string]depEntry { + return map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{depKey: depOpts})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", nil)}, + } + } + for _, tc := range []struct { + name string + depKey string + depOpts map[string]any + want []string + }{ + {"same ref and options", "ghcr.io/x/b:1", map[string]any{}, + []string{"ghcr.io/x/b:1", "ghcr.io/x/a:1"}}, + {"explicit default", "ghcr.io/x/b:1", map[string]any{"flavor": "plain"}, + []string{"ghcr.io/x/b:1", "ghcr.io/x/a:1"}}, + {"other tag, same digest", "ghcr.io/x/b:1.0", map[string]any{}, + []string{"ghcr.io/x/b:1", "ghcr.io/x/a:1"}}, + {"different options", "ghcr.io/x/b:1", map[string]any{"flavor": "spicy"}, + []string{"ghcr.io/x/b:1", "ghcr.io/x/b:1{spicy}", "ghcr.io/x/a:1"}}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{},"ghcr.io/x/b:1":{}}`, table(tc.depOpts, tc.depKey)) + cfg, err := f.layer(LockfileDefault) + if err != nil { + t.Fatal(err) + } + if got := installed(cfg); !reflect.DeepEqual(got, tc.want) { + t.Errorf("installed %v, want %v", got, tc.want) + } + }) + } +} + +func TestDependsOn_CycleThroughPulledInFeature(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", map[string]map[string]any{"ghcr.io/x/a:1": {}})}, + }) + _, err := f.layer(LockfileDefault) + var ce *feature.FeatureCycleError + if !errors.As(err, &ce) { + t.Fatalf("got %v, want *FeatureCycleError", err) + } + if n := strings.Count(strings.Join(f.store.fetched, " "), "ghcr.io/x/a"); n != 1 { + t.Errorf("fetched a %d times: %v", n, f.store.fetched) + } +} + +func TestDependsOn_PulledInFeaturesAreLocked(t *testing.T) { + table := map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", nil)}, + } + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, table) + if _, err := f.layer(LockfileDefault); err != nil { + t.Fatal(err) + } + lockPath := filepath.Join(f.ws, ".devcontainer", "devcontainer-lock.json") + lf, err := feature.ReadLockfile(lockPath) + if err != nil { + t.Fatal(err) + } + want := &feature.Lockfile{Features: map[string]feature.LockedFeature{ + "ghcr.io/x/a:1": {Version: "1.0.0", Resolved: "ghcr.io/x/a@" + digestOf('a'), Integrity: digestOf('a'), DependsOn: []string{"ghcr.io/x/b:1"}}, + "ghcr.io/x/b:1": {Version: "1.0.0", Resolved: "ghcr.io/x/b@" + digestOf('b'), Integrity: digestOf('b')}, + }} + if !lf.Equal(want) { + t.Errorf("lockfile differs in %v", lf.Changed(want)) + } + + // The pulled-in feature is pinned like a declared one. + f.store.fetched = nil + if _, err := f.layer(LockfileFrozen); err != nil { + t.Fatal(err) + } + if want := []string{"ghcr.io/x/a@" + digestOf('a'), "ghcr.io/x/b@" + digestOf('b')}; !reflect.DeepEqual(f.store.fetched, want) { + t.Errorf("frozen fetched %v, want %v", f.store.fetched, want) + } + + // A lockfile without the pulled-in feature is out of date. + delete(lf.Features, "ghcr.io/x/b:1") + if err := feature.WriteLockfile(lockPath, lf); err != nil { + t.Fatal(err) + } + var me *feature.LockfileMismatchError + if _, err := f.layer(LockfileFrozen); !errors.As(err, &me) || !reflect.DeepEqual(me.Changed, []string{"ghcr.io/x/b:1"}) { + t.Fatalf("got %v, want *LockfileMismatchError naming ghcr.io/x/b:1", err) + } +} + +// TestDependsOn_BakedDependency: a dependency the base image's label +// carries at a recorded artifact is not fetched or installed again, but +// its own dependencies are still followed. A label that records them +// says the image has them; one written before they were recorded does +// not, so the baked feature's metadata is read at its artifact to find +// them. +func TestDependsOn_BakedDependency(t *testing.T) { + table := map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", map[string]map[string]any{"ghcr.io/x/c:1": {}})}, + "ghcr.io/x/c": {digestOf('c'), meta("c", nil)}, + } + bakedB := `{"id":"b","version":"1.0.0","resolvedRef":"ghcr.io/x/b@` + digestOf('b') + `"` + bakedC := `{"id":"c","version":"1.0.0","resolvedRef":"ghcr.io/x/c@` + digestOf('c') + `","dependsOn":{}}` + for _, tc := range []struct { + name string + label string + wantFetched []string + want []string + }{ + { + name: "label records the dependencies, which the image has", + label: `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}},` + bakedC + `]`, + wantFetched: []string{"ghcr.io/x/a:1"}, + want: []string{"ghcr.io/x/c:1(baked)", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + }, + { + name: "label records the dependencies, which the image lacks", + label: `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}}]`, + wantFetched: []string{"ghcr.io/x/a:1", "ghcr.io/x/c:1"}, + want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + }, + { + name: "label written before dependencies were recorded", + label: `[` + bakedB + `}]`, + wantFetched: []string{"ghcr.io/x/a:1", "ghcr.io/x/b@" + digestOf('b'), "ghcr.io/x/c:1"}, + want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, table) + f.rt.imagesByRef["alpine:3.20"] = &runtime.ImageDetails{Labels: map[string]string{feature.MetadataLabel: tc.label}} + cfg, err := f.layer(LockfileDefault) + if err != nil { + t.Fatal(err) + } + if got := installed(cfg); !reflect.DeepEqual(got, tc.want) { + t.Errorf("installed %v, want %v", got, tc.want) + } + if !reflect.DeepEqual(f.store.fetched, tc.wantFetched) { + t.Errorf("fetched %v, want %v", f.store.fetched, tc.wantFetched) + } + }) + } +} + +// TestDependsOn_DuplicateKeyIsLocked: a dependency dropped as the same +// content as a declared feature under another reference is still a +// requested key, so it is locked (to that content) and pinned next time. +func TestDependsOn_DuplicateKeyIsLocked(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{},"ghcr.io/x/b:1":{}}`, map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1.0": {}})}, + "ghcr.io/x/b": {digestOf('b'), meta("b", nil)}, + }) + cfg, err := f.layer(LockfileDefault) + if err != nil { + t.Fatal(err) + } + if got, want := installed(cfg), []string{"ghcr.io/x/b:1", "ghcr.io/x/a:1"}; !reflect.DeepEqual(got, want) { + t.Errorf("installed %v, want %v", got, want) + } + lf, err := feature.ReadLockfile(filepath.Join(f.ws, ".devcontainer", "devcontainer-lock.json")) + if err != nil { + t.Fatal(err) + } + entry, ok := lf.Entry("ghcr.io/x/b:1.0") + if !ok || entry.Integrity != digestOf('b') { + t.Fatalf("lock entry for the dependency key = (%+v, %v), want integrity %s", entry, ok, digestOf('b')) + } + if _, err := f.layer(LockfileFrozen); err != nil { + t.Errorf("frozen rerun: %v", err) + } +} + +// TestDependsOn_LocalDependencyOfLocalFeature: a local feature's +// dependsOn on another local feature resolves relative to +// devcontainer.json, as a declared local feature does. +func TestDependsOn_LocalDependencyOfLocalFeature(t *testing.T) { + ws := writeImageDevcontainer(t, `{"image":"alpine:3.20","features":{"./a":{}}}`) + writeLocalFeature(t, filepath.Join(ws, ".devcontainer", "a"), `{"id":"a","version":"1.0.0","dependsOn":{"./b":{}}}`) + writeLocalFeature(t, filepath.Join(ws, ".devcontainer", "b"), `{"id":"b","version":"1.0.0"}`) + eng, err := New(EngineOptions{Runtime: newFakeRuntime()}) + if err != nil { + t.Fatal(err) + } + f := &depFixture{t: t, eng: eng, ws: ws} + cfg, err := f.layer(LockfileDisabled) + if err != nil { + t.Fatal(err) + } + if got, want := installed(cfg), []string{"./b", "./a"}; !reflect.DeepEqual(got, want) { + t.Errorf("installed %v, want %v", got, want) + } + if want := filepath.Join(ws, ".devcontainer", "b"); cfg.Features[0].Dir != want { + t.Errorf("./b resolved to %s, want %s", cfg.Features[0].Dir, want) + } +} + +func writeLocalFeature(t *testing.T, dir, metadata string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "devcontainer-feature.json"), []byte(metadata), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } +} diff --git a/design/features.md b/design/features.md index 3d4dcc2..558dcc6 100644 --- a/design/features.md +++ b/design/features.md @@ -162,8 +162,78 @@ Ordered `[]ResolvedFeature` per spec rules: 2. Remaining features: topo-sorted via leaf-extraction over a graph built from `installsAfter` and `dependsOn`. -`dependsOn` is recursive (transitively pulls in unlisted features); -`installsAfter` is a soft "if both listed, this one waits" hint. +`dependsOn` is a hard dependency: the engine installs it whether or not +`devcontainer.json` declares it. `installsAfter` is a soft "if both +listed, this one waits" hint. See §5.1 for how dependencies are pulled +in. + +### 5.1 Pulling in `dependsOn` + +The spec and the reference CLI agree on what counts as one feature. Two +features are equal when they have the same content (manifest digest for +OCI, body sha256 for HTTPS) **and** the same options. So one feature +requested with two sets of options is installed twice. `layerFeatures` +already fetches each feature in turn. That loop now also appends the +features a fetched feature's `dependsOn` requires, which the loop then +fetches like declared ones: pinned through the lockfile, with defaults +applied and options validated. No new graph structure is needed. + +Duplicates are recognised at two points. Both look at the feature list +built so far: + +- **Before fetching**, a dependency already requested with the same + key and options is skipped. This is what stops a cycle and avoids + refetching a shared dependency. +- **After fetching**, a feature whose content and merged options match + an earlier one is dropped. This covers the same feature reached + under a different reference (`:1` and `:1.0` resolving to the same + digest). + +`topoSort` maps each id to all of its entries. A `dependsOn` edge goes +to the entries installed with the options the dependency asks for. +`installsAfter` edges go to every entry for the id. Ties are broken by +ref, then by options. + +Decisions: + +1. **Options are compared after defaults are applied.** The spec + compares "the options executed against the Feature"; the CLI + compares the options as written. Comparing merged options means + `{}` and `{"version": "latest"}` (when `latest` is the default) are + one install, not two of the same thing. No file format depends on + this, so following the spec costs no compatibility. +2. **Declared features are deduplicated by content too.** If + `devcontainer.json` lists two references that resolve to the same + digest with the same options, the feature is installed once. + Previously it was installed twice. This is the CLI's behaviour as + well. +3. **Local `dependsOn` paths resolve against `devcontainer.json`'s + directory**, as declared local features do and as the CLI does. They + are not resolved against the declaring feature's directory. +4. **A dependency the base image's label satisfies is not fetched or + installed again**, using the same match as declared features + (`bakedFeature`, which includes the lockfile check of §8.2). **Its + own dependencies are still followed**, as are a declared baked + feature's, because an image built before this change does not have + them. Labels now record each installed feature's `dependsOn` (as + `{}` when it has none), so for an image built from now on they come + from the label without a fetch. For a label written before that, + the baked feature's metadata is fetched at the artifact the label + records, a digest reference the feature cache serves locally once + seen, to find them. The baked feature itself is never reinstalled. +6. **A dependency dropped as a duplicate is still locked.** When a + `dependsOn` key resolves to the same content and options as a + feature already in the set, it is not installed twice, but its key + still gets a lockfile entry for that content, so the reference it was + requested by stays pinned. +5. **OCI references are lowercased before parsing**, as the CLI's + `getRef` does. Published `dependsOn` keys such as + `ghcr.io/codspace/dependson/A` are written by feature authors, not + the user, and go-containerregistry rejects uppercase repository + names. + +Cycles that run through a pulled-in feature are reported the same way +as any other cycle, as `*FeatureCycleError`. Cycle detection at edge insertion: DFS for back-edge before adding. Returns a typed `*FeatureCycleError{Path []string}` so callers can show @@ -413,11 +483,18 @@ Decisions: compose working directory in the wrong place for a root `.devcontainer.json`. -Known gap: the engine installs only the features declared in -`devcontainer.json`. It does not pull in features reachable only through -another feature's `dependsOn`. The CLI does, and locks them too. The -lockfile here therefore lists declared features only. Transitive -install is separate work. +5. **Features pulled in through `dependsOn` (§5.1) are locked and + pinned like declared ones.** Their key is the `dependsOn` key as + written, as in the CLI. A frozen lockfile that lacks one is out of + date. Verified against the CLI's own `lockfile-dependson` fixture. +6. **The compose inputs hash stays declared-only, with one + exception.** A pulled-in feature is implied by the declaring + feature's entry: a local feature's hashed content holds its + `dependsOn` keys and options, and an OCI feature is identified by + its reference as written. The exception is a *local* dependency's + own content, which nothing else covers, so it is hashed + transitively. Projects without one hash exactly as before, so there + is no recreate on upgrade. Concurrency: a small lockfile (`extracted/.lock`) per cache entry so parallel `Resolve` calls for the same ref don't race. diff --git a/engine_test.go b/engine_test.go index dff7878..ec3a53c 100644 --- a/engine_test.go +++ b/engine_test.go @@ -557,7 +557,7 @@ func TestUp_PrebakedImageSkipsFeatureFetch(t *testing.T) { ID: "sha256:base", Tags: []string{"alpine:3.20"}, Labels: map[string]string{ - "devcontainer.metadata": `[{"id":"pre-baked-feature","version":"1.5.0","resolvedRef":"ghcr.io/x/pre-baked-feature@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}]`, + "devcontainer.metadata": `[{"id":"pre-baked-feature","version":"1.5.0","resolvedRef":"ghcr.io/x/pre-baked-feature@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","dependsOn":{}}]`, }, } eng, _ := New(EngineOptions{Runtime: rt}) diff --git a/feature/disk_store.go b/feature/disk_store.go index f0396d8..e7de247 100644 --- a/feature/disk_store.go +++ b/feature/disk_store.go @@ -116,7 +116,7 @@ func fetchLocal(absPath string) (Fetched, error) { return Fetched{}, fmt.Errorf("local feature %s: missing %s: %w", absPath, installScript, err) } - meta, err := parseMetadata(absPath) + meta, err := ParseMetadata(absPath) if err != nil { return Fetched{}, err } diff --git a/feature/dockerfile_test.go b/feature/dockerfile_test.go index 0d04f84..5cbec13 100644 --- a/feature/dockerfile_test.go +++ b/feature/dockerfile_test.go @@ -253,6 +253,49 @@ func TestParseMetadataLabel_ResolvedRefSurvivesRebuild(t *testing.T) { } } +// TestMetadataLabel_RecordsDependsOn: a newly installed feature's entry +// records its dependsOn, as {} when it has none, so a build on top +// knows the image carries those dependencies; an older label without +// it parses as unrecorded (nil), and both survive a rebuild as written. +func TestMetadataLabel_RecordsDependsOn(t *testing.T) { + git := fixtureFeature(t, "git", "1.0") + node := fixtureFeature(t, "node", "20") + node.Metadata.DependsOn = map[string]map[string]any{"ghcr.io/test/git:1": {}} + raw, err := buildMetadataLabel(BuildPlan{BaseImage: "base", Features: []config.ResolvedFeature{git, node}}) + if err != nil { + t.Fatal(err) + } + parsed, err := ParseMetadataLabel(string(raw)) + if err != nil { + t.Fatal(err) + } + if parsed[0].DependsOn == nil || len(parsed[0].DependsOn) != 0 { + t.Errorf("git dependsOn = %#v, want recorded and empty", parsed[0].DependsOn) + } + if _, ok := parsed[1].DependsOn["ghcr.io/test/git:1"]; !ok { + t.Errorf("node dependsOn = %#v, want ghcr.io/test/git:1", parsed[1].DependsOn) + } + + old, err := ParseMetadataLabel(`[{"id":"git","version":"1.0"}]`) + if err != nil { + t.Fatal(err) + } + if old[0].DependsOn != nil { + t.Errorf("older label dependsOn = %#v, want nil (unrecorded)", old[0].DependsOn) + } + raw, err = buildMetadataLabel(BuildPlan{BaseImage: "base", BaseImageMetadata: append(old, parsed[0])}) + if err != nil { + t.Fatal(err) + } + rebuilt, err := ParseMetadataLabel(string(raw)) + if err != nil { + t.Fatal(err) + } + if rebuilt[0].DependsOn != nil || rebuilt[1].DependsOn == nil { + t.Errorf("rebuilt dependsOn = %#v, %#v; want nil, recorded", rebuilt[0].DependsOn, rebuilt[1].DependsOn) + } +} + func TestParseMetadataLabel_PreservesFinalEntryFields(t *testing.T) { // Reference behavior: the final no-ID resolved-config entry carries // remoteUser/containerUser. ParseMetadataLabel must round-trip them diff --git a/feature/graph.go b/feature/graph.go index 576d94b..2637b62 100644 --- a/feature/graph.go +++ b/feature/graph.go @@ -1,7 +1,9 @@ package feature import ( + "encoding/json" "fmt" + "reflect" "sort" "strings" @@ -53,12 +55,14 @@ func topoSort(features []config.ResolvedFeature) ([]config.ResolvedFeature, []co return features, nil, nil } - // Build a map from a feature's id (ignoring tag) to its index, so - // installsAfter / dependsOn entries can be resolved against the - // feature set even if their refs differ in tag specificity. - idx := make(map[string]int, len(features)) + // Map a feature's id (ignoring tag) to its indices, so installsAfter + // / dependsOn entries resolve against the feature set even if their + // refs differ in tag specificity. One id can have several entries: + // a feature installed once per distinct set of options. + idx := make(map[string][]int, len(features)) for i, f := range features { - idx[normalizeID(f.Ref)] = i + id := normalizeID(f.Ref) + idx[id] = append(idx[id], i) } // inDegree[i] = number of edges pointing TO features[i] (i.e. @@ -66,23 +70,23 @@ func topoSort(features []config.ResolvedFeature) ([]config.ResolvedFeature, []co inDegree := make([]int, len(features)) // adj[i] = features that depend on i (out-edges). adj := make([][]int, len(features)) + edge := func(from, to int) { + adj[from] = append(adj[from], to) + inDegree[to]++ + } for i, f := range features { + // installsAfter is satisfied by any entry for the id; unrelated + // ids are ignored per spec. for _, dep := range f.Metadata.InstallsAfter { - j, ok := idx[normalizeID(dep)] - if !ok { - continue // unrelated installsAfter — ignored per spec + for _, j := range idx[normalizeID(dep)] { + edge(j, i) } - adj[j] = append(adj[j], i) - inDegree[i]++ } - for dep := range f.Metadata.DependsOn { - j, ok := idx[normalizeID(dep)] - if !ok { - continue + for dep, opts := range f.Metadata.DependsOn { + for _, j := range dependencyEntries(features, idx[normalizeID(dep)], opts) { + edge(j, i) } - adj[j] = append(adj[j], i) - inDegree[i]++ } } @@ -97,7 +101,7 @@ func topoSort(features []config.ResolvedFeature) ([]config.ResolvedFeature, []co } } sort.Slice(ready, func(a, b int) bool { - return features[ready[a]].Ref < features[ready[b]].Ref + return featureLess(features[ready[a]], features[ready[b]]) }) out := make([]config.ResolvedFeature, 0, len(features)) @@ -117,7 +121,7 @@ func topoSort(features []config.ResolvedFeature) ([]config.ResolvedFeature, []co // Sort children alphabetically before processing for determinism. children := append([]int(nil), adj[i]...) sort.Slice(children, func(a, b int) bool { - return features[children[a]].Ref < features[children[b]].Ref + return featureLess(features[children[a]], features[children[b]]) }) for _, j := range children { if depth[i]+1 > depth[j] { @@ -150,6 +154,50 @@ func topoSort(features []config.ResolvedFeature) ([]config.ResolvedFeature, []co return out, warnings, nil } +// dependencyEntries returns the entries among candidates (indices of +// features sharing a dependsOn key's id) that satisfy the dependency: +// those installed with the options it asks for once their defaults are +// applied. If none do, every candidate is returned, so the edge still +// orders the feature after its dependency's id as before entries were +// told apart by options. +func dependencyEntries(features []config.ResolvedFeature, candidates []int, opts map[string]any) []int { + var out []int + for _, j := range candidates { + if SameOptions(features[j], opts) { + out = append(out, j) + } + } + if len(out) == 0 { + return candidates + } + return out +} + +// SameOptions reports whether f is installed with the options a +// request for it with opts resolves to, after f's defaults apply. +func SameOptions(f config.ResolvedFeature, opts map[string]any) bool { + merged, _, err := MergeOptions(f.Metadata, opts) + return err == nil && reflect.DeepEqual(merged, mergedOrEmpty(f.Options)) +} + +func mergedOrEmpty(opts map[string]any) map[string]any { + if opts == nil { + return map[string]any{} + } + return opts +} + +// featureLess orders features by ref, then by options, so entries for +// one ref installed with different options sort deterministically. +func featureLess(a, b config.ResolvedFeature) bool { + if a.Ref != b.Ref { + return a.Ref < b.Ref + } + ao, _ := json.Marshal(a.Options) + bo, _ := json.Marshal(b.Options) + return string(ao) < string(bo) +} + // buildDeepestPath reconstructs a chain to a node at `depth[target]` // by walking backward through predecessors. Used for diagnostics, not // hot-path; O(N*chain_len). diff --git a/feature/https.go b/feature/https.go index 70ea929..8887c33 100644 --- a/feature/https.go +++ b/feature/https.go @@ -84,7 +84,7 @@ func (s *DiskStore) fetchHTTPS(ctx context.Context, ref string) (Fetched, error) if _, err := os.Stat(filepath.Join(extractedDir, metadataFile)); err == nil { // Cache hit on a previous run. - meta, err := parseMetadata(extractedDir) + meta, err := ParseMetadata(extractedDir) if err == nil { return Fetched{Dir: extractedDir, ResolvedRef: digest, Metadata: meta}, nil } @@ -114,7 +114,7 @@ func (s *DiskStore) fetchHTTPS(ctx context.Context, ref string) (Fetched, error) if _, err := os.Stat(filepath.Join(extractedDir, installScript)); err != nil { return Fetched{}, fmt.Errorf("HTTPS feature %s: missing %s after extract: %w", ref, installScript, err) } - meta, err := parseMetadata(extractedDir) + meta, err := ParseMetadata(extractedDir) if err != nil { return Fetched{}, err } diff --git a/feature/lockfile.go b/feature/lockfile.go index aa5e307..4a8b1de 100644 --- a/feature/lockfile.go +++ b/feature/lockfile.go @@ -173,7 +173,7 @@ func PinnedRef(f config.ResolvedFeature, entry LockedFeature) (string, error) { if f.SourceKind != config.FeatureSourceOCI { return f.Ref, nil } - ref, err := name.ParseReference(f.Ref) + ref, err := name.ParseReference(strings.ToLower(f.Ref)) if err != nil { return "", fmt.Errorf("parse OCI ref %q: %w", f.Ref, err) } diff --git a/feature/lockfile_test.go b/feature/lockfile_test.go index 1c5dc34..ee4cf91 100644 --- a/feature/lockfile_test.go +++ b/feature/lockfile_test.go @@ -193,6 +193,7 @@ func TestPinnedRef(t *testing.T) { for ref, want := range map[string]string{ "ghcr.io/x/f:1": "ghcr.io/x/f@" + digestA, "ghcr.io/x/f": "ghcr.io/x/f@" + digestA, + "ghcr.io/X/F:1": "ghcr.io/x/f@" + digestA, "ghcr.io/x/f@" + digestB: "ghcr.io/x/f@" + digestA, } { got, err := PinnedRef(config.ResolvedFeature{Ref: ref, SourceKind: config.FeatureSourceOCI}, entry) diff --git a/feature/metadata.go b/feature/metadata.go index db8484d..ba0ec4d 100644 --- a/feature/metadata.go +++ b/feature/metadata.go @@ -24,6 +24,11 @@ type metadataEntry struct { ID string `json:"id,omitempty"` Version string `json:"version,omitempty"` ResolvedRef string `json:"resolvedRef,omitempty"` + // DependsOn records the dependsOn of the feature the entry installed, + // as {} when it has none, so a build on top knows the image already + // carries those dependencies. Absent from labels written before it + // was recorded. + DependsOn *map[string]map[string]any `json:"dependsOn,omitempty"` // Mergeable surface. RemoteUser string `json:"remoteUser,omitempty"` @@ -82,6 +87,11 @@ func buildMetadataLabel(plan BuildPlan) ([]byte, error) { } e := metadataFromFeatureMetadata(f.Metadata) e.ResolvedRef = f.ResolvedRef + deps := f.Metadata.DependsOn + if deps == nil { + deps = map[string]map[string]any{} + } + e.DependsOn = &deps entries = append(entries, e) } @@ -117,6 +127,10 @@ func metadataFromFeatureMetadata(m config.FeatureMetadata) metadataEntry { Entrypoint: m.Entrypoint, Customizations: m.Customizations, } + if m.DependsOn != nil { + deps := m.DependsOn + out.DependsOn = &deps + } if m.HostRequirements != nil { out.HostRequirements = &rawHostReq{ CPUs: m.HostRequirements.CPUs, @@ -244,6 +258,12 @@ func (e metadataEntry) toFeatureMetadata() (config.FeatureMetadata, error) { Entrypoint: e.Entrypoint, Customizations: e.Customizations, } + if e.DependsOn != nil { + out.DependsOn = *e.DependsOn + if out.DependsOn == nil { + out.DependsOn = map[string]map[string]any{} + } + } if e.HostRequirements != nil { out.HostRequirements = &config.HostRequirements{ CPUs: e.HostRequirements.CPUs, diff --git a/feature/oci.go b/feature/oci.go index f669a03..6df244d 100644 --- a/feature/oci.go +++ b/feature/oci.go @@ -46,7 +46,10 @@ const devContainerFeatureMediaType = "application/vnd.devcontainers" // successful online fetch. The recorded digest is only used when its // extracted content is still in the cache. func (s *DiskStore) fetchOCI(ctx context.Context, ref string) (Fetched, error) { - parsed, err := name.ParseReference(ref) + // OCI repository names are lowercase; the reference CLI lowercases + // the whole reference, so feature ids such as ghcr.io/owner/A:1 + // resolve. + parsed, err := name.ParseReference(strings.ToLower(ref)) if err != nil { return Fetched{}, fmt.Errorf("parse OCI ref %q: %w", ref, err) } @@ -143,7 +146,7 @@ func (s *DiskStore) cachedOCI(repo, digest string) (Fetched, bool) { if _, err := os.Stat(filepath.Join(extractedDir, installScript)); err != nil { return Fetched{}, false } - meta, err := parseMetadata(extractedDir) + meta, err := ParseMetadata(extractedDir) if err != nil { return Fetched{}, false } @@ -254,7 +257,7 @@ func (s *DiskStore) extractOCI(ref, repo string, digest v1.Hash, img v1.Image) ( if _, err := os.Stat(filepath.Join(tmpDir, installScript)); err != nil { return Fetched{}, fmt.Errorf("OCI feature %s: missing %s after extract: %w", ref, installScript, err) } - if _, err := parseMetadata(tmpDir); err != nil { + if _, err := ParseMetadata(tmpDir); err != nil { return Fetched{}, err } if err := os.RemoveAll(extractedDir); err != nil { @@ -263,7 +266,7 @@ func (s *DiskStore) extractOCI(ref, repo string, digest v1.Hash, img v1.Image) ( if err := os.Rename(tmpDir, extractedDir); err != nil { return Fetched{}, fmt.Errorf("install extracted feature %s: %w", ref, err) } - meta, err := parseMetadata(extractedDir) + meta, err := ParseMetadata(extractedDir) if err != nil { return Fetched{}, err } diff --git a/feature/parse.go b/feature/parse.go index 5428b8b..c229157 100644 --- a/feature/parse.go +++ b/feature/parse.go @@ -18,10 +18,10 @@ const metadataFile = "devcontainer-feature.json" // instead of opaque mid-build errors. const installScript = "install.sh" -// parseMetadata reads /devcontainer-feature.json into a +// ParseMetadata reads /devcontainer-feature.json into a // config.FeatureMetadata. Missing fields are tolerated (zero value); // JSON parse errors are returned with the file path included. -func parseMetadata(dir string) (config.FeatureMetadata, error) { +func ParseMetadata(dir string) (config.FeatureMetadata, error) { path := filepath.Join(dir, metadataFile) data, err := os.ReadFile(path) if err != nil { diff --git a/lockfile_test.go b/lockfile_test.go index 5dfe078..d0f20f2 100644 --- a/lockfile_test.go +++ b/lockfile_test.go @@ -250,7 +250,7 @@ func TestLockfile_HTTPSIntegrityMismatchFails(t *testing.T) { // one, which is then locked. Anything else is fetched (at the pin). func TestLockfile_BakedFeature(t *testing.T) { baked := func(resolvedRef string) string { - return `[{"id":"a","version":"1.0.0","resolvedRef":"` + resolvedRef + `"}]` + return `[{"id":"a","version":"1.0.0","resolvedRef":"` + resolvedRef + `","dependsOn":{}}]` } pinned := &feature.Lockfile{Features: map[string]feature.LockedFeature{ "ghcr.io/x/a:1": ociEntry("ghcr.io/x/a", lockDigestOld), @@ -273,7 +273,7 @@ func TestLockfile_BakedFeature(t *testing.T) { mode: LockfileFrozen, wantFetched: []string{"ghcr.io/x/a@" + lockDigestOld}, wantLock: pinned, }, { - name: "unrecorded artifact", label: `[{"id":"a","version":"1.0.0"}]`, lock: pinned, + name: "unrecorded artifact", label: `[{"id":"a","version":"1.0.0","dependsOn":{}}]`, lock: pinned, mode: LockfileFrozen, wantFetched: []string{"ghcr.io/x/a@" + lockDigestOld}, wantLock: pinned, }, { @@ -285,7 +285,7 @@ func TestLockfile_BakedFeature(t *testing.T) { lock: &feature.Lockfile{Features: map[string]feature.LockedFeature{}}, mode: LockfileFrozen, wantErr: true, }, { - name: "disabled trusts the label", label: `[{"id":"a","version":"1.0.0"}]`, mode: LockfileDisabled, + name: "disabled trusts the label", label: `[{"id":"a","version":"1.0.0","dependsOn":{}}]`, mode: LockfileDisabled, }, { // The tag now resolves to lockDigestNew; an older baked diff --git a/test/integration/depends_on_test.go b/test/integration/depends_on_test.go new file mode 100644 index 0000000..d93094c --- /dev/null +++ b/test/integration/depends_on_test.go @@ -0,0 +1,82 @@ +//go:build integration + +package integration + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + devcontainer "github.com/crunchloop/devcontainer" +) + +// cliDependsOnLockfile is the reference CLI's own expected lockfile for +// these features (src/test/container-features/configs/lockfile-dependson +// in devcontainers/cli): A:2 depends on E, which is a different +// artifact from the declared E:1. The fixture's third feature, an HTTPS +// tarball, is left out with its entry: it is an uncompressed tar named +// .tgz, which the HTTPS fetcher does not extract. +const cliDependsOnLockfile = `{ + "features": { + "ghcr.io/codspace/dependson/A:2": { + "version": "2.0.1", + "resolved": "ghcr.io/codspace/dependson/a@sha256:932027ef71da186210e6ceb3294c3459caaf6b548d2b547d5d26be3fc4b2264a", + "integrity": "sha256:932027ef71da186210e6ceb3294c3459caaf6b548d2b547d5d26be3fc4b2264a", + "dependsOn": [ + "ghcr.io/codspace/dependson/E" + ] + }, + "ghcr.io/codspace/dependson/E": { + "version": "2.0.0", + "resolved": "ghcr.io/codspace/dependson/e@sha256:9f36f159c70f8bebff57f341904b030733adb17ef12a5d58d4b3d89b2a6c7d5a", + "integrity": "sha256:9f36f159c70f8bebff57f341904b030733adb17ef12a5d58d4b3d89b2a6c7d5a" + }, + "ghcr.io/codspace/dependson/E:1": { + "version": "1.0.0", + "resolved": "ghcr.io/codspace/dependson/e@sha256:90b84127edab28ecb169cd6c6f2101ce0ea1d77589cee01951fec7f879f3a11c", + "integrity": "sha256:90b84127edab28ecb169cd6c6f2101ce0ea1d77589cee01951fec7f879f3a11c" + } + } +} +` + +// TestDependsOn_MatchesCLILockfile builds the reference CLI's +// lockfile-dependson fixture against its expected lockfile in frozen +// mode, which passes only if the engine pulls in and locks exactly the +// features the CLI does, then checks a default build leaves the file +// byte-for-byte as the CLI wrote it. +func TestDependsOn_MatchesCLILockfile(t *testing.T) { + if testing.Short() { + t.Skip() + } + eng, rt := newEngineWith(t, devcontainer.EngineOptions{FeatureCacheDir: t.TempDir()}) + defer rt.Close() + + dir := t.TempDir() + mustWrite(t, filepath.Join(dir, ".devcontainer", "devcontainer.json"), `{ + "image": "debian:bookworm-slim", + "features": { + "ghcr.io/codspace/dependson/A:2": {}, + "ghcr.io/codspace/dependson/E:1": {} + } + }`) + lockPath := filepath.Join(dir, ".devcontainer", "devcontainer-lock.json") + mustWrite(t, lockPath, cliDependsOnLockfile) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + for _, mode := range []devcontainer.LockfileMode{devcontainer.LockfileFrozen, devcontainer.LockfileDefault} { + if _, err := eng.Build(ctx, devcontainer.BuildOptions{LocalWorkspaceFolder: dir, Lockfile: mode}); err != nil { + t.Fatalf("Build (lockfile mode %q): %v", mode, err) + } + } + got, err := os.ReadFile(lockPath) + if err != nil { + t.Fatal(err) + } + if string(got) != cliDependsOnLockfile { + t.Errorf("lockfile rewritten:\n%s", got) + } +} diff --git a/up.go b/up.go index 51210f1..e238faa 100644 --- a/up.go +++ b/up.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "slices" "sort" "time" @@ -409,36 +410,120 @@ func (e *Engine) createFresh(ctx context.Context, cfg *config.ResolvedConfig, op // ref's last path component. Local / HTTPS features always go through // fetch (cheap for local; HTTPS fetch is what gives us the id anyway). func (e *Engine) markAlreadyInstalled(cfg *config.ResolvedConfig, baked []config.FeatureMetadata, lock *featureLock) { - mode := feature.MatchPermissive - if e.opts.StrictFeatureVersionMatch { - mode = feature.MatchStrict - } for i := range cfg.Features { f := &cfg.Features[i] if f.AlreadyInstalled { continue } - if f.SourceKind != config.FeatureSourceOCI { - continue + if meta, ok := e.bakedFeature(*f, baked, lock); ok { + f.AlreadyInstalled = true + // Preserve baked metadata so the regenerated label can + // include it without re-fetching. + f.Metadata = meta + f.ResolvedRef = meta.ResolvedRef } - req := config.FeatureMetadata{ - ID: ociRefBareID(f.Ref), - Version: optionVersionString(f.Options), + } +} + +// bakedFeature returns the base-image label entry that satisfies f, if +// any. See markAlreadyInstalled. +func (e *Engine) bakedFeature(f config.ResolvedFeature, baked []config.FeatureMetadata, lock *featureLock) (config.FeatureMetadata, bool) { + if f.SourceKind != config.FeatureSourceOCI { + return config.FeatureMetadata{}, false + } + req := config.FeatureMetadata{ + ID: ociRefBareID(f.Ref), + Version: optionVersionString(f.Options), + } + if req.ID == "" { + return config.FeatureMetadata{}, false + } + mode := feature.MatchPermissive + if e.opts.StrictFeatureVersionMatch { + mode = feature.MatchStrict + } + for _, b := range baked { + if feature.Matches(b, req, mode) && lock.acceptsBaked(f, b) { + return b, true } - if req.ID == "" { + } + return config.FeatureMetadata{}, false +} + +// dependencies returns the features f's dependsOn requires that +// features does not already hold, in key order, unfetched. One the +// base image's label satisfies is returned AlreadyInstalled, as +// markAlreadyInstalled marks a declared feature, and so is not +// fetched; layerFeatures still follows its own dependencies. +func (e *Engine) dependencies(features []config.ResolvedFeature, f config.ResolvedFeature, baked []config.FeatureMetadata, lock *featureLock, bus *eventBus) []config.ResolvedFeature { + keys := make([]string, 0, len(f.Metadata.DependsOn)) + for key := range f.Metadata.DependsOn { + keys = append(keys, key) + } + sort.Strings(keys) + + var out []config.ResolvedFeature + for _, key := range keys { + opts := f.Metadata.DependsOn[key] + if requested(features, key, opts) || requested(out, key, opts) { continue } - for _, b := range baked { - if feature.Matches(b, req, mode) && lock.acceptsBaked(*f, b) { - f.AlreadyInstalled = true - // Preserve baked metadata so the regenerated label can - // include it without re-fetching. - f.Metadata = b - f.ResolvedRef = b.ResolvedRef - break - } + dep := config.ResolvedFeature{Ref: key, Options: opts, SourceKind: config.ClassifyFeatureRef(key)} + if meta, ok := e.bakedFeature(dep, baked, lock); ok { + dep.AlreadyInstalled = true + dep.Metadata = meta + dep.ResolvedRef = meta.ResolvedRef + bus.Emit(events.FeatureSkippedEvent{Ref: key, Reason: "base_image_label"}) } + out = append(out, dep) } + return out +} + +// bakedDependsOn returns the dependsOn of a feature the base image +// installs whose label entry does not record it, read from the +// feature's metadata at the artifact the image carries (a digest +// reference, which the feature cache serves without a registry call +// once fetched). Never nil, so the label of an image built on top +// records it. +func (e *Engine) bakedDependsOn(ctx context.Context, f config.ResolvedFeature) (map[string]map[string]any, error) { + ref := f.Ref + if f.ResolvedRef != "" && f.SourceKind == config.FeatureSourceOCI { + ref = f.ResolvedRef + } + fetched, err := e.featureStore.Fetch(ctx, ref, f.SourceKind) + if err != nil { + return nil, fmt.Errorf("read dependsOn of baked feature %s: %w", f.Ref, err) + } + if fetched.Metadata.DependsOn == nil { + return map[string]map[string]any{}, nil + } + return fetched.Metadata.DependsOn, nil +} + +// requested reports whether features already holds the dependency +// key with opts: the same reference installed with the same options. +// It tells a cycle or a repeated dependency apart before fetching; +// duplicateFeature catches the same content under another reference +// after. +func requested(features []config.ResolvedFeature, key string, opts map[string]any) bool { + for _, g := range features { + if g.Ref == key && feature.SameOptions(g, opts) { + return true + } + } + return false +} + +// duplicateFeature reports whether earlier holds a fetched feature with +// f's content and options. +func duplicateFeature(earlier []config.ResolvedFeature, f config.ResolvedFeature) bool { + for _, g := range earlier { + if g.ResolvedRef != "" && g.ResolvedRef == f.ResolvedRef && reflect.DeepEqual(g.Options, f.Options) { + return true + } + } + return false } // emitFeatureSkippedFromLabel fires FeatureSkipped events for each request @@ -1305,10 +1390,32 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, // skip(s) → resolve_start → resolved → ... emitFeatureSkippedFromLabel(opts.bus, cfg.Features) - // Fetch each not-yet-installed feature. Resolve refs in place. - for i := range cfg.Features { + // Fetch each not-yet-installed feature. Resolve refs in place. Each + // fetched feature appends the features its dependsOn requires, which + // the spec installs whether or not devcontainer.json declares them, + // so the loop fetches those in turn. + // + // A feature dropped as the duplicate of another under a different + // reference is still a requested key; aliases keeps it to be locked. + var aliases []config.ResolvedFeature + for i := 0; i < len(cfg.Features); i++ { f := &cfg.Features[i] - if f.AlreadyInstalled || f.Dir != "" { + if f.AlreadyInstalled { + // The base image installs f, but its dependencies only if the + // image was built with them: from a label that records them, + // or, for a label written before they were recorded, as f's + // own metadata names them. f itself is not installed again. + if f.Metadata.DependsOn == nil { + deps, err := e.bakedDependsOn(ctx, *f) + if err != nil { + return "", nil, err + } + f.Metadata.DependsOn = deps + } + cfg.Features = append(cfg.Features, e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, opts.bus)...) + continue + } + if f.Dir != "" { continue } ref := f.Ref @@ -1345,9 +1452,19 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, } f.Options = merged cfg.Warnings = append(cfg.Warnings, mwarns...) + + // Spec equality: the same content installed with the same + // options is one feature, however its reference was spelled. + if duplicateFeature(cfg.Features[:i], *f) { + aliases = append(aliases, *f) + cfg.Features = append(cfg.Features[:i], cfg.Features[i+1:]...) + i-- + continue + } + cfg.Features = append(cfg.Features, e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, opts.bus)...) } - if err := lock.commit(cfg.Features); err != nil { + if err := lock.commit(slices.Concat(cfg.Features, aliases)); err != nil { return "", nil, err } diff --git a/up_compose_reattach.go b/up_compose_reattach.go index 6f8cc5e..0c2e66a 100644 --- a/up_compose_reattach.go +++ b/up_compose_reattach.go @@ -123,6 +123,7 @@ func (e *Engine) composeInputsHash(cfg *config.ResolvedConfig, opts UpOptions, p } configDir := composeWorkingDir(opts) features := make([]composeHashedFeature, 0, len(cfg.Features)) + var local []string for _, f := range cfg.Features { hf := composeHashedFeature{Ref: f.Ref, SourceKind: f.SourceKind, Options: f.Options} if f.SourceKind == config.FeatureSourceLocal { @@ -131,9 +132,44 @@ func (e *Engine) composeInputsHash(cfg *config.ResolvedConfig, opts UpOptions, p return "", fmt.Errorf("compose inputs hash: local feature %s: %w", f.Ref, err) } hf.LocalContent = digest + local = append(local, f.Ref) } features = append(features, hf) } + // Features a feature's dependsOn pulls in are implied by the + // declaring feature's content, which holds the dependsOn keys and + // options, except for the content of a local dependency: hash that + // too, transitively. A project with none hashes as before. + hashed := map[string]bool{} + for _, ref := range local { + hashed[ref] = true + } + for len(local) > 0 { + ref := local[0] + local = local[1:] + meta, err := feature.ParseMetadata(feature.ResolveLocalRef(configDir, ref)) + if err != nil { + // Its content is hashed; the build reports the bad metadata. + continue + } + deps := make([]string, 0, len(meta.DependsOn)) + for dep := range meta.DependsOn { + deps = append(deps, dep) + } + sort.Strings(deps) + for _, dep := range deps { + if hashed[dep] || config.ClassifyFeatureRef(dep) != config.FeatureSourceLocal { + continue + } + hashed[dep] = true + digest, err := hashDirContent(feature.ResolveLocalRef(configDir, dep), maxLocalFeatureHashBytes) + if err != nil { + return "", fmt.Errorf("compose inputs hash: local feature %s: %w", dep, err) + } + features = append(features, composeHashedFeature{Ref: dep, SourceKind: config.FeatureSourceLocal, LocalContent: digest}) + local = append(local, dep) + } + } in := composeHashInput{ Version: composeInputsHashVersion, Config: composeHashedConfig{ diff --git a/up_compose_reattach_test.go b/up_compose_reattach_test.go index 21ebde5..d4b9222 100644 --- a/up_compose_reattach_test.go +++ b/up_compose_reattach_test.go @@ -177,6 +177,25 @@ func TestComposeInputsHash_LocalFeatureContent(t *testing.T) { } } +// A local feature the declared one pulls in through dependsOn is +// installed from its own content, which the declaring feature's +// content does not cover, so it is hashed too. +func TestComposeInputsHash_LocalDependencyContent(t *testing.T) { + dir := t.TempDir() + json := replaceOnce(t, hashBaseJSON, `"ghcr.io/devcontainers/features/docker-in-docker:2": { "moby": false }`, `"./local-feature": {}`) + writeLocalFeature(t, filepath.Join(dir, ".devcontainer", "local-feature"), `{"id":"local-feature","dependsOn":{"./dep":{}}}`) + depDir := filepath.Join(dir, ".devcontainer", "dep") + writeLocalFeature(t, depDir, `{"id":"dep"}`) + f := newHashFixture(t, dir, json, hashBaseCompose) + h1 := f.hash(t, "sha256:base") + if err := os.WriteFile(filepath.Join(depDir, "install.sh"), []byte("#!/bin/sh\necho changed\n"), 0o755); err != nil { + t.Fatal(err) + } + if h2 := f.hash(t, "sha256:base"); h1 == h2 { + t.Fatal("hash unchanged after editing a pulled-in local feature's install.sh") + } +} + // A local feature file that is a symlink is copied into the build // context as the content it names, so that content is what the hash // covers: editing the link's target outside the feature directory must From a4567eb9c81db3b25d0bba0b2210e8e3c2661882 Mon Sep 17 00:00:00 2001 From: bilby91 <2201079+bilby91@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:20:22 +0000 Subject: [PATCH 4/7] docs(features): record the baked match's option gap for dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DAP review of #151 (finding 3903) notes a baked dependency is reused whatever options it was requested with. The pre-baked match has never compared options, for declared features either; record the limitation in §5.1 and track the fix to the match itself in #154. Co-Authored-By: Claude Opus 5.5 --- design/features.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/design/features.md b/design/features.md index 558dcc6..e087077 100644 --- a/design/features.md +++ b/design/features.md @@ -221,6 +221,16 @@ Decisions: the baked feature's metadata is fetched at the artifact the label records, a digest reference the feature cache serves locally once seen, to find them. The baked feature itself is never reinstalled. + + **Known limitation, accepted here:** the baked match does not + compare options. A dependency requested with options other than the + ones the image installed it with is still taken from the image, so + those options are not applied. This is the pre-baked match's + behaviour for declared features since it was introduced (id and + version only). Fixing it means recording applied options in the + label and comparing them after applying defaults, which the label + cannot do today. That is a change to the pre-baked match itself, + tracked in #154, not to dependency resolution. 6. **A dependency dropped as a duplicate is still locked.** When a `dependsOn` key resolves to the same content and options as a feature already in the set, it is not installed twice, but its key From 9a4e5c2b3176d7a990ddaeba61a7f22cb6cc6577 Mon Sep 17 00:00:00 2001 From: bilby91 <2201079+bilby91@users.noreply.github.com> Date: Wed, 4 Nov 2026 20:33:18 +0000 Subject: [PATCH 5/7] test(features): lock an unpinned baked feature with its dependsOn The DAP review of #153 (finding 3904) noted a baked feature accepted without a pin was locked from its label entry, which then had no dependsOn. This change fills it in before commit; pin it with a test for a label that records dependsOn and for an older one. Co-Authored-By: Claude Opus 5.5 --- depends_on_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/depends_on_test.go b/depends_on_test.go index 60565dc..b73a719 100644 --- a/depends_on_test.go +++ b/depends_on_test.go @@ -370,3 +370,36 @@ func writeLocalFeature(t *testing.T, dir, metadata string) { t.Fatal(err) } } + +// TestDependsOn_BakedFeatureLockedWithItsDependsOn: a baked feature the +// lockfile does not pin yet is locked from the artifact its label +// records, and its entry carries its dependsOn like a fetched one's: +// from the label, or for an older label from its metadata. +func TestDependsOn_BakedFeatureLockedWithItsDependsOn(t *testing.T) { + table := map[string]depEntry{ + "ghcr.io/x/b": {digestOf('b'), meta("b", map[string]map[string]any{"ghcr.io/x/c:1": {}})}, + "ghcr.io/x/c": {digestOf('c'), meta("c", nil)}, + } + bakedB := `{"id":"b","version":"1.0.0","resolvedRef":"ghcr.io/x/b@` + digestOf('b') + `"` + bakedC := `{"id":"c","version":"1.0.0","resolvedRef":"ghcr.io/x/c@` + digestOf('c') + `","dependsOn":{}}` + for _, tc := range []struct{ name, label string }{ + {"label records dependsOn", `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}},` + bakedC + `]`}, + {"older label", `[` + bakedB + `},` + bakedC + `]`}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/b:1":{}}`, table) + f.rt.imagesByRef["alpine:3.20"] = &runtime.ImageDetails{Labels: map[string]string{feature.MetadataLabel: tc.label}} + if _, err := f.layer(LockfileDefault); err != nil { + t.Fatal(err) + } + lf, err := feature.ReadLockfile(filepath.Join(f.ws, ".devcontainer", "devcontainer-lock.json")) + if err != nil { + t.Fatal(err) + } + entry, ok := lf.Entry("ghcr.io/x/b:1") + if !ok || entry.Integrity != digestOf('b') || !reflect.DeepEqual(entry.DependsOn, []string{"ghcr.io/x/c:1"}) { + t.Errorf("lock entry for b = (%+v, %v), want integrity %s and dependsOn [ghcr.io/x/c:1]", entry, ok, digestOf('b')) + } + }) + } +} From 9fa2e7b14b1a68c74432e005f969e013bd5a90f4 Mon Sep 17 00:00:00 2001 From: bilby91 <2201079+bilby91@users.noreply.github.com> Date: Wed, 4 Nov 2026 21:20:19 +0000 Subject: [PATCH 6/7] fix(features): confine local dependsOn paths The DAP review of #151 (findings 3912, 3919, 3917) showed a fetched feature's dependsOn could name a local path, absolute or escaping with .., which layerFeatures resolved against devcontainer.json's directory, copied into the build context and ran: published metadata choosing a host directory to execute. The compose inputs hash also missed such a dependency, since it follows local dependencies from declared local features only. A local dependsOn key is now followed only from a local feature, and only as a relative path that resolves, symlinks included, inside the directory of devcontainer.json, as the reference CLI requires of local features; anything else fails Up and Build. With remote features unable to reach local dependencies, the compose hash's traversal covers every one. Declared local features keep their existing resolution. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +++++ depends_on_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++ design/features.md | 23 ++++++++++++++++--- up.go | 50 +++++++++++++++++++++++++++++++++++++---- 4 files changed, 127 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41e558d..c93877b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 metadata is fetched at its recorded artifact to find them. - A dependency dropped as a duplicate of another reference is still locked under its own key. + - A local (path) dependency is followed only from a local feature, + and only when it is a relative path that resolves, symlinks + included, inside the directory of `devcontainer.json`; otherwise + `Up`/`Build` fail. A published feature's metadata cannot make the + build copy and run a host directory. - A local dependency's content is part of the compose inputs hash. - See design/features.md §5.1. - **features** — OCI feature references are lowercased before they are diff --git a/depends_on_test.go b/depends_on_test.go index b73a719..3d95977 100644 --- a/depends_on_test.go +++ b/depends_on_test.go @@ -403,3 +403,59 @@ func TestDependsOn_BakedFeatureLockedWithItsDependsOn(t *testing.T) { }) } } + +// TestDependsOn_LocalDependencyConfined: a local dependsOn key is +// followed only from a local feature, only as a relative path, and only +// when it resolves (symlinks included) inside the directory of +// devcontainer.json; otherwise feature metadata could make the build +// copy and run any host directory. +func TestDependsOn_LocalDependencyConfined(t *testing.T) { + t.Run("from a remote feature", func(t *testing.T) { + f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, map[string]depEntry{ + "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"./b": {}})}, + }) + writeLocalFeature(t, filepath.Join(f.ws, ".devcontainer", "b"), `{"id":"b","version":"1.0.0"}`) + if _, err := f.layer(LockfileDisabled); err == nil || !strings.Contains(err.Error(), "only a local feature") { + t.Fatalf("got %v, want a refusal of the local dependency", err) + } + }) + + outside := t.TempDir() + writeLocalFeature(t, filepath.Join(outside, "evil"), `{"id":"evil","version":"1.0.0"}`) + for _, tc := range []struct { + name, key, want string + traverse, link bool + }{ + {name: "absolute path", key: filepath.Join(outside, "evil"), want: "relative path"}, + {name: "parent traversal", traverse: true, want: "outside"}, + {name: "symlink out", key: "./link", link: true, want: "outside"}, + } { + t.Run(tc.name, func(t *testing.T) { + ws := writeImageDevcontainer(t, `{"image":"alpine:3.20","features":{"./a":{}}}`) + dc := filepath.Join(ws, ".devcontainer") + key := tc.key + if tc.traverse { + // Reach the outside directory from .devcontainer with "..". + rel, err := filepath.Rel(dc, filepath.Join(outside, "evil")) + if err != nil { + t.Fatal(err) + } + key = rel + } + if tc.link { + if err := os.Symlink(filepath.Join(outside, "evil"), filepath.Join(dc, "link")); err != nil { + t.Fatal(err) + } + } + writeLocalFeature(t, filepath.Join(dc, "a"), `{"id":"a","version":"1.0.0","dependsOn":{"`+key+`":{}}}`) + eng, err := New(EngineOptions{Runtime: newFakeRuntime()}) + if err != nil { + t.Fatal(err) + } + f := &depFixture{t: t, eng: eng, ws: ws} + if _, err := f.layer(LockfileDisabled); err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("dependsOn %q: got %v, want an error containing %q", key, err, tc.want) + } + }) + } +} diff --git a/design/features.md b/design/features.md index e087077..cd6f48c 100644 --- a/design/features.md +++ b/design/features.md @@ -209,7 +209,22 @@ Decisions: well. 3. **Local `dependsOn` paths resolve against `devcontainer.json`'s directory**, as declared local features do and as the CLI does. They - are not resolved against the declaring feature's directory. + are not resolved against the declaring feature's directory. They are + also **confined**, because feature metadata, not the user, chooses + them, and the directory they name is copied into the build and its + `install.sh` run: + - Only a local feature may declare one. A published feature cannot + know the consumer's files; a local key from it is refused. This is + stricter than the CLI, which follows it if it stays inside + `.devcontainer/`. + - The key must be relative and resolve, symlinks included, inside + the directory of `devcontainer.json`, as the CLI requires of local + features (`containerFeaturesConfiguration.ts`: no absolute paths, + no escape from `.devcontainer/`). + + Declared local features keep their existing, unconfined resolution: + the user wrote those paths, and confining them would break configs + that point outside the config directory. 4. **A dependency the base image's label satisfies is not fetched or installed again**, using the same match as declared features (`bakedFeature`, which includes the lockfile check of §8.2). **Its @@ -503,8 +518,10 @@ Decisions: `dependsOn` keys and options, and an OCI feature is identified by its reference as written. The exception is a *local* dependency's own content, which nothing else covers, so it is hashed - transitively. Projects without one hash exactly as before, so there - is no recreate on upgrade. + transitively from the declared local features. That reaches every + local dependency, since only a local feature may declare one + (§5.1 decision 3). Projects without one hash exactly as before, so + there is no recreate on upgrade. Concurrency: a small lockfile (`extracted/.lock`) per cache entry so parallel `Resolve` calls for the same ref don't race. diff --git a/up.go b/up.go index e238faa..a097334 100644 --- a/up.go +++ b/up.go @@ -9,6 +9,7 @@ import ( "reflect" "slices" "sort" + "strings" "time" composetypes "github.com/compose-spec/compose-go/v2/types" @@ -455,7 +456,7 @@ func (e *Engine) bakedFeature(f config.ResolvedFeature, baked []config.FeatureMe // base image's label satisfies is returned AlreadyInstalled, as // markAlreadyInstalled marks a declared feature, and so is not // fetched; layerFeatures still follows its own dependencies. -func (e *Engine) dependencies(features []config.ResolvedFeature, f config.ResolvedFeature, baked []config.FeatureMetadata, lock *featureLock, bus *eventBus) []config.ResolvedFeature { +func (e *Engine) dependencies(features []config.ResolvedFeature, f config.ResolvedFeature, baked []config.FeatureMetadata, lock *featureLock, configDir string, bus *eventBus) ([]config.ResolvedFeature, error) { keys := make([]string, 0, len(f.Metadata.DependsOn)) for key := range f.Metadata.DependsOn { keys = append(keys, key) @@ -469,6 +470,11 @@ func (e *Engine) dependencies(features []config.ResolvedFeature, f config.Resolv continue } dep := config.ResolvedFeature{Ref: key, Options: opts, SourceKind: config.ClassifyFeatureRef(key)} + if dep.SourceKind == config.FeatureSourceLocal { + if err := checkLocalDependency(configDir, f, key); err != nil { + return nil, err + } + } if meta, ok := e.bakedFeature(dep, baked, lock); ok { dep.AlreadyInstalled = true dep.Metadata = meta @@ -477,7 +483,35 @@ func (e *Engine) dependencies(features []config.ResolvedFeature, f config.Resolv } out = append(out, dep) } - return out + return out, nil +} + +// checkLocalDependency enforces where a local dependsOn key of f may +// point. Only a local feature may declare one: a published feature +// cannot know the consumer's files, and following its local key would +// copy and run whatever host directory its metadata names. The key must +// also be relative and resolve, symlinks included, inside the +// directory of devcontainer.json, as the reference CLI requires of +// local features. +func checkLocalDependency(configDir string, f config.ResolvedFeature, key string) error { + if f.SourceKind != config.FeatureSourceLocal { + return fmt.Errorf("feature %s: dependsOn %q: only a local feature may depend on a local path", f.Ref, key) + } + if filepath.IsAbs(key) { + return fmt.Errorf("feature %s: dependsOn %q: a local dependency must be a relative path", f.Ref, key) + } + root, err := filepath.EvalSymlinks(configDir) + if err != nil { + return fmt.Errorf("feature %s: dependsOn %q: %w", f.Ref, key, err) + } + path, err := filepath.EvalSymlinks(feature.ResolveLocalRef(configDir, key)) + if err != nil { + return fmt.Errorf("feature %s: dependsOn %q: %w", f.Ref, key, err) + } + if rel, err := filepath.Rel(root, path); err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return fmt.Errorf("feature %s: dependsOn %q resolves to %s, outside %s", f.Ref, key, path, configDir) + } + return nil } // bakedDependsOn returns the dependsOn of a feature the base image @@ -1412,7 +1446,11 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, } f.Metadata.DependsOn = deps } - cfg.Features = append(cfg.Features, e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, opts.bus)...) + deps, err := e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, configDir, opts.bus) + if err != nil { + return "", nil, err + } + cfg.Features = append(cfg.Features, deps...) continue } if f.Dir != "" { @@ -1461,7 +1499,11 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, i-- continue } - cfg.Features = append(cfg.Features, e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, opts.bus)...) + deps, err := e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, configDir, opts.bus) + if err != nil { + return "", nil, err + } + cfg.Features = append(cfg.Features, deps...) } if err := lock.commit(slices.Concat(cfg.Features, aliases)); err != nil { From 4811e0aecb4e81f0166eb4316efd1c84480500c1 Mon Sep 17 00:00:00 2001 From: bilby91 <2201079+bilby91@users.noreply.github.com> Date: Thu, 5 Nov 2026 13:40:47 +0000 Subject: [PATCH 7/7] fix(features): never take a dependsOn dependency from the base image The DAP review of #151 (finding 3913) showed a dependency the base image's label listed was reused whatever options its dependsOn entry asked for: the pre-baked match compares id and version only, and those options are the feature author's, which no user config could route through that match before. Dependencies are now always fetched and installed; a declared baked feature still is not reinstalled, and its dependencies are installed. Until #154 adds option matching, a pre-baked image whose features have dependencies reinstalls them on every build. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++++++--- depends_on_test.go | 43 ++++++++++++++++++++++--------------------- design/features.md | 42 +++++++++++++++++++----------------------- up.go | 32 +++++++++++++++----------------- 4 files changed, 62 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c93877b..1d86cd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,9 +75,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 options is installed once per set of options. - Declared features that resolve to the same content with the same options are now installed once rather than twice. - - A dependency the base image already carries (at the artifact the - lockfile accepts) is not installed again, but its own dependencies - are still followed. Image labels now record each feature's + - Dependencies are always installed, even when the base image's label + lists them: the pre-baked match does not compare options yet + (#154). A pre-baked image whose features have dependencies + therefore reinstalls those on every build until then. + - A declared feature the base image carries is not reinstalled, but + its dependencies are. Image labels now record each feature's `dependsOn`; for a label written before that, the baked feature's metadata is fetched at its recorded artifact to find them. - A dependency dropped as a duplicate of another reference is still diff --git a/depends_on_test.go b/depends_on_test.go index 3d95977..ad84154 100644 --- a/depends_on_test.go +++ b/depends_on_test.go @@ -250,12 +250,12 @@ func TestDependsOn_PulledInFeaturesAreLocked(t *testing.T) { } } -// TestDependsOn_BakedDependency: a dependency the base image's label -// carries at a recorded artifact is not fetched or installed again, but -// its own dependencies are still followed. A label that records them -// says the image has them; one written before they were recorded does -// not, so the baked feature's metadata is read at its artifact to find -// them. +// TestDependsOn_BakedDependency: a dependency is fetched and installed +// even when the base image's label lists it, since the label match does +// not compare the options its dependsOn entry asks for (#154). A +// declared feature the image carries is not installed again, but its +// dependencies are: named by a label that records them, or for a label +// written before that, by its metadata read at its recorded artifact. func TestDependsOn_BakedDependency(t *testing.T) { table := map[string]depEntry{ "ghcr.io/x/a": {digestOf('a'), meta("a", map[string]map[string]any{"ghcr.io/x/b:1": {}})}, @@ -265,32 +265,33 @@ func TestDependsOn_BakedDependency(t *testing.T) { bakedB := `{"id":"b","version":"1.0.0","resolvedRef":"ghcr.io/x/b@` + digestOf('b') + `"` bakedC := `{"id":"c","version":"1.0.0","resolvedRef":"ghcr.io/x/c@` + digestOf('c') + `","dependsOn":{}}` for _, tc := range []struct { - name string - label string - wantFetched []string - want []string + name, declared, label string + wantFetched, want []string }{ { - name: "label records the dependencies, which the image has", + name: "baked dependency is installed anyway", + declared: `{"ghcr.io/x/a:1":{}}`, label: `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}},` + bakedC + `]`, - wantFetched: []string{"ghcr.io/x/a:1"}, - want: []string{"ghcr.io/x/c:1(baked)", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + wantFetched: []string{"ghcr.io/x/a:1", "ghcr.io/x/b:1", "ghcr.io/x/c:1"}, + want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1", "ghcr.io/x/a:1"}, }, { - name: "label records the dependencies, which the image lacks", - label: `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}}]`, - wantFetched: []string{"ghcr.io/x/a:1", "ghcr.io/x/c:1"}, - want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + name: "baked declared feature, label records its dependencies", + declared: `{"ghcr.io/x/b:1":{}}`, + label: `[` + bakedB + `,"dependsOn":{"ghcr.io/x/c:1":{}}},` + bakedC + `]`, + wantFetched: []string{"ghcr.io/x/c:1"}, + want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)"}, }, { - name: "label written before dependencies were recorded", + name: "baked declared feature, label written before dependencies were recorded", + declared: `{"ghcr.io/x/b:1":{}}`, label: `[` + bakedB + `}]`, - wantFetched: []string{"ghcr.io/x/a:1", "ghcr.io/x/b@" + digestOf('b'), "ghcr.io/x/c:1"}, - want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)", "ghcr.io/x/a:1"}, + wantFetched: []string{"ghcr.io/x/b@" + digestOf('b'), "ghcr.io/x/c:1"}, + want: []string{"ghcr.io/x/c:1", "ghcr.io/x/b:1(baked)"}, }, } { t.Run(tc.name, func(t *testing.T) { - f := newDepFixture(t, `{"ghcr.io/x/a:1":{}}`, table) + f := newDepFixture(t, tc.declared, table) f.rt.imagesByRef["alpine:3.20"] = &runtime.ImageDetails{Labels: map[string]string{feature.MetadataLabel: tc.label}} cfg, err := f.layer(LockfileDefault) if err != nil { diff --git a/design/features.md b/design/features.md index cd6f48c..e8f91e2 100644 --- a/design/features.md +++ b/design/features.md @@ -225,33 +225,29 @@ Decisions: Declared local features keep their existing, unconfined resolution: the user wrote those paths, and confining them would break configs that point outside the config directory. -4. **A dependency the base image's label satisfies is not fetched or - installed again**, using the same match as declared features - (`bakedFeature`, which includes the lockfile check of §8.2). **Its - own dependencies are still followed**, as are a declared baked - feature's, because an image built before this change does not have - them. Labels now record each installed feature's `dependsOn` (as - `{}` when it has none), so for an image built from now on they come - from the label without a fetch. For a label written before that, - the baked feature's metadata is fetched at the artifact the label - records, a digest reference the feature cache serves locally once - seen, to find them. The baked feature itself is never reinstalled. - - **Known limitation, accepted here:** the baked match does not - compare options. A dependency requested with options other than the - ones the image installed it with is still taken from the image, so - those options are not applied. This is the pre-baked match's - behaviour for declared features since it was introduced (id and - version only). Fixing it means recording applied options in the - label and comparing them after applying defaults, which the label - cannot do today. That is a change to the pre-baked match itself, - tracked in #154, not to dependency resolution. -6. **A dependency dropped as a duplicate is still locked.** When a +4. **A dependency is never taken from the base image**, even when its + label lists it: it is fetched and installed like any other feature. + The pre-baked match compares id and version only, and a `dependsOn` + entry's options are chosen by the feature's author, not the user, so + an image built with other options would silently stand in. #154 adds + option matching to the pre-baked match; dependencies can reuse the + image once it lands. The cost until then: a pre-baked image whose + features have dependencies reinstalls those dependencies on every + build. + + A declared feature the image carries is still not reinstalled, but + its dependencies are installed, so they must be known. Labels now + record each installed feature's `dependsOn` (as `{}` when it has + none), so for an image built from now on they come from the label + without a fetch. For a label written before that, the baked + feature's metadata is fetched at the artifact the label records, a + digest reference the feature cache serves locally once seen. +5. **A dependency dropped as a duplicate is still locked.** When a `dependsOn` key resolves to the same content and options as a feature already in the set, it is not installed twice, but its key still gets a lockfile entry for that content, so the reference it was requested by stays pinned. -5. **OCI references are lowercased before parsing**, as the CLI's +6. **OCI references are lowercased before parsing**, as the CLI's `getRef` does. Published `dependsOn` keys such as `ghcr.io/codspace/dependson/A` are written by feature authors, not the user, and go-containerregistry rejects uppercase repository diff --git a/up.go b/up.go index a097334..9c1a295 100644 --- a/up.go +++ b/up.go @@ -452,11 +452,14 @@ func (e *Engine) bakedFeature(f config.ResolvedFeature, baked []config.FeatureMe } // dependencies returns the features f's dependsOn requires that -// features does not already hold, in key order, unfetched. One the -// base image's label satisfies is returned AlreadyInstalled, as -// markAlreadyInstalled marks a declared feature, and so is not -// fetched; layerFeatures still follows its own dependencies. -func (e *Engine) dependencies(features []config.ResolvedFeature, f config.ResolvedFeature, baked []config.FeatureMetadata, lock *featureLock, configDir string, bus *eventBus) ([]config.ResolvedFeature, error) { +// features does not already hold, in key order, unfetched. +// +// A dependency is never taken from the base image, even when its label +// lists it: the pre-baked match compares id and version only, and the +// options a dependsOn entry asks for are its author's, not the user's, +// so an image built with other options would silently stand in (#154). +// It is fetched and installed like any other feature. +func dependencies(features []config.ResolvedFeature, f config.ResolvedFeature, configDir string) ([]config.ResolvedFeature, error) { keys := make([]string, 0, len(f.Metadata.DependsOn)) for key := range f.Metadata.DependsOn { keys = append(keys, key) @@ -475,12 +478,6 @@ func (e *Engine) dependencies(features []config.ResolvedFeature, f config.Resolv return nil, err } } - if meta, ok := e.bakedFeature(dep, baked, lock); ok { - dep.AlreadyInstalled = true - dep.Metadata = meta - dep.ResolvedRef = meta.ResolvedRef - bus.Emit(events.FeatureSkippedEvent{Ref: key, Reason: "base_image_label"}) - } out = append(out, dep) } return out, nil @@ -1435,10 +1432,11 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, for i := 0; i < len(cfg.Features); i++ { f := &cfg.Features[i] if f.AlreadyInstalled { - // The base image installs f, but its dependencies only if the - // image was built with them: from a label that records them, - // or, for a label written before they were recorded, as f's - // own metadata names them. f itself is not installed again. + // The base image installs f; its dependencies are installed + // regardless (see dependencies), so they must be known: from + // a label that records them, or, for a label written before + // they were recorded, from f's own metadata. f itself is not + // installed again. if f.Metadata.DependsOn == nil { deps, err := e.bakedDependsOn(ctx, *f) if err != nil { @@ -1446,7 +1444,7 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, } f.Metadata.DependsOn = deps } - deps, err := e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, configDir, opts.bus) + deps, err := dependencies(cfg.Features, cfg.Features[i], configDir) if err != nil { return "", nil, err } @@ -1499,7 +1497,7 @@ func (e *Engine) layerFeatures(ctx context.Context, cfg *config.ResolvedConfig, i-- continue } - deps, err := e.dependencies(cfg.Features, cfg.Features[i], baseMeta, lock, configDir, opts.bus) + deps, err := dependencies(cfg.Features, cfg.Features[i], configDir) if err != nil { return "", nil, err }