Merge pull request containers#1800 from umohnani8/img-filters

Fix image filters parsing

libimage/filters.go

@@ -20,14 +20,11 @@ import (
 // indicates that the image matches the criteria.
 type filterFunc func(*Image) (bool, error)
 
-// Apply the specified filters.  At least one filter of each key must apply.
+// Apply the specified filters.  All filters of each key must apply.
 func (i *Image) applyFilters(ctx context.Context, filters map[string][]filterFunc) (bool, error) {
-	matches := false
-	for key := range filters { // and
-		matches = false
-		for _, filter := range filters[key] { // or
-			var err error
-			matches, err = filter(i)
+	for key := range filters {
+		for _, filter := range filters[key] {
+			matches, err := filter(i)
 			if err != nil {
 				// Some images may have been corrupted in the
 				// meantime, so do an extra check and make the
@@ -38,15 +35,13 @@ func (i *Image) applyFilters(ctx context.Context, filters map[string][]filterFun
 				}
 				return false, err
 			}
-			if matches {
-				break
+			// If any filter within a group doesn't match, return false
+			if !matches {
+				return false, nil
 			}
 		}
-		if !matches {
-			return false, nil
-		}
 	}
-	return matches, nil
+	return true, nil
 }
 
 // filterImages returns a slice of images which are passing all specified
@@ -92,6 +87,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
 		return tree, nil
 	}
 
+	var wantedReferenceMatches, unwantedReferenceMatches []string
 	filters := map[string][]filterFunc{}
 	duplicate := map[string]string{}
 	for _, f := range options.Filters {
@@ -183,7 +179,12 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
 			filter = filterManifest(ctx, manifest)
 
 		case "reference":
-			filter = filterReferences(r, value)
+			if negate {
+				unwantedReferenceMatches = append(unwantedReferenceMatches, value)
+			} else {
+				wantedReferenceMatches = append(wantedReferenceMatches, value)
+			}
+			continue
 
 		case "until":
 			until, err := r.until(value)
@@ -201,6 +202,11 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
 		filters[key] = append(filters[key], filter)
 	}
 
+	// reference filters is a special case as it does an OR for positive matches
+	// and an AND logic for negative matches
+	filter := filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches)
+	filters["reference"] = append(filters["reference"], filter)
+
 	return filters, nil
 }
 
@@ -272,55 +278,97 @@ func filterManifest(ctx context.Context, value bool) filterFunc {
 	}
 }
 
