Fix ancestor filter to resolve image names to IDs #27778
Conversation
…since image names are mutable and can thus point to different image IDs as time goes by. The image ID, on the other hand, stays immutable and thus is the best approach in finding a container's ancestor. Fixes: containers#27743 Signed-off-by: Zaid Marwat <zaidratify123@gmail.com>
|
Hi, this is my first contribution to Podman, so please bear with me as I’m still learning the project’s testing expectations. I wanted to note that I ran make validatepr locally, but I am consistently hitting a Darwin (macOS) build failure. I verified that this does not appear to be related to my changes: I checked out a clean branch from main with no modifications and ran validatepr there as well, and it failed with the same Darwin build error. Based on that, I believe the failure is environmental or pre-existing rather than caused by this PR. I am still working through the testing requirements and would appreciate any guidance on whether additional tests are expected beyond what I’ve added, or if there is a preferred way to validate this change. Lastly, please let me know if you think this change warrants any documentation updates. I’m happy to add documentation if needed. Thank you for your time! |
Honny1
left a comment
There was a problem hiding this comment.
Hi, thanks for the contribution. I have several comments on the PR.
First, please create a test that reproduces this issue. It can be a simple BATS test written in Bash or an E2E test in Golang. Also, please validate the new behavior with Docker. If Padman produce same results.
There is also an issue with the validatepr image. To run this, you need to remove that image and ideally build your own; I think the new image isn't built yet for ARM64. Here are the commands:
$ podman rmi quay.io/libpod/validatepr:latest
$ podman build -t quay.io/libpod/validatepr:latest . -f contrib/validatepr/Containerfile
| for _, filterValue := range filterValues { | ||
| img, _, err := imgRuntime.LookupImage(filterValue, nil) | ||
| if err == nil && img != nil { | ||
| // filterValue is a resolvable image name[:tag] |
There was a problem hiding this comment.
CI is not happy
pkg/domain/filters/containers.go:101:1: File is not properly formatted (gofumpt)
// filterValue is a resolvable image name[:tag]
^
1 issues:
There was a problem hiding this comment.
Silly mistake, ill get that fixed
| // - ancestor=(<image-name>[:tag]|<image-id>| ⟨image@digest⟩) - containers created from an image or a descendant. | ||
| imgRuntime := r.LibimageRuntime() | ||
| var resolvedIDs []string | ||
| var passthrough []string |
There was a problem hiding this comment.
I think this is not necessary because LookupImage also accepts IDs.
There was a problem hiding this comment.
Sounds good! I didn't catch that on the first scan of LookupImage.
| } | ||
| imgRuntime := r.LibimageRuntime() | ||
| var resolvedIDs []string | ||
| var passthrough []string |
| case "ancestor": | ||
| // This needs to refine to match docker | ||
| // - ancestor=(<image-name>[:tag]|<image-id>| ⟨image@digest⟩) - containers created from an image or a descendant. | ||
| imgRuntime := r.LibimageRuntime() |
There was a problem hiding this comment.
Both changes in the "ancestor" case seem similar. I would like to see some deduplication to keep it DRY.
| case "ancestor": | ||
| // This needs to refine to match docker | ||
| // - ancestor=(<image-name>[:tag]|<image-id>| ⟨image@digest⟩) - containers created from an image or a descendant. | ||
| imgRuntime := r.LibimageRuntime() |
There was a problem hiding this comment.
All of this feels like it ought to be in the returned function, not here?
There was a problem hiding this comment.
Please correct me on any misconceptions I might have but, it looks to me like the code within the return function runs for every container while the code outside is just called once per filter. So I kept this logic outside the returned function to avoid doing expensive work repeatedly (which, taking a look at LibimageRuntime(), seems to be the case). Resolving image names to IDs with LookupImage only needs to happen once, and putting it inside the returned function would cause it to run for every container.
Fix ancestor filter to resolve image names to IDs. This is necessary since image names are mutable and can thus point to different image IDs as time goes by. The image ID, on the other hand, stays immutable and thus is the best approach in finding a container's ancestor.
Fixes: #27743
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?