From ae2a95196e9e5a3519b396105d01a3661de330ba Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 20:02:54 +0100 Subject: 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_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'libpod/image/pull_test.go') diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index 37b45dc83..4a0119070 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. -- cgit v1.2.3-54-g00ecf From 2171a393904051ab724fa08d01a964adbf7c1880 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 20:43:13 +0100 Subject: 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(-) (limited to 'libpod/image/pull_test.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index ee5e333e3..70f89ea7d 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 4a0119070..2176678d3 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, }, -- cgit v1.2.3-54-g00ecf From 81204487dbf9903f4a8784be9a580f8a46d1d381 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 20:55:53 +0100 Subject: 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(-) (limited to 'libpod/image/pull_test.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 70f89ea7d..af6b84c34 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 2176678d3..3890c5e6c 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, }, -- cgit v1.2.3-54-g00ecf