Add ArgsEscaped support for CRI #8198
Conversation
|
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 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. |
|
Looks like I accidentally deleted the fork that had the branch with the changes in PR #7135 . I was unable to add the new changes to the old PR #7135 , so I closed it and created this new one. Apologies for this! @dmcgowan @dcantah The only changes this PR in addition to PR #7135 are the following:
func WithProcessCommandLineOrArgsForWindows(config *runtime.ContainerConfig, image *imagespec.ImageConfig) oci.SpecOpts { Could you please take a look at this PR instead for this work? Thanks a lot! |
|
@kiashok Can you look at the linter issues? |
|
@kevpar Added you as a reviewer as you were on the old one, lets try and land this for 1.7 |
The linter run wasn't loading for me, but the annotations did; that seems to be what the CI is whining about |
kiashok
force-pushed
the
argsEscapedSupportInCri
branch
from
6db0bc0 to
8675e91
Compare
@dcantah took care of this and the tests are passing |
| // Ctr entrypoint ctr cmd image entrypoint image cmd firstArgFromImg | ||
| // -------------------------------------------------------------------------------- | ||
| // nil nil exists nil true | ||
| // nil nil nil exists true |
There was a problem hiding this comment.
(spaces/tabs seem to be screwed up)
There was a problem hiding this comment.
Shows up alright on vscode for me. Not sure whats happening in the PR
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
force-pushed
the
argsEscapedSupportInCri
branch
from
8675e91 to
8137e41
Compare
|
@kiashok would this commit address the issue outlined here? coder/coder#6657 |
The PR containerd#8198 fixed this for CRI but missed clearing the commandline in the forked SB server. This simply adds that back in Signed-off-by: James Sturtevant <[email protected]>
The PR containerd#8198 fixed this for CRI but missed clearing the commandline in the forked SB server. This simply adds that back in Signed-off-by: James Sturtevant <[email protected]>
6 participants
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.