From 05e8c64e476334ef68db70b51646b60042fb5f79 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Thu, 24 Sep 2026 22:58:07 +0800 Subject: [PATCH] =?UTF-8?q?fix(imagepin):=20=E5=B8=A6=20digest=20=E7=9A=84?= =?UTF-8?q?=E9=95=9C=E5=83=8F=E5=BC=95=E7=94=A8=E4=B9=9F=E5=90=91=20regist?= =?UTF-8?q?ry=20=E7=A1=AE=E8=AE=A4=E4=BB=8D=E5=AD=98=E5=9C=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/api/images.go | 3 +++ internal/imagepin/imagepin.go | 19 +++++++++++++--- internal/imagepin/imagepin_test.go | 35 +++++++++++++++++++++++++----- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/internal/api/images.go b/internal/api/images.go index 6cc4555..3a96064 100644 --- a/internal/api/images.go +++ b/internal/api/images.go @@ -270,6 +270,9 @@ func (a *API) pinImage(ctx context.Context, ref string) (string, error) { } pinned, err := a.Images.Pin(ctx, ref) switch { + case errors.Is(err, imagepin.ErrNotFound) && imagepin.Pinned(ref): + return "", newError(http.StatusBadRequest, "image_not_in_registry", + "the registry no longer holds build %q (nothing referenced it, so it was pruned); pick a current tag", ref) case errors.Is(err, imagepin.ErrNotFound): return "", newError(http.StatusBadRequest, "image_not_in_registry", "image %q is whitelisted but the registry does not hold it; build or push it first", ref) diff --git a/internal/imagepin/imagepin.go b/internal/imagepin/imagepin.go index 9b8663b..67f132e 100644 --- a/internal/imagepin/imagepin.go +++ b/internal/imagepin/imagepin.go @@ -73,10 +73,23 @@ func (r Resolver) Covers(ref string) bool { return r.Registry != "" && strings.HasPrefix(ref, r.Registry+"/") } -// Pin returns ref with the digest its tag names now appended. A ref that already -// carries a digest, or lives outside the registry, comes back unchanged. +// Pin returns ref with the digest its tag names now appended. A ref outside the +// registry comes back unchanged. A ref that already carries a digest comes back +// unchanged once the registry confirms it still holds that manifest: the registry +// pruner deletes builds nothing references, and a server set back to one of them +// would otherwise sit in ImagePullBackOff. func (r Resolver) Pin(ctx context.Context, ref string) (string, error) { - if Pinned(ref) || !r.Covers(ref) { + if !r.Covers(ref) { + return ref, nil + } + if name, pinned, ok := strings.Cut(ref, "@"); ok { + repo, _ := splitTag(strings.TrimPrefix(name, r.Registry+"/")) + if repo == "" || !digestRE.MatchString(pinned) { + return "", fmt.Errorf("imagepin: %q is not a valid pinned reference", ref) + } + if _, err := r.digest(ctx, repo, pinned); err != nil { + return "", fmt.Errorf("imagepin: resolve %s: %w", ref, err) + } return ref, nil } repo, tag := splitTag(strings.TrimPrefix(ref, r.Registry+"/")) diff --git a/internal/imagepin/imagepin_test.go b/internal/imagepin/imagepin_test.go index f7a0f34..220cc37 100644 --- a/internal/imagepin/imagepin_test.go +++ b/internal/imagepin/imagepin_test.go @@ -13,7 +13,8 @@ import ( const testDigest = "sha256:d2fcc09d2caa108678c540c99703db96d63038a5fc9e366402d7ef1712ec4d95" -// fakeRegistry serves /v2/felis/paper/manifests/demo and 404s everything else. +// fakeRegistry serves /v2/felis/paper/manifests/demo (and the same manifest by +// its digest) and 404s everything else. func fakeRegistry(t *testing.T, header bool) (*httptest.Server, *[]string) { t.Helper() var seen []string @@ -22,7 +23,7 @@ func fakeRegistry(t *testing.T, header bool) (*httptest.Server, *[]string) { if !strings.Contains(r.Header.Get("Accept"), "application/vnd.oci.image.index.v1+json") { t.Errorf("request without an OCI index Accept: %q", r.Header.Get("Accept")) } - if r.URL.Path != "/v2/felis/paper/manifests/demo" { + if r.URL.Path != "/v2/felis/paper/manifests/demo" && r.URL.Path != "/v2/felis/paper/manifests/"+testDigest { http.NotFound(w, r) return } @@ -78,9 +79,9 @@ func TestPinLeavesOtherRefsAlone(t *testing.T) { srv, seen := fakeRegistry(t, true) r := resolverFor(srv) for _, ref := range []string{ - "registry.felis.svc:5000/felis/paper:demo@" + testDigest, // already pinned - "docker.io/itzg/minecraft-server:java21", // external - "registry.felis.svc:50000/felis/paper:demo", // a different port is a different registry + "docker.io/itzg/minecraft-server:java21", // external + "docker.io/itzg/minecraft-server:java21@" + testDigest, // external, pinned + "registry.felis.svc:50000/felis/paper:demo", // a different port is a different registry } { got, err := r.Pin(context.Background(), ref) if err != nil || got != ref { @@ -92,6 +93,30 @@ func TestPinLeavesOtherRefsAlone(t *testing.T) { } } +// A pinned ref stays as it is, but only while the registry still holds that +// manifest: the pruner deletes builds nothing references, and setting a server +// back to one of them must fail here, not in ImagePullBackOff. +func TestPinChecksAPinnedRefStillExists(t *testing.T) { + srv, seen := fakeRegistry(t, true) + r := resolverFor(srv) + ref := "registry.felis.svc:5000/felis/paper:demo@" + testDigest + got, err := r.Pin(context.Background(), ref) + if err != nil || got != ref { + t.Fatalf("Pin(%q) = %q, %v; want it unchanged", ref, got, err) + } + if want := "HEAD /v2/felis/paper/manifests/" + testDigest; len(*seen) != 1 || (*seen)[0] != want { + t.Errorf("requests = %v, want [%s]", *seen, want) + } + + pruned := "registry.felis.svc:5000/felis/paper:demo@sha256:" + strings.Repeat("0", 64) + if _, err := r.Pin(context.Background(), pruned); !errors.Is(err, ErrNotFound) { + t.Errorf("pruned digest: err = %v, want ErrNotFound", err) + } + if _, err := r.Pin(context.Background(), "registry.felis.svc:5000/felis/paper:demo@sha256:nothex"); err == nil { + t.Error("malformed pinned ref accepted") + } +} + func TestPinImpliedLatest(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/v2/felis/paper/manifests/latest" {