From 4d2e31bee7bd6bcb5ea2fc38728b7bb0219512ae Mon Sep 17 00:00:00 2001 From: Urvashi Mohnani Date: Wed, 17 Jan 2024 11:06:55 -0500 Subject: [PATCH] Fix filter logic for reference key For the positive case, the reference key does an OR operation. For the negative case, the reference key does an AND operation. Signed-off-by: Urvashi Mohnani --- libimage/filters.go | 125 ++++++++++++++++++++++++++++----------- libimage/filters_test.go | 85 +++++++++++++++----------- 2 files changed, 139 insertions(+), 71 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index cae5f01ea..369eff94a 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -87,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 { @@ -178,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) @@ -196,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 } @@ -267,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. diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 7220e1eb8..86d3a1fe4 100644 --- a/libimage/filters_test.go +++ b/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) } }