Add ArgsEscaped field to image config

[jterry75]

This change officially adds ArgsEscaped to the image config. This field has already been used by Docker for several years, so adding it here allows images that depend on its behavior to work with other runtimes.

For certain Windows images created via Docker they may contain ArgsEscaped==true if the ENTRYPOINT or CMD is in the shell form and contains spaces, path characters, etc.

An example is the following:

FROM mcr.microsoft.com/windows/servercore:ltsc2019

SHELL ["powershell", "-Command"]
ENTRYPOINT "C:\Path To\MyExe.exe" -arg1 "-arg2 value2"

Will be encoded by Docker as:

"Config": {
  ...
  "ArgsEscaped": true,
  ...
  "Entrypoint": [
    "powershell.exe -Command \"C:\\My Data\\MyExe.exe\" -arg1 \"-arg2 value2\""
  ],
  ...
}

You can see that Entrypoint becomes a single element array with the entire argument list already escaped. To avoid a double escape problem Windows supports passing via OCI as a complete CommandLine.

@kevpar - Please let me know if you do not want me to carry this as your change or don't wan't your signature on the commit. I wanted to give you credit since you originally opened #829.

[jterry75]

Ref: (containerd/containerd#6479)

[sudo-bmitch]

My thoughts most closely align with kevpar's comment:

I like the idea of adding ArgsEscaped to OCI and immediately marking it as deprecated, and also figuring out the right long term solution (such as CommandLine).

I'd like to see us document what this is when people see it in an image, without actually recommending that anyone do this. Perhaps change the phrasing to indicate it's here only for historical context.

[katiewasnothere]

@jterry75 @kevpar what is the status with this?

[jterry75]

@katiewasnothere - I think its in limbo... Given the feedback we have I think I'll update the PR to signify exactly what was said. "ArgsEscaped" is a legacy compatibility with Docker and describe what it means. But mark it in the docs as deprecated and see if we can carry it from there. Sound good?

[katiewasnothere]

@jterry75 Sounds good! @kevpar, @dcantah, and I discussed this and we're fine with the change so long as it's marked deprecated, as you mentioned.

[jterry75]

Sweeeeeeeeeeet

[kevpar]

@jterry75 This looks good to me. Maybe we could try to clarify the exact behavior of what ArgsEscaped implies in Moby, but as we discussed it is quite complicated (and we don't want anyone to take a dependency on it if they don't have to). So I think this is fine. Maybe we should just add a note like "Exact behavior of ArgsEscaped is complex and subject to implementation details in Moby project"?

[jterry75]

@jterry75 This looks good to me. Maybe we could try to clarify the exact behavior of what ArgsEscaped implies in Moby, but as we discussed it is quite complicated (and we don't want anyone to take a dependency on it if they don't have to). So I think this is fine. Maybe we should just add a note like "Exact behavior of ArgsEscaped is complex and subject to implementation details in Moby project"?

Do you want me to write that in the doc of the ArgsEscaped param or in the compat matrix part?

[kevpar]

@jterry75 This looks good to me. Maybe we could try to clarify the exact behavior of what ArgsEscaped implies in Moby, but as we discussed it is quite complicated (and we don't want anyone to take a dependency on it if they don't have to). So I think this is fine. Maybe we should just add a note like "Exact behavior of ArgsEscaped is complex and subject to implementation details in Moby project"?

Do you want me to write that in the doc of the ArgsEscaped param or in the compat matrix part?

Probably the doc.

[jterry75]

@kevpar - Done. Ty

[kevpar]

LGTM (not a maintainer). Thanks!

[dcantah]

LGTM (also not a maintainer 🙂)

[sudo-bmitch]

Thanks for updating! I don't have a Windows system available to build an image with this. Is there a public example (repository:tag) I can use to test against some code?

[kiashok]

Thanks for updating! I don't have a Windows system available to build an image with this. Is there a public example (repository:tag) I can use to test against some code?

I created a simple repro on nanoserver which should make use of the argsEscaped field at cplatpublic.azurecr.io/args-escaped-test-image-ns:latest

[sudo-bmitch]

Just one nit from me. I debated if this should be deprecated in the config.go following their defined method (https://go.dev/blog/godoc), but knowing how this would trigger build failures for anyone supporting legacy features, I'm leaning against that.

[sudo-bmitch]

Nit: one sentence per line.

[jterry75]

Done

[jterry75]

@kevpar / @dmcgowan - Any ideas? We certainly never handled ArgsEscaped for Linux case. Is there a type of Linux image that we need to be concerned with here? Also I dont see that it did anything in the example above. Yes its true but Cmd is not escaped. Seems like a image build bug honestly but I dont know enough about the Linux flow here.

[kevpar]

This is interesting. I'm not sure how ArgsEscaped could be set on a Linux image. Looking at the moby code, it appears the ArgsEscaped value should come from resolveCmdLine, which has differing implementations for unix and windows. You can see that the unix implementation never returns true for the second return value, which is what gets used for ArgsEscaped. I wonder if there is older or other tooling that could set this field inadvertently.

As a test, I tried building a simple image on Linux with the following Dockerfile:

FROM ubuntu:latest
ENTRYPOINT "echo foo"

This produced an image with ArgsEscaped not set. However, a comparable image built on Windows did have it set. This seems in line with expectations that this value is not normally set for Linux images.

Regardless of if the value could be set on a Linux image, from what I can see the only place ArgsEscaped is used to determine behavior at runtime is here, which is a Windows-only file. So I think it is safe to treat ArgsEscaped as Windows-only.

[sudo-bmitch]

The images I'm building are from buildkit (and buildx), where it appears to be set on every image that defines a CMD:

https://github.com/moby/buildkit/blob/cdc17fe2beb46178e34eecd1368740b004ad6592/frontend/dockerfile/dockerfile2llb/convert.go#L1266

I don't think buildkit has support for windows yet (and definitely didn't when this was added). Pinging @tonistiigi in case there's any other use cases we should keep in mind.

[sudo-bmitch]

Don't spend any effort debugging the CI failures, they should be fixed with a rebase.

[tonistiigi]

@sudo-bmitch You can create windows images in buildkit, if not via wcow then with cross-compilation. If the analysis shows that no runtime uses it on linux we can drop it based on the platform check.

[sudo-bmitch]

@sudo-bmitch You can create windows images in buildkit, if not via wcow then with cross-compilation. If the analysis shows that no runtime uses it on linux we can drop it based on the platform check.

@tonistiigi my bad, I was confusing with the native windows build that was holding buildkit back from being the default builder for so long, but that doesn't apply here. As long as you don't see a need for it on Linux, I'm good with getting this merged. We're not so concerned with changing buildkit as we are that we're getting the spec right, and setting this on Linux is undefined behavior that no one should care about.

@jterry75 if you cleanup the nit and rebase, I'll add my LGTM and see if we can find a second maintainer to get this merged.

[jterry75]

Sorry folks didnt realize there was a NIT here. Done!

[AkihiroSuda]

This link doesn't seem needed

[tianon]

In the context of my making moby/buildkit#4723, I'm actually wondering why this is marked as deprecated? Windows does not use argc/argv (that behavior is implemented per-program) as the CreateProcessA system call over there actually takes a single string (https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa) and that's what binaries receive. IIRC there's a common "libc" implementation that parses and splits that per-application, but some like cmd.exe have legitimate reasons for not splitting/parsing that in the normal way.

All this to say, single-string CommandLine is actually how Windows works (and that doesn't seem likely to change any time soon), so it feels wrong for the spec to only acknowledge that in a deprecated field? If I were designing this today, I'd probably make it a special case inside Cmd or something intead (["WINDOWS-PRE-ESCAPED-COMMANDLINE", "cmd /C /S \"echo foo bar \""]), but since that ship has sailed, it feels a bit too reductive to document this as deprecated and discourage its use and I must be missing something.

[kevpar]

@tianon it was marked deprecated primarily because the exact way ArgsEscaped is interpreted by Docker has some weird edge cases so it is hard to standardize. IIRC, an example of this is that if ArgsEscaped is set on the image, even the args specified at the container level will be interpreted as a single string.

I agree that there should be proper recognition for Windows command line strings in the image config. My preference would probably be some new fields like EntrypointString and CmdString.

[tianon]

I have to admit I don't really understand the "logic is complicated" argument 😅

The logic for setting it correctly during build sure is, but the runtime logic is pretty straightforward:

args := append(append([]string{}, Entrypoint...), Cmd...)
escaped := []string{}
if ArgsEscaped {
	escaped = append(escaped, args[0])
	args = args[1:]
}
for _, a := range args {
	escaped = append(escaped, golang.org/x/sys/windows.EscapeArg(a)
}
CommandLine := strings.Join(escaped, " ")

(yes, most of this could be optimized with make(), etc since the slice sizes are all known in advance, but this gets the general point across -- combine entrypoint+cmd, escape everything except the first argument if argsescaped)

[tianon]

From what I can tell in containerd/containerd#6479 (comment), it's only complicated because there's a wrapper in use that turns Args into CommandLine, but has to also accept CommandLine as-is (as evidenced by that code even working), so it'd be much more straightforward if it didn't rely on the wrapper to set CommandLine from Args (and did the escaping directly in that block in both cases 😅)