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') 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