Skip to content

Existing images are tagged with correct reference on push #12525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 26, 2017
Merged

Existing images are tagged with correct reference on push #12525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions pkg/image/registry/imagestreammapping/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package imagestreammapping

import (
"github.com/golang/glog"

kapi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/errors"
"k8s.io/kubernetes/pkg/api/rest"
Expand Down Expand Up @@ -64,13 +66,27 @@ func (s *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err
tag = api.DefaultImageTag
}

if err := s.imageRegistry.CreateImage(ctx, &image); err != nil && !errors.IsAlreadyExists(err) {
return nil, err
imageCreateErr := s.imageRegistry.CreateImage(ctx, &image)
if imageCreateErr != nil && !errors.IsAlreadyExists(imageCreateErr) {
return nil, imageCreateErr
}

// prefer dockerImageReference set on image for the tagEvent if the image is new
ref := image.DockerImageReference
if errors.IsAlreadyExists(imageCreateErr) && image.Annotations[api.ManagedByOpenShiftAnnotation] == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kapi.ConditionTrue ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotation is always checked against "true", kapi.ConditionTrue is capitalized.

// the image is managed by us and, most probably, tagged in some other image stream
// let's make the reference local to this stream
if streamRef, err := api.DockerImageReferenceForStream(stream); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to log the error case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might. I'll add a debug statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging the error now.

streamRef.ID = image.Name
ref = streamRef.Exact()
} else {
glog.V(4).Infof("Failed to get dockerImageReference for stream %s/%s: %v", stream.Namespace, stream.Name, err)
}
}

next := api.TagEvent{
Created: unversioned.Now(),
DockerImageReference: image.DockerImageReference,
DockerImageReference: ref,
Image: image.Name,
}

Expand Down
181 changes: 155 additions & 26 deletions pkg/image/registry/imagestreammapping/rest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ import (
_ "github.com/openshift/origin/pkg/api/install"
)

var testDefaultRegistry = api.DefaultRegistryFunc(func() (string, bool) { return "defaultregistry:5000", true })
const testDefaultRegistryURL = "defaultregistry:5000"

var testDefaultRegistry = api.DefaultRegistryFunc(func() (string, bool) { return testDefaultRegistryURL, true })

type fakeSubjectAccessReviewRegistry struct {
}
Expand Down Expand Up @@ -74,6 +76,8 @@ func validImageStream() *api.ImageStream {
}
}

const testImageID = "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4"

