Skip to content
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

Allow multiple segments in DockerImageReference #11173

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Allow multiple segments in DockerImageReference #11173

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
56 changes: 52 additions & 4 deletions pkg/build/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,7 +1269,7 @@ func TestValidateCommonSpec(t *testing.T) {
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "DockerImage",
Name: "some/long/value/with/no/meaning",
Name: "///some/long/value/with/no/meaning",
},
},
},
Expand All @@ -1290,7 +1290,7 @@ func TestValidateCommonSpec(t *testing.T) {
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "ImageStream",
Name: "some/long/value/with/no/meaning",
Name: "///some/long/value/with/no/meaning",
},
},
},
Expand All @@ -1311,7 +1311,7 @@ func TestValidateCommonSpec(t *testing.T) {
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "ImageStreamTag",
Name: "some/long/value/with/no/meaning",
Name: "///some/long/value/with/no/meaning",
},
},
},
Expand All @@ -1332,7 +1332,7 @@ func TestValidateCommonSpec(t *testing.T) {
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "ImageStreamTag",
Name: "some/long/value/with/no/meaning:latest",
Name: "///some/long/value/with/no/meaning:latest",
},
},
},
Expand Down Expand Up @@ -2051,6 +2051,54 @@ func TestValidateCommonSpecSuccess(t *testing.T) {
},
},
},
// 6
{
CommonSpec: buildapi.CommonSpec{
Source: buildapi.BuildSource{
Git: &buildapi.GitBuildSource{
URI: "http://github.com/my/repository",
},
},
Strategy: buildapi.BuildStrategy{
SourceStrategy: &buildapi.SourceBuildStrategy{
From: kapi.ObjectReference{
Kind: "DockerImage",
Name: "reponame",
},
},
},
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "DockerImage",
Name: "registry/project/repository/data",
},
},
},
},
// 7
{
CommonSpec: buildapi.CommonSpec{
Source: buildapi.BuildSource{
Git: &buildapi.GitBuildSource{
URI: "http://github.com/my/repository",
},
},
Strategy: buildapi.BuildStrategy{
SourceStrategy: &buildapi.SourceBuildStrategy{
From: kapi.ObjectReference{
Kind: "DockerImage",
Name: "registry/project/repository/data",
},
},
},
Output: buildapi.BuildOutput{
To: &kapi.ObjectReference{
Kind: "DockerImage",
Name: "repository/data",
},
},
},
},
}
for count, config := range testCases {
errors := validateCommonSpec(&config.CommonSpec, nil)
Expand Down
29 changes: 28 additions & 1 deletion pkg/image/api/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,34 @@ func ParseDockerImageReference(spec string) (DockerImageReference, error) {
ref.ID = id
break
default:
// TODO: this is no longer true with V2
// Handle multiple segments in form: registry/namespace/namesegment1/namesegment2/.../name
Copy link

Choose a reason for hiding this comment

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

Regarding case 3: I feel like it's about time to recognize registry part based on special characters it contains. The same way upstream does it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miminar parser refactoring out of scope of PR. I can open following issue about it.

Copy link

Choose a reason for hiding this comment

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

Ahh, I hate devcuts.

// but not this form: http://registry/namespace/name. We don't support using the schema in
// the DockerImageReference.
if len(repoParts) > 3 && !strings.HasSuffix(repoParts[0], ":") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this going to miss validating myregistry.com:5000/a/b/c?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ncdc No. This check only for suffix, not for substring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see it now, thanks. Nevermind :-) But when would it have a suffix of :?

Copy link
Contributor Author

@legionus legionus Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

http://example.com/foo/bar

If you split it by / then first part (registry) will be http:.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not valid. We've never supported a scheme. Is that all you're testing the suffix for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also hostname can't have : at the end anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right... I guess this is ok if we actually have users putting in http:// at the front of their pull specs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ncdc We have a tests that verify this ban:

https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L290-L291

https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L298-L299

if len(repoParts[0]) == 0 {
return ref, fmt.Errorf("the docker pull spec %q cannot have empty segments", spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this error text consistent with the 2 below please

}
ref.Registry = repoParts[0]

if len(repoParts[1]) == 0 {
return ref, fmt.Errorf("the docker pull spec %q cannot have empty segments", spec)
}
ref.Namespace = repoParts[1]

for i, part := range repoParts[2:] {
if len(part) == 0 {
return ref, fmt.Errorf("the docker pull spec %q cannot have empty segments", spec)
}
if i > 0 {
ref.Name += "/"
}
ref.Name += part
}

ref.Tag = tag
ref.ID = id
break
}
return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec)
Copy link

