Skip to content

Commit 9863254

Browse files
authored
Merge pull request containerd#81 from kevpar/argsescaped
Fix Windows containers for Docker images relying on ArgsEscaped
2 parents 22108df + dde14f3 commit 9863254

File tree

7 files changed

+147
-37
lines changed

7 files changed

+147
-37
lines changed

pkg/server/container_create.go

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ import (
2525
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
2626
"github.com/opencontainers/runtime-tools/validate"
2727
"github.com/pkg/errors"
28+
"github.com/sirupsen/logrus"
2829
"github.com/syndtr/gocapability/capability"
30+
"golang.org/x/sys/windows"
2931
runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
3032

3133
containerstore "github.com/containerd/cri/pkg/store/container"
@@ -39,26 +41,75 @@ func init() {
3941

4042
// setOCIProcessArgs sets process args. It returns error if the final arg list
4143
// is empty.
42-
func setOCIProcessArgs(g *generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error {
44+
func setOCIProcessArgs(g *generator, config *runtime.ContainerConfig, image *imagespec.Image) error {
4345
command, args := config.GetCommand(), config.GetArgs()
4446
// The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go
4547
// TODO(random-liu): Clearly define the commands overwrite behavior.
4648
if len(command) == 0 {
4749
// Copy array to avoid data race.
4850
if len(args) == 0 {
49-
args = append([]string{}, imageConfig.Cmd...)
51+
args = append([]string{}, image.Config.Cmd...)
5052
}
5153
if command == nil {
52-
command = append([]string{}, imageConfig.Entrypoint...)
54+
command = append([]string{}, image.Config.Entrypoint...)
5355
}
5456
}
5557
if len(command) == 0 && len(args) == 0 {
5658
return errors.New("no command specified")
5759
}
58-
g.SetProcessArgs(append(command, args...))
60+
var ignoreArgsEscaped bool
61+
if ignoreArgsEscapedAnno, ok := config.Annotations["microsoft.io/ignore-args-escaped"]; ok {
62+
ignoreArgsEscaped = ignoreArgsEscapedAnno == "true"
63+
}
64+
setProcessArgs(g, image.OS == "windows", image.Config.ArgsEscaped && !ignoreArgsEscaped, append(command, args...))
5965
return nil
6066
}
6167

68+
// setProcessArgs sets either g.Config.Process.CommandLine or g.Config.Process.Args.
69+
// This is forked from g.SetProcessArgs to add argsEscaped support.
70+
func setProcessArgs(g *generator, isWindows bool, argsEscaped bool, args []string) {
71+
logrus.WithFields(logrus.Fields{
72+
"isWindows": isWindows,
73+
"argsEscaped": argsEscaped,
74+
"args": args,
75+
}).Info("Setting process args on OCI spec")
76+
if g.Config == nil {
77+
g.Config = &runtimespec.Spec{}
78+
}
79+
if g.Config.Process == nil {
80+
g.Config.Process = &runtimespec.Process{}
81+
}
82+
83+
if isWindows && argsEscaped {
84+
// argsEscaped is used for Windows containers to indicate that the command line should be
85+
// used from args[0] without escaping. This case seems to mostly result from use of
86+
// shell-form ENTRYPOINT or CMD in the Dockerfile. argsEscaped is a non-standard OCI
87+
// extension but we are using it here to support Docker images that rely on it. In the
88+
// future we should move to some properly standardized support in upstream OCI.
89+
// This logic is taken from https://github.com/moby/moby/blob/24f173a003727611aa482a55b812e0e39c67be65/daemon/oci_windows.go#L244
90+
//
91+
// Note: The approach taken here causes ArgsEscaped to change how commands passed directly
92+
// via CRI are interpreted as well. However, this actually matches with Docker's behavior
93+
// regarding commands specified at container create time, and seems non-trivial to fix,
94+
// so going to leave this way for now.
95+
g.Config.Process.CommandLine = args[0]
96+
if len(args[1:]) > 0 {
97+
g.Config.Process.CommandLine += " " + escapeArgs(args[1:])
98+
}
99+
} else {
100+
g.Config.Process.Args = args
101+
}
102+
}
103+
104+
// escapeArgs makes a Windows-style escaped command line from a set of arguments
105+
func escapeArgs(args []string) string {
106+
escapedArgs := make([]string, len(args))
107+
for i, a := range args {
108+
escapedArgs[i] = windows.EscapeArg(a)
109+
}
110+
return strings.Join(escapedArgs, " ")
111+
}
112+
62113
// addImageEnvs adds environment variables from image config. It returns error if
63114
// an invalid environment variable is encountered.
64115
func addImageEnvs(g *generator, imageEnvs []string) error {

pkg/server/container_create_windows.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
160160
sandboxPlatform = "windows/amd64"
161161
}
162162

163-
spec, err := c.generateContainerSpec(id, sandboxID, sandboxPid, sandbox.NetNSPath, config, sandboxConfig, sandboxPlatform, &image.ImageSpec.Config)
163+
spec, err := c.generateContainerSpec(id, sandboxID, sandboxPid, sandbox.NetNSPath, config, sandboxConfig, sandboxPlatform, &image.ImageSpec)
164164
if err != nil {
165165
return nil, errors.Wrapf(err, "failed to generate container %q spec", id)
166166
}
@@ -253,7 +253,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
253253
}
254254

255255
func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxPid uint32, netnsPath string, config *runtime.ContainerConfig,
256-
sandboxConfig *runtime.PodSandboxConfig, sandboxPlatform string, imageConfig *imagespec.ImageConfig) (*runtimespec.Spec, error) {
256+
sandboxConfig *runtime.PodSandboxConfig, sandboxPlatform string, image *imagespec.Image) (*runtimespec.Spec, error) {
257257
// Creates a spec Generator with the default spec.
258258
ctx := ctrdutil.NamespacedContext()
259259
spec, err := oci.GenerateSpecWithPlatform(ctx, nil, sandboxPlatform, &containers.Container{ID: id})
@@ -267,14 +267,14 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
267267
}
268268
g := newSpecGenerator(spec)
269269

270-
if err := setOCIProcessArgs(&g, config, imageConfig); err != nil {
270+
if err := setOCIProcessArgs(&g, config, image); err != nil {
271271
return nil, err
272272
}
273273

274274
if config.GetWorkingDir() != "" {
275275
g.SetProcessCwd(config.GetWorkingDir())
276-
} else if imageConfig.WorkingDir != "" {
277-
g.SetProcessCwd(imageConfig.WorkingDir)
276+
} else if image.Config.WorkingDir != "" {
277+
g.SetProcessCwd(image.Config.WorkingDir)
278278
}
279279

280280
g.SetProcessTerminal(config.GetTty())
@@ -287,7 +287,7 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
287287

288288
// Apply envs from image config first, so that envs from container config
289289
// can override them.
290-
if err := addImageEnvs(&g, imageConfig.Env); err != nil {
290+
if err := addImageEnvs(&g, image.Config.Env); err != nil {
291291
return nil, err
292292
}
293293
for _, e := range config.GetEnvs() {
@@ -486,7 +486,7 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
486486
if userstr == "" {
487487
// Lastly, since no user override was passed via CRI try to set via
488488
// OCI Image
489-
userstr = imageConfig.User
489+
userstr = image.Config.User
490490
}
491491
if userstr != "" {
492492
g.AddAnnotation("io.microsoft.lcow.userstr", userstr)
@@ -524,7 +524,7 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
524524
g.SetWindowsResourcesMemoryLimit(uint64(resources.GetMemoryLimitInBytes()))
525525
}
526526

527-
username := imageConfig.User
527+
username := image.Config.User
528528
securityContext := config.GetWindows().GetSecurityContext()
529529
if securityContext != nil {
530530
runAsUser := securityContext.GetRunAsUsername()

vendor.conf

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ github.com/Microsoft/hcsshim 936eeeb286fd1197f107acfc4c3c82e4a9afc2c8
3838
github.com/modern-go/concurrent 1.0.3
3939
github.com/modern-go/reflect2 1.0.1
4040
github.com/opencontainers/go-digest c9281466c8b2f606084ac71339773efd177436e7
41-
github.com/opencontainers/image-spec v1.0.1
41+
github.com/opencontainers/image-spec deb02d24daef68ac4a88d8f9883ba03b75bdc187 https://github.com/kevpar/image-spec.git # fork
4242
github.com/opencontainers/runc 12f6a991201fdb8f82579582d5e00e28fba06d0a
43-
github.com/opencontainers/runtime-spec eba862dc2470385a233c7507392675cbeadf7353
43+
github.com/opencontainers/runtime-spec v1.0.2
4444
github.com/opencontainers/runtime-tools 1d69bd0f9c39677d0630e50664fbc3154ae61b88
4545
github.com/opencontainers/selinux b6fa367ed7f534f9ba25391cc2d467085dbb445a
4646
github.com/pkg/errors v0.9.1

vendor/github.com/opencontainers/image-spec/specs-go/v1/config.go

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

vendor/github.com/opencontainers/runtime-spec/README.md

Lines changed: 4 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)