Fix unexpected results when filtering images #2413
Conversation
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]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Honny1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -308,19 +309,20 @@ func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatch | |||
| } | |||
|
|
|||
| // imageMatchesReferenceFilter returns true if an image matches the filter value given | |||
There was a problem hiding this comment.
Please update the comment … and maybe the function name.
Eg. do we even need the boolean return value?
| @@ -275,7 +275,7 @@ func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatch | |||
| unwantedMatched := false | |||
| // Go through the unwanted matches first | |||
| for _, value := range unwantedReferenceMatches { | |||
There was a problem hiding this comment.
Unwanted matches don’t affect the output names at all?
I’m not saying it’s obviously wrong, but I do think it is a design question that needs to be considered and explicitly decided.
There was a problem hiding this comment.
In case of an unwanted match the entire image is excluded from the output currently. Are you saying we should only remove the one matched name?
| if err != nil { | ||
| return false, err | ||
| } | ||
| if matches && !unwantedMatched { | ||
| img.cleanNames(resolvedNames) |
There was a problem hiding this comment.
A “filter” function editing its inputs/outputs is very unexpected.
Editing the Image values might be the correct design, after all, I don’t have an opinion on that yet (anyway, let’s settle the expected outputs, e.g. WRT != matches, first) — but if so, this semantics needs to be very thoroughly and visibly documented throughout the “filters” call stack.
Or, maybe, the filterFunc type should be changed so that these values are provided via a different channel.
| {[]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, []string{"localhost/image:tag", "docker.io/library/image:another-tag"}}, |
There was a problem hiding this comment.
(I like a lot how this test now checks what was matched, not just a count.)
| @@ -100,6 +101,11 @@ func TestFilterReference(t *testing.T) { | |||
| listedImages, err := runtime.ListImages(ctx, listOptions) | |||
| require.NoError(t, err, "%v", test) | |||
| require.Len(t, listedImages, test.matches, "%s -> %v", test.filters, listedImages) | |||
| for _, image := range listedImages { | |||
| for _, name := range image.Names() { | |||
| require.Contains(t, test.names, name, "filters: %s ", test.filters) | |||
There was a problem hiding this comment.
An EqualValues test, not just a subset, would be better.
There was a problem hiding this comment.
an in case the slice order is not deterministic ElementsMatch() is an option.
| @@ -112,6 +113,13 @@ func (i *Image) Names() []string { | |||
| return i.storageImage.Names | |||
| } | |||
|
|
|||
| // cleanNames removes all names that are not in the resolvedNames list. | |||
| func (i *Image) cleanNames(resolvedNames []string) { | |||
There was a problem hiding this comment.
“clean” is not quite what this does, and “resolved” is not important for the purpose of this function.
(But let’s settle the data passing mechanism first.)
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