Skip to content

Commit 3e00335

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 <hony.com@seznam.cz>
1 parent 95a7482 commit 3e00335

File tree

4 files changed

+117
-77
lines changed

4 files changed

+117
-77
lines changed

‎libimage/filters.go‎

Lines changed: 64 additions & 38 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,6 +22,12 @@ import (
2122
// indicates that the image matches the criteria.
2223
type filterFunc func(*Image, *layerTree) (bool, error)
2324

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+
2431
type compiledFilters map[string][]filterFunc
2532

2633
// Apply the specified filters. All filters of each key must apply.
@@ -51,13 +58,25 @@ func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree
5158
// filterImages returns a slice of images which are passing all specified
5259
// filters.
5360
// 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) {
61+
func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters compiledFilters, referenceFilter referenceFilterFunc, tree *layerTree) ([]*Image, error) {
5562
result := []*Image{}
5663
for i := range images {
5764
match, err := images[i].applyFilters(ctx, filters, tree)
5865
if err != nil {
5966
return nil, err
6067
}
68+
if referenceFilter != nil {
69+
referenceMatch, names, err := referenceFilter(images[i])
70+
if err != nil {
71+
return nil, err
72+
}
73+
if !referenceMatch || len(names) == 0 {
74+
continue
75+
}
76+
// If the image matches the reference filter, add the names to the image.
77+
images[i].setNames(names)
78+
}
79+
6180
if match {
6281
result = append(result, images[i])
6382
}
@@ -71,10 +90,10 @@ func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters com
7190
// after, since, before, containers, dangling, id, label, readonly, reference, intermediate
7291
//
7392
// compileImageFilters returns: compiled filters, if LayerTree is needed, error
74-
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (compiledFilters, bool, error) {
93+
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (compiledFilters, referenceFilterFunc, bool, error) {
7594
logrus.Tracef("Parsing image filters %s", options.Filters)
7695
if len(options.Filters) == 0 {
77-
return nil, false, nil
96+
return nil, nil, false, nil
7897
}
7998

8099
filterInvalidValue := `invalid image filter %q: must be in the format "filter=value or filter!=value"`
@@ -93,7 +112,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
93112
} else {
94113
split = strings.SplitN(f, "=", 2)
95114
if len(split) != 2 {
96-
return nil, false, fmt.Errorf(filterInvalidValue, f)
115+
return nil, nil, false, fmt.Errorf(filterInvalidValue, f)
97116
}
98117
}
99118

@@ -103,28 +122,28 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
103122
case "after", "since":
104123
img, err := r.time(key, value)
105124
if err != nil {
106-
return nil, false, err
125+
return nil, nil, false, err
107126
}
108127
key = "since"
109128
filter = filterAfter(img.Created())
110129

111130
case "before":
112131
img, err := r.time(key, value)
113132
if err != nil {
114-
return nil, false, err
133+
return nil, nil, false, err
115134
}
116135
filter = filterBefore(img.Created())
117136

118137
case "containers":
119138
if err := r.containers(duplicate, key, value, options.IsExternalContainerFunc); err != nil {
120-
return nil, false, err
139+
return nil, nil, false, err
121140
}
122141
filter = filterContainers(value, options.IsExternalContainerFunc)
123142

124143
case "dangling":
125144
dangling, err := r.bool(duplicate, key, value)
126145
if err != nil {
127-
return nil, false, err
146+
return nil, nil, false, err
128147
}
129148
needsLayerTree = true
130149
filter = filterDangling(ctx, dangling)
@@ -135,14 +154,14 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
135154
case "digest":
136155
f, err := filterDigest(value)
137156
if err != nil {
138-
return nil, false, err
157+
return nil, nil, false, err
139158
}
140159
filter = f
141160

142161
case "intermediate":
143162
intermediate, err := r.bool(duplicate, key, value)
144163
if err != nil {
145-
return nil, false, err
164+
return nil, nil, false, err
146165
}
147166
needsLayerTree = true
148167
filter = filterIntermediate(ctx, intermediate)
@@ -152,14 +171,14 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
152171
case "readonly":
153172
readOnly, err := r.bool(duplicate, key, value)
154173
if err != nil {
155-
return nil, false, err
174+
return nil, nil, false, err
156175
}
157176
filter = filterReadOnly(readOnly)
158177

159178
case "manifest":
160179
manifest, err := r.bool(duplicate, key, value)
161180
if err != nil {
162-
return nil, false, err
181+
return nil, nil, false, err
163182
}
164183
filter = filterManifest(ctx, manifest)
165184

@@ -174,12 +193,12 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
174193
case "until":
175194
until, err := r.until(value)
176195
if err != nil {
177-
return nil, false, err
196+
return nil, nil, false, err
178197
}
179198
filter = filterBefore(until)
180199

181200
default:
182-
return nil, false, fmt.Errorf(filterInvalidValue, key)
201+
return nil, nil, false, fmt.Errorf(filterInvalidValue, key)
183202
}
184203
if negate {
185204
filter = negateFilter(filter)
@@ -189,10 +208,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
189208

190209
// reference filters is a special case as it does an OR for positive matches
191210
// 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
211+
return filters, filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches), needsLayerTree, nil
196212
}
197213

198214
func negateFilter(f filterFunc) filterFunc {
@@ -265,62 +281,72 @@ func filterManifest(ctx context.Context, value bool) filterFunc {
265281

266282
// filterReferences creates a reference filter for matching the specified wantedReferenceMatches value (OR logic)
267283
// 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) {
284+
func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatches []string) referenceFilterFunc {
285+
return func(img *Image) (bool, []string, error) {
286+
namesThatMatch := img.Names()
270287
// Empty reference filters, return true
271288
if len(wantedReferenceMatches) == 0 && len(unwantedReferenceMatches) == 0 {
272-
return true, nil
289+
return true, namesThatMatch, nil
273290
}
274291

275292
unwantedMatched := false
293+
// TODO 6.0 podman: remove unwanted matches from the output names. https://github.com/containers/common/pull/2413#discussion_r2031749013
276294
// Go through the unwanted matches first
277295
for _, value := range unwantedReferenceMatches {
278-
matches, err := imageMatchesReferenceFilter(r, img, value)
296+
matchedNames, err := imageMatchesReferenceFilter(r, img, value)
279297
if err != nil {
280-
return false, err
298+
return false, namesThatMatch, err
281299
}
282-
if matches {
300+
if len(matchedNames) > 0 {
283301
unwantedMatched = true
284302
}
285303
}
286304

287305
// If there are no wanted match filters, then return false for the image
288306
// that matched the unwanted value otherwise return true
289307
if len(wantedReferenceMatches) == 0 {
290-
return !unwantedMatched, nil
308+
return !unwantedMatched, namesThatMatch, nil
291309
}
292310

293311
// Go through the wanted matches
294312
// If an image matches the wanted filter but it also matches the unwanted
295313
// filter, don't add it to the output
296314
for _, value := range wantedReferenceMatches {
297-
matches, err := imageMatchesReferenceFilter(r, img, value)
315+
matchedNames, err := imageMatchesReferenceFilter(r, img, value)
298316
if err != nil {
299-
return false, err
317+
return false, nil, err
300318
}
301-
if matches && !unwantedMatched {
302-
return true, nil
319+
if len(matchedNames) > 0 && !unwantedMatched {
320+
// Removes non-compliant names from image names
321+
namesThatMatch = slices.DeleteFunc(namesThatMatch, func(name string) bool {
322+
return !slices.ContainsFunc(matchedNames, func(matchedName string) bool {
323+
return strings.Contains(name, matchedName)
324+
})
325+
})
326+
return true, namesThatMatch, nil
303327
}
304328
}
305329

306-
return false, nil
330+
return false, namesThatMatch, nil
307331
}
308332
}
309333

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)
334+
// imageMatchesReferenceFilter returns a list of matching image references that match the specified filter value
335+
func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) ([]string, error) {
336+
lookedUp, resolvedName, _ := r.LookupImage(value, nil)
313337
if lookedUp != nil {
314338
if lookedUp.ID() == img.ID() {
315-
return true, nil
339+
resolvedName, _, _ := strings.Cut(resolvedName, "@")
340+
return []string{resolvedName}, nil
316341
}
317342
}
318343

319344
refs, err := img.NamesReferences()
320345
if err != nil {
321-
return false, err
346+
return nil, err
322347
}
323348

349+
resolvedNames := []string{}
324350
for _, ref := range refs {
325351
refString := ref.String() // FQN with tag/digest
326352
candidates := []string{refString}
@@ -348,12 +374,12 @@ func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, er
348374
// path.Match() is also used by Docker's reference.FamiliarMatch().
349375
matched, _ := path.Match(value, candidate)
350376
if matched {
351-
return true, nil
377+
resolvedNames = append(resolvedNames, refString)
378+
break
352379
}
353380
}
354381
}
355-
356-
return false, nil
382+
return resolvedNames, nil
357383
}
358384

359385
// 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: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,11 @@ func (i *Image) Names() []string {
112112
return i.storageImage.Names
113113
}
114114

115+
// setNames sets the names of the image.
116+
func (i *Image) setNames(names []string) {
117+
i.storageImage.Names = names
118+
}
119+
115120
// NamesReferences returns Names() as references.
116121
func (i *Image) NamesReferences() ([]reference.Reference, error) {
117122
if i.cached.namesReferences != nil {

0 commit comments

Comments
 (0)