[registry-facade] Fix stored MF download failures

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",
 			},
 		},
 		{

components/registry-facade/pkg/registry/manifest.go

@@ -315,14 +315,16 @@ 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
 			}
 			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

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")
+}