diff --git a/CHANGELOG.md b/CHANGELOG.md index 68adca8..1d86cd0 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. @@ -45,6 +52,49 @@ 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) +- **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. + - 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 + 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 + 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 a5f91e1..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 @@ -257,6 +260,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/depends_on_test.go b/depends_on_test.go new file mode 100644 index 0000000..ad84154 --- /dev/null +++ b/depends_on_test.go @@ -0,0 +1,462 @@ +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 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": {}})}, + "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, declared, label string + wantFetched, want []string + }{ + { + 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", "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: "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: "baked declared feature, label written before dependencies were recorded", + declared: `{"ghcr.io/x/b:1":{}}`, + label: `[` + bakedB + `}]`, + 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, 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 { + 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) + } +} + +// 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')) + } + }) + } +} + +// 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 3085114..e8f91e2 100644 --- a/design/features.md +++ b/design/features.md @@ -162,8 +162,99 @@ 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. 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 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. +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 + 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 @@ -369,7 +460,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 +475,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 @@ -398,11 +504,20 @@ 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 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/engine_test.go b/engine_test.go index e3cb445..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"}]`, + "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 6f12f13..5cbec13 100644 --- a/feature/dockerfile_test.go +++ b/feature/dockerfile_test.go @@ -226,6 +226,76 @@ 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) + } +} + +// 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 2d6fd0c..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) } @@ -100,6 +110,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), @@ -116,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, @@ -205,7 +220,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 +241,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), @@ -242,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.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..d0f20f2 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 + `","dependsOn":{}}]` + } + 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","dependsOn":{}}]`, 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","dependsOn":{}}]`, 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/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) { 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 2d1a588..9c1a295 100644 --- a/up.go +++ b/up.go @@ -6,8 +6,10 @@ import ( "fmt" "os" "path/filepath" + "reflect" "slices" "sort" + "strings" "time" composetypes "github.com/compose-spec/compose-go/v2/types" @@ -397,7 +399,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,36 +410,151 @@ 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) { - mode := feature.MatchPermissive - if e.opts.StrictFeatureVersionMatch { - mode = feature.MatchStrict - } +func (e *Engine) markAlreadyInstalled(cfg *config.ResolvedConfig, baked []config.FeatureMetadata, lock *featureLock) { 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. +// +// 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) + } + 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) { - f.AlreadyInstalled = true - // Preserve baked metadata so the regenerated label can - // include it without re-fetching. - f.Metadata = b - break + 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 } } + out = append(out, dep) } + 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 +// 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 @@ -1273,26 +1393,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 @@ -1300,10 +1421,37 @@ 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; 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 { + return "", nil, err + } + f.Metadata.DependsOn = deps + } + deps, err := dependencies(cfg.Features, cfg.Features[i], configDir) + if err != nil { + return "", nil, err + } + cfg.Features = append(cfg.Features, deps...) + continue + } + if f.Dir != "" { continue } ref := f.Ref @@ -1340,9 +1488,23 @@ 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 + } + deps, err := dependencies(cfg.Features, cfg.Features[i], configDir) + if err != nil { + return "", nil, err + } + cfg.Features = append(cfg.Features, deps...) } - 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