func validNewMappingWithName() *api.ImageStreamMapping {
return &api.ImageStreamMapping{
ObjectMeta: kapi.ObjectMeta{
Expand All @@ -82,9 +86,10 @@ func validNewMappingWithName() *api.ImageStreamMapping {
},
Image: api.Image{
ObjectMeta: kapi.ObjectMeta{
Name: "imageID1",
Name: testImageID,
Annotations: map[string]string{api.ManagedByOpenShiftAnnotation: "true"},
},
DockerImageReference: "localhost:5000/default/somerepo:imageID1",
DockerImageReference: "localhost:5000/default/somerepo@" + testImageID,
DockerImageMetadata: api.DockerImage{
Config: &api.DockerConfig{
Cmd: []string{"ls", "/"},
Expand Down Expand Up @@ -171,9 +176,9 @@ func TestCreateSuccessWithName(t *testing.T) {
t.Fatalf("Unexpected error creating mapping: %#v", err)
}

image, err := storage.imageRegistry.GetImage(ctx, "imageID1")
image, err := storage.imageRegistry.GetImage(ctx, testImageID)
if err != nil {
t.Errorf("Unexpected error retrieving image: %#v", err)
t.Fatalf("Unexpected error retrieving image: %#v", err)
}
if e, a := mapping.Image.DockerImageReference, image.DockerImageReference; e != a {
t.Errorf("Expected %s, got %s", e, a)
Expand All @@ -186,43 +191,33 @@ func TestCreateSuccessWithName(t *testing.T) {
if err != nil {
t.Errorf("Unexpected non-nil err: %#v", err)
}
if e, a := "imageID1", repo.Status.Tags["latest"].Items[0].Image; e != a {
if e, a := testImageID, repo.Status.Tags["latest"].Items[0].Image; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
}

func TestAddExistingImageWithNewTag(t *testing.T) {
imageID := "8d812da98d6dd61620343f1a5bf6585b34ad6ed16e5c5f7c7216a525d6aeb772"
imageID := "sha256:8d812da98d6dd61620343f1a5bf6585b34ad6ed16e5c5f7c7216a525d6aeb772"
existingRepo := &api.ImageStream{
ObjectMeta: kapi.ObjectMeta{
Name: "somerepo",
Namespace: "default",
},
Spec: api.ImageStreamSpec{
DockerImageRepository: "localhost:5000/someproject/somerepo",
/*
Tags: map[string]api.TagReference{
"existingTag": {
From: &kapi.ObjectReference{
Kind: "ImageStreamTag",

Tag: "existingTag", Reference: imageID},
},
*/
DockerImageRepository: "localhost:5000/default/somerepo",
},
Status: api.ImageStreamStatus{
Tags: map[string]api.TagEventList{
"existingTag": {Items: []api.TagEvent{{DockerImageReference: "localhost:5000/someproject/somerepo:" + imageID}}},
"existingTag": {Items: []api.TagEvent{{DockerImageReference: "localhost:5000/somens/somerepo@" + imageID}}},
},
},
}

existingImage := &api.Image{
ObjectMeta: kapi.ObjectMeta{
Name: imageID,
Namespace: "default",
Name: imageID,
},
DockerImageReference: "localhost:5000/someproject/somerepo:" + imageID,
DockerImageReference: "localhost:5000/somens/somerepo@" + imageID,
DockerImageMetadata: api.DockerImage{
Config: &api.DockerConfig{
Cmd: []string{"ls", "/"},
Expand All @@ -249,19 +244,153 @@ func TestAddExistingImageWithNewTag(t *testing.T) {

_, err = client.Put(
context.TODO(),
etcdtest.AddPrefix("/images/default/"+imageID), runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(v1.SchemeGroupVersion), existingImage),
etcdtest.AddPrefix("/images/"+imageID), runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(v1.SchemeGroupVersion), existingImage),
)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

mapping := api.ImageStreamMapping{
ObjectMeta: kapi.ObjectMeta{
Name: "somerepo",
},
Image: *existingImage,
Tag: "latest",
}
_, err = storage.Create(kapi.NewDefaultContext(), &mapping)
if !errors.IsInvalid(err) {
t.Fatalf("Unexpected non-error creating mapping: %#v", err)
ctx := kapi.NewDefaultContext()
_, err = storage.Create(ctx, &mapping)
if err != nil {
t.Errorf("Unexpected error creating image stream mapping%v", err)
}

image, err := storage.imageRegistry.GetImage(ctx, imageID)
if err != nil {
t.Errorf("Unexpected error retrieving image: %#v", err)
}
if e, a := mapping.Image.DockerImageReference, image.DockerImageReference; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
if !reflect.DeepEqual(mapping.Image.DockerImageMetadata, image.DockerImageMetadata) {
t.Errorf("Expected %#v, got %#v", mapping.Image, image)
}

repo, err := storage.imageStreamRegistry.GetImageStream(ctx, "somerepo")
if err != nil {
t.Fatalf("Unexpected non-nil err: %#v", err)
}
if e, a := imageID, repo.Status.Tags["latest"].Items[0].Image; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
tagEvent := api.LatestTaggedImage(repo, "latest")
if e, a := image.DockerImageReference, tagEvent.DockerImageReference; e != a {
t.Errorf("Unexpected tracking dockerImageReference: %q != %q", a, e)
}

pullSpec, ok := api.ResolveLatestTaggedImage(repo, "latest")
if !ok {
t.Fatalf("Failed to resolv latest tagged image")
}
if e, a := image.DockerImageReference, pullSpec; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
}

func TestAddExistingImageOverridingDockerImageReference(t *testing.T) {
imageID := "sha256:8d812da98d6dd61620343f1a5bf6585b34ad6ed16e5c5f7c7216a525d6aeb772"
newRepo := &api.ImageStream{
ObjectMeta: kapi.ObjectMeta{
Namespace: "default",
Name: "newrepo",
},
Spec: api.ImageStreamSpec{
DockerImageRepository: "localhost:5000/default/newrepo",
},
Status: api.ImageStreamStatus{
DockerImageRepository: "localhost:5000/default/newrepo",
},
}
existingImage := &api.Image{
ObjectMeta: kapi.ObjectMeta{
Name: imageID,
Annotations: map[string]string{api.ManagedByOpenShiftAnnotation: "true"},
},
DockerImageReference: "localhost:5000/someproject/somerepo@" + imageID,
DockerImageMetadata: api.DockerImage{
Config: &api.DockerConfig{
Cmd: []string{"ls", "/"},
Env: []string{"a=1"},
ExposedPorts: map[string]struct{}{"1234/tcp": {}},
Memory: 1234,
CPUShares: 99,
WorkingDir: "/workingDir",
},
},
}

client, server, storage := setup(t)
defer server.Terminate(t)

_, err := client.Put(
context.TODO(),
etcdtest.AddPrefix("/imagestreams/default/newrepo"),
runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(v1.SchemeGroupVersion), newRepo),
)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
_, err = client.Put(
context.TODO(),
etcdtest.AddPrefix("/images/"+imageID), runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(v1.SchemeGroupVersion), existingImage),
)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

mapping := api.ImageStreamMapping{
ObjectMeta: kapi.ObjectMeta{
Name: "newrepo",
},
Image: *existingImage,
Tag: "latest",
}
ctx := kapi.NewDefaultContext()
_, err = storage.Create(ctx, &mapping)
if err != nil {
t.Fatalf("Unexpected error creating mapping: %#v", err)
}

image, err := storage.imageRegistry.GetImage(ctx, imageID)
if err != nil {
t.Errorf("Unexpected error retrieving image: %#v", err)
}
if e, a := mapping.Image.DockerImageReference, image.DockerImageReference; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
if !reflect.DeepEqual(mapping.Image.DockerImageMetadata, image.DockerImageMetadata) {
t.Errorf("Expected %#v, got %#v", mapping.Image, image)
}

repo, err := storage.imageStreamRegistry.GetImageStream(ctx, "newrepo")
if err != nil {
t.Fatalf("Unexpected non-nil err: %#v", err)
}
if e, a := imageID, repo.Status.Tags["latest"].Items[0].Image; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
tagEvent := api.LatestTaggedImage(repo, "latest")
if e, a := testDefaultRegistryURL+"/default/newrepo@"+imageID, tagEvent.DockerImageReference; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
if tagEvent.DockerImageReference == image.DockerImageReference {
t.Errorf("Expected image stream to have dockerImageReference other than %q", image.DockerImageReference)
}

pullSpec, ok := api.ResolveLatestTaggedImage(repo, "latest")
if !ok {
t.Fatalf("Failed to resolv latest tagged image")
}
if e, a := testDefaultRegistryURL+"/default/newrepo@"+imageID, pullSpec; e != a {
t.Errorf("Expected %s, got %s", e, a)
}
}

Expand Down Expand Up @@ -291,7 +420,7 @@ func TestAddExistingImageAndTag(t *testing.T) {
Name: "existingImage",
Namespace: "default",
},
DockerImageReference: "localhost:5000/someproject/somerepo:imageID1",
DockerImageReference: "localhost:5000/someproject/somerepo@" + testImageID,
DockerImageMetadata: api.DockerImage{
Config: &api.DockerConfig{
Cmd: []string{"ls", "/"},
Expand Down