From aa453c08dbe22de1d89dcf260a52230c51806d33 Mon Sep 17 00:00:00 2001 From: Christian Weichel Date: Mon, 4 Apr 2022 06:43:23 +0000 Subject: [PATCH] [registry-facade] Fix stored MF download failures --- .../blobserve/pkg/blobserve/refstore_test.go | 2 +- .../registry-facade/pkg/registry/manifest.go | 55 ++++--- .../pkg/registry/manifest_test.go | 142 ++++++++++++++++++ 3 files changed, 179 insertions(+), 20 deletions(-) create mode 100644 components/registry-facade/pkg/registry/manifest_test.go diff --git a/components/blobserve/pkg/blobserve/refstore_test.go b/components/blobserve/pkg/blobserve/refstore_test.go index 4ed1e911edb8ce..9ee4ec756b6034 100644 --- a/components/blobserve/pkg/blobserve/refstore_test.go +++ b/components/blobserve/pkg/blobserve/refstore_test.go @@ -113,7 +113,7 @@ func TestBlobFor(t *testing.T) { refDescriptor: provideDescriptor, }, Expectation: Expectation{ - Error: "cannot download manifest: " + hashManifest + " not found", + Error: "cannot fetch manifest: " + hashManifest + " not found", }, }, { diff --git a/components/registry-facade/pkg/registry/manifest.go b/components/registry-facade/pkg/registry/manifest.go index 504f45aa18258d..b8fb06d293dfdf 100644 --- a/components/registry-facade/pkg/registry/manifest.go +++ b/components/registry-facade/pkg/registry/manifest.go @@ -315,6 +315,8 @@ func DownloadConfig(ctx context.Context, fetch FetcherFunc, ref string, desc oci if err != nil { return err } + defer w.Close() + n, err := w.Write(buf) if err != nil { return err @@ -322,7 +324,7 @@ func DownloadConfig(ctx context.Context, fetch FetcherFunc, ref string, desc oci if n != len(buf) { return io.ErrShortWrite } - w.Close() + return w.Commit(ctx, int64(len(buf)), digest.FromBytes(buf), content.WithLabels(contentTypeLabel(desc.MediaType))) }() if err != nil { @@ -378,29 +380,42 @@ func DownloadManifest(ctx context.Context, fetch FetcherFunc, desc ociv1.Descrip } var ( - rc io.ReadCloser placeInStore bool - mediaType string + rc io.ReadCloser + mediaType = desc.MediaType ) if opts.Store != nil { - nfo, err := opts.Store.Info(ctx, desc.Digest) - if errors.Cause(err) == errdefs.ErrNotFound { - // not in store yet - } else if err != nil { - log.WithError(err).WithField("desc", desc).Warn("cannot get manifest from store") - } - mediaType = nfo.Labels["Content-Type"] + func() { + nfo, err := opts.Store.Info(ctx, desc.Digest) + if errors.Is(err, errdefs.ErrNotFound) { + // not in store yet + return + } + if err != nil { + log.WithError(err).WithField("desc", desc).Warn("cannot get manifest from store") + return + } + if nfo.Labels["Content-Type"] == "" { + // we have broken data in the store - ignore it and overwrite + return + } - r, err := opts.Store.ReaderAt(ctx, desc) - if errors.Cause(err) == errdefs.ErrNotFound { - // not in store yet - } else if err != nil { - log.WithError(err).WithField("desc", desc).Warn("cannot get manifest from store") - } else { - rc = &reader{ReaderAt: r} - } + r, err := opts.Store.ReaderAt(ctx, desc) + if errors.Is(err, errdefs.ErrNotFound) { + // not in store yet + return + } + if err != nil { + log.WithError(err).WithField("desc", desc).Warn("cannot get manifest from store") + return + } + + mediaType, rc = nfo.Labels["Content-Type"], &reader{ReaderAt: r} + }() } if rc == nil { + // did not find in store, or there was no store. Either way, let's fetch this + // thing from the remote. placeInStore = true var fetcher remotes.Fetcher @@ -411,7 +426,7 @@ func DownloadManifest(ctx context.Context, fetch FetcherFunc, desc ociv1.Descrip rc, err = fetcher.Fetch(ctx, desc) if err != nil { - err = xerrors.Errorf("cannot download manifest: %w", err) + err = xerrors.Errorf("cannot fetch manifest: %w", err) return } mediaType = desc.MediaType @@ -429,6 +444,8 @@ func DownloadManifest(ctx context.Context, fetch FetcherFunc, desc ociv1.Descrip switch rdesc.MediaType { case images.MediaTypeDockerSchema2ManifestList, ociv1.MediaTypeImageIndex: + log.WithField("desc", rdesc).Debug("resolving image index") + // we received a manifest list which means we'll pick the default platform // and fetch that manifest var list ociv1.Index diff --git a/components/registry-facade/pkg/registry/manifest_test.go b/components/registry-facade/pkg/registry/manifest_test.go new file mode 100644 index 00000000000000..94c4d3e31a294f --- /dev/null +++ b/components/registry-facade/pkg/registry/manifest_test.go @@ -0,0 +1,142 @@ +// Copyright (c) 2020 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License-AGPL.txt in the project root for license information. + +package registry + +import ( + "context" + "encoding/json" + "fmt" + "testing" + + "github.com/containerd/containerd/content" + "github.com/containerd/containerd/errdefs" + "github.com/opencontainers/go-digest" + "github.com/opencontainers/image-spec/specs-go" + ociv1 "github.com/opencontainers/image-spec/specs-go/v1" +) + +func TestDownloadManifest(t *testing.T) { + tests := []struct { + Name string + Store BlobStore + FetchMF bool + }{ + { + Name: "no store", + }, + { + Name: "accepts nothing store", + Store: &alwaysNotFoundStore{}, + }, + { + Name: "misbehaving store", + Store: &misbehavingStore{}, + }, + { + Name: "fetch mf no store", + FetchMF: true, + }, + { + Name: "fetch mf with store", + Store: &alwaysNotFoundStore{}, + FetchMF: true, + }, + } + + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + cfg, err := json.Marshal(ociv1.ImageConfig{}) + if err != nil { + t.Fatal(err) + } + cfgDgst := digest.FromBytes(cfg) + + mf, err := json.Marshal(ociv1.Manifest{ + Versioned: specs.Versioned{SchemaVersion: 1}, + MediaType: ociv1.MediaTypeImageManifest, + Config: ociv1.Descriptor{ + MediaType: ociv1.MediaTypeImageConfig, + Digest: cfgDgst, + Size: int64(len(cfg)), + }, + }) + if err != nil { + t.Fatal(err) + } + mfDgst := digest.FromBytes(mf) + mfDesc := ociv1.Descriptor{ + MediaType: ociv1.MediaTypeImageManifest, + Digest: mfDgst, + Size: int64(len(mf)), + } + + mfl, err := json.Marshal(ociv1.Index{ + MediaType: ociv1.MediaTypeImageIndex, + Manifests: []ociv1.Descriptor{mfDesc}, + }) + if err != nil { + t.Fatal(err) + } + mflDgst := digest.FromBytes(mfl) + mflDesc := ociv1.Descriptor{ + MediaType: ociv1.MediaTypeImageIndex, + Digest: mflDgst, + Size: int64(len(mfl)), + } + + fetcher := AsFetcherFunc(&fakeFetcher{ + Content: map[string][]byte{ + mflDgst.Encoded(): mfl, + mfDgst.Encoded(): mf, + cfgDgst.Encoded(): cfg, + }, + }) + desc := mflDesc + if test.FetchMF { + desc = mfDesc + } + _, _, err = DownloadManifest(context.Background(), fetcher, desc, WithStore(test.Store)) + if err != nil { + t.Fatal(err) + } + }) + } +} + +type alwaysNotFoundStore struct{} + +func (fbs *alwaysNotFoundStore) ReaderAt(ctx context.Context, desc ociv1.Descriptor) (content.ReaderAt, error) { + return nil, errdefs.ErrNotFound +} + +// Some implementations require WithRef to be included in opts. +func (fbs *alwaysNotFoundStore) Writer(ctx context.Context, opts ...content.WriterOpt) (content.Writer, error) { + return nil, errdefs.ErrAlreadyExists +} + +// Info will return metadata about content available in the content store. +// +// If the content is not present, ErrNotFound will be returned. +func (fbs *alwaysNotFoundStore) Info(ctx context.Context, dgst digest.Digest) (content.Info, error) { + return content.Info{}, errdefs.ErrNotFound +} + +type misbehavingStore struct{} + +func (fbs *misbehavingStore) ReaderAt(ctx context.Context, desc ociv1.Descriptor) (content.ReaderAt, error) { + return nil, fmt.Errorf("foobar") +} + +// Some implementations require WithRef to be included in opts. +func (fbs *misbehavingStore) Writer(ctx context.Context, opts ...content.WriterOpt) (content.Writer, error) { + return nil, fmt.Errorf("some error") +} + +// Info will return metadata about content available in the content store. +// +// If the content is not present, ErrNotFound will be returned. +func (fbs *misbehavingStore) Info(ctx context.Context, dgst digest.Digest) (content.Info, error) { + return content.Info{}, fmt.Errorf("you wish") +}