-// filterReferences creates a reference filter for matching the specified value.
-func filterReferences(r *Runtime, value string) filterFunc {
-	lookedUp, _, _ := r.LookupImage(value, nil)
+// filterReferences creates a reference filter for matching the specified wantedReferenceMatches value (OR logic)
+// and for matching the unwantedReferenceMatches values (AND logic)
+func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatches []string) filterFunc {
 	return func(img *Image) (bool, error) {
-		if lookedUp != nil {
-			if lookedUp.ID() == img.ID() {
+		// Empty reference filters, return true
+		if len(wantedReferenceMatches) == 0 && len(unwantedReferenceMatches) == 0 {
+			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
+			}
+		}
+
+		// 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
+		}
+
+		// 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
+		for _, value := range wantedReferenceMatches {
+			matches, err := imageMatchesReferenceFilter(r, img, value)
+			if err != nil {
+				return false, err
+			}
+			if matches && !unwantedMatched {
 				return true, nil
 			}
 		}
 
-		refs, err := img.NamesReferences()
-		if err != nil {
-			return false, err
+		return false, nil
+	}
+}
+
+// imageMatchesReferenceFilter returns true if an image matches the filter value given
+func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, error) {
+	lookedUp, _, _ := r.LookupImage(value, nil)
+	if lookedUp != nil {
+		if lookedUp.ID() == img.ID() {
+			return true, nil
 		}
+	}
 
-		for _, ref := range refs {
-			refString := ref.String() // FQN with tag/digest
-			candidates := []string{refString}
+	refs, err := img.NamesReferences()
+	if err != nil {
+		return false, err
+	}
 
-			// Split the reference into 3 components (twice if digested/tagged):
-			// 1) Fully-qualified reference
-			// 2) Without domain
-			// 3) Without domain and path
-			if named, isNamed := ref.(reference.Named); isNamed {
+	for _, ref := range refs {
+		refString := ref.String() // FQN with tag/digest
+		candidates := []string{refString}
+
+		// Split the reference into 3 components (twice if digested/tagged):
+		// 1) Fully-qualified reference
+		// 2) Without domain
+		// 3) Without domain and path
+		if named, isNamed := ref.(reference.Named); isNamed {
+			candidates = append(candidates,
+				reference.Path(named),                           // path/name without tag/digest (Path() removes it)
+				refString[strings.LastIndex(refString, "/")+1:]) // name with tag/digest
+
+			trimmedString := reference.TrimNamed(named).String()
+			if refString != trimmedString {
+				tagOrDigest := refString[len(trimmedString):]
 				candidates = append(candidates,
-					reference.Path(named),                           // path/name without tag/digest (Path() removes it)
-					refString[strings.LastIndex(refString, "/")+1:]) // name with tag/digest
-
-				trimmedString := reference.TrimNamed(named).String()
-				if refString != trimmedString {
-					tagOrDigest := refString[len(trimmedString):]
-					candidates = append(candidates,
-						trimmedString,                     // FQN without tag/digest
-						reference.Path(named)+tagOrDigest, // path/name with tag/digest
-						trimmedString[strings.LastIndex(trimmedString, "/")+1:]) // name without tag/digest
-				}
+					trimmedString,                     // FQN without tag/digest
+					reference.Path(named)+tagOrDigest, // path/name with tag/digest
+					trimmedString[strings.LastIndex(trimmedString, "/")+1:]) // name without tag/digest
 			}
+		}
 
-			for _, candidate := range candidates {
-				// path.Match() is also used by Docker's reference.FamiliarMatch().
-				matched, _ := path.Match(value, candidate)
-				if matched {
-					return true, nil
-				}
+		for _, candidate := range candidates {
+			// path.Match() is also used by Docker's reference.FamiliarMatch().
+			matched, _ := path.Match(value, candidate)
+			if matched {
+				return true, nil
 			}
 		}
-
-		return false, nil
 	}
+
+	return false, nil
 }
 
 // filterLabel creates a label for matching the specified value.

libimage/filters_test.go

@@ -5,6 +5,7 @@ package libimage
 import (
 	"context"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -41,50 +42,64 @@ func TestFilterReference(t *testing.T) {
 	require.NoError(t, err)
 
 	for _, test := range []struct {
-		filter  string
+		filters []string
 		matches int
 	}{
-		{"image", 2},
-		{"*mage*", 2},
-		{"image:*", 2},
-		{"image:tag", 1},
-		{"image:another-tag", 1},
-		{"localhost/image", 1},
-		{"localhost/image:tag", 1},
-		{"library/image", 1},
-		{"docker.io/library/image*", 1},
-		{"docker.io/library/image:*", 1},
-		{"docker.io/library/image:another-tag", 1},
-		{"localhost/*", 2},
-		{"localhost/image:*tag", 1},
-		{"localhost/*mage:*ag", 2},
-		{"quay.io/libpod/busybox", 1},
-		{"quay.io/libpod/alpine", 1},
-		{"quay.io/libpod", 0},
-		{"quay.io/libpod/*", 2},
-		{"busybox", 1},
-		{"alpine", 1},
-		{"alpine@" + alpine.Digest().String(), 1},
-		{"alpine:latest@" + alpine.Digest().String(), 1},
-		{"quay.io/libpod/alpine@" + alpine.Digest().String(), 1},
-		{"quay.io/libpod/alpine:latest@" + alpine.Digest().String(), 1},
+		{[]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},
+		// 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},
 		// Make sure that tags are ignored
-		{"alpine:ignoreme@" + alpine.Digest().String(), 1},
-		{"alpine:123@" + alpine.Digest().String(), 1},
-		{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String(), 1},
-		{"quay.io/libpod/alpine:456@" + alpine.Digest().String(), 1},
+		{[]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},
 		// Make sure that repo and digest must match
-		{"alpine:busyboxdigest@" + busybox.Digest().String(), 0},
-		{"alpine:busyboxdigest@" + busybox.Digest().String(), 0},
-		{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0},
-		{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0},
+		{[]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},
 	} {
+		var filters []string
+		for _, filter := range test.filters {
+			if strings.HasPrefix(filter, "!") {
+				filters = append(filters, "reference!="+filter[1:])
+			} else {
+				filters = append(filters, "reference="+filter)
+			}
+		}
 		listOptions := &ListImagesOptions{
-			Filters: []string{"reference=" + test.filter},
+			Filters: filters,
 		}
 		listedImages, err := runtime.ListImages(ctx, nil, listOptions)
 		require.NoError(t, err, "%v", test)
-		require.Len(t, listedImages, test.matches, "%s -> %v", test.filter, listedImages)
+		require.Len(t, listedImages, test.matches, "%s -> %v", test.filters, listedImages)
 	}
 }
 

pkg/filters/filters.go

@@ -76,13 +76,10 @@ func FiltersFromRequest(r *http.Request) ([]string, error) {
 
 	libpodFilters := make([]string, 0, len(filters))
 	for filterKey, filterSlice := range filters {
-		f := filterKey
 		for _, filterValue := range filterSlice {
-			f += "=" + filterValue
+			libpodFilters = append(libpodFilters, fmt.Sprintf("%s=%s", filterKey, filterValue))
 		}
-		libpodFilters = append(libpodFilters, f)
 	}
-
 	return libpodFilters, nil
 }
 

pkg/filters/filters_test.go

@@ -1,7 +1,12 @@
 package filters
 
 import (
+	"net/http"
+	"net/url"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMatchLabelFilters(t *testing.T) {
@@ -185,3 +190,20 @@ func TestComputeUntilTimestamp(t *testing.T) {
 		})
 	}
 }
+
+func TestFiltersFromRequest(t *testing.T) {
+	// simulate https request
+	req := http.Request{
+		URL: &url.URL{
+			RawQuery: "all=false&filters=%7B%22label%22%3A%5B%22xyz%3Dbar%22%2C%22abc%22%5D%2C%22reference%22%3A%5B%22test%22%5D%7D",
+		},
+	}
+	// call req.ParseForm so it can parse the RawQuery data
+	err := req.ParseForm()
+	require.NoError(t, err)
+
+	expectedLibpodFilters := []string{"label=xyz=bar", "label=abc", "reference=test"}
+	got, err := FiltersFromRequest(&req)
+	require.NoError(t, err)
+	assert.Equal(t, expectedLibpodFilters, got)
+}