Import from repository doesn't panic

- Check the import status before a use of image object.
- Also to set tag names on import status objects in order to produce
  nice output (containing tag names that failed to import).

Signed-off-by: Michal Minar <[email protected]>

Author: Michal Minář

pkg/image/importer/importer.go

@@ -173,7 +173,7 @@ func importImages(ctx gocontext.Context, retriever RepositoryRetriever, isi *api
 			}
 			for _, index := range tags[j] {
 				if tag.Err != nil {
-					setImageImportStatus(isi, index, tag.Err)
+					setImageImportStatus(isi, index, tag.Name, tag.Err)
 					continue
 				}
 				copied := *tag.Image
@@ -194,7 +194,7 @@ func importImages(ctx gocontext.Context, retriever RepositoryRetriever, isi *api
 			}
 			for _, index := range ids[j] {
 				if digest.Err != nil {
-					setImageImportStatus(isi, index, digest.Err)
+					setImageImportStatus(isi, index, "", digest.Err)
 					continue
 				}
 				image := &isi.Status.Images[index]
@@ -267,6 +267,7 @@ func importFromRepository(ctx gocontext.Context, retriever RepositoryRetriever,
 	status.Status.Status = unversioned.StatusSuccess
 	status.Images = make([]api.ImageImportStatus, len(repo.Tags))
 	for i, tag := range repo.Tags {
+		status.Images[i].Tag = tag.Name
 		if tag.Err != nil {
 			failures++
 			status.Images[i].Status = imageImportStatus(tag.Err, "", "repository")
@@ -277,7 +278,6 @@ func importFromRepository(ctx gocontext.Context, retriever RepositoryRetriever,
 		copied := *tag.Image
 		ref.Tag, ref.ID = tag.Name, copied.Name
 		copied.DockerImageReference = ref.MostSpecific().Exact()
-		status.Images[i].Tag = tag.Name
 		status.Images[i].Image = &copied
 	}
 	if failures > 0 {
@@ -598,7 +598,8 @@ func imageImportStatus(err error, kind, position string) unversioned.Status {
 	}
 }
 
-func setImageImportStatus(images *api.ImageStreamImport, i int, err error) {
+func setImageImportStatus(images *api.ImageStreamImport, i int, tag string, err error) {
+	images.Status.Images[i].Tag = tag
 	images.Status.Images[i].Status = imageImportStatus(err, "", "")
 }
 

pkg/image/importer/importer_test.go

@@ -149,6 +149,12 @@ func TestImport(t *testing.T) {
 				if status := isi.Status.Images[3].Status; status.Status != "" {
 					t.Errorf("unexpected status: %#v", isi.Status.Images[3].Status)
 				}
+				expectedTags := []string{"latest", "", "", ""}
+				for i, image := range isi.Status.Images {
+					if image.Tag != expectedTags[i] {
+						t.Errorf("unexpected tag of status %d (%s != %s)", i, image.Tag, expectedTags[i])
+					}
+				}
 			},
 		},
 		{
@@ -186,6 +192,7 @@ func TestImport(t *testing.T) {
 				if len(isi.Status.Images) != 2 {
 					t.Errorf("unexpected number of images: %#v", isi.Status.Repository.Images)
 				}
+				expectedTags := []string{"", "tag"}
 				for i, image := range isi.Status.Images {
 					if image.Status.Status != unversioned.StatusSuccess {
 						t.Errorf("unexpected status %d: %#v", i, image.Status)
@@ -198,6 +205,9 @@ func TestImport(t *testing.T) {
 					if image.Image.DockerImageReference != "test@sha256:958608f8ecc1dc62c93b6c610f3a834dae4220c9642e6e8b4e0f2b3ad7cbd238" {
 						t.Errorf("unexpected ref %d: %#v", i, image.Image.DockerImageReference)
 					}
+					if image.Tag != expectedTags[i] {
+						t.Errorf("unexpected tag of status %d (%s != %s)", i, image.Tag, expectedTags[i])
+					}
 				}
 			},
 		},
@@ -222,10 +232,14 @@ func TestImport(t *testing.T) {
 				if len(isi.Status.Repository.Images) != 5 {
 					t.Errorf("unexpected number of images: %#v", isi.Status.Repository.Images)
 				}
+				expectedTags := []string{"3", "v2", "v1", "3.1", "abc"}
 				for i, image := range isi.Status.Repository.Images {
 					if image.Status.Status != unversioned.StatusFailure || image.Status.Message != "Internal error occurred: no such tag" {
 						t.Errorf("unexpected status %d: %#v", i, isi.Status.Repository.Images)
 					}
+					if image.Tag != expectedTags[i] {
+						t.Errorf("unexpected tag of status %d (%s != %s)", i, image.Tag, expectedTags[i])
+					}
 				}
 			},
 		},

pkg/image/registry/imagestreamimport/rest.go

@@ -177,6 +177,10 @@ func (r *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err
 
 	if spec := isi.Spec.Repository; spec != nil {
 		for i, status := range isi.Status.Repository.Images {
+			if checkImportFailure(status, stream, status.Tag, nextGeneration, now) {
+				continue
+			}
+
 			image := status.Image
 			ref, err := api.ParseDockerImageReference(image.DockerImageReference)
 			if err != nil {
@@ -196,10 +200,6 @@ func (r *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err
 			// we've imported a set of tags, ensure spec tag will point to this for later imports
 			from.ID, from.Tag = "", tag
 
-			if checkImportFailure(status, stream, tag, nextGeneration, now) {
-				continue
-			}
-
 			if updated, ok := r.importSuccessful(ctx, image, stream, tag, from.Exact(), nextGeneration, now, spec.ImportPolicy, importedImages, updatedImages); ok {
 				isi.Status.Repository.Images[i].Image = updated
 			}
@@ -278,6 +278,17 @@ func checkImportFailure(status api.ImageImportStatus, stream *api.ImageStream, t
 
 		LastTransitionTime: now,
 	}
+
+	if tag == "" {
+		if len(status.Tag) > 0 {
+			tag = status.Tag
+		} else if status.Image != nil {
+			if ref, err := api.ParseDockerImageReference(status.Image.DockerImageReference); err == nil {
+				tag = ref.Tag
+			}
+		}
+	}
+
 	if !api.HasTagCondition(stream, tag, condition) {
 		api.SetTagConditions(stream, tag, condition)
 		if tagRef, ok := stream.Spec.Tags[tag]; ok {

test/integration/imageimporter_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/distribution/registry/api/errcode"
 	gocontext "golang.org/x/net/context"
 
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -135,16 +136,27 @@ func TestImageStreamImport(t *testing.T) {
 	}
 }
 
-func mockRegistryHandler(t *testing.T, count *int) http.Handler {
+// mockRegistryHandler returns a registry mock handler with several repositories.
+// requireAuth causes handler to to return unauthorized and request basic authentication header if not given.
+// count is increased each time the handler is invoked.
+// There are three repositories:
+//  - test/image with phpManifest
+//  - test/image2 with etcdManifest
+//  - test/image3 with tags: v1, v2 and latest
+//    - the first points to etcdManifest
+//    - the others cause handler to return unknown error
+func mockRegistryHandler(t *testing.T, requireAuth bool, count *int) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		(*count)++
 		t.Logf("%d got %s %s", *count, r.Method, r.URL.Path)
 
 		w.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
-		if len(r.Header.Get("Authorization")) == 0 {
-			w.Header().Set("WWW-Authenticate", "BASIC")
-			w.WriteHeader(http.StatusUnauthorized)
-			return
+		if requireAuth {
+			if len(r.Header.Get("Authorization")) == 0 {
+				w.Header().Set("WWW-Authenticate", "BASIC")
+				w.WriteHeader(http.StatusUnauthorized)
+				return
+			}
 		}
 
 		switch r.URL.Path {
@@ -154,6 +166,12 @@ func mockRegistryHandler(t *testing.T, count *int) http.Handler {
 			w.Write([]byte(phpManifest))
 		case "/v2/test/image2/manifests/" + etcdDigest:
 			w.Write([]byte(etcdManifest))
+		case "/v2/test/image3/tags/list":
+			w.Write([]byte("{\"name\": \"test/image3\", \"tags\": [\"latest\", \"v1\", \"v2\"]}"))
+		case "/v2/test/image3/manifests/latest", "/v2/test/image3/manifests/v2", "/v2/test/image3/manifests/" + danglingDigest:
+			errcode.ServeJSON(w, errcode.ErrorCodeUnknown)
+		case "/v2/test/image3/manifests/v1", "/v2/test/image3/manifests/" + etcdDigest:
+			w.Write([]byte(etcdManifest))
 		default:
 			t.Fatalf("unexpected request %s: %#v", r.URL.Path, r)
 		}
@@ -164,7 +182,7 @@ func TestImageStreamImportAuthenticated(t *testing.T) {
 	testutil.RequireEtcd(t)
 	// start regular HTTP servers
 	count := 0
-	server := httptest.NewServer(mockRegistryHandler(t, &count))
+	server := httptest.NewServer(mockRegistryHandler(t, true, &count))
 	server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
 		if len(r.Header.Get("Authorization")) == 0 {
@@ -177,7 +195,7 @@ func TestImageStreamImportAuthenticated(t *testing.T) {
 
 	// start a TLS server
 	count2 := 0
-	server3 := httptest.NewTLSServer(mockRegistryHandler(t, &count2))
+	server3 := httptest.NewTLSServer(mockRegistryHandler(t, true, &count2))
 
 	url1, _ := url.Parse(server.URL)
 	url2, _ := url.Parse(server2.URL)
@@ -324,6 +342,105 @@ func TestImageStreamImportAuthenticated(t *testing.T) {
 	}
 }
 
+// Verifies that individual errors for particular tags are handled properly when pulling all tags from a
+// repository.
+func TestImageStreamImportTagsFromRepository(t *testing.T) {
+	testutil.RequireEtcd(t)
+	// start regular HTTP servers
+	count := 0
+	server := httptest.NewServer(mockRegistryHandler(t, false, &count))
+
+	url, _ := url.Parse(server.URL)
+
+	// start a master
+	_, clusterAdminKubeConfig, err := testserver.StartTestMaster()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	/*
+		_, err := testutil.GetClusterAdminKubeClient(clusterAdminKubeConfig)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	*/
+	c, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	err = testutil.CreateNamespace(clusterAdminKubeConfig, testutil.Namespace())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	importSpec := &api.ImageStreamImport{
+		ObjectMeta: kapi.ObjectMeta{Name: "test"},
+		Spec: api.ImageStreamImportSpec{
+			Import: true,
+			Repository: &api.RepositoryImportSpec{
+				From:            kapi.ObjectReference{Kind: "DockerImage", Name: url.Host + "/test/image3"},
+				ImportPolicy:    api.TagImportPolicy{Insecure: true},
+				IncludeManifest: true,
+			},
+		},
+	}
+
+	// import expecting regular image to pass
+	isi, err := c.ImageStreams(testutil.Namespace()).Import(importSpec)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(isi.Status.Images) != 0 {
+		t.Errorf("imported unexpected number of images (%d != 0)", len(isi.Status.Images))
+	}
+	if isi.Status.Repository == nil {
+		t.Fatalf("exported non-nil repository status")
+	}
+	if len(isi.Status.Repository.Images) != 3 {
+		t.Fatalf("imported unexpected number of tags (%d != 3)", len(isi.Status.Repository.Images))
+	}
+	for i, image := range isi.Status.Repository.Images {
+		switch i {
+		case 2:
+			if image.Status.Status != unversioned.StatusSuccess {
+				t.Errorf("import of image %d did not succeed: %#v", i, image.Status)
+			}
+			if image.Tag != "v1" {
+				t.Errorf("unexpected tag at position %d (%s != v1)", i, image.Tag)
+			}
+			if image.Image == nil {
+				t.Fatalf("expected image to be set")
+			}
+			if image.Image.DockerImageReference != url.Host+"/test/image3@"+etcdDigest {
+				t.Errorf("unexpected DockerImageReference (%s != %s)", image.Image.DockerImageReference, url.Host+"/test/image3@"+etcdDigest)
+			}
+			if image.Image.Name != etcdDigest {
+				t.Errorf("expected etcd digest as a name of the image (%s != %s)", image.Image.Name, etcdDigest)
+			}
+		default:
+			if image.Status.Status != unversioned.StatusFailure || image.Status.Reason != unversioned.StatusReasonInternalError {
+				t.Fatalf("import of image %d did not report internal server error: %#v", i, image.Status)
+			}
+			expectedTags := []string{"latest", "v2"}[i]
+			if image.Tag != expectedTags {
+				t.Errorf("unexpected tag at position %d (%s != %s)", i, image.Tag, expectedTags[i])
+			}
+		}
+	}
+
+	is, err := c.ImageStreams(testutil.Namespace()).Get("test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	tagEvent := api.LatestTaggedImage(is, "v1")
+	if tagEvent == nil {
+		t.Fatalf("no image tagged for v1: %#v", is)
+	}
+
+	if tagEvent == nil || tagEvent.Image != etcdDigest || tagEvent.DockerImageReference != url.Host+"/test/image3@"+etcdDigest {
+		t.Fatalf("expected the etcd image to be tagged: %#v", tagEvent)
+	}
+}
+
 // Verifies that the import scheduler fetches an image repeatedly (every 1s as per the default
 // test controller interval), updates the image stream only when there are changes, and if an
 // error occurs writes the error only once (instead of every interval)
@@ -861,3 +978,5 @@ const phpManifest = `{
       }
    ]
 }`
+
+const danglingDigest = `sha256:f374c0d9b59e6fdf9f8922d59e946b05fbeabaed70b0639d7b6b524f3299e87b`