From a6e668fac54b8dcc34b9810738b22fa7c8561325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 14 Jan 2019 03:49:08 +0100 Subject: [PATCH 01/25] Don't call image.DecomposeString in imageInListToContainerImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - It is used to fill Repository(misnamed)/Tag members which have no users; so it's completely unclear why this is useful. - Given the mishandling of tags by imageParts.tag, at the very least all new code should primarily use reference.Named (even if after a decompose() to internally deal with unqualified names first), introducing new uses of original decompose() just reintroduces known trouble - so without any provided rationale, reverting seems a reasonable default action. - This drags in all of libpod/image into the "remote client" build, which seems undesirable. Signed-off-by: Miloslav Trmač --- libpod/adapter/runtime_remote.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/libpod/adapter/runtime_remote.go b/libpod/adapter/runtime_remote.go index 0fe5c449ac..27301d90b0 100644 --- a/libpod/adapter/runtime_remote.go +++ b/libpod/adapter/runtime_remote.go @@ -5,13 +5,13 @@ package adapter import ( "context" "fmt" - "github.com/containers/libpod/cmd/podman/varlink" - "github.com/containers/libpod/libpod/image" - "github.com/opencontainers/go-digest" - "github.com/urfave/cli" - "github.com/varlink/go/varlink" "strings" "time" + + iopodman "github.com/containers/libpod/cmd/podman/varlink" + digest "github.com/opencontainers/go-digest" + "github.com/urfave/cli" + "github.com/varlink/go/varlink" ) // ImageRuntime is wrapper for image runtime @@ -59,8 +59,6 @@ type remoteImage struct { RepoDigests []string Parent string Size int64 - Tag string - Repository string Created time.Time InputName string Names []string @@ -91,10 +89,6 @@ func (r *LocalRuntime) GetImages() ([]*ContainerImage, error) { } func imageInListToContainerImage(i iopodman.ImageInList, name string, runtime *LocalRuntime) (*ContainerImage, error) { - imageParts, err := image.DecomposeString(name) - if err != nil { - return nil, err - } created, err := splitStringDate(i.Created) if err != nil { return nil, err @@ -108,8 +102,6 @@ func imageInListToContainerImage(i iopodman.ImageInList, name string, runtime *L Parent: i.ParentId, Size: i.Size, Created: created, - Tag: imageParts.Tag, - Repository: imageParts.Registry, Names: i.RepoTags, isParent: i.IsParent, Runtime: runtime, From 6486e2c41be0aa50bd5b70d859a23f36070d0ae1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 14 Jan 2019 03:58:35 +0100 Subject: [PATCH 02/25] Drop image.DecomposeString, make image.Parts private imageParts again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that DecomposeString has no users, make the type private again. Any new users of it should come with a rationale - and new users of the "none"/"latest" handling of untagged/digested names that is currently implemented should have an exceptionaly unusual rationale. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 8 ++++---- libpod/image/parts.go | 36 +++++++++++++++--------------------- libpod/image/parts_test.go | 4 ++-- libpod/image/pull.go | 4 ++-- libpod/image/utils.go | 6 +++--- 5 files changed, 26 insertions(+), 32 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index dda7533853..b51aced49a 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -25,7 +25,7 @@ import ( "github.com/containers/libpod/pkg/util" "github.com/containers/storage" "github.com/containers/storage/pkg/reexec" - "github.com/opencontainers/go-digest" + digest "github.com/opencontainers/go-digest" ociv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -460,7 +460,7 @@ func normalizeTag(tag string) (string, error) { } // If the input does not have a tag, we need to add one (latest) if !decomposedTag.isTagged { - tag = fmt.Sprintf("%s:%s", tag, decomposedTag.Tag) + tag = fmt.Sprintf("%s:%s", tag, decomposedTag.tag) } // If the input doesn't specify a registry, set the registry to localhost if !decomposedTag.hasRegistry { @@ -937,7 +937,7 @@ func (i *Image) MatchRepoTag(input string) (string, error) { if err != nil { return "", err } - if dcRepoName.Registry == dcImage.Registry && dcImage.Registry != "" { + if dcRepoName.registry == dcImage.registry && dcImage.registry != "" { count++ } if dcRepoName.name == dcImage.name && dcImage.name != "" { @@ -945,7 +945,7 @@ func (i *Image) MatchRepoTag(input string) (string, error) { } else if splitString(dcRepoName.name) == splitString(dcImage.name) { count++ } - if dcRepoName.Tag == dcImage.Tag { + if dcRepoName.tag == dcImage.tag { count++ } results[count] = append(results[count], repoName) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index b2a69f26c2..9adf26fb93 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -7,12 +7,12 @@ import ( "github.com/containers/image/docker/reference" ) -// Parts describes the parts of an image's name -type Parts struct { +// imageParts describes the parts of an image's name +type imageParts struct { transport string - Registry string + registry string name string - Tag string + tag string isTagged bool hasRegistry bool } @@ -34,16 +34,10 @@ func GetImageBaseName(input string) (string, error) { return splitImageName[len(splitImageName)-1], nil } -// DecomposeString decomposes a string name into imageParts description. This -// is a wrapper for decompose -func DecomposeString(input string) (Parts, error) { - return decompose(input) -} - // decompose breaks an input name into an imageParts description -func decompose(input string) (Parts, error) { +func decompose(input string) (imageParts, error) { var ( - parts Parts + parts imageParts hasRegistry bool tag string ) @@ -62,7 +56,7 @@ func decompose(input string) (Parts, error) { } registry := reference.Domain(imgRef.(reference.Named)) imageName := reference.Path(imgRef.(reference.Named)) - // Is this a Registry or a repo? + // Is this a registry or a repo? if isRegistry(registry) { hasRegistry = true } else { @@ -71,27 +65,27 @@ func decompose(input string) (Parts, error) { registry = "" } } - return Parts{ - Registry: registry, + return imageParts{ + registry: registry, hasRegistry: hasRegistry, name: imageName, - Tag: tag, + tag: tag, isTagged: isTagged, transport: DefaultTransport, }, nil } // assemble concatenates an image's parts into a string -func (ip *Parts) assemble() string { - spec := fmt.Sprintf("%s:%s", ip.name, ip.Tag) +func (ip *imageParts) assemble() string { + spec := fmt.Sprintf("%s:%s", ip.name, ip.tag) - if ip.Registry != "" { - spec = fmt.Sprintf("%s/%s", ip.Registry, spec) + if ip.registry != "" { + spec = fmt.Sprintf("%s/%s", ip.registry, spec) } return spec } // assemble concatenates an image's parts with transport into a string -func (ip *Parts) assembleWithTransport() string { +func (ip *imageParts) assembleWithTransport() string { return fmt.Sprintf("%s%s", ip.transport, ip.assemble()) } diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 1e01c85d37..518538f0b8 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -55,9 +55,9 @@ func TestDecompose(t *testing.T) { } else { assert.NoError(t, err, c.input) assert.Equal(t, c.transport, parts.transport, c.input) - assert.Equal(t, c.registry, parts.Registry, c.input) + assert.Equal(t, c.registry, parts.registry, c.input) assert.Equal(t, c.name, parts.name, c.input) - assert.Equal(t, c.tag, parts.Tag, c.input) + assert.Equal(t, c.tag, parts.tag, c.input) assert.Equal(t, c.isTagged, parts.isTagged, c.input) assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input) assert.Equal(t, c.assembled, parts.assemble(), c.input) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 203e943106..09935fe7c2 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -76,7 +76,7 @@ func (ir *Runtime) getPullRefPair(srcRef types.ImageReference, destName string) decomposedDest, err := decompose(destName) if err == nil && !decomposedDest.hasRegistry { // If the image doesn't have a registry, set it as the default repo - decomposedDest.Registry = DefaultLocalRegistry + decomposedDest.registry = DefaultLocalRegistry decomposedDest.hasRegistry = true destName = decomposedDest.assemble() } @@ -317,7 +317,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG } var refPairs []pullRefPair for _, registry := range searchRegistries { - decomposedImage.Registry = registry + decomposedImage.registry = registry imageName := decomposedImage.assembleWithTransport() if hasShaInInputName(inputName) { imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, inputName) diff --git a/libpod/image/utils.go b/libpod/image/utils.go index 135b470081..b944de1bb2 100644 --- a/libpod/image/utils.go +++ b/libpod/image/utils.go @@ -16,7 +16,7 @@ import ( // findImageInRepotags takes an imageParts struct and searches images' repotags for // a match on name:tag -func findImageInRepotags(search Parts, images []*Image) (*storage.Image, error) { +func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, error) { var results []*storage.Image for _, image := range images { for _, name := range image.Names() { @@ -25,12 +25,12 @@ func findImageInRepotags(search Parts, images []*Image) (*storage.Image, error) if err != nil { continue } - if d.name == search.name && d.Tag == search.Tag { + if d.name == search.name && d.tag == search.tag { results = append(results, image.image) continue } // account for registry:/somedir/image - if strings.HasSuffix(d.name, search.name) && d.Tag == search.Tag { + if strings.HasSuffix(d.name, search.name) && d.tag == search.tag { results = append(results, image.image) continue } From c19294c0113e4ddea7005bab5d8a31f7ecd6c120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 18:28:28 +0100 Subject: [PATCH 03/25] Record the original reference.Named in imageParts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will eventually want to eliminate most members of imageParts in favor of using the c/image/docker/reference API directly. For now, just record the reference.Named value, and we will replace uses of the other members before removing them. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 9adf26fb93..289a02e1f2 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -9,12 +9,13 @@ import ( // imageParts describes the parts of an image's name type imageParts struct { - transport string - registry string - name string - tag string - isTagged bool - hasRegistry bool + unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization + transport string + registry string + name string + tag string + isTagged bool + hasRegistry bool } // Registries must contain a ":" or a "." or be localhost @@ -45,6 +46,7 @@ func decompose(input string) (imageParts, error) { if err != nil { return parts, err } + unnormalizedNamed := imgRef.(reference.Named) ntag, isTagged := imgRef.(reference.NamedTagged) if !isTagged { tag = "latest" @@ -54,8 +56,8 @@ func decompose(input string) (imageParts, error) { } else { tag = ntag.Tag() } - registry := reference.Domain(imgRef.(reference.Named)) - imageName := reference.Path(imgRef.(reference.Named)) + registry := reference.Domain(unnormalizedNamed) + imageName := reference.Path(unnormalizedNamed) // Is this a registry or a repo? if isRegistry(registry) { hasRegistry = true @@ -66,12 +68,13 @@ func decompose(input string) (imageParts, error) { } } return imageParts{ - registry: registry, - hasRegistry: hasRegistry, - name: imageName, - tag: tag, - isTagged: isTagged, - transport: DefaultTransport, + unnormalizedRef: unnormalizedNamed, + registry: registry, + hasRegistry: hasRegistry, + name: imageName, + tag: tag, + isTagged: isTagged, + transport: DefaultTransport, }, nil } From 3d98c42a3f7822bf608834505adcffec7f75bf28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 18:47:09 +0100 Subject: [PATCH 04/25] Inline imageParts.assembleWithTransport into callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit imageParts.transport is a constant, and the design of imageParts is not transport-independent in any sense; we will want to eliminate the transport member entirely. As a first step, drop assembleWithTransport and inline an exact equivalent into all callers. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 5 ----- libpod/image/parts_test.go | 18 ++++++++---------- libpod/image/pull.go | 4 ++-- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 289a02e1f2..cf774030b7 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -87,8 +87,3 @@ func (ip *imageParts) assemble() string { } return spec } - -// assemble concatenates an image's parts with transport into a string -func (ip *imageParts) assembleWithTransport() string { - return fmt.Sprintf("%s%s", ip.transport, ip.assemble()) -} diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 518538f0b8..3ff175f0e3 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -14,39 +14,38 @@ func TestDecompose(t *testing.T) { transport, registry, name, tag string isTagged, hasRegistry bool assembled string - assembledWithTransport string }{ - {"#", "", "", "", "", false, false, "", ""}, // Entirely invalid input + {"#", "", "", "", "", false, false, ""}, // Entirely invalid input { // Fully qualified docker.io, name-only input "docker.io/library/busybox", "docker://", "docker.io", "library/busybox", "latest", false, true, - "docker.io/library/busybox:latest", "docker://docker.io/library/busybox:latest", + "docker.io/library/busybox:latest", }, { // Fully qualified example.com, name-only input "example.com/ns/busybox", "docker://", "example.com", "ns/busybox", "latest", false, true, - "example.com/ns/busybox:latest", "docker://example.com/ns/busybox:latest", + "example.com/ns/busybox:latest", }, { // Unqualified single-name input "busybox", "docker://", "", "busybox", "latest", false, false, - "busybox:latest", "docker://busybox:latest", + "busybox:latest", }, { // Unqualified namespaced input "ns/busybox", "docker://", "", "ns/busybox", "latest", false, false, - "ns/busybox:latest", "docker://ns/busybox:latest", + "ns/busybox:latest", }, { // name:tag "example.com/ns/busybox:notlatest", "docker://", "example.com", "ns/busybox", "notlatest", true, true, - "example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest", + "example.com/ns/busybox:notlatest", }, { // name@digest // FIXME? .tag == "none" "example.com/ns/busybox" + digestSuffix, "docker://", "example.com", "ns/busybox", "none", false, true, // FIXME: this drops the digest and replaces it with an incorrect tag. - "example.com/ns/busybox:none", "docker://example.com/ns/busybox:none", + "example.com/ns/busybox:none", }, { // name:tag@digest "example.com/ns/busybox:notlatest" + digestSuffix, "docker://", "example.com", "ns/busybox", "notlatest", true, true, // FIXME: This drops the digest - "example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest", + "example.com/ns/busybox:notlatest", }, } { parts, err := decompose(c.input) @@ -61,7 +60,6 @@ func TestDecompose(t *testing.T) { assert.Equal(t, c.isTagged, parts.isTagged, c.input) assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input) assert.Equal(t, c.assembled, parts.assemble(), c.input) - assert.Equal(t, c.assembledWithTransport, parts.assembleWithTransport(), c.input) } } } diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 09935fe7c2..82fe0fdd63 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -288,7 +288,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if hasShaInInputName(inputName) { imageName = fmt.Sprintf("%s%s", decomposedImage.transport, inputName) } else { - imageName = decomposedImage.assembleWithTransport() + imageName = fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble()) } srcRef, err := alltransports.ParseImageName(imageName) if err != nil { @@ -318,7 +318,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG var refPairs []pullRefPair for _, registry := range searchRegistries { decomposedImage.registry = registry - imageName := decomposedImage.assembleWithTransport() + imageName := fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble()) if hasShaInInputName(inputName) { imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, inputName) } From 99d2259f8a71dbe5a0d9e78757b51551ce0eb8b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 18:54:17 +0100 Subject: [PATCH 05/25] Simplify pullGoalFromPossiblyUnqualifiedName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After inlining assembleWithTransport, we have two branches with the same prepending of decomposedImage.transport; move that out of the branches. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 82fe0fdd63..d09ddd5991 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -286,11 +286,11 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if decomposedImage.hasRegistry { var imageName, destName string if hasShaInInputName(inputName) { - imageName = fmt.Sprintf("%s%s", decomposedImage.transport, inputName) + imageName = inputName } else { - imageName = fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble()) + imageName = decomposedImage.assemble() } - srcRef, err := alltransports.ParseImageName(imageName) + srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName)) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } @@ -318,11 +318,11 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG var refPairs []pullRefPair for _, registry := range searchRegistries { decomposedImage.registry = registry - imageName := fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble()) + imageName := decomposedImage.assemble() if hasShaInInputName(inputName) { - imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, inputName) + imageName = fmt.Sprintf("%s/%s", registry, inputName) } - srcRef, err := alltransports.ParseImageName(imageName) + srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName)) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } From e9721b757ae5da4dad4f68c5eb721f348cfe935a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 19:00:59 +0100 Subject: [PATCH 06/25] Remove imageParts.transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is only ever set to DefaulTransport, and all of the code is docker/reference-specific anyway, so there's no point in making this a variable. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 2 -- libpod/image/parts_test.go | 27 +++++++++++++-------------- libpod/image/pull.go | 4 ++-- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index cf774030b7..b7c5d12526 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -10,7 +10,6 @@ import ( // imageParts describes the parts of an image's name type imageParts struct { unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization - transport string registry string name string tag string @@ -74,7 +73,6 @@ func decompose(input string) (imageParts, error) { name: imageName, tag: tag, isTagged: isTagged, - transport: DefaultTransport, }, nil } diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 3ff175f0e3..733e8e8552 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -10,50 +10,49 @@ func TestDecompose(t *testing.T) { const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" for _, c := range []struct { - input string - transport, registry, name, tag string - isTagged, hasRegistry bool - assembled string + input string + registry, name, tag string + isTagged, hasRegistry bool + assembled string }{ - {"#", "", "", "", "", false, false, ""}, // Entirely invalid input + {"#", "", "", "", false, false, ""}, // Entirely invalid input { // Fully qualified docker.io, name-only input - "docker.io/library/busybox", "docker://", "docker.io", "library/busybox", "latest", false, true, + "docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true, "docker.io/library/busybox:latest", }, { // Fully qualified example.com, name-only input - "example.com/ns/busybox", "docker://", "example.com", "ns/busybox", "latest", false, true, + "example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true, "example.com/ns/busybox:latest", }, { // Unqualified single-name input - "busybox", "docker://", "", "busybox", "latest", false, false, + "busybox", "", "busybox", "latest", false, false, "busybox:latest", }, { // Unqualified namespaced input - "ns/busybox", "docker://", "", "ns/busybox", "latest", false, false, + "ns/busybox", "", "ns/busybox", "latest", false, false, "ns/busybox:latest", }, { // name:tag - "example.com/ns/busybox:notlatest", "docker://", "example.com", "ns/busybox", "notlatest", true, true, + "example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true, "example.com/ns/busybox:notlatest", }, { // name@digest // FIXME? .tag == "none" - "example.com/ns/busybox" + digestSuffix, "docker://", "example.com", "ns/busybox", "none", false, true, + "example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true, // FIXME: this drops the digest and replaces it with an incorrect tag. "example.com/ns/busybox:none", }, { // name:tag@digest - "example.com/ns/busybox:notlatest" + digestSuffix, "docker://", "example.com", "ns/busybox", "notlatest", true, true, + "example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true, // FIXME: This drops the digest "example.com/ns/busybox:notlatest", }, } { parts, err := decompose(c.input) - if c.transport == "" { + if c.assembled == "" { assert.Error(t, err, c.input) } else { assert.NoError(t, err, c.input) - assert.Equal(t, c.transport, parts.transport, c.input) assert.Equal(t, c.registry, parts.registry, c.input) assert.Equal(t, c.name, parts.name, c.input) assert.Equal(t, c.tag, parts.tag, c.input) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index d09ddd5991..342ee6f175 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -290,7 +290,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG } else { imageName = decomposedImage.assemble() } - srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName)) + srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName)) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } @@ -322,7 +322,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if hasShaInInputName(inputName) { imageName = fmt.Sprintf("%s/%s", registry, inputName) } - srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName)) + srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName)) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } From 788bc360219e83028b3b0996ab4b6afbd0530e9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 19:12:17 +0100 Subject: [PATCH 07/25] Simplify pullGoalFromPossiblyUnqualifiedName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both imageParts and this function implicitly assume docker.Transport troughout, so instead of pretending to be flexible about DefaultTransport, just hard-code docker.ParseReference directly. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 342ee6f175..d4be53ef8a 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -290,7 +290,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG } else { imageName = decomposedImage.assemble() } - srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName)) + srcRef, err := docker.ParseReference("//" + imageName) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } @@ -322,7 +322,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if hasShaInInputName(inputName) { imageName = fmt.Sprintf("%s/%s", registry, inputName) } - srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName)) + srcRef, err := docker.ParseReference("//" + imageName) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } From 035c732dedf37637b9ac5e37748ec1b0bea3c966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 19:49:53 +0100 Subject: [PATCH 08/25] Reorganize normalizeTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the registry defaulting before tag defaulting. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index b51aced49a..249beceace 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -458,14 +458,14 @@ func normalizeTag(tag string) (string, error) { if err != nil { return "", err } - // If the input does not have a tag, we need to add one (latest) - if !decomposedTag.isTagged { - tag = fmt.Sprintf("%s:%s", tag, decomposedTag.tag) - } // If the input doesn't specify a registry, set the registry to localhost if !decomposedTag.hasRegistry { tag = fmt.Sprintf("%s/%s", DefaultLocalRegistry, tag) } + // If the input does not have a tag, we need to add one (latest) + if !decomposedTag.isTagged { + tag = fmt.Sprintf("%s:%s", tag, decomposedTag.tag) + } return tag, nil } From ae2a95196e9e5a3519b396105d01a3661de330ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 20:02:54 +0100 Subject: [PATCH 09/25] Don't use imageParts.assemble when pulling from a qualified name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGES BEHAVIOR. If the name is qualified, instead of decomposing it into components and re-assembling, just use the input name unmodified: - For name:tag values, .assemble() just recreates the input. - For untagged values, .assemble() adds ":latest"; we keep the input as is, but both docker.ParseReference and storage.Transport.ParseStoreReference use reference.TagNameOnly() already. - For digested references, .assemble() adds ":none", but the code was already bypassing .assemble() on that path already - for the source reference. For the destination, this replaces a :none destination with a the @digest reference, as expected. Note that while decompose() has already parsed the input, it (intentionally) bypassed the docker.io/library normalization; therefore we parse the input again (via docker.ParseReference) to ensure that the reference is normalized. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 17 +++-------------- libpod/image/pull_test.go | 3 +-- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index d4be53ef8a..ee5e333e32 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -284,24 +284,13 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG return nil, err } if decomposedImage.hasRegistry { - var imageName, destName string - if hasShaInInputName(inputName) { - imageName = inputName - } else { - imageName = decomposedImage.assemble() - } - srcRef, err := docker.ParseReference("//" + imageName) + srcRef, err := docker.ParseReference("//" + inputName) if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } - if hasShaInInputName(inputName) { - destName = decomposedImage.assemble() - } else { - destName = inputName - } - destRef, err := is.Transport.ParseStoreReference(ir.store, destName) + destRef, err := is.Transport.ParseStoreReference(ir.store, inputName) if err != nil { - return nil, errors.Wrapf(err, "error parsing dest reference name %#v", destName) + return nil, errors.Wrapf(err, "error parsing dest reference name %#v", inputName) } ps := pullRefPair{ image: inputName, diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index 37b45dc833..4a0119070d 100644 --- a/libpod/image/pull_test.go +++ b/libpod/image/pull_test.go @@ -324,8 +324,7 @@ func TestPullGoalFromPossiblyUnqualifiedName(t *testing.T) { { // Qualified example.com, name@digest. "example.com/ns/busybox" + digestSuffix, []pullRefStrings{{"example.com/ns/busybox" + digestSuffix, "docker://example.com/ns/busybox" + digestSuffix, - // FIXME?! Why is .dstName dropping the digest, and adding :none?! - "example.com/ns/busybox:none"}}, + "example.com/ns/busybox" + digestSuffix}}, false, }, // Qualified example.com, name:tag@digest. This code is happy to try, but .srcRef parsing currently rejects such input. From 72777b7fee22e04edee08034927d5864ffc4bc3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 20:32:30 +0100 Subject: [PATCH 10/25] Add imageParts.referenceWithRegistry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the primary goal of decompose()+assemble(), to support qualifying an image name. Does not have any users yet, so does not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 18 ++++++++++++++++ libpod/image/parts_test.go | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index b7c5d12526..8d059e35b0 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/containers/image/docker/reference" + "github.com/pkg/errors" ) // imageParts describes the parts of an image's name @@ -76,6 +77,23 @@ func decompose(input string) (imageParts, error) { }, nil } +// referenceWithRegistry returns a (normalized) reference.Named composed of ip (with !ip.hasRegistry) +// qualified with registry. +func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, error) { + if ip.hasRegistry { + return nil, errors.Errorf("internal error: referenceWithRegistry called on imageParts with a registry (%#v)", *ip) + } + // We could build a reference.WithName+WithTag/WithDigest here, but we need to round-trip via a string + // and a ParseNormalizedNamed anyway to get the right normalization of docker.io/library, so + // just use a string directly. + qualified := registry + "/" + ip.unnormalizedRef.String() + ref, err := reference.ParseNormalizedNamed(qualified) + if err != nil { + return nil, errors.Wrapf(err, "error normalizing registry+unqualified reference %#v", qualified) + } + return ref, nil +} + // assemble concatenates an image's parts into a string func (ip *imageParts) assemble() string { spec := fmt.Sprintf("%s:%s", ip.name, ip.tag) diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 733e8e8552..b2cc0f8c0b 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDecompose(t *testing.T) { @@ -62,3 +63,44 @@ func TestDecompose(t *testing.T) { } } } + +func TestImagePartsReferenceWithRegistry(t *testing.T) { + const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + + for _, c := range []struct { + input string + withDocker, withNonDocker string + }{ + {"example.com/ns/busybox", "", ""}, // Fully-qualified input is invalid. + {"busybox", "docker.io/library/busybox", "example.com/busybox"}, // Single-name input + {"ns/busybox", "docker.io/ns/busybox", "example.com/ns/busybox"}, // Namespaced input + {"ns/busybox:notlatest", "docker.io/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"}, // name:tag + {"ns/busybox" + digestSuffix, "docker.io/ns/busybox" + digestSuffix, "example.com/ns/busybox" + digestSuffix}, // name@digest + { // name:tag@digest + "ns/busybox:notlatest" + digestSuffix, + "docker.io/ns/busybox:notlatest" + digestSuffix, "example.com/ns/busybox:notlatest" + digestSuffix, + }, + } { + parts, err := decompose(c.input) + require.NoError(t, err) + if c.withDocker == "" { + _, err := parts.referenceWithRegistry("docker.io") + assert.Error(t, err, c.input) + _, err = parts.referenceWithRegistry("example.com") + assert.Error(t, err, c.input) + } else { + ref, err := parts.referenceWithRegistry("docker.io") + require.NoError(t, err, c.input) + assert.Equal(t, c.withDocker, ref.String()) + ref, err = parts.referenceWithRegistry("example.com") + require.NoError(t, err, c.input) + assert.Equal(t, c.withNonDocker, ref.String()) + } + } + + // Invalid registry value + parts, err := decompose("busybox") + require.NoError(t, err) + _, err = parts.referenceWithRegistry("invalid@domain") + assert.Error(t, err) +} From 2171a393904051ab724fa08d01a964adbf7c1880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 20:43:13 +0100 Subject: [PATCH 11/25] Use imageParts.referenceWithRegistry in getPullRefPair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGES BEHAVIOR. This bypasses .assemble, and preserves the original lack of tag / original digest instead of adding :latest/:none (still subject to ParseStoreReference normalization). Using the original digest seems clearly correct; dropping the :latest suffix from .image strings only affects user-visible input; later uses of the return value of pullImageFrom... use ParseStoreReference, which calls reference.TagNameOnly, so the image name should be processed the same way whether it contains a tag or not. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 8 +++++--- libpod/image/pull_test.go | 12 ++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index ee5e333e32..70f89ea7d5 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -76,9 +76,11 @@ func (ir *Runtime) getPullRefPair(srcRef types.ImageReference, destName string) decomposedDest, err := decompose(destName) if err == nil && !decomposedDest.hasRegistry { // If the image doesn't have a registry, set it as the default repo - decomposedDest.registry = DefaultLocalRegistry - decomposedDest.hasRegistry = true - destName = decomposedDest.assemble() + ref, err := decomposedDest.referenceWithRegistry(DefaultLocalRegistry) + if err != nil { + return pullRefPair{}, err + } + destName = ref.String() } reference := destName diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index 4a0119070d..2176678d34 100644 --- a/libpod/image/pull_test.go +++ b/libpod/image/pull_test.go @@ -76,9 +76,7 @@ func TestGetPullRefPair(t *testing.T) { }, { // name, no registry, no tag: "dir:/dev/this-does-not-exist", "from-directory", - // FIXME(?) Adding a registry also adds a :latest tag. OTOH that actually matches the used destination. - // Either way it is surprising that the localhost/ addition changes this. (mitr hoping to remove the "image" member). - "localhost/from-directory:latest", "localhost/from-directory:latest", + "localhost/from-directory", "localhost/from-directory:latest", }, { // registry/name:tag : "dir:/dev/this-does-not-exist", "example.com/from-directory:notlatest", @@ -90,8 +88,7 @@ func TestGetPullRefPair(t *testing.T) { }, { // name@digest, no registry: "dir:/dev/this-does-not-exist", "from-directory" + digestSuffix, - // FIXME?! Why is this dropping the digest, and adding :none?! - "localhost/from-directory:none", "localhost/from-directory:none", + "localhost/from-directory" + digestSuffix, "localhost/from-directory" + digestSuffix, }, { // registry/name@digest: "dir:/dev/this-does-not-exist", "example.com/from-directory" + digestSuffix, @@ -211,14 +208,13 @@ func TestPullGoalFromImageReference(t *testing.T) { false, }, { // Relative path, single element. - // FIXME? Note the :latest difference in .image. "dir:this-does-not-exist", - []expected{{"localhost/this-does-not-exist:latest", "localhost/this-does-not-exist:latest"}}, + []expected{{"localhost/this-does-not-exist", "localhost/this-does-not-exist:latest"}}, false, }, { // Relative path, multiple elements. "dir:testdata/this-does-not-exist", - []expected{{"localhost/testdata/this-does-not-exist:latest", "localhost/testdata/this-does-not-exist:latest"}}, + []expected{{"localhost/testdata/this-does-not-exist", "localhost/testdata/this-does-not-exist:latest"}}, false, }, From 81204487dbf9903f4a8784be9a580f8a46d1d381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 20:55:53 +0100 Subject: [PATCH 12/25] Use imageParts.referenceWithRegistry in pullGoalFromPossiblyUnqualifiedName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGES BEHAVIOR. This bypasses .assemble, and preserves the original lack of tag / original digest instead of adding :latest/:none (still subject to ParseStoreReference normalization). Using the original digest seems clearly correct; dropping the :latest suffix from .image strings, and adding /library to docker.io/shortname, only affects user-visible input; later uses of the return value of pullImageFrom... use ParseStoreReference, which calls reference.ParseNormalizedNamed and reference.TagNameOnly, so the image name should be processed the same way whether it contains a tag, or libray/, or not. This also allows us to drop the problematic hasShaInInputName heuristic/condition/helper. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 19 ++++++------------- libpod/image/pull_test.go | 13 ++++++------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 70f89ea7d5..af6b84c34b 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "strings" cp "github.com/containers/image/copy" "github.com/containers/image/directory" @@ -272,12 +271,6 @@ func (ir *Runtime) doPullImage(ctx context.Context, sc *types.SystemContext, goa return images, nil } -// hasShaInInputName returns a bool as to whether the user provided an image name that includes -// a reference to a specific sha -func hasShaInInputName(inputName string) bool { - return strings.Contains(inputName, "@sha256:") -} - // pullGoalFromPossiblyUnqualifiedName looks at inputName and determines the possible // image references to try pulling in combination with the registries.conf file as well func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullGoal, error) { @@ -308,17 +301,17 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG } var refPairs []pullRefPair for _, registry := range searchRegistries { - decomposedImage.registry = registry - imageName := decomposedImage.assemble() - if hasShaInInputName(inputName) { - imageName = fmt.Sprintf("%s/%s", registry, inputName) + ref, err := decomposedImage.referenceWithRegistry(registry) + if err != nil { + return nil, err } + imageName := ref.String() srcRef, err := docker.ParseReference("//" + imageName) if err != nil { - return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) + return nil, errors.Wrapf(err, "unable to parse '%s'", imageName) } ps := pullRefPair{ - image: decomposedImage.assemble(), + image: imageName, srcRef: srcRef, } ps.dstRef, err = is.Transport.ParseStoreReference(ir.store, ps.image) diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index 2176678d34..3890c5e6c5 100644 --- a/libpod/image/pull_test.go +++ b/libpod/image/pull_test.go @@ -328,16 +328,16 @@ func TestPullGoalFromPossiblyUnqualifiedName(t *testing.T) { { // Unqualified, single-name, name-only "busybox", []pullRefStrings{ - {"example.com/busybox:latest", "docker://example.com/busybox:latest", "example.com/busybox:latest"}, + {"example.com/busybox", "docker://example.com/busybox:latest", "example.com/busybox:latest"}, // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) - {"docker.io/busybox:latest", "docker://busybox:latest", "docker.io/library/busybox:latest"}, + {"docker.io/library/busybox", "docker://busybox:latest", "docker.io/library/busybox:latest"}, }, true, }, { // Unqualified, namespaced, name-only "ns/busybox", []pullRefStrings{ - {"example.com/ns/busybox:latest", "docker://example.com/ns/busybox:latest", "example.com/ns/busybox:latest"}, + {"example.com/ns/busybox", "docker://example.com/ns/busybox:latest", "example.com/ns/busybox:latest"}, }, true, }, @@ -346,17 +346,16 @@ func TestPullGoalFromPossiblyUnqualifiedName(t *testing.T) { []pullRefStrings{ {"example.com/busybox:notlatest", "docker://example.com/busybox:notlatest", "example.com/busybox:notlatest"}, // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) - {"docker.io/busybox:notlatest", "docker://busybox:notlatest", "docker.io/library/busybox:notlatest"}, + {"docker.io/library/busybox:notlatest", "docker://busybox:notlatest", "docker.io/library/busybox:notlatest"}, }, true, }, { // Unqualified, name@digest "busybox" + digestSuffix, []pullRefStrings{ - // FIXME?! Why is .input and .dstName dropping the digest, and adding :none?! - {"example.com/busybox:none", "docker://example.com/busybox" + digestSuffix, "example.com/busybox:none"}, + {"example.com/busybox" + digestSuffix, "docker://example.com/busybox" + digestSuffix, "example.com/busybox" + digestSuffix}, // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) - {"docker.io/busybox:none", "docker://busybox" + digestSuffix, "docker.io/library/busybox:none"}, + {"docker.io/library/busybox" + digestSuffix, "docker://busybox" + digestSuffix, "docker.io/library/busybox" + digestSuffix}, }, true, }, From 633501b1b7450676c47d891092b53680206b8398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:01:56 +0100 Subject: [PATCH 13/25] Use getPullRefPair / getSinglePullRefPairGoal in pullGoalFromPossiblyUnqualifiedName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This shortens the code a bit, but most importantly ensures that all pulls from docker.Transport are processed exactly the same way, and there is only a single store.ParseStoreReference in the pull code. It's a bit wasteful to call decompose() in getPullRefPair just after pullGoalFromPossiblyUnqualifiedName has qualified the name, but on balance only having exactly one code path seems worth it. Alternatively we could split getPullRefPairToQualifiedDestination from getPullRefPair. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/pull.go | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index af6b84c34b..434b835201 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -283,16 +283,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", inputName) } - destRef, err := is.Transport.ParseStoreReference(ir.store, inputName) - if err != nil { - return nil, errors.Wrapf(err, "error parsing dest reference name %#v", inputName) - } - ps := pullRefPair{ - image: inputName, - srcRef: srcRef, - dstRef: destRef, - } - return singlePullRefPairGoal(ps), nil + return ir.getSinglePullRefPairGoal(srcRef, inputName) } searchRegistries, err := registries.GetRegistries() @@ -310,13 +301,9 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG if err != nil { return nil, errors.Wrapf(err, "unable to parse '%s'", imageName) } - ps := pullRefPair{ - image: imageName, - srcRef: srcRef, - } - ps.dstRef, err = is.Transport.ParseStoreReference(ir.store, ps.image) + ps, err := ir.getPullRefPair(srcRef, imageName) if err != nil { - return nil, errors.Wrapf(err, "error parsing dest reference name %#v", ps.image) + return nil, err } refPairs = append(refPairs, ps) } From e5c764ec3cc11982cdb8e127554e4ac61ce9a854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:11:32 +0100 Subject: [PATCH 14/25] Remove no longer used imageParts.assemble() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 11 ----------- libpod/image/parts_test.go | 15 ++------------- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 8d059e35b0..566a3842c3 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -1,7 +1,6 @@ package image import ( - "fmt" "strings" "github.com/containers/image/docker/reference" @@ -93,13 +92,3 @@ func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, e } return ref, nil } - -// assemble concatenates an image's parts into a string -func (ip *imageParts) assemble() string { - spec := fmt.Sprintf("%s:%s", ip.name, ip.tag) - - if ip.registry != "" { - spec = fmt.Sprintf("%s/%s", ip.registry, spec) - } - return spec -} diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index b2cc0f8c0b..3ca39b1dc2 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -14,43 +14,33 @@ func TestDecompose(t *testing.T) { input string registry, name, tag string isTagged, hasRegistry bool - assembled string }{ - {"#", "", "", "", false, false, ""}, // Entirely invalid input + {"#", "", "", "", false, false}, // Entirely invalid input { // Fully qualified docker.io, name-only input "docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true, - "docker.io/library/busybox:latest", }, { // Fully qualified example.com, name-only input "example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true, - "example.com/ns/busybox:latest", }, { // Unqualified single-name input "busybox", "", "busybox", "latest", false, false, - "busybox:latest", }, { // Unqualified namespaced input "ns/busybox", "", "ns/busybox", "latest", false, false, - "ns/busybox:latest", }, { // name:tag "example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true, - "example.com/ns/busybox:notlatest", }, { // name@digest // FIXME? .tag == "none" "example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true, - // FIXME: this drops the digest and replaces it with an incorrect tag. - "example.com/ns/busybox:none", }, { // name:tag@digest "example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true, - // FIXME: This drops the digest - "example.com/ns/busybox:notlatest", }, } { parts, err := decompose(c.input) - if c.assembled == "" { + if c.name == "" { assert.Error(t, err, c.input) } else { assert.NoError(t, err, c.input) @@ -59,7 +49,6 @@ func TestDecompose(t *testing.T) { assert.Equal(t, c.tag, parts.tag, c.input) assert.Equal(t, c.isTagged, parts.isTagged, c.input) assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input) - assert.Equal(t, c.assembled, parts.assemble(), c.input) } } } From e58aa74766c14844700330e11e8f0b48843884be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:18:53 +0100 Subject: [PATCH 15/25] Use imageparts.referenceWithRegistry in normalizeTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... instead of open-coding something similar. Eventually we will use the reference type further in here. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 249beceace..3d8cf2651b 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -460,7 +460,11 @@ func normalizeTag(tag string) (string, error) { } // If the input doesn't specify a registry, set the registry to localhost if !decomposedTag.hasRegistry { - tag = fmt.Sprintf("%s/%s", DefaultLocalRegistry, tag) + ref, err := decomposedTag.referenceWithRegistry(DefaultLocalRegistry) + if err != nil { + return "", err + } + tag = ref.String() } // If the input does not have a tag, we need to add one (latest) if !decomposedTag.isTagged { From 1c19d19c6ef9627413e34b30a643bef9b2970acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:33:01 +0100 Subject: [PATCH 16/25] Add imageParts.normalizedReference() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be used in normalizeTag to work with references instead of strings. Not used anywhere yet, should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 14 ++++++++++++++ libpod/image/parts_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 566a3842c3..a4eb575196 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -92,3 +92,17 @@ func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, e } return ref, nil } + +// normalizedReference returns a (normalized) reference for ip (with ip.hasRegistry) +func (ip *imageParts) normalizedReference() (reference.Named, error) { + if !ip.hasRegistry { + return nil, errors.Errorf("internal error: normalizedReference called on imageParts without a registry (%#v)", *ip) + } + // We need to round-trip via a string to get the right normalization of docker.io/library + s := ip.unnormalizedRef.String() + ref, err := reference.ParseNormalizedNamed(s) + if err != nil { // Should never happen + return nil, errors.Wrapf(err, "error normalizing qualified reference %#v", s) + } + return ref, nil +} diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 3ca39b1dc2..9955a20eae 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -93,3 +93,31 @@ func TestImagePartsReferenceWithRegistry(t *testing.T) { _, err = parts.referenceWithRegistry("invalid@domain") assert.Error(t, err) } + +func TestImagePartsNormalizedReference(t *testing.T) { + const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + + for _, c := range []struct{ input, expected string }{ + {"busybox", ""}, // Unqualified input is invalid + {"docker.io/busybox", "docker.io/library/busybox"}, // docker.io single-name + {"example.com/busybox", "example.com/busybox"}, // example.com single-name + {"docker.io/ns/busybox", "docker.io/ns/busybox"}, // docker.io namespaced + {"example.com/ns/busybox", "example.com/ns/busybox"}, // example.com namespaced + {"example.com/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"}, // name:tag + {"example.com/ns/busybox" + digestSuffix, "example.com/ns/busybox" + digestSuffix}, // name@digest + { // name:tag@digest + "example.com/ns/busybox:notlatest" + digestSuffix, "example.com/ns/busybox:notlatest" + digestSuffix, + }, + } { + parts, err := decompose(c.input) + require.NoError(t, err) + if c.expected == "" { + _, err := parts.normalizedReference() + assert.Error(t, err, c.input) + } else { + ref, err := parts.normalizedReference() + require.NoError(t, err, c.input) + assert.Equal(t, c.expected, ref.String()) + } + } +} From e060a19c8768c9dd65d1b8dcbd6da7ec9faa1163 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:35:04 +0100 Subject: [PATCH 17/25] Use imageParts.normalizedReference in normalizeTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is another step to using reference values instead of strings here. CHANGES BEHAVIOR: docker.io/busybox is now normalized to docker.io/library/busybox. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 10 ++++++++-- libpod/image/image_test.go | 1 + 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 3d8cf2651b..2457a1c229 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -459,13 +459,19 @@ func normalizeTag(tag string) (string, error) { return "", err } // If the input doesn't specify a registry, set the registry to localhost + var ref reference.Named if !decomposedTag.hasRegistry { - ref, err := decomposedTag.referenceWithRegistry(DefaultLocalRegistry) + ref, err = decomposedTag.referenceWithRegistry(DefaultLocalRegistry) + if err != nil { + return "", err + } + } else { + ref, err = decomposedTag.normalizedReference() if err != nil { return "", err } - tag = ref.String() } + tag = ref.String() // If the input does not have a tag, we need to add one (latest) if !decomposedTag.isTagged { tag = fmt.Sprintf("%s:%s", tag, decomposedTag.tag) diff --git a/libpod/image/image_test.go b/libpod/image/image_test.go index 2a68fd273b..a85cd5386d 100644 --- a/libpod/image/image_test.go +++ b/libpod/image/image_test.go @@ -268,6 +268,7 @@ func TestNormalizeTag(t *testing.T) { {"example.com/busybox:notlatest" + digestSuffix, "example.com/busybox:notlatest" + digestSuffix}, // Qualified name:tag@digest {"busybox:latest", "localhost/busybox:latest"}, // Unqualified name-only {"ns/busybox:latest", "localhost/ns/busybox:latest"}, // Unqualified with a dot-less namespace + {"docker.io/busybox:latest", "docker.io/library/busybox:latest"}, // docker.io without /library/ } { res, err := normalizeTag(c.input) if c.expected == "" { From b9c0f2c987688a8e7ee2c8f4e4fbf6b1a8fee024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:38:49 +0100 Subject: [PATCH 18/25] Use reference.TagNameOnly instead of manually adding imageParts.tag in normalizeTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Again, rely on the official API, instead of the suprising "suspiciousTagValueForSearch" value (set to :latest on untagged images, and :none on digested ones!) CHANGES BEHAVIOR, but the previous output of normalization of digested values was not even syntatically valid, so this can't really be worse. Still, maybe we should refuse to tag with digested references in the first place. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 7 ++----- libpod/image/image_test.go | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 2457a1c229..0b743e1441 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -471,12 +471,9 @@ func normalizeTag(tag string) (string, error) { return "", err } } - tag = ref.String() // If the input does not have a tag, we need to add one (latest) - if !decomposedTag.isTagged { - tag = fmt.Sprintf("%s:%s", tag, decomposedTag.tag) - } - return tag, nil + ref = reference.TagNameOnly(ref) + return ref.String(), nil } // TagImage adds a tag to the given image diff --git a/libpod/image/image_test.go b/libpod/image/image_test.go index a85cd5386d..96148c2cd5 100644 --- a/libpod/image/image_test.go +++ b/libpod/image/image_test.go @@ -264,7 +264,7 @@ func TestNormalizeTag(t *testing.T) { {"#", ""}, // Clearly invalid {"example.com/busybox", "example.com/busybox:latest"}, // Qualified name-only {"example.com/busybox:notlatest", "example.com/busybox:notlatest"}, // Qualified name:tag - {"example.com/busybox" + digestSuffix, "example.com/busybox" + digestSuffix + ":none"}, // Qualified name@digest; FIXME: The result is not even syntactically valid! + {"example.com/busybox" + digestSuffix, "example.com/busybox" + digestSuffix}, // Qualified name@digest; FIXME? Should we allow tagging with a digest at all? {"example.com/busybox:notlatest" + digestSuffix, "example.com/busybox:notlatest" + digestSuffix}, // Qualified name:tag@digest {"busybox:latest", "localhost/busybox:latest"}, // Unqualified name-only {"ns/busybox:latest", "localhost/ns/busybox:latest"}, // Unqualified with a dot-less namespace From f92c3ce35033a558a9c86c824a829368a472f873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:42:37 +0100 Subject: [PATCH 19/25] Return a reference.Named from normalizedTag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of returning a string, return a native value and convert it into the string in the caller, to make it that small bit more common to use reference types. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 18 +++++++++--------- libpod/image/image_test.go | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 0b743e1441..7442f19454 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -452,42 +452,42 @@ func getImageDigest(ctx context.Context, src types.ImageReference, sc *types.Sys return "@" + digest.Hex(), nil } -// normalizeTag returns the canonical version of tag for use in Image.Names() -func normalizeTag(tag string) (string, error) { +// normalizedTag returns the canonical version of tag for use in Image.Names() +func normalizedTag(tag string) (reference.Named, error) { decomposedTag, err := decompose(tag) if err != nil { - return "", err + return nil, err } // If the input doesn't specify a registry, set the registry to localhost var ref reference.Named if !decomposedTag.hasRegistry { ref, err = decomposedTag.referenceWithRegistry(DefaultLocalRegistry) if err != nil { - return "", err + return nil, err } } else { ref, err = decomposedTag.normalizedReference() if err != nil { - return "", err + return nil, err } } // If the input does not have a tag, we need to add one (latest) ref = reference.TagNameOnly(ref) - return ref.String(), nil + return ref, nil } // TagImage adds a tag to the given image func (i *Image) TagImage(tag string) error { i.reloadImage() - tag, err := normalizeTag(tag) + ref, err := normalizedTag(tag) if err != nil { return err } tags := i.Names() - if util.StringInSlice(tag, tags) { + if util.StringInSlice(ref.String(), tags) { return nil } - tags = append(tags, tag) + tags = append(tags, ref.String()) if err := i.imageruntime.store.SetNames(i.ID(), tags); err != nil { return err } diff --git a/libpod/image/image_test.go b/libpod/image/image_test.go index 96148c2cd5..077ae460ea 100644 --- a/libpod/image/image_test.go +++ b/libpod/image/image_test.go @@ -257,7 +257,7 @@ func Test_stripSha256(t *testing.T) { assert.Equal(t, stripSha256("sha256:a"), "a") } -func TestNormalizeTag(t *testing.T) { +func TestNormalizedTag(t *testing.T) { const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" for _, c := range []struct{ input, expected string }{ @@ -270,12 +270,12 @@ func TestNormalizeTag(t *testing.T) { {"ns/busybox:latest", "localhost/ns/busybox:latest"}, // Unqualified with a dot-less namespace {"docker.io/busybox:latest", "docker.io/library/busybox:latest"}, // docker.io without /library/ } { - res, err := normalizeTag(c.input) + res, err := normalizedTag(c.input) if c.expected == "" { assert.Error(t, err, c.input) } else { assert.NoError(t, err, c.input) - assert.Equal(t, c.expected, res) + assert.Equal(t, c.expected, res.String()) } } } From d559365d7aea4f9957e3efb869480aaeb33b9ec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:44:05 +0100 Subject: [PATCH 20/25] Don't try to look up local images with an explicit :latest suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit imageruntime.getImage, through ParseStoreReference, already uses reference.TagNameOnly on the input, so this extra lookup is completely redundant to the lookup that has already happened. Should not change behavior, apart from speeding up the code a bit. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 7442f19454..899b8792fd 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -226,7 +226,6 @@ func (i *Image) getLocalImage() (*storage.Image, error) { i.InputName = dest.DockerReference().String() } - var taggedName string img, err := i.imageruntime.getImage(stripSha256(i.InputName)) if err == nil { return img.image, err @@ -240,15 +239,6 @@ func (i *Image) getLocalImage() (*storage.Image, error) { return nil, err } - // the inputname isn't tagged, so we assume latest and try again - if !decomposedImage.isTagged { - taggedName = fmt.Sprintf("%s:latest", i.InputName) - img, err = i.imageruntime.getImage(taggedName) - if err == nil { - return img.image, nil - } - } - // The image has a registry name in it and we made sure we looked for it locally // with a tag. It cannot be local. if decomposedImage.hasRegistry { From cf40b7161476414e7abecd19d9d9eefacd8a4ef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:50:40 +0100 Subject: [PATCH 21/25] Use imageParts.referenceWithRegistry in Image.getLocalImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to make sure everything uses the same code path. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 899b8792fd..381c28fac2 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -243,12 +243,14 @@ func (i *Image) getLocalImage() (*storage.Image, error) { // with a tag. It cannot be local. if decomposedImage.hasRegistry { return nil, errors.Wrapf(ErrNoSuchImage, imageError) - } - // if the image is saved with the repository localhost, searching with localhost prepended is necessary // We don't need to strip the sha because we have already determined it is not an ID - img, err = i.imageruntime.getImage(fmt.Sprintf("%s/%s", DefaultLocalRegistry, i.InputName)) + ref, err := decomposedImage.referenceWithRegistry(DefaultLocalRegistry) + if err != nil { + return nil, err + } + img, err = i.imageruntime.getImage(ref.String()) if err == nil { return img.image, err } From fa42f97507f5b5c661df2339603383b00293689f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 19:41:50 +0100 Subject: [PATCH 22/25] FIXME? Introduce imageParts.suspiciousRefNameTagValuesForSearch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Image.MatchRepoTag and findImageInRepoTags do some kind of heuristic search; the motivation and design of both, and how they should deal with digests, is not obvious to me. Instead of figuring that out now, just factor it out into a scary-named method and leave the "tag" value (with its "latest"/"none" value) alone. Similarly, the .registry and .name fields should typically not be used; users should use either hasRegistry or normalized reference types; so, isolate the difficult-to-understand search code, and computation of these values, into this new search-specific helper. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/image.go | 10 ++++++---- libpod/image/parts.go | 24 ++++++++++++++++++++++++ libpod/image/parts_test.go | 15 ++++++++------- libpod/image/utils.go | 10 ++++++---- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/libpod/image/image.go b/libpod/image/image.go index 381c28fac2..ea326d820f 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -930,21 +930,23 @@ func (i *Image) MatchRepoTag(input string) (string, error) { if err != nil { return "", err } + imageRegistry, imageName, imageSuspiciousTagValueForSearch := dcImage.suspiciousRefNameTagValuesForSearch() for _, repoName := range i.Names() { count := 0 dcRepoName, err := decompose(repoName) if err != nil { return "", err } - if dcRepoName.registry == dcImage.registry && dcImage.registry != "" { + repoNameRegistry, repoNameName, repoNameSuspiciousTagValueForSearch := dcRepoName.suspiciousRefNameTagValuesForSearch() + if repoNameRegistry == imageRegistry && imageRegistry != "" { count++ } - if dcRepoName.name == dcImage.name && dcImage.name != "" { + if repoNameName == imageName && imageName != "" { count++ - } else if splitString(dcRepoName.name) == splitString(dcImage.name) { + } else if splitString(repoNameName) == splitString(imageName) { count++ } - if dcRepoName.tag == dcImage.tag { + if repoNameSuspiciousTagValueForSearch == imageSuspiciousTagValueForSearch { count++ } results[count] = append(results[count], repoName) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index a4eb575196..ab6a867674 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -76,6 +76,30 @@ func decompose(input string) (imageParts, error) { }, nil } +// suspiciousRefNameTagValuesForSearch returns a "tag" value used in a previous implementation. +// This exists only to preserve existing behavior in heuristic code; it’s dubious that that behavior is correct, +// gespecially for the tag value. +func (ip *imageParts) suspiciousRefNameTagValuesForSearch() (string, string, string) { + registry := reference.Domain(ip.unnormalizedRef) + imageName := reference.Path(ip.unnormalizedRef) + // ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed, + // does not use the standard heuristics for domains vs. namespaces/repos. + if registry != "" && !isRegistry(registry) { + imageName = registry + "/" + imageName + registry = "" + } + + var tag string + if tagged, isTagged := ip.unnormalizedRef.(reference.NamedTagged); isTagged { + tag = tagged.Tag() + } else if _, hasDigest := ip.unnormalizedRef.(reference.Digested); hasDigest { + tag = "none" + } else { + tag = "latest" + } + return registry, imageName, tag +} + // referenceWithRegistry returns a (normalized) reference.Named composed of ip (with !ip.hasRegistry) // qualified with registry. func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, error) { diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 9955a20eae..dd4a7b27f3 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -11,9 +11,9 @@ func TestDecompose(t *testing.T) { const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" for _, c := range []struct { - input string - registry, name, tag string - isTagged, hasRegistry bool + input string + registry, name, suspiciousTagValueForSearch string + isTagged, hasRegistry bool }{ {"#", "", "", "", false, false}, // Entirely invalid input { // Fully qualified docker.io, name-only input @@ -32,7 +32,7 @@ func TestDecompose(t *testing.T) { "example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true, }, { // name@digest - // FIXME? .tag == "none" + // FIXME? .suspiciousTagValueForSearch == "none" "example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true, }, { // name:tag@digest @@ -44,9 +44,10 @@ func TestDecompose(t *testing.T) { assert.Error(t, err, c.input) } else { assert.NoError(t, err, c.input) - assert.Equal(t, c.registry, parts.registry, c.input) - assert.Equal(t, c.name, parts.name, c.input) - assert.Equal(t, c.tag, parts.tag, c.input) + registry, name, suspiciousTagValueForSearch := parts.suspiciousRefNameTagValuesForSearch() + assert.Equal(t, c.registry, registry, c.input) + assert.Equal(t, c.name, name, c.input) + assert.Equal(t, c.suspiciousTagValueForSearch, suspiciousTagValueForSearch, c.input) assert.Equal(t, c.isTagged, parts.isTagged, c.input) assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input) } diff --git a/libpod/image/utils.go b/libpod/image/utils.go index b944de1bb2..ad027f32ad 100644 --- a/libpod/image/utils.go +++ b/libpod/image/utils.go @@ -17,6 +17,7 @@ import ( // findImageInRepotags takes an imageParts struct and searches images' repotags for // a match on name:tag func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, error) { + _, searchName, searchSuspiciousTagValueForSearch := search.suspiciousRefNameTagValuesForSearch() var results []*storage.Image for _, image := range images { for _, name := range image.Names() { @@ -25,21 +26,22 @@ func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, er if err != nil { continue } - if d.name == search.name && d.tag == search.tag { + _, dName, dSuspiciousTagValueForSearch := d.suspiciousRefNameTagValuesForSearch() + if dName == searchName && dSuspiciousTagValueForSearch == searchSuspiciousTagValueForSearch { results = append(results, image.image) continue } // account for registry:/somedir/image - if strings.HasSuffix(d.name, search.name) && d.tag == search.tag { + if strings.HasSuffix(dName, searchName) && dSuspiciousTagValueForSearch == searchSuspiciousTagValueForSearch { results = append(results, image.image) continue } } } if len(results) == 0 { - return &storage.Image{}, errors.Errorf("unable to find a name and tag match for %s in repotags", search.name) + return &storage.Image{}, errors.Errorf("unable to find a name and tag match for %s in repotags", searchName) } else if len(results) > 1 { - return &storage.Image{}, errors.Errorf("found multiple name and tag matches for %s in repotags", search.name) + return &storage.Image{}, errors.Errorf("found multiple name and tag matches for %s in repotags", searchName) } return results[0], nil } From ad90c44f8d648ba17f07db7f05f28a67c16796d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 22:10:53 +0100 Subject: [PATCH 23/25] Use imageParts.unnormalizedRef in GetImageBaseName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to remove the last user of imageParts.name. Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index ab6a867674..b5b05c32df 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -30,7 +30,7 @@ func GetImageBaseName(input string) (string, error) { if err != nil { return "", err } - splitImageName := strings.Split(decomposedImage.name, "/") + splitImageName := strings.Split(decomposedImage.unnormalizedRef.Name(), "/") return splitImageName[len(splitImageName)-1], nil } From 797d194050d4b7b78d255d19f4099463dc048425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 22:18:25 +0100 Subject: [PATCH 24/25] Clarify comments about isRegistry a bit. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index b5b05c32df..62cca2757f 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -17,7 +17,8 @@ type imageParts struct { hasRegistry bool } -// Registries must contain a ":" or a "." or be localhost +// Registries must contain a ":" or a "." or be localhost; this helper exists for users of reference.Parse. +// For inputs that should use the docker.io[/library] normalization, use reference.ParseNormalizedNamed instead. func isRegistry(name string) bool { return strings.ContainsAny(name, ".:") || name == "localhost" } @@ -57,7 +58,9 @@ func decompose(input string) (imageParts, error) { } registry := reference.Domain(unnormalizedNamed) imageName := reference.Path(unnormalizedNamed) - // Is this a registry or a repo? + // ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed, + // does not use the standard heuristics for domains vs. namespaces/repos, so we need to check + // explicitly. if isRegistry(registry) { hasRegistry = true } else { From 449116af19305a567933effaced5a0a21a8c71be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 9 Jan 2019 21:47:50 +0100 Subject: [PATCH 25/25] Remove imageParts.{isTagged,registry,name,tag} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finally, these members no longer have any users. Future users should usually call referenceWithRegistry / normalizedReference, and work with the returned value, instead of reintroducing these variables. Similarly, direct uses of unnormalizedRef should be rare (only for cases where the registry and/or path truly does not matter). Should not change behavior. Signed-off-by: Miloslav Trmač --- libpod/image/parts.go | 35 ++--------------------------------- libpod/image/parts_test.go | 19 +++++++++---------- 2 files changed, 11 insertions(+), 43 deletions(-) diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 62cca2757f..dfdf0b08a7 100644 --- a/libpod/image/parts.go +++ b/libpod/image/parts.go @@ -10,10 +10,6 @@ import ( // imageParts describes the parts of an image's name type imageParts struct { unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization - registry string - name string - tag string - isTagged bool hasRegistry bool } @@ -37,45 +33,18 @@ func GetImageBaseName(input string) (string, error) { // decompose breaks an input name into an imageParts description func decompose(input string) (imageParts, error) { - var ( - parts imageParts - hasRegistry bool - tag string - ) imgRef, err := reference.Parse(input) if err != nil { - return parts, err + return imageParts{}, err } unnormalizedNamed := imgRef.(reference.Named) - ntag, isTagged := imgRef.(reference.NamedTagged) - if !isTagged { - tag = "latest" - if _, hasDigest := imgRef.(reference.Digested); hasDigest { - tag = "none" - } - } else { - tag = ntag.Tag() - } - registry := reference.Domain(unnormalizedNamed) - imageName := reference.Path(unnormalizedNamed) // ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed, // does not use the standard heuristics for domains vs. namespaces/repos, so we need to check // explicitly. - if isRegistry(registry) { - hasRegistry = true - } else { - if registry != "" { - imageName = registry + "/" + imageName - registry = "" - } - } + hasRegistry := isRegistry(reference.Domain(unnormalizedNamed)) return imageParts{ unnormalizedRef: unnormalizedNamed, - registry: registry, hasRegistry: hasRegistry, - name: imageName, - tag: tag, - isTagged: isTagged, }, nil } diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index dd4a7b27f3..726e55e867 100644 --- a/libpod/image/parts_test.go +++ b/libpod/image/parts_test.go @@ -13,30 +13,30 @@ func TestDecompose(t *testing.T) { for _, c := range []struct { input string registry, name, suspiciousTagValueForSearch string - isTagged, hasRegistry bool + hasRegistry bool }{ - {"#", "", "", "", false, false}, // Entirely invalid input + {"#", "", "", "", false}, // Entirely invalid input { // Fully qualified docker.io, name-only input - "docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true, + "docker.io/library/busybox", "docker.io", "library/busybox", "latest", true, }, { // Fully qualified example.com, name-only input - "example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true, + "example.com/ns/busybox", "example.com", "ns/busybox", "latest", true, }, { // Unqualified single-name input - "busybox", "", "busybox", "latest", false, false, + "busybox", "", "busybox", "latest", false, }, { // Unqualified namespaced input - "ns/busybox", "", "ns/busybox", "latest", false, false, + "ns/busybox", "", "ns/busybox", "latest", false, }, { // name:tag - "example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true, + "example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, }, { // name@digest // FIXME? .suspiciousTagValueForSearch == "none" - "example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true, + "example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", true, }, { // name:tag@digest - "example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true, + "example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, }, } { parts, err := decompose(c.input) @@ -48,7 +48,6 @@ func TestDecompose(t *testing.T) { assert.Equal(t, c.registry, registry, c.input) assert.Equal(t, c.name, name, c.input) assert.Equal(t, c.suspiciousTagValueForSearch, suspiciousTagValueForSearch, c.input) - assert.Equal(t, c.isTagged, parts.isTagged, c.input) assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input) } }