Skip to content

Commit 96058ab

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. Fixes: containers/podman#25725 Fixes: https://issues.redhat.com/browse/RUN-2726 Signed-off-by: Jan Rodák <[email protected]>
1 parent f15bcef commit 96058ab

File tree

4 files changed

+142
-74
lines changed

4 files changed

+142
-74
lines changed

‎libimage/filters.go

Lines changed: 52 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,17 @@ import (
2121
// indicates that the image matches the criteria.
2222
type filterFunc func(*Image, *layerTree) (bool, error)
2323

24-
type compiledFilters map[string][]filterFunc
24+
type compiledFilters struct {
25+
filters map[string][]filterFunc
26+
needsLayerTree bool
27+
}
2528

2629
// Apply the specified filters. All filters of each key must apply.
2730
// 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] {
31+
// 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.
32+
func (i *Image) applyFilters(ctx context.Context, f *compiledFilters, tree *layerTree) (bool, error) {
33+
for key := range f.filters {
34+
for _, filter := range f.filters[key] {
3135
matches, err := filter(i, tree)
3236
if err != nil {
3337
// Some images may have been corrupted in the
@@ -51,7 +55,8 @@ func (i *Image) applyFilters(ctx context.Context, filters compiledFilters, tree
5155
// filterImages returns a slice of images which are passing all specified
5256
// filters.
5357
// 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) {
58+
// 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.
59+
func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters *compiledFilters, tree *layerTree) ([]*Image, error) {
5560
result := []*Image{}
5661
for i := range images {
5762
match, err := images[i].applyFilters(ctx, filters, tree)
@@ -71,17 +76,19 @@ func (r *Runtime) filterImages(ctx context.Context, images []*Image, filters com
7176
// after, since, before, containers, dangling, id, label, readonly, reference, intermediate
7277
//
7378
// compileImageFilters returns: compiled filters, if LayerTree is needed, error
74-
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (compiledFilters, bool, error) {
79+
func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOptions) (*compiledFilters, error) {
7580
logrus.Tracef("Parsing image filters %s", options.Filters)
7681
if len(options.Filters) == 0 {
77-
return nil, false, nil
82+
return &compiledFilters{}, nil
7883
}
7984

8085
filterInvalidValue := `invalid image filter %q: must be in the format "filter=value or filter!=value"`
8186

8287
var wantedReferenceMatches, unwantedReferenceMatches []string
83-
filters := compiledFilters{}
84-
needsLayerTree := false
88+
cf := compiledFilters{
89+
filters: map[string][]filterFunc{},
90+
needsLayerTree: false,
91+
}
8592
duplicate := map[string]string{}
8693
for _, f := range options.Filters {
8794
var key, value string
@@ -93,7 +100,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
93100
} else {
94101
split = strings.SplitN(f, "=", 2)
95102
if len(split) != 2 {
96-
return nil, false, fmt.Errorf(filterInvalidValue, f)
103+
return nil, fmt.Errorf(filterInvalidValue, f)
97104
}
98105
}
99106

@@ -103,30 +110,30 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
103110
case "after", "since":
104111
img, err := r.time(key, value)
105112
if err != nil {
106-
return nil, false, err
113+
return nil, err
107114
}
108115
key = "since"
109116
filter = filterAfter(img.Created())
110117

111118
case "before":
112119
img, err := r.time(key, value)
113120
if err != nil {
114-
return nil, false, err
121+
return nil, err
115122
}
116123
filter = filterBefore(img.Created())
117124

118125
case "containers":
119126
if err := r.containers(duplicate, key, value, options.IsExternalContainerFunc); err != nil {
120-
return nil, false, err
127+
return nil, err
121128
}
122129
filter = filterContainers(value, options.IsExternalContainerFunc)
123130

124131
case "dangling":
125132
dangling, err := r.bool(duplicate, key, value)
126133
if err != nil {
127-
return nil, false, err
134+
return nil, err
128135
}
129-
needsLayerTree = true
136+
cf.needsLayerTree = true
130137
filter = filterDangling(ctx, dangling)
131138

132139
case "id":
@@ -135,31 +142,31 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
135142
case "digest":
136143
f, err := filterDigest(value)
137144
if err != nil {
138-
return nil, false, err
145+
return nil, err
139146
}
140147
filter = f
141148

142149
case "intermediate":
143150
intermediate, err := r.bool(duplicate, key, value)
144151
if err != nil {
145-
return nil, false, err
152+
return nil, err
146153
}
147-
needsLayerTree = true
154+
cf.needsLayerTree = true
148155
filter = filterIntermediate(ctx, intermediate)
149156

150157
case "label":
151158
filter = filterLabel(ctx, value)
152159
case "readonly":
153160
readOnly, err := r.bool(duplicate, key, value)
154161
if err != nil {
155-
return nil, false, err
162+
return nil, err
156163
}
157164
filter = filterReadOnly(readOnly)
158165

159166
case "manifest":
160167
manifest, err := r.bool(duplicate, key, value)
161168
if err != nil {
162-
return nil, false, err
169+
return nil, err
163170
}
164171
filter = filterManifest(ctx, manifest)
165172

@@ -174,25 +181,25 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp
174181
case "until":
175182
until, err := r.until(value)
176183
if err != nil {
177-
return nil, false, err
184+
return nil, err
178185
}
179186
filter = filterBefore(until)
180187

181188
default:
182-
return nil, false, fmt.Errorf(filterInvalidValue, key)
189+
return nil, fmt.Errorf(filterInvalidValue, key)
183190
}
184191
if negate {
185192
filter = negateFilter(filter)
186193
}
187-
filters[key] = append(filters[key], filter)
194+
cf.filters[key] = append(cf.filters[key], filter)
188195
}
189196

190197
// reference filters is a special case as it does an OR for positive matches
191-
// and an AND logic for negative matches
198+
// and an AND logic for negative matches and the filter function type is different.
192199
filter := filterReferences(r, wantedReferenceMatches, unwantedReferenceMatches)
193-
filters["reference"] = append(filters["reference"], filter)
200+
cf.filters["reference"] = append(cf.filters["reference"], filter)
194201

195-
return filters, needsLayerTree, nil
202+
return &cf, nil
196203
}
197204

198205
func negateFilter(f filterFunc) filterFunc {
@@ -290,15 +297,31 @@ func filterReferences(r *Runtime, wantedReferenceMatches, unwantedReferenceMatch
290297
return !unwantedMatched, nil
291298
}
292299

293-
// Go through the wanted matches
294-
// If an image matches the wanted filter but it also matches the unwanted
295-
// filter, don't add it to the output
296300
for _, value := range wantedReferenceMatches {
297301
matches, err := imageMatchesReferenceFilter(r, img, value)
298302
if err != nil {
299303
return false, err
300304
}
301305
if matches && !unwantedMatched {
306+
ref, err := reference.ParseNamed(value)
307+
if err == nil {
308+
if !reference.IsNameOnly(ref) {
309+
namesThatMatch := []string{}
310+
containsDigest := strings.Contains(value, "@")
311+
for _, name := range img.Names() {
312+
if containsDigest {
313+
if strings.HasPrefix(name, ref.Name()) {
314+
namesThatMatch = append(namesThatMatch, name)
315+
continue
316+
}
317+
}
318+
if name == value {
319+
namesThatMatch = append(namesThatMatch, name)
320+
}
321+
}
322+
img.setEphemeralNames(namesThatMatch)
323+
}
324+
}
302325
return true, nil
303326
}
304327
}
@@ -352,7 +375,6 @@ func imageMatchesReferenceFilter(r *Runtime, img *Image, value string) (bool, er
352375
}
353376
}
354377
}
355-
356378
return false, nil
357379
}
358380

‎libimage/filters_test.go

Lines changed: 71 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -40,51 +40,74 @@ func TestFilterReference(t *testing.T) {
4040
require.NoError(t, err)
4141
err = alpine.Tag("docker.io/library/image:another-tag")
4242
require.NoError(t, err)
43+
err = alpine.Tag("docker.io/library/image:latest")
44+
require.NoError(t, err)
45+
46+
allAlpineNames := []string{
47+
"docker.io/library/image:another-tag",
48+
"quay.io/libpod/alpine:latest",
49+
"localhost/another-image:tag",
50+
"docker.io/library/image:latest",
51+
}
52+
allBusyBoxNames := []string{
53+
"quay.io/libpod/busybox:latest",
54+
"localhost/image:tag",
55+
}
56+
allNames := []string{}
57+
allNames = append(allNames, allBusyBoxNames...)
58+
allNames = append(allNames, allAlpineNames...)
4359

4460
for _, test := range []struct {
45-
filters []string
46-
matches int
61+
filters []string
62+
matchedImages int
63+
names []string
4764
}{
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},
65+
{[]string{"image"}, 2, allNames},
66+
{[]string{"*mage*"}, 2, allNames},
67+
{[]string{"image:*"}, 2, allNames},
68+
{[]string{"image:tag"}, 1, allBusyBoxNames},
69+
{[]string{"image:another-tag"}, 1, allAlpineNames},
70+
{[]string{"localhost/image"}, 1, allBusyBoxNames},
71+
{[]string{"localhost/image:tag"}, 1, []string{"localhost/image:tag"}},
72+
{[]string{"library/image"}, 1, allAlpineNames},
73+
{[]string{"docker.io/library/image*"}, 1, allAlpineNames},
74+
{[]string{"docker.io/library/image:*"}, 1, allAlpineNames},
75+
{[]string{"docker.io/library/image:another-tag"}, 1, []string{"docker.io/library/image:another-tag"}},
76+
{[]string{"localhost/*"}, 2, allNames},
77+
{[]string{"localhost/image:*tag"}, 1, allBusyBoxNames},
78+
{[]string{"localhost/*mage:*ag"}, 2, allNames},
79+
{[]string{"quay.io/libpod/busybox"}, 1, allBusyBoxNames},
80+
{[]string{"quay.io/libpod/alpine"}, 1, allAlpineNames},
81+
{[]string{"quay.io/libpod"}, 0, []string{}},
82+
{[]string{"quay.io/libpod/*"}, 2, allNames},
83+
{[]string{"busybox"}, 1, allBusyBoxNames},
84+
{[]string{"alpine"}, 1, allAlpineNames},
85+
7286
// 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},
87+
{[]string{"!alpine"}, 1, allBusyBoxNames},
88+
{[]string{"!alpine", "!busybox"}, 0, []string{}},
89+
{[]string{"!alpine", "busybox"}, 1, allBusyBoxNames},
90+
{[]string{"alpine", "busybox"}, 2, allNames},
91+
{[]string{"*test", "!*box"}, 1, allAlpineNames},
92+
93+
{[]string{"quay.io/libpod/alpine@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
94+
95+
{[]string{"alpine@" + alpine.Digest().String()}, 1, allAlpineNames},
96+
{[]string{"alpine:latest@" + alpine.Digest().String()}, 1, allAlpineNames},
97+
{[]string{"quay.io/libpod/alpine:latest@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
98+
{[]string{"docker.io/library/image@" + alpine.Digest().String()}, 1, []string{"docker.io/library/image:latest", "docker.io/library/image:another-tag"}},
99+
78100
// 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},
101+
{[]string{"alpine:ignoreme@" + alpine.Digest().String()}, 1, allAlpineNames},
102+
{[]string{"alpine:123@" + alpine.Digest().String()}, 1, allAlpineNames},
103+
{[]string{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
104+
{[]string{"quay.io/libpod/alpine:456@" + alpine.Digest().String()}, 1, []string{"quay.io/libpod/alpine:latest"}},
105+
83106
// 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},
107+
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
108+
{[]string{"alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
109+
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
110+
{[]string{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String()}, 0, []string{}},
88111
} {
89112
var filters []string
90113
for _, filter := range test.filters {
@@ -94,12 +117,20 @@ func TestFilterReference(t *testing.T) {
94117
filters = append(filters, "reference="+filter)
95118
}
96119
}
120+
97121
listOptions := &ListImagesOptions{
98122
Filters: filters,
99123
}
100124
listedImages, err := runtime.ListImages(ctx, listOptions)
125+
101126
require.NoError(t, err, "%v", test)
102-
require.Len(t, listedImages, test.matches, "%s -> %v", test.filters, listedImages)
127+
require.Len(t, listedImages, test.matchedImages, "%s -> %v", test.filters, listedImages)
128+
129+
resultNames := []string{}
130+
for _, image := range listedImages {
131+
resultNames = append(resultNames, image.Names()...)
132+
}
133+
require.ElementsMatch(t, test.names, resultNames, "filters: %s ", test.filters)
103134
}
104135
}
105136

‎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 {

0 commit comments

Comments
 (0)