From fbe98cdb328de874f5ae259cd1bb60e9df632475 Mon Sep 17 00:00:00 2001 From: Josh Sandlin Date: Thu, 6 Aug 2026 11:25:11 -0400 Subject: [PATCH] fix this up --- .github/workflows/deploy.yml | 4 +-- sidecar/internal/reaper/reaper.go | 14 ++++++++-- sidecar/internal/reaper/reaper_test.go | 26 +++++++++++++++++- sidecar/internal/registry/registry.go | 9 +++++++ sidecar/internal/registry/registry_test.go | 17 ++++++++++++ sidecar/internal/server/server.go | 16 +++++++++++ sidecar/internal/server/server_test.go | 31 ++++++++++++++++++++++ 7 files changed, 112 insertions(+), 5 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index f0e6c8f..bfeb3a7 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -3,8 +3,8 @@ name: Deploy ttl.sh on: push: branches: - # - prerelease # Temporary: deploy on prerelease for testing to staging.ttl.sh - - main + - joshs/tuning # Temporary: deploy on prerelease for testing to staging.ttl.sh + # - main workflow_dispatch: # Manual trigger for emergencies concurrency: diff --git a/sidecar/internal/reaper/reaper.go b/sidecar/internal/reaper/reaper.go index 35f36a5..1406341 100644 --- a/sidecar/internal/reaper/reaper.go +++ b/sidecar/internal/reaper/reaper.go @@ -6,9 +6,11 @@ package reaper import ( "context" + "errors" "log" "time" + "github.com/replicatedhq/ttl.sh/sidecar/internal/registry" "github.com/replicatedhq/ttl.sh/sidecar/internal/store" ) @@ -71,11 +73,19 @@ func (r *Reaper) sweepOnce(ctx context.Context) { } log.Printf("sweep: %d expired tag(s)", len(rows)) for _, row := range rows { - if err := r.registry.DeleteManifest(ctx, row.Repository, row.Tag); err != nil { + switch err := r.registry.DeleteManifest(ctx, row.Repository, row.Tag); { + case err == nil: + log.Printf("sweep: deleted %s:%s", row.Repository, row.Tag) + case errors.Is(err, registry.ErrManifestReferenced): + // Retrying can never succeed, so drop the row instead of failing + // this pair on every tick. The index that holds this manifest has a + // row of its own; once that expires, zot's untagged retention + // collects the child. + log.Printf("sweep: %s:%s held by an index, untracking: %v", row.Repository, row.Tag, err) + default: log.Printf("sweep: delete %s:%s: %v", row.Repository, row.Tag, err) continue } - log.Printf("sweep: deleted %s:%s", row.Repository, row.Tag) if err := r.store.Delete(row.Repository, row.Tag); err != nil { log.Printf("sweep: row delete %s:%s: %v", row.Repository, row.Tag, err) } diff --git a/sidecar/internal/reaper/reaper_test.go b/sidecar/internal/reaper/reaper_test.go index e4c4cc1..f4c57d2 100644 --- a/sidecar/internal/reaper/reaper_test.go +++ b/sidecar/internal/reaper/reaper_test.go @@ -3,10 +3,12 @@ package reaper import ( "context" "errors" + "fmt" "sync" "testing" "time" + "github.com/replicatedhq/ttl.sh/sidecar/internal/registry" "github.com/replicatedhq/ttl.sh/sidecar/internal/store" ) @@ -43,13 +45,17 @@ func (f *fakeStore) deletedCalls() [][2]string { type fakeDeleter struct { mu sync.Mutex calls [][2]string - failOn map[string]bool // key "repo:tag" -> return error + failOn map[string]bool // key "repo:tag" -> return a generic error + errOn map[string]error // key "repo:tag" -> return this specific error } func (d *fakeDeleter) DeleteManifest(_ context.Context, repo, tag string) error { d.mu.Lock() defer d.mu.Unlock() d.calls = append(d.calls, [2]string{repo, tag}) + if err, ok := d.errOn[repo+":"+tag]; ok { + return err + } if d.failOn[repo+":"+tag] { return errors.New("boom") } @@ -134,6 +140,24 @@ func TestSweepOnceDeleterErrorPreservesRow(t *testing.T) { } } +// A child manifest of a multi-arch index can never be deleted on its own, so +// the row has to be untracked rather than retried on every tick — the opposite +// of how an ordinary delete failure is handled. +func TestSweepOnceUntracksManifestReferencedByIndex(t *testing.T) { + digest := "sha256:d3d669c9a5ef6483b05164101265237d0fff3a6495f659242262a1d8d68e2dda" + fs := &fakeStore{expired: []store.Row{row("r", digest)}} + fd := &fakeDeleter{errOn: map[string]error{ + "r:" + digest: fmt.Errorf("DELETE ...: %w", registry.ErrManifestReferenced), + }} + rp := New(time.Hour, fs, fd) + + rp.sweepOnce(context.Background()) + + if got := fs.deletedCalls(); len(got) != 1 || got[0] != [2]string{"r", digest} { + t.Fatalf("store.Delete calls = %v, want one [r %s]", got, digest) + } +} + func TestSweepOnceStoreDeleteError(t *testing.T) { fs := &fakeStore{ expired: []store.Row{row("r", "old")}, diff --git a/sidecar/internal/registry/registry.go b/sidecar/internal/registry/registry.go index 9f35789..997e68c 100644 --- a/sidecar/internal/registry/registry.go +++ b/sidecar/internal/registry/registry.go @@ -4,12 +4,19 @@ package registry import ( "context" + "errors" "fmt" "io" "net/http" "time" ) +// ErrManifestReferenced is zot's 405/DENIED: the reference names a manifest an +// index still points at. Despite the "access denied" wording it is not an +// authorization failure, and it will not clear on retry — only deleting the +// index releases the child. +var ErrManifestReferenced = errors.New("manifest is referenced by an index") + type Client struct { baseURL string http *http.Client @@ -45,6 +52,8 @@ func (c *Client) DeleteManifest(ctx context.Context, repo, tag string) error { switch resp.StatusCode { case http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound: return nil + case http.StatusMethodNotAllowed: + return fmt.Errorf("DELETE %s: %w", endpoint, ErrManifestReferenced) default: // zot describes the failure in the body; %q keeps it on one log line. return fmt.Errorf("DELETE %s -> %d: %q", endpoint, resp.StatusCode, body) diff --git a/sidecar/internal/registry/registry_test.go b/sidecar/internal/registry/registry_test.go index 841c80f..6462c9f 100644 --- a/sidecar/internal/registry/registry_test.go +++ b/sidecar/internal/registry/registry_test.go @@ -2,6 +2,7 @@ package registry import ( "context" + "errors" "net/http" "net/http/httptest" "testing" @@ -54,6 +55,22 @@ func TestDeleteManifestStatus(t *testing.T) { } } +// zot answers 405/DENIED when the reference is a manifest an index still +// points at. That has to arrive as ErrManifestReferenced, not as a generic +// error, or the reaper would retry it every tick forever. +func TestDeleteManifestReferencedByIndex(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusMethodNotAllowed) + _, _ = w.Write([]byte(`{"errors":[{"code":"DENIED","message":"requested access to the resource is denied"}]}`)) + })) + defer srv.Close() + + err := New(srv.URL).DeleteManifest(context.Background(), "foo/bar", "sha256:abc") + if !errors.Is(err, ErrManifestReferenced) { + t.Fatalf("err = %v, want ErrManifestReferenced", err) + } +} + // Port 1 refuses connections, so http.Client.Do fails and the error surfaces. func TestDeleteManifestTransportError(t *testing.T) { c := New("http://127.0.0.1:1") diff --git a/sidecar/internal/server/server.go b/sidecar/internal/server/server.go index 93f18cb..22d52d7 100644 --- a/sidecar/internal/server/server.go +++ b/sidecar/internal/server/server.go @@ -6,6 +6,7 @@ import ( "io" "log" "net/http" + "strings" "time" "github.com/replicatedhq/ttl.sh/sidecar/internal/events" @@ -68,6 +69,15 @@ func (s *Server) handleEvents(w http.ResponseWriter, r *http.Request) { return } + // A multi-arch push emits one event per child manifest, referenced by + // digest, before the event for the index's tag. zot refuses to delete a + // manifest an index still references, so tracking those children would only + // produce rows the reaper can never clear; they go away with their index. + if isDigest(evt.Data.Reference) { + w.WriteHeader(http.StatusNoContent) + return + } + now := s.clock() expires := ttl.ComputeExpiry(evt.Data.Reference, now, s.defaultTTL, s.maxTTL) @@ -96,6 +106,12 @@ func (s *Server) Routes() http.Handler { return mux } +// isDigest reports whether reference is a digest rather than a tag. An OCI tag +// cannot contain a colon, so the separator alone is decisive. +func isDigest(reference string) bool { + return strings.Contains(reference, ":") +} + func shortDigest(d string) string { if len(d) > 19 { // "sha256:" + 12 hex chars return d[:19] diff --git a/sidecar/internal/server/server_test.go b/sidecar/internal/server/server_test.go index 2439285..586b617 100644 --- a/sidecar/internal/server/server_test.go +++ b/sidecar/internal/server/server_test.go @@ -90,6 +90,37 @@ func TestHandleEventsImageUpdatedUpserts(t *testing.T) { } } +// A multi-arch push emits an image.updated per child manifest, referenced by +// digest, before the one carrying the index's tag. Only the tag is trackable: +// zot answers 405/DENIED to a delete of an index's child, so recording the +// children would wedge the reaper on rows it can never clear. +func TestHandleEventsIgnoresDigestReference(t *testing.T) { + srv, fs := newTestServer() + digest := "sha256:d3d669c9a5ef6483b05164101265237d0fff3a6495f659242262a1d8d68e2dda" + incoming := []events.ImageUpdatedData{ + {Name: "foo/bar", Reference: digest, Digest: digest}, // child manifest + {Name: "foo/bar", Reference: "sha256:abc", Digest: "sha256:abc"}, // child manifest + {Name: "foo/bar", Reference: "1h", Digest: "sha256:index"}, // the index's tag + } + for _, data := range incoming { + body, _ := json.Marshal(data) + req := httptest.NewRequest(http.MethodPost, "/events", bytes.NewReader(body)) + req.Header.Set("Ce-Type", events.ImageUpdatedType) + rec := httptest.NewRecorder() + srv.handleEvents(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("reference %q: got %d want 204", data.Reference, rec.Code) + } + } + + if len(fs.rows) != 1 { + t.Fatalf("rows = %+v, want only the tagged index", fs.rows) + } + if fs.rows[0].Tag != "1h" { + t.Errorf("tracked tag = %q, want %q", fs.rows[0].Tag, "1h") + } +} + func TestHandleEventsOtherTypeAcked(t *testing.T) { srv, fs := newTestServer() body := []byte(`{"name":"foo","reference":"1h","digest":"sha256:x"}`)