diff --git a/sidecar/internal/registry/registry.go b/sidecar/internal/registry/registry.go index 997e68c..3069fe1 100644 --- a/sidecar/internal/registry/registry.go +++ b/sidecar/internal/registry/registry.go @@ -4,10 +4,12 @@ package registry import ( "context" + "encoding/json" "errors" "fmt" "io" "net/http" + "slices" "time" ) @@ -33,10 +35,37 @@ func New(baseURL string) *Client { } } +// gone lists the distribution error codes that mean the delete has nothing left +// to do: the repository or the manifest is already absent. zot returns +// NAME_UNKNOWN with a 400, not a 404, once a repo's last tag is reaped and the +// repo itself disappears — a cosign .sig tag outliving its image hits this. +var gone = []string{"NAME_UNKNOWN", "MANIFEST_UNKNOWN"} + +// isGone reports whether an error body carries one of the `gone` codes. Matching +// on the body rather than the status keeps a genuinely malformed request, which +// is also a 400, retryable and visible. +func isGone(body []byte) bool { + var payload struct { + Errors []struct { + Code string `json:"code"` + } `json:"errors"` + } + if err := json.Unmarshal(body, &payload); err != nil { + return false + } + for _, e := range payload.Errors { + if slices.Contains(gone, e.Code) { + return true + } + } + return false +} + // DeleteManifest issues DELETE /v2//manifests/; zot resolves the tag -// to its digest. 200/202/204/404 all mean the tag is gone; any other status is -// a transient error to retry on the next tick. ctx bounds the request, so a -// wedged registry cannot outlive a shutdown. +// to its digest. 200/202/204/404, and any error naming a missing repo or +// manifest, all mean the tag is gone; any other status is a transient error to +// retry on the next tick. ctx bounds the request, so a wedged registry cannot +// outlive a shutdown. func (c *Client) DeleteManifest(ctx context.Context, repo, tag string) error { endpoint := fmt.Sprintf("%s/v2/%s/manifests/%s", c.baseURL, repo, tag) req, err := http.NewRequestWithContext(ctx, http.MethodDelete, endpoint, nil) @@ -49,10 +78,14 @@ func (c *Client) DeleteManifest(ctx context.Context, repo, tag string) error { } defer func() { _ = resp.Body.Close() }() body, _ := io.ReadAll(resp.Body) - switch resp.StatusCode { - case http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound: + switch { + case resp.StatusCode == http.StatusOK, + resp.StatusCode == http.StatusAccepted, + resp.StatusCode == http.StatusNoContent, + resp.StatusCode == http.StatusNotFound, + isGone(body): return nil - case http.StatusMethodNotAllowed: + case resp.StatusCode == 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. diff --git a/sidecar/internal/registry/registry_test.go b/sidecar/internal/registry/registry_test.go index 6462c9f..fc52c70 100644 --- a/sidecar/internal/registry/registry_test.go +++ b/sidecar/internal/registry/registry_test.go @@ -71,6 +71,63 @@ func TestDeleteManifestReferencedByIndex(t *testing.T) { } } +// Once a repo's last tag is reaped the repo itself disappears, and zot answers a +// delete against it with 400/NAME_UNKNOWN rather than 404. A cosign .sig tag +// outliving its image lands here, so it has to read as success — otherwise the +// row is never dropped and the reaper retries it every tick forever. +func TestDeleteManifestRepoAlreadyGone(t *testing.T) { + cases := []struct { + name string + status int + body string + wantErr bool + }{ + { + name: "NAME_UNKNOWN", + status: http.StatusBadRequest, + body: `{"errors":[{"code":"NAME_UNKNOWN","message":"repository name not known to registry","detail":{"name":"c1e2d663-74a5-47d1-a395-d1b8cdacb137"}}]}`, + }, + { + name: "MANIFEST_UNKNOWN", + status: http.StatusBadRequest, + body: `{"errors":[{"code":"MANIFEST_UNKNOWN","message":"manifest unknown"}]}`, + }, + { + // A malformed request is also a 400, and that one is worth retrying + // and surfacing rather than silently untracking. + name: "other 400 stays an error", + status: http.StatusBadRequest, + body: `{"errors":[{"code":"UNSUPPORTED","message":"the operation is unsupported"}]}`, + wantErr: true, + }, + { + name: "unparseable body stays an error", + status: http.StatusBadRequest, + body: `not json`, + wantErr: true, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + })) + defer srv.Close() + + err := New(srv.URL).DeleteManifest(context.Background(), "foo/bar", "sha256-abc.sig") + if tc.wantErr && err == nil { + t.Fatal("expected error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("expected nil, got error: %v", 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")