Merge pull request #2413 from Honny1/list-images

Fix unexpected results when filtering images

Author: openshift-merge-bot[bot]

libimage/filters.go

@@ -25,6 +25,7 @@ type compiledFilters map[string][]filterFunc
 
 // Apply the specified filters.  All filters of each key must apply.
 // tree must be provided if compileImageFilters indicated it is necessary.
+// WARNING: Application of filterReferences sets the image names to matched names, but this only affects the values in memory, they are not written to storage.
 func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree *layerTree) (bool, error) {
 	for key := range filters {
 		for _, filter := range filters[key] {
@@ -51,6 +52,7 @@ func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree
 // filterImages returns a slice of images which are passing all specified
 // filters.
 // tree must be provided if compileImageFilters indicated it is necessary.
+// WARNING: Application of filterReferences sets the image names to matched names, but this only affects the values in memory, they are not written to storage.
 func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters compiledFilters, tree *layerTree) ([]*Image, error) {
 	result := []*Image{}
 	for i := range images {
@@ -272,39 +274,87 @@ func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatch
 			return true, nil
 		}
 
-		unwantedMatched := false
 		// Go through the unwanted matches first
 		for _, value := range unwantedReferenceMatches {
 			matches, err := imageMatchesReferenceFilter(r, img, value)
 			if err != nil {
 				return false, err
 			}
 			if matches {
-				unwantedMatched = true
+				return false, nil
 			}
 		}
 
 		// If there are no wanted match filters, then return false for the image
 		// that matched the unwanted value otherwise return true
 		if len(wantedReferenceMatches) == 0 {
-			return !unwantedMatched, nil
+			return true, nil
 		}
 
-		// Go through the wanted matches
-		// If an image matches the wanted filter but it also matches the unwanted
-		// filter, don't add it to the output
+		matchedReference := ""
 		for _, value := range wantedReferenceMatches {
 			matches, err := imageMatchesReferenceFilter(r, img, value)
 			if err != nil {
 				return false, err
 			}
-			if matches && !unwantedMatched {
+			if matches {
+				matchedReference = value
+				break
+			}
+		}
+
+		if matchedReference == "" {
+			return false, nil
+		}
+
+		// If there is exactly one wanted reference match and no unwanted matches,
+		// the filter is treated as a query, so it sets the matching names to
+		// the image in memory.
+		if len(wantedReferenceMatches) == 1 && len(unwantedReferenceMatches) == 0 {
+			ref, ok := isFullyQualifiedReference(matchedReference)
+			if !ok {
 				return true, nil
 			}
+			namesThatMatch := []string{}
+			for _, name := range img.Names() {
+				if nameMatchesReference(name, ref) {
+					namesThatMatch = append(namesThatMatch, name)
+				}
+			}
+			img.setEphemeralNames(namesThatMatch)
 		}
+		return true, nil
+	}
+}
+
+// isFullyQualifiedReference checks if the provided string is a fully qualified
+// reference (i.e., it contains a domain, path, and tag or digest).
+// It returns a reference.Named and a boolean indicating whether the
+// reference is fully qualified. If the reference is not fully qualified,
+// it returns nil and false.
+func isFullyQualifiedReference(r string) (reference.Named, bool) {
+	ref, err := reference.ParseNamed(r)
+	// If there is an error parsing the reference, it is not a valid reference
+	if err != nil {
+		return nil, false
+	}
+	// If it's name-only (no tag/digest), it's not fully qualified
+	if reference.IsNameOnly(ref) {
+		return nil, false
+	}
+	return ref, true
+}
 
-		return false, nil
+func nameMatchesReference(name string, ref reference.Named) bool {
+	_, containsDigest := ref.(reference.Digested)
+	if containsDigest {
+		nameRef, err := reference.ParseNamed(name)
+		if err != nil {
+			return false
+		}
+		return nameRef.Name() == ref.Name()
 	}
+	return name == ref.String()
 }
 
 // imageMatchesReferenceFilter returns true if an image matches the filter value given
@@ -352,7 +402,6 @@ func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, er
 			}
 		}
 	}
-
 	return false, nil
 }
 

libimage/filters_test.go

