FIXME? Introduce imageParts.suspiciousRefNameTagValuesForSearch

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č <mitr@redhat.com>

4 files changed, 44 insertions, 15 deletions

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
 }