prevent ctr from creating tags with forbidden characters #9027
Conversation
|
/cc @dmcgowan There is also an alternate solution for this that I can think of as described here #8844 (comment) |
|
Hi @akhilerm. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cc @samuelkarp |
|
I think my preference is the alternate approach you outlined in #8844 (comment); validating in the service would prevent any client from creating an invalid tag rather than just ctr. But @dmcgowan might have a different opinion since that's arguably a breaking change. |
|
/ok-to-test |
|
@akhilerm: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
We have intentionally not added any sort of image name validation in the image service, this should be done by clients or components interacting with the registry. The reason being that the definition of a valid tag is highly contextual. We don't need to block users from creating a tag with What is the use case trying to be prevented here though? |
|
Also related (have not looked at the code in this PR yet, but the PR description caught my eye); |
Isnt this defined as per the OCI spec?
for that we will have to do the tagging via transfer API right, which is not enabled by default.
The images tagged by ctr with invalid tags cannot be seen by CRI as those tags are omitted @SergeyKanzhelev might be able to provide more use cases for it. |
It is defined for image distribution and for the layout annotations, maybe elsewhere as well. We always aim to support the super set for local image management though and our data model and API really has no limitation on what the image name could be. The expectation is that image name is enforced via build tooling, registry limitations, and what is supported via import. Tag was an operation we initially didn't allow in
We could switch the default for tagging to use the transfer API
Maybe they don't even need to be omitted. CRI does not support push anyway. Any attempt to pull or refresh the image from a registry will not succeed though. |
|
I am LGTM on this if we add support for |
|
Will update it. Better than |
check if the target tag that is to be created using ctr image tag is valid and does not contain any forbidden characters. Signed-off-by: Akhil Mohan <makhil@vmware.com>
58eeffd to
9311b38
Compare
cmd/ctr/commands/images/tag.go
Outdated
|
|
||
| if !context.BoolT("local") { | ||
| for _, targetRef := range context.Args()[1:] { | ||
| if _, err := reference.ParseAnyReference(targetRef); !context.Bool("skip-reference-check") && err != nil { |
There was a problem hiding this comment.
@dmcgowan Added a new flag, to skip the strict reference name check.
But IMHO, doesn't it make the code less clean?
Also, should we have this as a global flag, so that other commands can also have the flag to skip the strict checks
|
Should we also add a proper release note as this is a breaking change for the |
|
Can I get another round of review on the PR? Have addressed all the comments. |
Signed-off-by: Akhil Mohan <makhil@vmware.com>
9311b38 to
4b59d67
Compare
|
@samuelkarp Can you take a look once again. I have made the changes as suggested. |
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
check if the target tag that is to be created using ctr image tag is valid and does not contain any forbidden characters.
A new flag
--skip-reference-checkhas been added so that this check for forbidden characters can be skipped.Fixes #8844