Skip to content

Commit

Permalink
Allow multiple segments in DockerImageReference
Browse files Browse the repository at this point in the history 
We need to be able to handle a repository with more than one component
in the name (separated by slash). This necessity follows from the fact
that docker/distribution supports such names, but we can't parse them.

https://bugzilla.redhat.com/show_bug.cgi?id=1373281

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>
  • Loading branch information
@legionus
legionus committed Oct 4, 2016
1 parent b3ec794 commit eeff969
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 19 deletions.
56 changes: 52 additions & 4 deletions pkg/build/api/validation/validation_test.go
View file
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
33 changes: 32 additions & 1 deletion pkg/image/api/helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,38 @@ 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
// 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], ":") {
if IsRegistryDockerHub(repoParts[0]) {
// DockerHub doesn't support multiple segments.
return ref, fmt.Errorf("the docker pull spec %q must be of the form / to be valid for the docker hub", spec)
}
if len(repoParts[0]) == 0 {
return ref, fmt.Errorf("the docker pull spec %q must contain non-empty registry name", spec)
}
ref.Registry = repoParts[0]

if len(repoParts[1]) == 0 {
return ref, fmt.Errorf("the docker pull spec %q must not contain 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 must not contain 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)
}

Expand Down
18 changes: 17 additions & 1 deletion pkg/image/api/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,21 @@ func TestParseDockerImageReference(t *testing.T) {
Err: true,
},
{
From: "bar/foo/baz/biz",
From: "bar/foo/baz/biz",
Registry: "bar",
Namespace: "foo",
Name: "baz/biz",
},
{
From: "bar/foo/baz////biz",
Err: true,
},
{
From: "//foo/baz/biz",
Err: true,
},
{
From: "docker.io/library/foo/myapp",
Err: true,
},
{
Expand All @@ -313,6 +327,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
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\" must not contain 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\" must not contain empty segments"),
},
},
"invalid dockerImageRepository with tag": {
Expand Down
4 changes: 2 additions & 2 deletions pkg/image/importer/importer_test.go
View file
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\" must not contain 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
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

0 comments on commit eeff969

Please sign in to comment.