Add ArgsEscaped support for CRI #7135

kiashok · 2022-07-06T00:59:07Z

This PR addresses windows images that are broken when created using the "shell form" entrypoint/cmd in Dockerfile. Associated bug - #5067
The approach taken to fix this bug is as discussed by @kevpar here - #6479 (comment)

Docker uses non-OCI field called ArgsEscaped to address how the image 'ENTRYPOINT' and 'CMD' are interpreted on windows. This PR adds support for ArgsEscaped in containerd without changes to the OCI spec and also adds changes to handle cases when image ENTRYPOINT and CMD can be overriden by 'Command' and 'Args' specified in the podspec.

The fix in this PR does the following:

Reads the ArgsEscaped value from the image got from docker without making OCI spec changes (In file image.go)
Image ENTRYPOINT and CMD fields can be overriden by the container 'Command' and 'Args' fields specified through the podspec. But the ArgsEscaped field should really only affect the behavior of the image values, and not the overall evaluation of args for the container. Therefore, we need to evaluate if the first arg in our list is coming from image entrypoint/cmd and only if it is coming from the image, we should we treat the first arg as already escaped. (in file spec.go spec_windows.go)

Regression tests have also been added to this PR.

k8s-ci-robot · 2022-07-06T00:59:16Z

Hi @kiashok. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

jterry75 · 2022-07-07T17:35:10Z

I dont do any CRI plugin work anymore so I will defer to @kevpar / @dcantah here. But thank you for picking up this work. It would be nice to see CRI support for ArgsEscaped images working on Kube+CriContainerd

oci/spec_opts.go

pkg/cri/opts/spec_windows.go

pkg/cri/server/container_create_linux_test.go

pkg/cri/server/container_create_windows_test.go

pkg/cri/store/image/image.go

dcantah · 2022-08-19T19:16:54Z

I really think for full testing sanity we should host an image that is built with docker that would set ArgsEscaped, and then have a test in the CRI /integration suite that launches a container and verifies we're all good.

dcantah · 2022-08-19T19:18:05Z

I think the Project Checks are failing because you're using github.com/pkg/errors in a spot here and it was removed as a direct dependency

dcantah · 2022-08-19T19:20:13Z

PR description is great but please add some description to the commit message as well describing the need for this change and what it fixes

pkg/cri/opts/spec.go

mikebrow

Thank you! I see a lot of work here. IMO this change may best be done after the merge of opencontainers/image-spec#892 till then this could proto on a branch with that change merged, which would mean a lot less code necessary, wouldn't have to revert some of Maksym's recent changes that moved get spec to the client.. wouldn't need to unmarshal twice to get the extended (deprecated docker) field....

dcantah · 2022-08-19T20:57:50Z

I think the reason was that PR had been stalled for awhile and wasn't sure if it'd be done in time for 1.7. Would it be undesirable to use the double unmarshal to get the ArgsEscaped field in the meantime until we have that in imagespec, and we can do a follow-up change?

For the reverting Maksym's changes bit, which part of this change was that?

dcantah · 2022-08-24T20:40:48Z

@mikebrow pingaroo on the above. Question: given we don't vendor an exact tag of the spec here today already, am I right in assuming we could re-vendor image-spec from the latest in main once that change is merged? If it's highly preferred to wait for that to land we can try and drive that forward ASAP. If that doesn't seem to be making progress in time for 1.7, could we move forward with what's here for now and retrofit after?

mikebrow · 2022-08-31T23:05:54Z

@mikebrow pingaroo on the above. Question: given we don't vendor an exact tag of the spec here today already, am I right in assuming we could re-vendor image-spec from the latest in main once that change is merged? If it's highly preferred to wait for that to land we can try and drive that forward ASAP. If that doesn't seem to be making progress in time for 1.7, could we move forward with what's here for now and retrofit after?

nod.. but let's track the OCI pr for a bit... esp. with the linux args escaped news...

marosset · 2022-10-05T22:07:50Z

opencontainers/image-spec#892 merged :)

jterry75

Great start. Left some comments.

kiashok · 2022-11-23T16:56:19Z

Great start. Left some comments.

Hi @jterry75 , I addressed the comments. Could you please let me know if they look ok? Thanks a lot!

kiashok · 2023-01-17T19:30:16Z

Hi @dcantah , would you be able to take a second look at the changes when you have some time please? Thanks!

dcantah · 2023-01-17T19:32:17Z

@kiashok Yep! I'll try and get to this tomorrow evening

kiashok · 2023-01-17T20:35:53Z

@kiashok Yep! I'll try and get to this tomorrow evening

Thanks Danny!

dcantah · 2023-01-18T19:32:17Z

Today's lookin a little slammed.. 🫠 I'll definitely get to this sometime this week @kiashok (please ping if I don't)

kiashok · 2023-01-18T21:48:55Z

Today's lookin a little slammed.. 🫠 I'll definitely get to this sometime this week @kiashok (please ping if I don't)

Sure Danny no worries, thanks a lot! :)

integration/images/image_list.go

integration/windows_hostprocess_test.go

pkg/cri/server/container_create_windows_test.go

dcantah · 2023-01-25T21:20:04Z

@kiashok I'll try and get a final pass sometime this week. Thanks for looking at the feedback!

kiashok · 2023-01-26T18:06:16Z

@kiashok I'll try and get a final pass sometime this week. Thanks for looking at the feedback!

Thanks a lot for looking into this @dcantah!
Addressed the lint errors hit in the second commit

pkg/cri/opts/spec_windows_opts.go

pkg/cri/server/container_create_windows.go

This commit adds supports for the ArgsEscaped value for the image got from the dockerfile. It is used to evaluate and process the image entrypoint/cmd and container entrypoint/cmd options got from the podspec. Signed-off-by: Kirtana Ashok <[email protected]>

kiashok · 2023-02-09T19:04:03Z

@mxpv could you please take a look at the new changes and see if they look good?

Also, the CI failures seem to be unrelated to the change

pkg/cri/opts/spec_nonwindows.go

pkg/cri/sbserver/container_create.go

pkg/cri/opts/spec_windows.go

kiashok · 2023-03-02T20:39:38Z

Looks like I accidentally deleted the fork that had the branch with the changes in this PR. Addressed the comments that @dmcgowan left this morning and wanted to submit new changes and realized this. Since am unable to update this PR with the new changes, I closed this PR and created a new one here - #8198. Apologies for this!

k8s-ci-robot added the needs-ok-to-test label Jul 6, 2022

kiashok marked this pull request as draft July 7, 2022 16:48

kiashok marked this pull request as ready for review July 7, 2022 21:31

kiashok changed the title ~~Add ArgsEscaped support in upstream containerd~~ Add ArgsEscaped support for CRI Jul 21, 2022

kevpar self-assigned this Aug 3, 2022