Skip to content

Commit c1d4722

Browse files
Honny1Honny1
committed
Fix unexpected results when filtering images
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]>
1 parent 95a7482 commit c1d4722

File tree

4 files changed

+146
-71
lines changed

4 files changed

+146
-71
lines changed

‎libimage/filters.go

Lines changed: 87 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"errors"
88
"fmt"
99
"path"
10+
"slices"
1011
"strconv"
1112
"strings"
1213
"time"
@@ -21,13 +22,23 @@ import (
2122
// indicates that the image matches the criteria.
2223
type filterFunc func(*Image, *layerTree) (bool, error)
2324

24-
type compiledFilters map[string][]filterFunc
25+
// referenceFilterFunc is a prototype for a filter that returns a list of
26+
// references. The first return value indicates whether the image matches the
27+
// criteria. The second return value is a list of names that match the
28+
// criteria. The third return value is an error.
29+
type referenceFilterFunc func(*Image) (bool, []string, error)
30+
31+
type compiledFilters struct {
32+
Filters map[string][]filterFunc
33+
ReferenceFilter referenceFilterFunc
34+
}
2535

2636
// Apply the specified filters. All filters of each key must apply.
2737
// tree must be provided if compileImageFilters indicated it is necessary.
28-
func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree *layerTree) (bool, error) {
29-
for key := range filters {
30-
for _, filter := range filters[key] {
38+
// WARNING: Application of referenceFilter sets the image names to matched names, but this only affects the values in memory, they are not written to storage.
39+
func (i *Image) applyFilters(ctx context.Context, f *compiledFilters, tree *layerTree) (bool, error) {
40+
for key := range f.Filters {
41+
for _, filter := range f.Filters[key] {
3142
matches, err := filter(i, tree)
3243
if err != nil {
3344
// Some images may have been corrupted in the
@@ -45,13 +56,30 @@ func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree
4556
}
4657
}
4758
}
59+
if f.ReferenceFilter != nil {
60+
referenceMatch, names, err := f.ReferenceFilter(i)
61+
if err != nil {
62+
return false, err
63+
}
64+
if !referenceMatch {
65+
return false, nil
66+
}
67+
if len(names) > 0 {
68+
i.setEphemeralNames(names)
69+
}
70+
}
4871
return true, nil
4972
}
5073

5174
// filterImages returns a slice of images which are passing all specified
5275
// filters.
5376
// tree must be provided if compileImageFilters indicated it is necessary.
54-
func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters compiledFilters, tree *layerTree) ([]*Image, error) {
77+
// WARNING: Application of referenceFilter sets the image names to matched names, but this only affects the values in memory, they are not written to storage.
78+
func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters *compiledFilters, tree *layerTree) ([]*Image, error) {
79+
if filters == nil {
80+
logrus.Debug("No filters specified, returning all images")
81+
return images, nil
82+
}
5583
result := []*Image{}
5684
for i := range images {
5785
match, err := images[i].applyFilters(ctx, filters, tree)
@@ -71,7 +99,7 @@ func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters com
7199
// after, since, before, containers, dangling, id, label, readonly, reference, intermediate
72100
//
73101
// compileImageFilters returns: compiled filters, if LayerTree is needed, error
74-
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (compiledFilters, bool, error) {
102+
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (*compiledFilters, bool, error) {
75103
logrus.Tracef("Parsing image filters %s", options.Filters)
76104
if len(options.Filters) == 0 {
77105
return nil, false, nil
@@ -80,7 +108,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
80108
filterInvalidValue := `invalid image filter %q: must be in the format "filter=value or filter!=value"`
81109

82110
var wantedReferenceMatches, unwantedReferenceMatches []string
83-
filters := compiledFilters{}
111+
filters := map[string][]filterFunc{}
84112
needsLayerTree := false
85113
duplicate := map[string]string{}
86114
for _, f := range options.Filters {
@@ -187,12 +215,13 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
187215
filters[key] = append(filters[key], filter)
188216
}
189217

190-
// reference filters is a special case as it does an OR for positive matches
191-
// and an AND logic for negative matches
192-
filter := filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches)
193-
filters["reference"] = append(filters["reference"], filter)
194-
195-
return filters, needsLayerTree, nil
218+
cf := compiledFilters{
219+
Filters: filters,
220+
// reference filters is a special case as it does an OR for positive matches
221+
// and an AND logic for negative matches
222+
ReferenceFilter: filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches),
223+
}
224+
return &cf, needsLayerTree, nil
196225
}
197226

198227
func negateFilter(f filterFunc) filterFunc {
@@ -265,62 +294,86 @@ func filterManifest(ctx context.Context, value bool) filterFunc {
265294

266295
// filterReferences creates a reference filter for matching the specified wantedReferenceMatches value (OR logic)
267296
// and for matching the unwantedReferenceMatches values (AND logic)
268-
func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatches []string) filterFunc {
269-
return func(img *Image, _ *layerTree) (bool, error) {
297+
func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatches []string) referenceFilterFunc {
298+
return func(img *Image) (bool, []string, error) {
299+
namesThatMatch := img.Names()
270300
// Empty reference filters, return true
271301
if len(wantedReferenceMatches) == 0 && len(unwantedReferenceMatches) == 0 {
272-
return true, nil
302+
return true, nil, nil
273303
}
274304

275305
unwantedMatched := false
276306
// Go through the unwanted matches first
307+
// TODO 6.0 podman: remove unwanted matches from the output names. https://github.com/containers/common/pull/2413#discussion_r2031749013
277308
for _, value := range unwantedReferenceMatches {
278-
matches, err := imageMatchesReferenceFilter(r, img, value)
309+
names, err := getMatchedImageNames(r, img, value)
279310
if err != nil {
280-
return false, err
311+
return false, nil, err
281312
}
282-
if matches {
313+
if len(names) > 0 {
283314
unwantedMatched = true
284315
}
285316
}
286317

287318
// If there are no wanted match filters, then return false for the image
288319
// that matched the unwanted value otherwise return true
289320
if len(wantedReferenceMatches) == 0 {
290-
return !unwantedMatched, nil
321+
return !unwantedMatched, namesThatMatch, nil
291322
}
292323

324+
if unwantedMatched {
325+
return false, nil, nil
326+
}
293327
// Go through the wanted matches
294328
// If an image matches the wanted filter but it also matches the unwanted
295329
// filter, don't add it to the output
330+
matchedNames := []string{}
296331
for _, value := range wantedReferenceMatches {
297-
matches, err := imageMatchesReferenceFilter(r, img, value)
332+
names, err := getMatchedImageNames(r, img, value)
298333
if err != nil {
299-
return false, err
300-
}
301-
if matches && !unwantedMatched {
302-
return true, nil
334+
return false, nil, err
303335
}
336+
matchedNames = append(matchedNames, names...)
337+
}
338+
if len(matchedNames) > 0 {
339+
// Removes non-compliant names from image names
340+
namesThatMatch = slices.DeleteFunc(namesThatMatch, func(name string) bool {
341+
return !slices.ContainsFunc(matchedNames, func(matchedName string) bool {
342+
return name == matchedName
343+
})
344+
})
345+
return true, namesThatMatch, nil
304346
}
305347

306-
return false, nil
348+
return false, nil, nil
307349
}
308350
}
309351

310-
// imageMatchesReferenceFilter returns true if an image matches the filter value given
311-
func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, error) {
312-
lookedUp, _, _ := r.LookupImage(value, nil)
352+
// getMatchedImageNames returns a list of matching image names that match the specified filter value, or an empty list if the image does not match the filter.
353+
func getMatchedImageNames(r *Runtime, img *Image, value string) ([]string, error) {
354+
lookedUp, resolvedName, _ := r.LookupImage(value, nil)
313355
if lookedUp != nil {
314356
if lookedUp.ID() == img.ID() {
315-
return true, nil
357+
prefix, _, found := strings.Cut(resolvedName, "@")
358+
if found {
359+
out := []string{}
360+
for _, name := range lookedUp.Names() {
361+
if strings.HasPrefix(name, prefix) {
362+
out = append(out, name)
363+
}
364+
}
365+
return out, nil
366+
}
367+
return []string{resolvedName}, nil
316368
}
317369
}
318370

319371
refs, err := img.NamesReferences()
320372
if err != nil {
321-
return false, err
373+
return nil, err
322374
}
323375

376+
resolvedNames := []string{}
324377
for _, ref := range refs {
325378
refString := ref.String() // FQN with tag/digest
326379
candidates := []string{refString}
@@ -348,12 +401,12 @@ func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, er
348401
// path.Match() is also used by Docker's reference.FamiliarMatch().
349402
matched, _ := path.Match(value, candidate)
350403
if matched {
351-
return true, nil
404+
resolvedNames = append(resolvedNames, refString)
405+
break
352406
}
353407
}
354408
}
355-
356-
return false, nil
409+
return resolvedNames, nil
357410
}
358411

359412
// filterLabel creates a label for matching the specified value.

‎libimage/filters_test.go

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -44,47 +44,48 @@ func TestFilterReference(t *testing.T) {
4444
for _, test := range []struct {
4545
filters []string
4646
matches int
47+
names []string
4748
}{
48-
{[]string{"image"}, 2},
49-
{[]string{"*mage*"}, 2},
50-
{[]string{"image:*"}, 2},
51-
{[]string{"image:tag"}, 1},
52-
{[]string{"image:another-tag"}, 1},
53-
{[]string{"localhost/image"}, 1},
54-
{[]string{"localhost/image:tag"}, 1},
55-
{[]string{"library/image"}, 1},
56-
{[]string{"docker.io/library/image*"}, 1},
57-
{[]string{"docker.io/library/image:*"}, 1},
58-
{[]string{"docker.io/library/image:another-tag"}, 1},
59-
{[]string{"localhost/*"}, 2},
60-
{[]string{"localhost/image:*tag"}, 1},
61-
{[]string{"localhost/*mage:*ag"}, 2},
62-
{[]string{"quay.io/libpod/busybox"}, 1},
63-
{[]string{"quay.io/libpod/alpine"}, 1},
64-
{[]string{"quay.io/libpod"}, 0},
65-
{[]string{"quay.io/libpod/*"}, 2},
66-
{[]string{"busybox"}, 1},
67-
{[]string{"alpine"}, 1},
68-
{[]string{"alpine@" + alpine.Digest().String()}, 1},
69-
{[]string{"alpine:latest@" + alpine.Digest().String()}, 1},
70-
{[]string{"quay.io/libpod/alpine@" + alpine.Digest().String()}, 1},
71-
{[]string{"quay.io/libpod/alpine:latest@" + alpine.Digest().String()}, 1},
49+
{[]string{"image"}, 2, []string{"localhost/image:tag", "docker.io/library/image:another-tag"}},
50+
{[]string{"*mage*"}, 2, []string{"localhost/image:tag", "localhost/another-image:tag", "docker.io/library/image:another-tag"}},
51+
{[]string{"image:*"}, 2, []string{"localhost/image:tag", "docker.io/library/image:another-tag"}},
52+
{[]string{"image:tag"}, 1, []string{"localhost/image:tag"}},
53+
{[]string{"image:another-tag"}, 1, []string{"docker.io/library/image:another-tag"}},
54+
{[]string{"localhost/image"}, 1, []string{"localhost/image:tag"}},
55+
{[]string{"localhost/image:tag"}, 1, []string{"localhost/image:tag"}},
56+
{[]string{"library/image"}, 1, []string{"docker.io/library/image:another-tag"}},
57+
{[]string{"docker.io/library/image*"}, 1, []string{"docker.io/library/image:another-tag"}},
58+
{[]string{"docker.io/library/image:*"}, 1, []string{"docker.io/library/image:another-tag"}},
59+
{[]string{"docker.io/library/image:another-tag"}, 1, []string{"docker.io/library/image:another-tag"}},
60+
{[]string{"localhost/*"}, 2, []string{"localhost/image:tag", "localhost/another-image:tag"}},
61+
{[]string{"localhost/image:*tag"}, 1, []string{"localhost/image:tag"}},
62+
{[]string{"localhost/*mage:*ag"}, 2, []string{"localhost/image:tag", "localhost/another-image:tag"}},
63+
{[]string{"quay.io/libpod/busybox"}, 1, []string{"quay.io/libpod/busybox:latest"}},
64+
{[]string{"quay.io/libpod/alpine"}, 1, []string{"quay.io/libpod/alpine:latest"}},
65+
{[]string{"quay.io/libpod"}, 0, []string{}},
66+
{[]string{"quay.io/libpod/*"}, 2, []string{"quay.io/libpod/busybox:latest", "quay.io/libpod/alpine:latest"}},
67+
{[]string{"busybox"}, 1, []string{"quay.io/libpod/busybox:latest"}},
68+
{[]string{"alpine"}, 1, []string{"quay.io/libpod/alpine:latest"}},
69+
{[]string{"alpine@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
70+
{[]string{"alpine:latest@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
71+
{[]string{"quay.io/libpod/alpine@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
72+
{[]string{"quay.io/libpod/alpine:latest@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
7273
// Make sure negate works as expected
73-
{[]string{"!alpine"}, 1},
74-
{[]string{"!alpine", "!busybox"}, 0},
75-
{[]string{"!alpine", "busybox"}, 1},
76-
{[]string{"alpine", "busybox"}, 2},
77-
{[]string{"*test", "!*box"}, 1},
74+
{[]string{"!alpine"}, 1, []string{"quay.io/libpod/busybox:latest", "localhost/image:tag"}},
75+
{[]string{"!alpine", "!busybox"}, 0, []string{}},
76+
{[]string{"!alpine", "busybox"}, 1, []string{"quay.io/libpod/busybox:latest"}},
77+
{[]string{"alpine", "busybox"}, 2, []string{"quay.io/libpod/busybox:latest", "quay.io/libpod/alpine:latest"}},
78+
{[]string{"*test", "!*box"}, 1, []string{"quay.io/libpod/alpine:latest"}},
7879
// Make sure that tags are ignored
79-
{[]string{"alpine:ignoreme@" + alpine.Digest().String()}, 1},
80-
{[]string{"alpine:123@" + alpine.Digest().String()}, 1},
81-
{[]string{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String()}, 1},
82-
{[]string{"quay.io/libpod/alpine:456@" + alpine.Digest().String()}, 1},
80+
{[]string{"alpine:ignoreme@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
81+
{[]string{"alpine:123@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
82+
{[]string{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
83+
{[]string{"quay.io/libpod/alpine:456@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
8384
// Make sure that repo and digest must match
84-
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
85-
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
86-
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
87-
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0},
85+
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
86+
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
87+
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
88+
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
8889
} {
8990
var filters []string
9091
for _, filter := range test.filters {
@@ -94,12 +95,20 @@ func TestFilterReference(t *testing.T) {
9495
filters = append(filters, "reference="+filter)
9596
}
9697
}
98+
9799
listOptions := &ListImagesOptions{
98100
Filters: filters,
99101
}
100102
listedImages, err := runtime.ListImages(ctx, listOptions)
103+
101104
require.NoError(t, err, "%v", test)
102105
require.Len(t, listedImages, test.matches, "%s -> %v", test.filters, listedImages)
106+
107+
resultNames := []string{}
108+
for _, image := range listedImages {
109+
resultNames = append(resultNames, image.Names()...)
110+
}
111+
require.ElementsMatch(t, test.names, resultNames, "filters: %s ", test.filters)
103112
}
104113
}
105114

‎libimage/image.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,13 @@ func (i *Image) Names() []string {
112112
return i.storageImage.Names
113113
}
114114

115+
// setEphemeralNames sets the names of the image.
116+
//
117+
// WARNING: this only affects the in-memory values, they are not written into the backing storage.
118+
func (i *Image) setEphemeralNames(names []string) {
119+
i.storageImage.Names = names
120+
}
121+
115122
// NamesReferences returns Names() as references.
116123
func (i *Image) NamesReferences() ([]reference.Reference, error) {
117124
if i.cached.namesReferences != nil {

‎libimage/runtime.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,12 @@ func (r *Runtime) ListImagesByNames(names []string) ([]*Image, error) {
599599
}
600600

601601
// ListImages lists the images in the local container storage and filter the images by ListImagesOptions
602+
//
603+
// podman images consumes the output of ListImages and produces one line for each tag in each Image.Names value,
604+
// rather than one line for each Image with all Names, so it makes more sense for the user to see only the corresponding names
605+
// in the output, not all the names of the deduplicated image; therefore, we make the corresponding names available
606+
// to the caller by overwriting the actual image names with the corresponding names.
607+
// This overwriting is done only in memory and is not written to storage in any way.
602608
func (r *Runtime) ListImages(ctx context.Context, options *ListImagesOptions) ([]*Image, error) {
603609
if options == nil {
604610
options = &ListImagesOptions{}

0 commit comments

Comments
 (0)