mirror of
https://tangled.org/evan.jarrett.net/at-container-registry
synced 2026-09-03 00:36:56 +00:00
c035f50fixed a hand-built tag rkey in DeleteManifestHandler and shipped with no test. The hazard is not specific to that handler: io.atcr.tag rkeys come from RepositoryTagToRKey, which encodes "/" as "~", so any code building one by hand targets a record that does not exist — and deleteRecord being idempotent makes that a silent no-op. The local view looks right and the tag returns on the next backfill. The by-digest path now builds tag rkeys too (594d73b), so it could reintroduce exactly this bug. TestManifestDelete_NestedRepoTagRKey pins it there: push to stream/cache, delete by digest, and assert the tag is no longer listed. Listing is what catches a survivor — TagStore.All reads the records back from the PDS and filters by repository, so a stale one is still reported. Mutation-verified by hand-building the rkey as "repo:tag": the nested test fails with the tag still listed, and TestManifestDelete passes unchanged. That second half is the point — every existing delete test uses a flat repository name, and a flat name cannot reproduce this bug at all. batch11-nested-rkey.mjs drives the same property through the UI handler thatc035f50actually fixed, asserting against the PDS record rather than the page, since the page looks correct either way until a backfill runs. It needs an interactive appview login in the Playwright profile and is not yet run; the session that exists belongs to a different browser profile. Two instrument notes are baked in: probe /settings rather than the repo page to detect a session, because /r/ renders for anonymous visitors and can never report a missing one, and use maxRedirects:0, because RequireAuth 302s and a followed redirect surfaces as a confusing 405 on DELETE /login. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
235 lines
8.2 KiB
Go
235 lines
8.2 KiB
Go
//go:build integration
|
|
|
|
package integration
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"slices"
|
|
"testing"
|
|
|
|
"github.com/google/go-containerregistry/pkg/crane"
|
|
"github.com/google/go-containerregistry/pkg/v1/random"
|
|
|
|
"atcr.io/internal/testharness"
|
|
|
|
_ "github.com/distribution/distribution/v3/registry/auth/token"
|
|
_ "github.com/distribution/distribution/v3/registry/storage/driver/inmemory"
|
|
)
|
|
|
|
// TestManifestDelete is the end-to-end regression test for OCI manifest
|
|
// deletion. It guards the core fix (storage.delete.enabled) — without it,
|
|
// distribution v3.1.1's DeleteManifest handler answers every DELETE with an
|
|
// UNSUPPORTED error before reaching our ATProto-backed stores, which is exactly
|
|
// the `skopeo delete ... unsupported` symptom this feature addresses.
|
|
//
|
|
// It covers both reference forms an OCI DELETE can take:
|
|
// - by digest (what `skopeo delete` issues after resolving the tag) -> our
|
|
// ManifestStore.Delete
|
|
// - by tag -> our TagStore.Untag
|
|
func TestManifestDelete(t *testing.T) {
|
|
h := testharness.New(t)
|
|
alice := h.AddSailor("alice.test")
|
|
creds := h.RegistryCreds(alice)
|
|
|
|
authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure}
|
|
|
|
t.Run("by digest", func(t *testing.T) {
|
|
repoRef := fmt.Sprintf("%s/%s/del-digest", h.AppViewHostPort(), alice.Handle())
|
|
tagRef := repoRef + ":tag"
|
|
|
|
img, err := random.Image(1<<20, 2)
|
|
if err != nil {
|
|
t.Fatalf("build image: %v", err)
|
|
}
|
|
if err := crane.Push(img, tagRef, authOpts...); err != nil {
|
|
t.Fatalf("push: %v", err)
|
|
}
|
|
|
|
dgst, err := img.Digest()
|
|
if err != nil {
|
|
t.Fatalf("digest: %v", err)
|
|
}
|
|
digestRef := fmt.Sprintf("%s@%s", repoRef, dgst.String())
|
|
|
|
// Sanity: it is pullable before deletion.
|
|
if _, err := crane.Head(tagRef, authOpts...); err != nil {
|
|
t.Fatalf("pre-delete head: %v", err)
|
|
}
|
|
|
|
// Delete by digest — must succeed (202), not return UNSUPPORTED.
|
|
if err := crane.Delete(digestRef, authOpts...); err != nil {
|
|
t.Fatalf("delete by digest: %v", err)
|
|
}
|
|
|
|
// The manifest (and its tag) are gone.
|
|
if _, err := crane.Head(digestRef, authOpts...); err == nil {
|
|
t.Error("expected HEAD by digest to fail after delete, got nil")
|
|
}
|
|
if _, err := crane.Head(tagRef, authOpts...); err == nil {
|
|
t.Error("expected HEAD by tag to fail after digest delete, got nil")
|
|
}
|
|
})
|
|
|
|
t.Run("by tag", func(t *testing.T) {
|
|
tagRef := fmt.Sprintf("%s/%s/del-tag:tag", h.AppViewHostPort(), alice.Handle())
|
|
|
|
img, err := random.Image(1<<20, 2)
|
|
if err != nil {
|
|
t.Fatalf("build image: %v", err)
|
|
}
|
|
if err := crane.Push(img, tagRef, authOpts...); err != nil {
|
|
t.Fatalf("push: %v", err)
|
|
}
|
|
|
|
// Delete by tag — must succeed, exercising TagStore.Untag.
|
|
if err := crane.Delete(tagRef, authOpts...); err != nil {
|
|
t.Fatalf("delete by tag: %v", err)
|
|
}
|
|
|
|
// The tag no longer resolves.
|
|
if _, err := crane.Head(tagRef, authOpts...); err == nil {
|
|
t.Error("expected HEAD by tag to fail after tag delete, got nil")
|
|
}
|
|
})
|
|
|
|
t.Run("anonymous delete denied", func(t *testing.T) {
|
|
tagRef := fmt.Sprintf("%s/%s/del-anon:tag", h.AppViewHostPort(), alice.Handle())
|
|
|
|
img, err := random.Image(1<<20, 1)
|
|
if err != nil {
|
|
t.Fatalf("build image: %v", err)
|
|
}
|
|
if err := crane.Push(img, tagRef, authOpts...); err != nil {
|
|
t.Fatalf("push: %v", err)
|
|
}
|
|
|
|
// No credentials: the token endpoint must refuse a delete scope, so the
|
|
// DELETE fails and the image remains pullable by its owner.
|
|
if err := crane.Delete(tagRef, crane.Insecure); err == nil {
|
|
t.Error("expected anonymous delete to be denied, got nil")
|
|
}
|
|
if _, err := crane.Head(tagRef, authOpts...); err != nil {
|
|
t.Errorf("image should survive denied anonymous delete: %v", err)
|
|
}
|
|
})
|
|
}
|
|
|
|
// TestManifestDelete_SharedDigestAcrossRepos is the two-repo case the plan
|
|
// calls for, and it is the one shape none of the delete tests covered.
|
|
//
|
|
// The io.atcr.manifest record is keyed by digest alone (ManifestStore.Delete →
|
|
// digestToRKey), so a single record backs every repository of a user holding
|
|
// identical content. Deleting through one repository therefore reaches into
|
|
// every other repository sharing that digest — and ManifestStore.Delete then
|
|
// calls purgeDeletedManifest, which asks the hold to drop the layer records and
|
|
// free the blobs. The second repo loses both its manifest and its bytes.
|
|
//
|
|
// 2580dcd fixed exactly this class for the tag-delete UI handler and added the
|
|
// helper for it (db.ShouldCascadeDeleteManifest / db.IsManifestTaggedAnyRepo),
|
|
// but the OCI path never consulted it.
|
|
func TestManifestDelete_SharedDigestAcrossRepos(t *testing.T) {
|
|
h := testharness.New(t)
|
|
alice := h.AddSailor("alice.test")
|
|
creds := h.RegistryCreds(alice)
|
|
authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure}
|
|
|
|
repoA := fmt.Sprintf("%s/%s/shared-a", h.AppViewHostPort(), alice.Handle())
|
|
repoB := fmt.Sprintf("%s/%s/shared-b", h.AppViewHostPort(), alice.Handle())
|
|
tagA, tagB := repoA+":v1", repoB+":v1"
|
|
|
|
img, err := random.Image(1<<20, 2)
|
|
if err != nil {
|
|
t.Fatalf("build image: %v", err)
|
|
}
|
|
if err := crane.Push(img, tagA, authOpts...); err != nil {
|
|
t.Fatalf("push a: %v", err)
|
|
}
|
|
// The same content in a second repository — what `crane copy` produces, and
|
|
// what any two repos built from the same base layer produce naturally.
|
|
if err := crane.Push(img, tagB, authOpts...); err != nil {
|
|
t.Fatalf("push b: %v", err)
|
|
}
|
|
|
|
dgst, err := img.Digest()
|
|
if err != nil {
|
|
t.Fatalf("digest: %v", err)
|
|
}
|
|
if _, err := crane.Head(tagB, authOpts...); err != nil {
|
|
t.Fatalf("pre-delete head b: %v", err)
|
|
}
|
|
|
|
// Delete through repo A only.
|
|
if err := crane.Delete(fmt.Sprintf("%s@%s", repoA, dgst.String()), authOpts...); err != nil {
|
|
t.Fatalf("delete a by digest: %v", err)
|
|
}
|
|
|
|
// Repo B must be untouched. Pull rather than head: the manifest and the
|
|
// layer bytes are two separate casualties here, and a HEAD would only
|
|
// notice the first.
|
|
if _, err := crane.Head(tagB, authOpts...); err != nil {
|
|
t.Fatalf("repo B lost its manifest when repo A was deleted: %v", err)
|
|
}
|
|
if _, err := (craneClient{}).Pull(context.Background(), tagB, creds); err != nil {
|
|
t.Fatalf("repo B is no longer pullable after repo A was deleted: %v", err)
|
|
}
|
|
}
|
|
|
|
// TestManifestDelete_NestedRepoTagRKey covers the encoding hazard c035f50 fixed
|
|
// in the UI handler, on the by-digest path.
|
|
//
|
|
// io.atcr.tag rkeys come from RepositoryTagToRKey, which encodes "/" as "~", so
|
|
// stream/cache:v1 is stored as "stream~cache_v1". Any code that hand-builds the
|
|
// rkey ("repo:tag", "repo_tag") targets a record that does not exist, and
|
|
// because deleteRecord is idempotent it fails silently: the local view looks
|
|
// right and the tag comes back on the next backfill.
|
|
//
|
|
// A flat repository name cannot reproduce it — the bug is entirely in the "/"
|
|
// encoding — which is why every earlier delete test missed it. Listing tags
|
|
// after the delete is what catches a surviving record: TagStore.All reads the
|
|
// records back from the PDS and filters by repository, so a stale one is still
|
|
// reported.
|
|
func TestManifestDelete_NestedRepoTagRKey(t *testing.T) {
|
|
h := testharness.New(t)
|
|
alice := h.AddSailor("alice.test")
|
|
creds := h.RegistryCreds(alice)
|
|
authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure}
|
|
|
|
repo := fmt.Sprintf("%s/%s/stream/cache", h.AppViewHostPort(), alice.Handle())
|
|
tagRef := repo + ":v1"
|
|
|
|
img, err := random.Image(1<<20, 2)
|
|
if err != nil {
|
|
t.Fatalf("build image: %v", err)
|
|
}
|
|
if err := crane.Push(img, tagRef, authOpts...); err != nil {
|
|
t.Fatalf("push nested: %v", err)
|
|
}
|
|
|
|
tags, err := crane.ListTags(repo, authOpts...)
|
|
if err != nil {
|
|
t.Fatalf("list tags before delete: %v", err)
|
|
}
|
|
if !slices.Contains(tags, "v1") {
|
|
t.Fatalf("fixture is wrong: v1 not listed before delete, got %v", tags)
|
|
}
|
|
|
|
dgst, err := img.Digest()
|
|
if err != nil {
|
|
t.Fatalf("digest: %v", err)
|
|
}
|
|
if err := crane.Delete(fmt.Sprintf("%s@%s", repo, dgst.String()), authOpts...); err != nil {
|
|
t.Fatalf("delete nested by digest: %v", err)
|
|
}
|
|
|
|
tags, err = crane.ListTags(repo, authOpts...)
|
|
if err != nil {
|
|
t.Fatalf("list tags after delete: %v", err)
|
|
}
|
|
if slices.Contains(tags, "v1") {
|
|
t.Errorf("tag v1 survived the delete on a nested repository: %v — the tag rkey did not match "+
|
|
"the slash-encoded key the write path uses, so the record is still on the PDS and will "+
|
|
"reappear on the next backfill", tags)
|
|
}
|
|
}
|