Choose a reason for hiding this comment

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

The error message looks outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. This is the error message that was returned by ParseDockerImageReference in same case.

Copy link

Choose a reason for hiding this comment

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

The error message contradicts a comment two lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be OK if I put comment // http://registry/namespace/name inside if ?

Copy link

Choose a reason for hiding this comment

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

I don't quite understand, why a function that is supposed to parse a reference with way more than 3 segments would ever return "the docker pull spec %q must be two or three segments separated by slashes". The error statement just isn't right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miminar because this case for http://example.com/namespace/name.

But I just realized that I can't act like that and in case of specifying of schema I need to parse the rest path. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miminar I was wrong again. We don't support schema in DockerImageReference:
https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L290-L291
https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L298-L299

Copy link
Contributor

Choose a reason for hiding this comment

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

To get to this line of code, it means either:

  1. len(repoParts) > 3 and repoparts[0] is a scheme (http: or https:)
  2. len(repoParts) = 0

Either way, I don't think we should keep the error message as is, saying that it has to be two or three segments. I'm not sure what the best error message is, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ncdc This error message describes the common requirements for the DockerImageReference. It seems to me that it is better to leave it as is. Otherwise we need to add much more checks to make it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave it for now, to get this PR in, but I would like it fixed when we refactor parsing.

}

Expand Down
20 changes: 19 additions & 1 deletion pkg/image/api/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,12 @@ func TestParseDockerImageReference(t *testing.T) {
Namespace: "user",
Name: "myapp",
},
{
From: "docker.io/user/project/myapp",
Registry: "docker.io",
Namespace: "user",
Name: "project/myapp",
},
{
From: "index.docker.io/bar",
Registry: "index.docker.io",
Expand Down Expand Up @@ -291,7 +297,17 @@ func TestParseDockerImageReference(t *testing.T) {
Err: true,
},
{
From: "bar/foo/baz/biz",
From: "bar/foo/baz/biz",
Registry: "bar",
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, I think if the docker hub ever starts supporting multiple components, this would resolve to registry=docker.io, namespace=bar, name=foo/baz/biz.

Copy link

Choose a reason for hiding this comment

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

That's right. See my above comment https://github.com/openshift/origin/pull/11173/files#r81712056

Namespace: "foo",
Name: "baz/biz",
},
{
From: "bar/foo/baz////biz",
Err: true,
},
{
From: "//foo/baz/biz",
Err: true,
},
{
Expand All @@ -313,6 +329,8 @@ func TestParseDockerImageReference(t *testing.T) {
case err == nil && testCase.Err:
t.Errorf("%s: unexpected non-error", testCase.From)
continue
case err != nil && testCase.Err:
continue
}
if e, a := testCase.Registry, ref.Registry; e != a {
t.Errorf("%s: registry: expected %q, got %q", testCase.From, e, a)
Expand Down
6 changes: 3 additions & 3 deletions pkg/image/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ func TestValidateImageStreamMappingNotOK(t *testing.T) {
ObjectMeta: kapi.ObjectMeta{
Namespace: "default",
},
DockerImageRepository: "registry/extra/openshift/ruby-19-centos",
DockerImageRepository: "registry/extra/openshift//ruby-19-centos",
Tag: api.DefaultImageTag,
Image: api.Image{
ObjectMeta: kapi.ObjectMeta{
Expand Down Expand Up @@ -416,7 +416,7 @@ func TestValidateImageStream(t *testing.T) {
name: "foo",
dockerImageRepository: "a-|///bbb",
expected: field.ErrorList{
field.Invalid(field.NewPath("spec", "dockerImageRepository"), "a-|///bbb", "the docker pull spec \"a-|///bbb\" must be two or three segments separated by slashes"),
field.Invalid(field.NewPath("spec", "dockerImageRepository"), "a-|///bbb", "the docker pull spec \"a-|///bbb\" cannot have empty segments"),
},
},
"invalid dockerImageRepository with tag": {
Expand Down Expand Up @@ -719,7 +719,7 @@ func TestValidateImageStreamImport(t *testing.T) {
"invalid dockerImageRepository": {
isi: &api.ImageStreamImport{ObjectMeta: validMeta, Spec: repoFn("a-|///bbb")},
expected: field.ErrorList{
field.Invalid(field.NewPath("spec", "repository", "from", "name"), "a-|///bbb", "the docker pull spec \"a-|///bbb\" must be two or three segments separated by slashes"),
field.Invalid(field.NewPath("spec", "repository", "from", "name"), "a-|///bbb", "the docker pull spec \"a-|///bbb\" cannot have empty segments"),
},
},
"invalid dockerImageRepository with tag": {
Expand Down
4 changes: 2 additions & 2 deletions pkg/image/importer/importer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestImport(t *testing.T) {
Images: []api.ImageImportSpec{
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "test"}},
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "test@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"}},
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "test/un/parse/able/image"}},
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "test///un/parse/able/image"}},
{From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"}},
},
},
Expand All @@ -104,7 +104,7 @@ func TestImport(t *testing.T) {
if !expectStatusError(isi.Status.Images[1].Status, "Internal error occurred: no such digest") {
t.Errorf("unexpected status: %#v", isi.Status.Images[1].Status)
}
if !expectStatusError(isi.Status.Images[2].Status, " \"\" is invalid: from.name: Invalid value: \"test/un/parse/able/image\": invalid name: the docker pull spec \"test/un/parse/able/image\" must be two or three segments separated by slashes") {
if !expectStatusError(isi.Status.Images[2].Status, " \"\" is invalid: from.name: Invalid value: \"test///un/parse/able/image\": invalid name: the docker pull spec \"test///un/parse/able/image\" cannot have empty segments") {
t.Errorf("unexpected status: %#v", isi.Status.Images[2].Status)
}
// non DockerImage refs are no-ops
Expand Down
24 changes: 16 additions & 8 deletions test/integration/imageimporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestImageStreamImport(t *testing.T) {
},
Spec: api.ImageStreamImportSpec{
Images: []api.ImageImportSpec{
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "a/a/a/a/a/redis:latest"}, To: &kapi.LocalObjectReference{Name: "tag"}},
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "///a/a/a/a/a/redis:latest"}, To: &kapi.LocalObjectReference{Name: "tag"}},
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "redis:latest"}},
},
},
Expand Down Expand Up @@ -182,7 +182,7 @@ func mockRegistryHandler(t *testing.T, requireAuth bool, count *int) http.Handle
})
}