@@ -40,51 +40,82 @@ func TestFilterReference(t *testing.T) {
 	require.NoError(t, err)
 	err = alpine.Tag("docker.io/library/image:another-tag")
 	require.NoError(t, err)
+	err = alpine.Tag("docker.io/library/image:latest")
+	require.NoError(t, err)
+	err = busybox.Tag("localhost/aa:tag")
+	require.NoError(t, err)
+	err = busybox.Tag("localhost/a:tag")
+	require.NoError(t, err)
+
+	allAlpineNames := []string{
+		"docker.io/library/image:another-tag",
+		"quay.io/libpod/alpine:latest",
+		"localhost/another-image:tag",
+		"docker.io/library/image:latest",
+	}
+	allBusyBoxNames := []string{
+		"quay.io/libpod/busybox:latest",
+		"localhost/image:tag",
+		"localhost/a:tag",
+		"localhost/aa:tag",
+	}
+	allNames := []string{}
+	allNames = append(allNames, allBusyBoxNames...)
+	allNames = append(allNames, allAlpineNames...)
 
 	for _, test := range []struct {
-		filters []string
-		matches int
+		filters       []string
+		matchedImages int
+		names         []string
 	}{
-		{[]string{"image"}, 2},
-		{[]string{"*mage*"}, 2},
-		{[]string{"image:*"}, 2},
-		{[]string{"image:tag"}, 1},
-		{[]string{"image:another-tag"}, 1},
-		{[]string{"localhost/image"}, 1},
-		{[]string{"localhost/image:tag"}, 1},
-		{[]string{"library/image"}, 1},
-		{[]string{"docker.io/library/image*"}, 1},
-		{[]string{"docker.io/library/image:*"}, 1},
-		{[]string{"docker.io/library/image:another-tag"}, 1},
-		{[]string{"localhost/*"}, 2},
-		{[]string{"localhost/image:*tag"}, 1},
-		{[]string{"localhost/*mage:*ag"}, 2},
-		{[]string{"quay.io/libpod/busybox"}, 1},
-		{[]string{"quay.io/libpod/alpine"}, 1},
-		{[]string{"quay.io/libpod"}, 0},
-		{[]string{"quay.io/libpod/*"}, 2},
-		{[]string{"busybox"}, 1},
-		{[]string{"alpine"}, 1},
-		{[]string{"alpine@" + alpine.Digest().String()}, 1},
-		{[]string{"alpine:latest@" + alpine.Digest().String()}, 1},
-		{[]string{"quay.io/libpod/alpine@" + alpine.Digest().String()}, 1},
-		{[]string{"quay.io/libpod/alpine:latest@" + alpine.Digest().String()}, 1},
+		{[]string{"image"}, 2, allNames},
+		{[]string{"*mage*"}, 2, allNames},
+		{[]string{"image:*"}, 2, allNames},
+		{[]string{"image:tag"}, 1, allBusyBoxNames},
+		{[]string{"image:another-tag"}, 1, allAlpineNames},
+		{[]string{"localhost/image"}, 1, allBusyBoxNames},
+		{[]string{"localhost/image:tag"}, 1, []string{"localhost/image:tag"}},
+		{[]string{"library/image"}, 1, allAlpineNames},
+		{[]string{"docker.io/library/image*"}, 1, allAlpineNames},
+		{[]string{"docker.io/library/image:*"}, 1, allAlpineNames},
+		{[]string{"docker.io/library/image:another-tag"}, 1, []string{"docker.io/library/image:another-tag"}},
+		{[]string{"localhost/*"}, 2, allNames},
+		{[]string{"localhost/image:*tag"}, 1, allBusyBoxNames},
+		{[]string{"localhost/*mage:*ag"}, 2, allNames},
+		{[]string{"quay.io/libpod/busybox"}, 1, allBusyBoxNames},
+		{[]string{"quay.io/libpod/alpine"}, 1, allAlpineNames},
+		{[]string{"quay.io/libpod"}, 0, []string{}},
+		{[]string{"quay.io/libpod/*"}, 2, allNames},
+		{[]string{"busybox"}, 1, allBusyBoxNames},
+		{[]string{"alpine"}, 1, allAlpineNames},
+
 		// Make sure negate works as expected
-		{[]string{"!alpine"}, 1},
-		{[]string{"!alpine", "!busybox"}, 0},
-		{[]string{"!alpine", "busybox"}, 1},
-		{[]string{"alpine", "busybox"}, 2},
-		{[]string{"*test", "!*box"}, 1},
+		{[]string{"!alpine"}, 1, allBusyBoxNames},
+		{[]string{"!alpine", "!busybox"}, 0, []string{}},
+		{[]string{"!alpine", "busybox"}, 1, allBusyBoxNames},
+		{[]string{"alpine", "busybox"}, 2, allNames},
+		{[]string{"*test", "!*box"}, 1, allAlpineNames},
+
+		{[]string{"quay.io/libpod/alpine@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
+
+		{[]string{"alpine@" + alpine.Digest().String()}, 1, allAlpineNames},
+		{[]string{"alpine:latest@" + alpine.Digest().String()}, 1, allAlpineNames},
+		{[]string{"quay.io/libpod/alpine:latest@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
+		{[]string{"docker.io/library/image@" + alpine.Digest().String()}, 1, []string{"docker.io/library/image:latest", "docker.io/library/image:another-tag"}},
+		{[]string{"localhost/aa@" + busybox.Digest().String()}, 1, []string{"localhost/aa:tag"}},
+		{[]string{"localhost/a@" + busybox.Digest().String()}, 1, []string{"localhost/a:tag"}},
+
 		// Make sure that tags are ignored
-		{[]string{"alpine:ignoreme@" + alpine.Digest().String()}, 1},
-		{[]string{"alpine:123@" + alpine.Digest().String()}, 1},
-		{[]string{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String()}, 1},
-		{[]string{"quay.io/libpod/alpine:456@" + alpine.Digest().String()}, 1},
+		{[]string{"alpine:ignoreme@" + alpine.Digest().String()}, 1, allAlpineNames},
+		{[]string{"alpine:123@" + alpine.Digest().String()}, 1, allAlpineNames},
+		{[]string{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
+		{[]string{"quay.io/libpod/alpine:456@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
+
 		// Make sure that repo and digest must match
-		{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
-		{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
-		{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
-		{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
+		{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
+		{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
+		{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
+		{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
 	} {
 		var filters []string
 		for _, filter := range test.filters {
@@ -94,12 +125,20 @@ func TestFilterReference(t *testing.T) {
 				filters = append(filters, "reference="+filter)
 			}
 		}
+
 		listOptions := &ListImagesOptions{
 			Filters: filters,
 		}
 		listedImages, err := runtime.ListImages(ctx, listOptions)
+
 		require.NoError(t, err, "%v", test)
-		require.Len(t, listedImages, test.matches, "%s -> %v", test.filters, listedImages)
+		require.Len(t, listedImages, test.matchedImages, "%s -> %v", test.filters, listedImages)
+
+		resultNames := []string{}
+		for _, image := range listedImages {
+			resultNames = append(resultNames, image.Names()...)
+		}
+		require.ElementsMatch(t, test.names, resultNames, "filters: %s ", test.filters)
 	}
 }
 

libimage/image.go

@@ -112,6 +112,13 @@ func (i *Image) Names() []string {
 	return i.storageImage.Names
 }
 
+// setEphemeralNames sets the names of the image.
+//
+// WARNING: this only affects the in-memory values, they are not written into the backing storage.
+func (i *Image) setEphemeralNames(names []string) {
+	i.storageImage.Names = names
+}
+
 // NamesReferences returns Names() as references.
 func (i *Image) NamesReferences() ([]reference.Reference, error) {
 	if i.cached.namesReferences != nil {

libimage/runtime.go

@@ -599,6 +599,16 @@ func (r *Runtime) ListImagesByNames(names []string) ([]*Image, error) {
 }
 
 // ListImages lists the images in the local container storage and filter the images by ListImagesOptions
+//
+// podman images consumes the output of ListImages and produces one line for each tag in each Image.Names value,
+// rather than one line for each Image with all Names, so if options.Filters contains one reference filter
+// with a fully qualified image name without negation, it is considered a query so it makes more sense for
+// the user to see only the corresponding names in the output, not all the names of the deduplicated
+// image; therefore, we make the corresponding names available to the caller by overwriting the actual image names
+// with the corresponding names when the reference filter matches and the reference is a fully qualified image name
+// (i.e., contains a tag or digest, not just a bare repository name).
+//
+// This overwriting is done only in memory and is not written to storage in any way.
 func (r *Runtime) ListImages(ctx context.Context, options *ListImagesOptions) ([]*Image, error) {
 	if options == nil {
 		options = &ListImagesOptions{}