diff --git a/CHANGELOG.md b/CHANGELOG.md index 68adca8..84f5511 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