From 6486e2c41be0aa50bd5b70d859a23f36070d0ae1 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Mon, 14 Jan 2019 03:58:35 +0100 Subject: 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/pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 203e94310..09935fe7c 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) -- cgit v1.2.3-54-g00ecf From 3d98c42a3f7822bf608834505adcffec7f75bf28 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 18:47:09 +0100 Subject: 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(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 289a02e1f..cf774030b 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 518538f0b..3ff175f0e 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 09935fe7c..82fe0fdd6 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) } -- cgit v1.2.3-54-g00ecf From 99d2259f8a71dbe5a0d9e78757b51551ce0eb8b1 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 18:54:17 +0100 Subject: 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(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 82fe0fdd6..d09ddd599 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) } -- cgit v1.2.3-54-g00ecf From e9721b757ae5da4dad4f68c5eb721f348cfe935a Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 19:00:59 +0100 Subject: 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(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index cf774030b..b7c5d1252 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 3ff175f0e..733e8e855 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 d09ddd599..342ee6f17 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) } -- cgit v1.2.3-54-g00ecf From 788bc360219e83028b3b0996ab4b6afbd0530e9c Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 19:12:17 +0100 Subject: 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(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 342ee6f17..d4be53ef8 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) } -- cgit v1.2.3-54-g00ecf 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.go | 17 +++-------------- libpod/image/pull_test.go | 3 +-- 2 files changed, 4 insertions(+), 16 deletions(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index d4be53ef8..ee5e333e3 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 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.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.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 From 633501b1b7450676c47d891092b53680206b8398 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 21:01:56 +0100 Subject: 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(-) (limited to 'libpod/image/pull.go') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index af6b84c34..434b83520 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) } -- cgit v1.2.3-54-g00ecf