Handle filters reference more like Docker does #826
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
| "foo.bar", | ||
| "registry.access.redhat.com", | ||
| } | ||
| names := []string{ |
There was a problem hiding this comment.
no test for bar not matching localhost/foobar (as in containers/podman#11905)? :)
There was a problem hiding this comment.
Added a match for *box to match busybox
There was a problem hiding this comment.
I was rather thinking about ensuring that with "localhost/foo:latest" and "localhost/foobar:latest" in names a test for {"bar:*", 1} succeeds.
|
@vrothberg @giuseppe @flouthoc PTAL |
libimage/filters.go
Outdated
| for _, reg := range registries { | ||
| newName = strings.TrimPrefix(name, reg+"/") | ||
| newName = strings.TrimPrefix(newName, "library/") | ||
| fmt.Println("DAN", filter, newName) |
There was a problem hiding this comment.
I think this is left here by mistake. Could we remove this Println
libimage/runtime.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return filterImages(images, filters, append([]string{"localhost"}, registries...)) |
There was a problem hiding this comment.
Not sure but i think filterImages has a signature func filterImages(images []*Image, filters []filterFunc, registries []string) . I think this should fail with too many arguments in call to filterImages but haven't tried running with podman.
There was a problem hiding this comment.
no my bad this is correct i missed closing )
|
LGTM just one nit #826 (comment) |
|
Please don't merge. I want to have a look at the exact behavior of Docker. |
libimage/filters.go
Outdated
| @@ -16,11 +16,11 @@ import ( | |||
|
|
|||
| // filterFunc is a prototype for a positive image filter. Returning `true` | |||
| // indicates that the image matches the criteria. | |||
| type filterFunc func(*Image) (bool, error) | |||
| type filterFunc func(*Image, []string) (bool, error) | |||
There was a problem hiding this comment.
Can you turn []string to []interface{} to make it generic?
libimage/filters.go
Outdated
| @@ -158,33 +158,54 @@ func (r *Runtime) compileImageFilters(ctx context.Context, options *ListImagesOp | |||
| return filterFuncs, nil | |||
| } | |||
|
|
|||
| func filterNames(filter string, names, registries []string) (bool, error) { | |||
There was a problem hiding this comment.
The entire function should be replaced with https://github.com/containers/image/blob/main/docker/reference/helpers.go#L36. That will match the exact behavior of Docker (I tracked it down in the moby code).
There was a problem hiding this comment.
I don't believe that is true. We need to remove the registry just like docker remove docker.io to be our own flavor. But if you have other ideas, I say take this PR over and make it the way you believe it should be.
There was a problem hiding this comment.
Here's the location in moby: https://github.com/moby/moby/blob/master/daemon/images/images.go#L145
libimage/filters.go
Outdated
|
|
||
| // filterImages returns a slice of images which are passing all specified | ||
| // filters. | ||
| func filterImages(images []*Image, filters []filterFunc) ([]*Image, error) { | ||
| func filterImages(images []*Image, filters []filterFunc, registries []string) ([]*Image, error) { |
There was a problem hiding this comment.
registries should be of type []c/image/docker/reference.Reference.
There was a problem hiding this comment.
Why? registries, err := sysregistriesv2.UnqualifiedSearchRegistries(r.systemContextCopy())
Returns a []string
Seems like we would need to convert this to string to a reference only later to be converted back to a string.
There was a problem hiding this comment.
Currently we match *reference*, which is incorrect. Docker hard codes the current registry and matches the reference exactly, allowing users to pass in globs. This fix will truncate the registry name and localhost/ as well has the constant library/ off of images and then attempt to match. Helps Fix: containers/podman#11905 Signed-off-by: Daniel J Walsh <[email protected]>
|
@vrothberg PTAL |
|
@vrothberg I would like to move forward on this. |
|
@vrothberg Friendly ping. |
Thanks! I was buried in the docker.io enforcement work but have some cycles today. |
|
-> #847 |
Currently we match reference, which is incorrect.
Docker hard codes the current registry and matches the reference
exactly, allowing users to pass in globs.
This fix will truncate the registry name and localhost/ as well has
the constant library/ off of images and then attempt to match.
Helps Fix: containers/podman#11905
Signed-off-by: Daniel J Walsh [email protected]