Use canonical image name when comparing images

The implementation of `ImageID.Repository()` was changed recently so
that it returned a canonical image _path_.

Prior to that change, it would return a minimal name for the image,
i.e., by omitting the parts implied by convention. This provided a
canonical name, in the sense of it being unambiguous provided the
conventions didn't change. It also included the registry domain e.g,
quay.io, if it was not DockerHub.

After the change, `ImageID.Repository()` gives a canonical image path
by adding in the implied parts (i.e., the prefix `library/`, for
DockerHub images) where missing, but does not ever include the
registry name.

While we were storing image metadata against the canonical
_name_ (registry and path), we were now querying using only the _path_
-- and for images not from DockerHub, nothing would be found.

The solution is to use the canonical name for storing and querying,
and the path only for the registry operations that require exactly
that.

cluster/kubernetes/update.go

@@ -99,7 +99,7 @@ func tryUpdate(def []byte, container string, newImage flux.ImageID, out io.Write
 		if err != nil {
 			return fmt.Errorf("could not parse image %s", c.Image)
 		}
-		if currentImage.Repository() == newImage.Repository() {
+		if currentImage.CanonicalName() == newImage.CanonicalName() {
 			matchingContainers[i] = c
 		}
 		_, _, oldImageTag := currentImage.Components()

daemon/daemon.go

@@ -431,7 +431,7 @@ func containers2containers(cs []cluster.Container) []flux.Container {
 func containersWithAvailable(service cluster.Controller, images update.ImageMap) (res []flux.Container) {
 	for _, c := range service.ContainersOrNil() {
 		id, _ := flux.ParseImageID(c.Image)
-		repo := id.Repository()
+		repo := id.CanonicalName()
 		available := images[repo]
 		res = append(res, flux.Container{
 			Name: c.Name,

daemon/images.go

@@ -52,7 +52,7 @@ func (d *Daemon) pollForNewImages(logger log.Logger) {
 			}
 
 			pattern := getTagPattern(candidateServices, service.ID, container.Name)
-			repo := currentImageID.Repository()
+			repo := currentImageID.CanonicalName()
 			logger.Log("repo", repo, "pattern", pattern)
 
 			if latest := imageMap.LatestImage(repo, pattern); latest != nil && latest.ID != currentImageID {

image_test.go

@@ -32,24 +32,25 @@ func TestImageID_ParseImageID(t *testing.T) {
 	for _, x := range []struct {
 		test     string
 		registry string
+		repo     string
 		canon    string
 	}{
 		// Library images can have the domain omitted; a
 		// single-element path is understood to be prefixed with "library".
-		{"alpine", dockerHubHost, "index.docker.io/library/alpine"},
-		{"library/alpine", dockerHubHost, "index.docker.io/library/alpine"},
-		{"alpine:mytag", dockerHubHost, "index.docker.io/library/alpine:mytag"},
+		{"alpine", dockerHubHost, "library/alpine", "index.docker.io/library/alpine"},
+		{"library/alpine", dockerHubHost, "library/alpine", "index.docker.io/library/alpine"},
+		{"alpine:mytag", dockerHubHost, "library/alpine", "index.docker.io/library/alpine:mytag"},
 		// The old registry path should be replaced with the new one
-		{"docker.io/library/alpine", dockerHubHost, "index.docker.io/library/alpine"},
+		{"docker.io/library/alpine", dockerHubHost, "library/alpine", "index.docker.io/library/alpine"},
 		// It's possible to have a domain with a single-element path
-		{"localhost/hello:v1.1", "localhost", "localhost/hello:v1.1"},
-		{"localhost:5000/hello:v1.1", "localhost:5000", "localhost:5000/hello:v1.1"},
-		{"example.com/hello:v1.1", "example.com", "example.com/hello:v1.1"},
+		{"localhost/hello:v1.1", "localhost", "hello", "localhost/hello:v1.1"},
+		{"localhost:5000/hello:v1.1", "localhost:5000", "hello", "localhost:5000/hello:v1.1"},
+		{"example.com/hello:v1.1", "example.com", "hello", "example.com/hello:v1.1"},
 		// The path can have an arbitrary number of elements
-		{"quay.io/library/alpine", "quay.io", "quay.io/library/alpine"},
-		{"quay.io/library/alpine:latest", "quay.io", "quay.io/library/alpine:latest"},
-		{"quay.io/library/alpine:mytag", "quay.io", "quay.io/library/alpine:mytag"},
-		{"localhost:5000/path/to/repo/alpine:mytag", "localhost:5000", "localhost:5000/path/to/repo/alpine:mytag"},
+		{"quay.io/library/alpine", "quay.io", "library/alpine", "quay.io/library/alpine"},
+		{"quay.io/library/alpine:latest", "quay.io", "library/alpine", "quay.io/library/alpine:latest"},
+		{"quay.io/library/alpine:mytag", "quay.io", "library/alpine", "quay.io/library/alpine:mytag"},
+		{"localhost:5000/path/to/repo/alpine:mytag", "localhost:5000", "path/to/repo/alpine", "localhost:5000/path/to/repo/alpine:mytag"},
 	} {
 		i, err := ParseImageID(x.test)
 		if err != nil {
@@ -61,6 +62,9 @@ func TestImageID_ParseImageID(t *testing.T) {
 		if i.Registry() != x.registry {
 			t.Errorf("%q registry: expected %q, got %q", x.test, x.registry, i.Registry())
 		}
+		if i.Repository() != x.repo {
+			t.Errorf("%q repo: expected %q, got %q", x.test, x.repo, i.Repository())
+		}
 		if i.CanonicalRef() != x.canon {
 			t.Errorf("%q full ID: expected %q, got %q", x.test, x.canon, i.CanonicalRef())
 		}

registry/warming.go

@@ -101,8 +101,8 @@ func (w *Warmer) warm(id flux.ImageID, creds Credentials) {
 	for _, tag := range tags {
 		// See if we have the manifest already cached
 		// We don't want to re-download a manifest again.
-		i := id.WithNewTag(tag)
-		key, err := cache.NewManifestKey(username, i)
+		newID := id.WithNewTag(tag)
+		key, err := cache.NewManifestKey(username, newID)
 		if err != nil {
 			w.Logger.Log("err", errors.Wrap(err, "creating key for memcache"))
 			continue
@@ -117,7 +117,7 @@ func (w *Warmer) warm(id flux.ImageID, creds Credentials) {
 			// If we're within the expiry buffer, we need to update quick!
 			expired = true
 		}
-		toUpdate = append(toUpdate, i)
+		toUpdate = append(toUpdate, newID)
 	}
 
 	if len(toUpdate) == 0 {

update/images.go

@@ -57,7 +57,7 @@ func CollectAvailableImages(reg registry.Registry, services []cluster.Controller
 				// container is running an invalid image id? what?
 				return nil, err
 			}
-			images[id.Repository()] = nil
+			images[id.CanonicalName()] = nil
 		}
 	}
 	for repo := range images {
@@ -90,7 +90,7 @@ func exactImages(reg registry.Registry, images []flux.ImageID) (ImageMap, error)
 		if !exist {
 			return m, errors.Wrap(flux.ErrInvalidImageID, fmt.Sprintf("image %q does not exist", id))
 		}
-		m[id.Repository()] = []flux.Image{{ID: id}}
+		m[id.CanonicalName()] = []flux.Image{{ID: id}}
 	}
 	return m, nil
 }

update/release.go

@@ -201,7 +201,7 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont
 		var image flux.ImageID
 		image, err = s.ImageSpec.AsID()
 		if err == nil {
-			repo = image.Repository()
+			repo = image.CanonicalName()
 			images, err = exactImages(rc.Registry(), []flux.ImageID{image})
 		}
 	}
@@ -237,9 +237,9 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont
 				return nil, err
 			}
 
-			latestImage := images.LatestImage(currentImageID.Repository(), "*")
+			latestImage := images.LatestImage(currentImageID.CanonicalName(), "*")
 			if latestImage == nil {
-				if currentImageID.Repository() != repo {
+				if currentImageID.CanonicalName() != repo {
 					ignoredOrSkipped = ReleaseStatusIgnored
 				} else {
 					ignoredOrSkipped = ReleaseStatusUnknown