Skip to content

Commit

Permalink
Import from repository doesn't panic
Browse files Browse the repository at this point in the history 
- 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]>
  • Loading branch information
Michal Minář committed Apr 24, 2016
1 parent 8fb838f commit 8862811
Show file tree
Hide file tree
Showing 4 changed files with 160 additions and 15 deletions.
9 changes: 5 additions & 4 deletions pkg/image/importer/importer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -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")
Expand All @@ -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 {
Expand Down Expand Up @@ -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, "", "")
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/image/importer/importer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}
},
},
{
Expand Down Expand Up @@ -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)
Expand All @@ -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])
}
}
},
},
Expand All @@ -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])
}
}
},
},
Expand Down
19 changes: 15 additions & 4 deletions pkg/image/registry/imagestreamimport/rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
133 changes: 126 additions & 7 deletions test/integration/imageimporter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
Expand All @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -861,3 +978,5 @@ const phpManifest = `{
}
]
}`

const danglingDigest = `sha256:f374c0d9b59e6fdf9f8922d59e946b05fbeabaed70b0639d7b6b524f3299e87b`

0 comments on commit 8862811

Please sign in to comment.