fix(container): correct image parsing with registry port#1186
fix(container): correct image parsing with registry port#1186suzutan wants to merge 2 commits intofalcosecurity:mainfrom
Conversation
|
Welcome @suzutan! It looks like this is your first PR to falcosecurity/plugins 🎉 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: suzutan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
7bb0773 to
c3f9c14
Compare
Container image name/tag parsing failed when the registry URL included a port number (e.g., registry.example.com:5000/foo/bar:latest). The previous implementation used strings.Split(image, ":") which split on all colons, unable to distinguish between registry port (:5000) and image tag (:latest). Changes: - Add parseImageRepoTag() helper function in engine.go that only splits on the last colon appearing after the last slash - Update all engine implementations (docker, podman, containerd, cri) to use the new parser - Add comprehensive test cases covering various image formats including registry with port, multi-level paths, and digest-based images Fixes: falcosecurity#1139 Signed-off-by: suzutan <6679870+suzutan@users.noreply.github.com>
c3f9c14 to
0936b5e
Compare
irozzo-1A
left a comment
There was a problem hiding this comment.
LGTM overall, just a nit
Rules files suggestions |
…arsing Improve parseImageRepoTag to properly handle digest-based image references by stripping the digest portion and returning an empty tag, preventing the digest from being incorrectly parsed as a tag. Signed-off-by: suzutan <6679870+suzutan@users.noreply.github.com>
260d098 to
01d4cc8
Compare
Rules files suggestions |
irozzo-1A
left a comment
There was a problem hiding this comment.
Some minor changes and it's ready to go. Thanks!
| expectedRepo: "registry.example.com:5000/foo/bar", | ||
| expectedTag: "", | ||
| }, | ||
| "Multi-level path with registry port and tag": { |
There was a problem hiding this comment.
https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests
refers to the namespace of the repository. MUST be either (a) the digest of the manifest or (b) a tag. The MUST NOT be in any other format. Throughout this document, MUST match the following regular expression:
[a-z0-9]+((.||__|-+)[a-z0-9]+)*(/[a-z0-9]+((.||__|-+)[a-z0-9]+))
I think you can have both digest and tag, from my tests with docker at least. The OCI spec says or, but I think it's an inclusive or. I would add a test to cover this scenario e.g.
| "Multi-level path with registry port and tag": { | |
| "Both tag and digest": { | |
| image: "registry.example.com:5000/foo/bar:latest@sha256:abc123", | |
| expectedRepo: "registry.example.com:5000/foo/bar", | |
| expectedTag: "latest", | |
| }, |
| // Split at the last colon | ||
| if digestRef { | ||
| return image[:lastColon], "" | ||
| } |
There was a problem hiding this comment.
Why don't we return the tag in case we have both tag and digest?
| // Split at the last colon | |
| if digestRef { | |
| return image[:lastColon], "" | |
| } |
| if at := strings.Index(image, "@"); at != -1 { | ||
| image = image[:at] | ||
| digestRef = true | ||
| } |
There was a problem hiding this comment.
See my previous comment:
| } | |
| if at := strings.Index(image, "@"); at != -1 { | |
| image = image[:at] | |
| } |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area plugins
What this PR does / why we need it:
Fixes container image parsing when the registry URL contains a port number.
Previously, the code used
strings.Split(image, ":")which incorrectly split on ALL colons, making it unable to distinguish between registry ports and image tags. For example:registry.example.com:5000/foo/bar:latestwould fail parsing (3 parts, fails len==2 check)registry.example.com:5000/foo/barwould incorrectly parseregistry.example.comas repo and5000/foo/baras tagThis PR introduces a new
parseImageRepoTagfunction that correctly parses image references by:The fix has been applied to all container runtime engines: Docker, Podman, containerd, and CRI.
Which issue(s) this PR fixes:
Fixes #1139
Special notes for your reviewer: