Merge pull request #448 from flavianmissi/OCPBUGS-35036

OCPBUGS-35036: fail image import when both image and error are nil

pkg/image/apiserver/importer/importer.go

@@ -2,6 +2,7 @@ package importer
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"runtime"
@@ -256,6 +257,22 @@ func (imp *ImageStreamImporter) importImages(ctx context.Context, isi *imageapi.
 				cache[j] = tag.Image
 			}
 			for _, index := range tags[j] {
+				// we have seen the below state in our internal CI clusters. we think
+				// it's caused by outages in the upstream registry, but we're not sure
+				// exactly how we end up in this situation where neither error or image
+				// are set. to protect against that we throw a generic error when it
+				// happens.
+				// See https://issues.redhat.com/browse/OCPBUGS-35036 for details.
+				if tag.Image == nil && tag.Err == nil {
+					errMsg := fmt.Sprintf(
+						"unknown error while importing tag %q from repository %q with import mode %q, please try again.",
+						tag.Name,
+						repo.Name,
+						tag.ImportMode,
+					)
+					tag.Err = errors.New(errMsg)
+					klog.Infof("importImages: both tag image and error are nil! repo=%+v tag=%+v", repo, tag)
+				}
 				if tag.Err != nil {
 					setImageImportStatus(isi, index, tag.Name, tag.Err)
 					continue

pkg/image/apiserver/importer/importer_test.go

@@ -71,6 +71,7 @@ func (r *mockRepository) Blobs(ctx context.Context) distribution.BlobStore { ret
 func (r *mockRepository) Exists(ctx context.Context, dgst godigest.Digest) (bool, error) {
 	return false, r.getErr
 }
+
 func (r *mockRepository) Get(ctx context.Context, dgst godigest.Digest, options ...distribution.ManifestServiceOption) (distribution.Manifest, error) {
 	r.manifestReqs = append(r.manifestReqs, dgst)
 	for d, manifest := range r.extraManifests {
@@ -85,12 +86,15 @@ func (r *mockRepository) Get(ctx context.Context, dgst godigest.Digest, options
 	}
 	return r.manifest, r.getErr
 }
+
 func (r *mockRepository) Delete(ctx context.Context, dgst godigest.Digest) error {
 	return fmt.Errorf("not implemented")
 }
+
 func (r *mockRepository) Put(ctx context.Context, manifest distribution.Manifest, options ...distribution.ManifestServiceOption) (godigest.Digest, error) {
 	return "", fmt.Errorf("not implemented")
 }
+
 func (r *mockRepository) Tags(ctx context.Context) distribution.TagService {
 	return &mockTagService{repo: r}
 }
@@ -223,11 +227,13 @@ func TestImport(t *testing.T) {
 		},
 	}
 	testCases := []struct {
+		name      string
 		retriever RepositoryRetriever
 		isi       imageapi.ImageStreamImport
 		expect    func(*imageapi.ImageStreamImport, *testing.T)
 	}{
 		{
+			name:      "insecure import policy",
 			retriever: insecureRetriever,
 			isi: imageapi.ImageStreamImport{
 				Spec: imageapi.ImageStreamImportSpec{
@@ -243,6 +249,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
+			name: "missing tag, digest, and invalid image reference",
 			retriever: &mockRetriever{
 				repo: &mockRepository{
 					getTagErr:   fmt.Errorf("no such tag"),
@@ -283,6 +290,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
+			name:      "failed repository import",
 			retriever: &mockRetriever{err: fmt.Errorf("error")},
 			isi: imageapi.ImageStreamImport{
 				Spec: imageapi.ImageStreamImportSpec{
@@ -304,6 +312,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
+			name:      "successfull import by tag and digest",
 			retriever: &mockRetriever{repo: &mockRepository{manifest: etcdManifestSchema1}},
 			isi: imageapi.ImageStreamImport{
 				Spec: imageapi.ImageStreamImportSpec{
@@ -337,6 +346,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
+			name: "successful import by tag",
 			retriever: &mockRetriever{
 				repo: &mockRepository{
 					blobs: &mockBlobStore{
@@ -379,6 +389,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
+			name: "import repository with additional tags",
 			retriever: &mockRetriever{
 				repo: &mockRepository{
 					manifest: etcdManifestSchema1,
@@ -421,13 +432,15 @@ func TestImport(t *testing.T) {
 		},
 	}
 	for i, test := range testCases {
-		im := NewImageStreamImporter(test.retriever, nil, 5, nil, nil)
-		if err := im.Import(nil, &test.isi, &imageapi.ImageStream{}); err != nil {
-			t.Errorf("%d: %v", i, err)
-		}
-		if test.expect != nil {
-			test.expect(&test.isi, t)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			im := NewImageStreamImporter(test.retriever, nil, 5, nil, nil)
+			if err := im.Import(nil, &test.isi, &imageapi.ImageStream{}); err != nil {
+				t.Errorf("%d: %v", i, err)
+			}
+			if test.expect != nil {
+				test.expect(&test.isi, t)
+			}
+		})
 	}
 }