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/parts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go index 1e01c85d3..518538f0b 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) -- 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/parts_test.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 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/parts_test.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 72777b7fee22e04edee08034927d5864ffc4bc3e Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 20:32:30 +0100 Subject: 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(+) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index b7c5d1252..8d059e35b 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 733e8e855..b2cc0f8c0 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) +} -- cgit v1.2.3-54-g00ecf From e5c764ec3cc11982cdb8e127554e4ac61ce9a854 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 21:11:32 +0100 Subject: 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(-) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 8d059e35b..566a3842c 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 b2cc0f8c0..3ca39b1dc 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) } } } -- cgit v1.2.3-54-g00ecf From 1c19d19c6ef9627413e34b30a643bef9b2970acb Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 21:33:01 +0100 Subject: 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(+) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 566a3842c..a4eb57519 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 3ca39b1dc..9955a20ea 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()) + } + } +} -- cgit v1.2.3-54-g00ecf From fa42f97507f5b5c661df2339603383b00293689f Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 19:41:50 +0100 Subject: 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(-) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/image.go b/libpod/image/image.go index 381c28fac..ea326d820 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 a4eb57519..ab6a86767 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 9955a20ea..dd4a7b27f 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 b944de1bb..ad027f32a 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 } -- cgit v1.2.3-54-g00ecf From 449116af19305a567933effaced5a0a21a8c71be Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 9 Jan 2019 21:47:50 +0100 Subject: 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(-) (limited to 'libpod/image/parts_test.go') diff --git a/libpod/image/parts.go b/libpod/image/parts.go index 62cca2757..dfdf0b08a 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 dd4a7b27f..726e55e86 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) } } -- cgit v1.2.3-54-g00ecf