Merge pull request containers#1503 from vrothberg/fix-podman-18445

libimage: fix reference filters
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Author: openshift-merge-robot

libimage/filters.go

@@ -178,7 +178,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
 			filter = filterManifest(ctx, manifest)
 
 		case "reference":
-			filter = filterReferences(value)
+			filter = filterReferences(r, value)
 
 		case "until":
 			until, err := r.until(value)
@@ -268,8 +268,15 @@ func filterManifest(ctx context.Context, value bool) filterFunc {
 }
 
 // filterReferences creates a reference filter for matching the specified value.
-func filterReferences(value string) filterFunc {
+func filterReferences(r *Runtime, value string) filterFunc {
+	lookedUp, _, _ := r.LookupImage(value, nil)
 	return func(img *Image) (bool, error) {
+		if lookedUp != nil {
+			if lookedUp.ID() == img.ID() {
+				return true, nil
+			}
+		}
+
 		refs, err := img.NamesReferences()
 		if err != nil {
 			return false, err
@@ -306,6 +313,7 @@ func filterReferences(value string) filterFunc {
 				}
 			}
 		}
+
 		return false, nil
 	}
 }

libimage/filters_test.go

@@ -62,6 +62,8 @@ func TestFilterReference(t *testing.T) {
 		{"quay.io/libpod/*", 2},
 		{"busybox", 1},
 		{"alpine", 1},
+		{"alpine@" + alpine.Digest().String(), 0},
+		{"quay.io/libpod/alpine@" + alpine.Digest().String(), 1},
 	} {
 		listOptions := &ListImagesOptions{
 			Filters: []string{"reference=" + test.filter},

libimage/image_test.go

@@ -58,15 +58,11 @@ func TestImageFunctions(t *testing.T) {
 	// Just make sure that the ID has 64 characters.
 	require.True(t, len(image.ID()) == 64, "ID should be 64 characters long")
 
-	// Make sure that the image we pulled by digest is the same one we
-	// pulled by tag.
-	require.Equal(t, origDigest.String(), image.Digest().String(), "digests of pulled images should match")
-
 	// NOTE: we're recording two digests. One for the image and one for the
 	// manifest list we chose it from.
 	digests := image.Digests()
 	require.Len(t, digests, 2)
-	require.Equal(t, origDigest.String(), digests[0].String(), "first recoreded digest should be the one of the image")
+	require.Equal(t, origDigest.String(), digests[1].String(), "second recoreded digest should be the one of the image")
 
 	// containers/podman/issues/12729: make sure manifest lookup returns
 	// the correct error for both digests.

libimage/normalize.go

@@ -100,22 +100,22 @@ func ToNameTagPairs(repoTags []reference.Named) ([]NameTagPair, error) {
 // normalizeTaggedDigestedString strips the tag off the specified string iff it
 // is tagged and digested. Note that the tag is entirely ignored to match
 // Docker behavior.
-func normalizeTaggedDigestedString(s string) (string, error) {
+func normalizeTaggedDigestedString(s string) (string, reference.Named, error) {
 	// Note that the input string is not expected to be parseable, so we
 	// return it verbatim in error cases.
 	ref, err := reference.Parse(s)
 	if err != nil {
-		return "", err
+		return "", nil, err
 	}
 	named, ok := ref.(reference.Named)
 	if !ok {
-		return s, nil
+		return s, nil, nil
 	}
 	named, err = normalizeTaggedDigestedNamed(named)
 	if err != nil {
-		return "", err
+		return "", nil, err
 	}
-	return named.String(), nil
+	return named.String(), named, nil
 }
 
 // normalizeTaggedDigestedNamed strips the tag off the specified named

libimage/normalize_test.go

@@ -132,7 +132,7 @@ func TestNormalizeTaggedDigestedString(t *testing.T) {
 		{"localhost/fedora:anothertag" + digestSuffix, "localhost/fedora" + digestSuffix},
 		{"localhost:5000/fedora:v1.2.3.4.5" + digestSuffix, "localhost:5000/fedora" + digestSuffix},
 	} {
-		res, err := normalizeTaggedDigestedString(test.input)
+		res, _, err := normalizeTaggedDigestedString(test.input)
 		if test.expected == "" {
 			assert.Error(t, err, "%v", test)
 		} else {

libimage/pull.go

@@ -86,7 +86,7 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP
 		// Docker compat: strip off the tag iff name is tagged and digested
 		// (e.g., fedora:latest@sha256...).  In that case, the tag is stripped
 		// off and entirely ignored.  The digest is the sole source of truth.
-		normalizedName, normalizeError := normalizeTaggedDigestedString(name)
+		normalizedName, _, normalizeError := normalizeTaggedDigestedString(name)
 		if normalizeError != nil {
 			return nil, normalizeError
 		}

libimage/runtime.go

@@ -239,7 +239,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 	// Docker compat: strip off the tag iff name is tagged and digested
 	// (e.g., fedora:latest@sha256...).  In that case, the tag is stripped
 	// off and entirely ignored.  The digest is the sole source of truth.
-	normalizedName, err := normalizeTaggedDigestedString(name)
+	normalizedName, namedName, err := normalizeTaggedDigestedString(name)
 	if err != nil {
 		return nil, "", err
 	}
@@ -316,21 +316,31 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 		return img, name, err
 	}
 
-	return r.lookupImageInDigestsAndRepoTags(name, options)
+	return r.lookupImageInRepoTags(name, namedName, options)
 }
 
 // lookupImageInLocalStorage looks up the specified candidate for name in the
 // storage and checks whether it's matching the system context.
 func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *LookupImageOptions) (*Image, error) {
 	logrus.Debugf("Trying %q ...", candidate)
-	img, err := r.store.Image(candidate)
+
+	// First parse the reference and then look up the image to properly
+	// support digests.
+	ref, err := storageTransport.Transport.ParseStoreReference(r.store, candidate)
+	if err != nil {
+		return nil, nil
+	}
+	img, err := storageTransport.Transport.GetStoreImage(r.store, ref)
 	if err != nil && !errors.Is(err, storage.ErrImageUnknown) {
 		return nil, err
 	}
 	if img == nil {
 		return nil, nil
 	}
-	ref, err := storageTransport.Transport.ParseStoreReference(r.store, img.ID)
+	// The image may have been looked up by digest which may have referred
+	// to the parent manifest list.  Hence, reparse the reference with the
+	// ID to make sure we resolve the correct object.
+	ref, err = storageTransport.Transport.ParseStoreReference(r.store, img.ID)
 	if err != nil {
 		return nil, err
 	}
@@ -414,61 +424,34 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo
 	return image, nil
 }
 
-// lookupImageInDigestsAndRepoTags attempts to match name against any image in
-// the local containers storage.  If name is digested, it will be compared
-// against image digests.  Otherwise, it will be looked up in the repo tags.
-func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, options *LookupImageOptions) (*Image, string, error) {
-	// Until now, we've tried very hard to find an image but now it is time
-	// for limbo.  If the image includes a digest that we couldn't detect
-	// verbatim in the storage, we must have a look at all digests of all
-	// images.  Those may change over time (e.g., via manifest lists).
-	// Both Podman and Buildah want us to do that dance.
-	allImages, err := r.ListImages(context.Background(), nil, nil)
-	if err != nil {
-		return nil, "", err
-	}
-
-	ref, err := reference.Parse(name) // Warning! This is not ParseNormalizedNamed
-	if err != nil {
-		return nil, "", err
-	}
-	named, isNamed := ref.(reference.Named)
-	if !isNamed {
+// lookupImageInRepoTags attempts to match named against any image in the local
+// containers storage by comparing the name to all known repo tags.
+func (r *Runtime) lookupImageInRepoTags(name string, namedName reference.Named, options *LookupImageOptions) (*Image, string, error) {
+	if namedName == nil {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
-	digested, isDigested := named.(reference.Digested)
-	if isDigested {
-		logrus.Debug("Looking for image with matching recorded digests")
-		digest := digested.Digest()
-		for _, image := range allImages {
-			for _, d := range image.Digests() {
-				if d != digest {
-					continue
-				}
-				// Also make sure that the matching image fits all criteria (e.g., manifest list).
-				if _, err := r.lookupImageInLocalStorage(name, image.ID(), options); err != nil {
-					return nil, "", err
-				}
-				return image, name, nil
-
-			}
-		}
+	if _, isDigested := namedName.(reference.Digested); isDigested {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
 	if !shortnames.IsShortName(name) {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
-	named = reference.TagNameOnly(named) // Make sure to add ":latest" if needed
-	namedTagged, isNammedTagged := named.(reference.NamedTagged)
+	namedName = reference.TagNameOnly(namedName) // Docker compat: make sure to add ":latest" if needed
+	namedTagged, isNammedTagged := namedName.(reference.NamedTagged)
 	if !isNammedTagged {
 		// NOTE: this should never happen since we already know it's
 		// not a digested reference.
 		return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", name, storage.ErrImageUnknown)
 	}
 
+	allImages, err := r.ListImages(context.Background(), nil, nil)
+	if err != nil {
+		return nil, "", err
+	}
+
 	for _, image := range allImages {
 		named, err := image.inRepoTags(namedTagged)
 		if err != nil {