func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
func testImageStreamImportWithPath(t *testing.T, reponame string) {
imageDigest := "sha256:815d06b56f4138afacd0009b8e3799fcdce79f0507bf8d0588e219b93ab6fd4d"
descriptors := map[string]int64{
"sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4": 3000,
Expand Down Expand Up @@ -219,9 +219,9 @@ func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
switch r.URL.Path {
case "/v2/":
w.Write([]byte(`{}`))
case "/v2/test/image/tags/list":
w.Write([]byte("{\"name\": \"test/image\", \"tags\": [\"testtag\"]}"))
case "/v2/test/image/manifests/testtag", "/v2/test/image/manifests/" + imageDigest:
case "/v2/" + reponame + "/tags/list":
w.Write([]byte("{\"name\": \"" + reponame + "\", \"tags\": [\"testtag\"]}"))
case "/v2/" + reponame + "/manifests/testtag", "/v2/" + reponame + "/manifests/" + imageDigest:
if r.Method == "HEAD" {
w.Header().Set("Content-Length", fmt.Sprintf("%d", len(convertedManifest)))
w.Header().Set("Docker-Content-Digest", imageDigest)
Expand All @@ -230,9 +230,9 @@ func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
w.Write([]byte(convertedManifest))
}
default:
if strings.HasPrefix(r.URL.Path, "/v2/test/image/blobs/") {
if strings.HasPrefix(r.URL.Path, "/v2/"+reponame+"/blobs/") {
for dgst, size := range descriptors {
if r.URL.Path != "/v2/test/image/blobs/"+dgst {
if r.URL.Path != "/v2/"+reponame+"/blobs/"+dgst {
continue
}
if r.Method == "HEAD" {
Expand Down Expand Up @@ -276,7 +276,7 @@ func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
Import: true,
Images: []api.ImageImportSpec{
{
From: kapi.ObjectReference{Kind: "DockerImage", Name: url.Host + "/test/image:testtag"},
From: kapi.ObjectReference{Kind: "DockerImage", Name: url.Host + "/" + reponame + ":testtag"},
To: &kapi.LocalObjectReference{Name: "other"},
ImportPolicy: api.TagImportPolicy{Insecure: true},
},
Expand Down Expand Up @@ -316,6 +316,14 @@ func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
}
}

func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) {
testImageStreamImportWithPath(t, "test/image")
}

func TestImageStreamImportOfMultiSegmentDockerReference(t *testing.T) {
testImageStreamImportWithPath(t, "test/foo/bar/image")
}

func TestImageStreamImportAuthenticated(t *testing.T) {
testutil.RequireEtcd(t)
defer testutil.DumpEtcdOnFailure(t)
Expand Down