Fix unexpected results when filtering images #2413
Conversation
Honny1
force-pushed
the
list-images
branch
4 times, most recently
from
3e00335 to
8dd85f6
Compare
libimage/filters.go
Outdated
| if lookedUp != nil { | ||
| if lookedUp.ID() == img.ID() { | ||
| return true, nil | ||
| resolvedName, _, _ := strings.Cut(resolvedName, "@") |
There was a problem hiding this comment.
What is this doing?
In the later loop, when we are making various imprecise matches, the return value is the full refString. This seems to be dropping digests from the returned values. Why?
There was a problem hiding this comment.
I still have no idea what this is doing. AFAICS LookupImage can easily be a (precise) repo:tag or repo@digest match. And the range refs loop below always collects the precise ref.String() values that match. Why is this ignoring the digest part (if that is what it is) and returning all tags (if that is what it is)?
And, even apart from the again, HasPrefix just looks implausible. Why should example.com/a match example.com/ab?
There was a problem hiding this comment.
I only want to return names that match the name if the name@digest reference format is used.
So, for example, for an image with the following names:
quay.io/libpod/alpine:latestquay.io/libpod/al:latestquay.io/libpod/al:tag
I expect the result for filtering with the al@digest reference, these names:
quay.io/libpod/al:latestquay.io/libpod/al:tag
Usage HasPrefix is wrong. I fixed that.
Honny1
force-pushed
the
list-images
branch
7 times, most recently
from
f644901 to
c1d4722
Compare
|
PTAL @mtrmac |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
libimage/filters.go
Outdated
| // and an AND logic for negative matches | ||
| ReferenceFilter: filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches), | ||
| } | ||
| return &cf, needsLayerTree, nil |
There was a problem hiding this comment.
(Absolutely non-blocking: Even the needsLayerTree value could be returned in the struct; that would turn an unnamed bool return value into a named one, and shorten the error paths. That’s unrelated to this PR.)
libimage/filters.go
Outdated
| cf := compiledFilters{ | ||
| Filters: filters, | ||
| // reference filters is a special case as it does an OR for positive matches | ||
| // and an AND logic for negative matches |
There was a problem hiding this comment.
… and the filter function type is different.
libimage/filters.go
Outdated
| Filters map[string][]filterFunc | ||
| ReferenceFilter referenceFilterFunc |
There was a problem hiding this comment.
(Absolutely non-blocking: The field names can be package-private. I expect the compiler can figure that out itself.)
libimage/filters.go
Outdated
| // 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 !unwantedMatched, namesThatMatch, nil | ||
| } | ||
|
|
||
| if unwantedMatched { | ||
| return false, nil, nil | ||
| } |
There was a problem hiding this comment.
(Absolutely non-blocking: possible refactoring steps:
if unwantedMatched { return false }; if len(wantedRM) == 0 { return true }- Then replace
unwantedMatchedby an early return from the loop.
This is pre-existing.)
| } | ||
| matchedNames = append(matchedNames, names...) | ||
| } | ||
| if len(matchedNames) > 0 { |
There was a problem hiding this comment.
Non-blocking: Conceptually matchedNames is a set, and implementing that as a map[string]struct{} would make this O(N) instead of O(N^2). Probably doesn’t matter in practice.
| } | ||
| if len(matchedNames) > 0 { | ||
| // Removes non-compliant names from image names | ||
| namesThatMatch = slices.DeleteFunc(namesThatMatch, func(name string) bool { |
There was a problem hiding this comment.
slices.DeleteFunc edits a slice in-place. namesThatMatch directly points into Image.Names, it is not a private copy, so it should not be modified in-place.
(Moving the namesThatMatch initialization much closer to these lines would make that a tiny bit easier to see, please do so.)
libimage/filters.go
Outdated
| if lookedUp != nil { | ||
| if lookedUp.ID() == img.ID() { | ||
| return true, nil | ||
| resolvedName, _, _ := strings.Cut(resolvedName, "@") |
There was a problem hiding this comment.
I still have no idea what this is doing. AFAICS LookupImage can easily be a (precise) repo:tag or repo@digest match. And the range refs loop below always collects the precise ref.String() values that match. Why is this ignoring the digest part (if that is what it is) and returning all tags (if that is what it is)?
And, even apart from the again, HasPrefix just looks implausible. Why should example.com/a match example.com/ab?
Honny1
force-pushed
the
list-images
branch
2 times, most recently
from
fb462fe to
3d1f37e
Compare
The filtered image contains all Names of image. This causes the podman to list images that are not expected. For example: If an image `box:latest` is taged as `example:latest` and then images are filtered with `reference=example`, `box:latest` and `example:latest` will be output because the image has multiple names. Fixes: containers/podman#25725 Fixes: https://issues.redhat.com/browse/RUN-2726 Signed-off-by: Jan Rodák <[email protected]>
|
@mtrmac PTAL, I have implemented your feedback. |
The filtered image contains all Names of image. This causes the podman to list images that are not expected.
For example:
If an image
box:latestis taged asexample:latestand then images are filtered withreference=example,box:latestandexample:latestwill be output because the image has multiple names.Fixes: containers/podman#25725
Fixes: https://issues.redhat.com/browse/RUN-2726