diff --git a/integration/images/image_list.go b/integration/images/image_list.go index 5be031d1f..ccdd373b7 100644 --- a/integration/images/image_list.go +++ b/integration/images/image_list.go @@ -36,6 +36,7 @@ type ImageList struct { ResourceConsumer string VolumeCopyUp string VolumeOwnership string + ArgsEscaped string } var ( @@ -53,6 +54,7 @@ func initImages(imageListFile string) { ResourceConsumer: "registry.k8s.io/e2e-test-images/resource-consumer:1.10", VolumeCopyUp: "ghcr.io/containerd/volume-copy-up:2.1", VolumeOwnership: "ghcr.io/containerd/volume-ownership:2.1", + ArgsEscaped: "cplatpublic.azurecr.io/args-escaped-test-image-ns:latest", } if imageListFile != "" { @@ -88,6 +90,8 @@ const ( VolumeCopyUp // VolumeOwnership image VolumeOwnership + // Test image for ArgsEscaped windows bug + ArgsEscaped ) func initImageMap(imageList ImageList) map[int]string { @@ -98,6 +102,7 @@ func initImageMap(imageList ImageList) map[int]string { images[ResourceConsumer] = imageList.ResourceConsumer images[VolumeCopyUp] = imageList.VolumeCopyUp images[VolumeOwnership] = imageList.VolumeOwnership + images[ArgsEscaped] = imageList.ArgsEscaped return images } diff --git a/integration/images/image_list.sample.toml b/integration/images/image_list.sample.toml index ecf126a62..b317d43a5 100644 --- a/integration/images/image_list.sample.toml +++ b/integration/images/image_list.sample.toml @@ -3,3 +3,4 @@ busybox = "docker.io/library/busybox:latest" pause = "registry.k8s.io/pause:3.7" VolumeCopyUp = "ghcr.io/containerd/volume-copy-up:2.1" VolumeOwnership = "ghcr.io/containerd/volume-ownership:2.1" +ArgsEscaped = "cplatpublic.azurecr.io/args-escaped-test-image-ns:latest" diff --git a/integration/windows_hostprocess_test.go b/integration/windows_hostprocess_test.go index f47a49003..7176d163f 100644 --- a/integration/windows_hostprocess_test.go +++ b/integration/windows_hostprocess_test.go @@ -21,12 +21,15 @@ package integration import ( "fmt" "os" + "strconv" "testing" "time" + "github.com/Microsoft/hcsshim/osversion" "github.com/containerd/containerd/integration/images" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/windows/registry" runtime "k8s.io/cri-api/pkg/apis/runtime/v1" v1 "k8s.io/cri-api/pkg/apis/runtime/v1" ) @@ -123,3 +126,65 @@ func runHostProcess(t *testing.T, expectErr bool, image string, action hpcAction action(t, cn, containerConfig) } + +func startAndTestContainer(t *testing.T, sb string, sbConfig *runtime.PodSandboxConfig, cnConfig *runtime.ContainerConfig) { + t.Log("Create the container") + cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig) + require.NoError(t, err) + t.Log("Start the container") + require.NoError(t, runtimeService.StartContainer(cn)) + + t.Log("Stop the container") + require.NoError(t, runtimeService.StopContainer(cn, 0)) + t.Log("Remove the container") + require.NoError(t, runtimeService.RemoveContainer(cn)) +} + +func TestArgsEscapedImagesOnWindows(t *testing.T) { + // the ArgsEscaped test image is based on nanoserver:ltsc2022, so ensure we run on the correct OS version + k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion`, registry.QUERY_VALUE) + if err != nil { + t.Skip("Error in getting OS version") + } + defer k.Close() + + b, _, _ := k.GetStringValue("CurrentBuild") + buildNum, _ := strconv.Atoi(b) + if buildNum < osversion.V21H2Server { + t.Skip() + } + + containerName := "test-container" + testImage := images.Get(images.ArgsEscaped) + sbConfig := &runtime.PodSandboxConfig{ + Metadata: &runtime.PodSandboxMetadata{ + Name: "sandbox", + Namespace: testImage, + }, + Windows: &runtime.WindowsPodSandboxConfig{}, + } + sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, runtimeService.StopPodSandbox(sb)) + assert.NoError(t, runtimeService.RemovePodSandbox(sb)) + }) + + EnsureImageExists(t, testImage) + + cnConfigWithCtrCmd := ContainerConfig( + containerName, + testImage, + WithCommand("ping", "-t", "127.0.0.1"), + localSystemUsername, + ) + + cnConfigNoCtrCmd := ContainerConfig( + containerName, + testImage, + localSystemUsername, + ) + + startAndTestContainer(t, sb, sbConfig, cnConfigWithCtrCmd) + startAndTestContainer(t, sb, sbConfig, cnConfigNoCtrCmd) +} diff --git a/oci/spec_opts_windows.go b/oci/spec_opts_windows.go index 1a6cd942e..7624daf11 100644 --- a/oci/spec_opts_windows.go +++ b/oci/spec_opts_windows.go @@ -35,6 +35,16 @@ func escapeAndCombineArgs(args []string) string { return strings.Join(escaped, " ") } +// WithProcessCommandLine replaces the command line on the generated spec +func WithProcessCommandLine(cmdLine string) SpecOpts { + return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + setProcess(s) + s.Process.Args = nil + s.Process.CommandLine = cmdLine + return nil + } +} + // WithHostDevices adds all the hosts device nodes to the container's spec // // Not supported on windows diff --git a/pkg/cri/opts/spec_nonwindows.go b/pkg/cri/opts/spec_nonwindows.go new file mode 100644 index 000000000..4759711d3 --- /dev/null +++ b/pkg/cri/opts/spec_nonwindows.go @@ -0,0 +1,36 @@ +//go:build !windows + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package opts + +import ( + "context" + + "github.com/containerd/containerd/containers" + "github.com/containerd/containerd/errdefs" + "github.com/containerd/containerd/oci" + imagespec "github.com/opencontainers/image-spec/specs-go/v1" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" +) + +func WithProcessCommandLineOrArgsForWindows(config *runtime.ContainerConfig, image *imagespec.ImageConfig) oci.SpecOpts { + return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) { + return errdefs.ErrNotImplemented + } +} diff --git a/pkg/cri/opts/spec_windows.go b/pkg/cri/opts/spec_windows.go new file mode 100644 index 000000000..0964084ca --- /dev/null +++ b/pkg/cri/opts/spec_windows.go @@ -0,0 +1,121 @@ +//go:build windows + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package opts + +import ( + "context" + "errors" + "strings" + + "github.com/containerd/containerd/containers" + "github.com/containerd/containerd/oci" + imagespec "github.com/opencontainers/image-spec/specs-go/v1" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" + "golang.org/x/sys/windows" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" +) + +func escapeAndCombineArgsWindows(args []string) string { + escaped := make([]string, len(args)) + for i, a := range args { + escaped[i] = windows.EscapeArg(a) + } + return strings.Join(escaped, " ") +} + +// WithProcessCommandLineOrArgsForWindows sets the process command line or process args on the spec based on the image +// and runtime config +// If image.ArgsEscaped field is set, this function sets the process command line and if not, it sets the +// process args field +func WithProcessCommandLineOrArgsForWindows(config *runtime.ContainerConfig, image *imagespec.ImageConfig) oci.SpecOpts { + if image.ArgsEscaped { + return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) { + // firstArgFromImg is a flag that is returned to indicate that the first arg in the slice comes from either the + // image Entrypoint or Cmd. If the first arg instead comes from the container config (e.g. overriding the image values), + // it should be false. This is done to support the non-OCI ArgsEscaped field that Docker used to determine how the image + // entrypoint and cmd should be interpreted. + // + args, firstArgFromImg, err := getArgs(image.Entrypoint, image.Cmd, config.GetCommand(), config.GetArgs()) + if err != nil { + return err + } + + var cmdLine string + if image.ArgsEscaped && firstArgFromImg { + cmdLine = args[0] + if len(args) > 1 { + cmdLine += " " + escapeAndCombineArgsWindows(args[1:]) + } + } else { + cmdLine = escapeAndCombineArgsWindows(args) + } + + return oci.WithProcessCommandLine(cmdLine)(ctx, client, c, s) + } + } + // if ArgsEscaped is not set + return func(ctx context.Context, client oci.Client, c *containers.Container, s *runtimespec.Spec) (err error) { + args, _, err := getArgs(image.Entrypoint, image.Cmd, config.GetCommand(), config.GetArgs()) + if err != nil { + return err + } + return oci.WithProcessArgs(args...)(ctx, client, c, s) + } +} + +// getArgs is used to evaluate the overall args for the container by taking into account the image command and entrypoints +// along with the container command and entrypoints specified through the podspec if any +func getArgs(imgEntrypoint []string, imgCmd []string, ctrEntrypoint []string, ctrCmd []string) ([]string, bool, error) { + //nolint:dupword + // firstArgFromImg is a flag that is returned to indicate that the first arg in the slice comes from either the image + // Entrypoint or Cmd. If the first arg instead comes from the container config (e.g. overriding the image values), + // it should be false. + // Essentially this means firstArgFromImg should be true iff: + // Ctr entrypoint ctr cmd image entrypoint image cmd firstArgFromImg + // -------------------------------------------------------------------------------- + // nil nil exists nil true + // nil nil nil exists true + + // This is needed to support the non-OCI ArgsEscaped field used by Docker. ArgsEscaped is used for + // Windows images to indicate that the command has already been escaped and should be + // used directly as the command line. + var firstArgFromImg bool + entrypoint, cmd := ctrEntrypoint, ctrCmd + // The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go + // TODO(random-liu): Clearly define the commands overwrite behavior. + if len(entrypoint) == 0 { + // Copy array to avoid data race. + if len(cmd) == 0 { + cmd = append([]string{}, imgCmd...) + if len(imgCmd) > 0 { + firstArgFromImg = true + } + } + if entrypoint == nil { + entrypoint = append([]string{}, imgEntrypoint...) + if len(imgEntrypoint) > 0 || len(ctrCmd) == 0 { + firstArgFromImg = true + } + } + } + if len(entrypoint) == 0 && len(cmd) == 0 { + return nil, false, errors.New("no command specified") + } + return append(entrypoint, cmd...), firstArgFromImg, nil +} diff --git a/pkg/cri/opts/spec_windows_opts.go b/pkg/cri/opts/spec_windows_opts.go index b42a6d582..b613c9886 100644 --- a/pkg/cri/opts/spec_windows_opts.go +++ b/pkg/cri/opts/spec_windows_opts.go @@ -24,12 +24,11 @@ import ( "sort" "strings" - runtimespec "github.com/opencontainers/runtime-spec/specs-go" - runtime "k8s.io/cri-api/pkg/apis/runtime/v1" - "github.com/containerd/containerd/containers" "github.com/containerd/containerd/oci" osinterface "github.com/containerd/containerd/pkg/os" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" ) // namedPipePath returns true if the given path is to a named pipe. diff --git a/pkg/cri/sbserver/container_create.go b/pkg/cri/sbserver/container_create.go index 35b2f703e..84c159733 100644 --- a/pkg/cri/sbserver/container_create.go +++ b/pkg/cri/sbserver/container_create.go @@ -740,9 +740,8 @@ func (c *criService) buildWindowsSpec( extraMounts []*runtime.Mount, ociRuntime criconfig.Runtime, ) (_ []oci.SpecOpts, retErr error) { - specOpts := []oci.SpecOpts{ - customopts.WithProcessArgs(config, imageConfig), - } + var specOpts []oci.SpecOpts + specOpts = append(specOpts, customopts.WithProcessCommandLineOrArgsForWindows(config, imageConfig)) // All containers in a pod need to have HostProcess set if it was set on the pod, // and vice versa no containers in the pod can be HostProcess if the pods spec diff --git a/pkg/cri/server/container_create_windows.go b/pkg/cri/server/container_create_windows.go index c5ba138c4..6b3a74c78 100644 --- a/pkg/cri/server/container_create_windows.go +++ b/pkg/cri/server/container_create_windows.go @@ -51,9 +51,8 @@ func (c *criService) containerSpec( extraMounts []*runtime.Mount, ociRuntime config.Runtime, ) (*runtimespec.Spec, error) { - specOpts := []oci.SpecOpts{ - customopts.WithProcessArgs(config, imageConfig), - } + var specOpts []oci.SpecOpts + specOpts = append(specOpts, customopts.WithProcessCommandLineOrArgsForWindows(config, imageConfig)) // All containers in a pod need to have HostProcess set if it was set on the pod, // and vice versa no containers in the pod can be HostProcess if the pods spec diff --git a/pkg/cri/server/container_create_windows_test.go b/pkg/cri/server/container_create_windows_test.go index 684d45bf5..7f8578b8f 100644 --- a/pkg/cri/server/container_create_windows_test.go +++ b/pkg/cri/server/container_create_windows_test.go @@ -22,12 +22,27 @@ import ( imagespec "github.com/opencontainers/image-spec/specs-go/v1" runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" runtime "k8s.io/cri-api/pkg/apis/runtime/v1" "github.com/containerd/containerd/pkg/cri/annotations" "github.com/containerd/containerd/pkg/cri/config" ) +func getSandboxConfig() *runtime.PodSandboxConfig { + return &runtime.PodSandboxConfig{ + Metadata: &runtime.PodSandboxMetadata{ + Name: "test-sandbox-name", + Uid: "test-sandbox-uid", + Namespace: "test-sandbox-ns", + Attempt: 2, + }, + Windows: &runtime.WindowsPodSandboxConfig{}, + Hostname: "test-hostname", + Annotations: map[string]string{"c": "d"}, + } +} + func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandboxConfig, *imagespec.ImageConfig, func(*testing.T, string, string, uint32, *runtimespec.Spec)) { config := &runtime.ContainerConfig{ @@ -76,17 +91,7 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox }, }, } - sandboxConfig := &runtime.PodSandboxConfig{ - Metadata: &runtime.PodSandboxMetadata{ - Name: "test-sandbox-name", - Uid: "test-sandbox-uid", - Namespace: "test-sandbox-ns", - Attempt: 2, - }, - Windows: &runtime.WindowsPodSandboxConfig{}, - Hostname: "test-hostname", - Annotations: map[string]string{"c": "d"}, - } + sandboxConfig := getSandboxConfig() imageConfig := &imagespec.ImageConfig{ Env: []string{"ik1=iv1", "ik2=iv2", "ik3=iv3=iv3bis", "ik4=iv4=iv4bis=boop"}, Entrypoint: []string{"/entrypoint"}, @@ -248,3 +253,104 @@ func TestHostProcessRequirements(t *testing.T) { }) } } + +func TestEntrypointAndCmdForArgsEscaped(t *testing.T) { + testID := "test-id" + testSandboxID := "sandbox-id" + testContainerName := "container-name" + testPid := uint32(1234) + nsPath := "test-ns" + c := newTestCRIService() + + for name, test := range map[string]struct { + imgEntrypoint []string + imgCmd []string + command []string + args []string + expectedArgs []string + expectedCommandLine string + ArgsEscaped bool + }{ + // override image entrypoint and cmd in shell form with container args and verify expected runtime spec + "TestShellFormImgEntrypointCmdWithCtrArgs": { + imgEntrypoint: []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`}, + imgCmd: []string{`cmd -args "hello world"`}, + command: nil, + args: []string{`cmd -args "additional args"`}, + expectedArgs: nil, + expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value" "cmd -args \"additional args\""`, + ArgsEscaped: true, + }, + // check image entrypoint and cmd in shell form without overriding with container command and args and verify expected runtime spec + "TestShellFormImgEntrypointCmdWithoutCtrArgs": { + imgEntrypoint: []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`}, + imgCmd: []string{`cmd -args "hello world"`}, + command: nil, + args: nil, + expectedArgs: nil, + expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "test value" "cmd -args \"hello world\""`, + ArgsEscaped: true, + }, + // override image entrypoint and cmd by container command and args in shell form and verify expected runtime spec + "TestShellFormImgEntrypointCmdWithCtrEntrypointAndArgs": { + imgEntrypoint: []string{`"C:\My Folder\MyProcess.exe" -arg1 "test value"`}, + imgCmd: []string{`cmd -args "hello world"`}, + command: []string{`C:\My Folder\MyProcess.exe`, "-arg1", "additional test value"}, + args: []string{"cmd", "-args", "additional args"}, + expectedArgs: nil, + expectedCommandLine: `"C:\My Folder\MyProcess.exe" -arg1 "additional test value" cmd -args "additional args"`, + ArgsEscaped: true, + }, + // override image cmd by container args in exec form and verify expected runtime spec + "TestExecFormImgEntrypointCmdWithCtrArgs": { + imgEntrypoint: []string{`C:\My Folder\MyProcess.exe`, "-arg1", "test value"}, + imgCmd: []string{"cmd", "-args", "hello world"}, + command: nil, + args: []string{"additional", "args"}, + expectedArgs: []string{`C:\My Folder\MyProcess.exe`, "-arg1", "test value", "additional", "args"}, + expectedCommandLine: "", + ArgsEscaped: false, + }, + // check image entrypoint and cmd in exec form without overriding with container command and args and verify expected runtime spec + "TestExecFormImgEntrypointCmdWithoutCtrArgs": { + imgEntrypoint: []string{`C:\My Folder\MyProcess.exe`, "-arg1", "test value"}, + imgCmd: []string{"cmd", "-args", "hello world"}, + command: nil, + args: nil, + expectedArgs: []string{`C:\My Folder\MyProcess.exe`, "-arg1", "test value", "cmd", "-args", "hello world"}, + expectedCommandLine: "", + ArgsEscaped: false, + }, + } { + t.Run(name, func(t *testing.T) { + imageConfig := &imagespec.ImageConfig{ + Entrypoint: test.imgEntrypoint, + Cmd: test.imgCmd, + ArgsEscaped: test.ArgsEscaped, + } + sandboxConfig := getSandboxConfig() + containerConfig := &runtime.ContainerConfig{ + Metadata: &runtime.ContainerMetadata{ + Name: "test-name", + Attempt: 1, + }, + Image: &runtime.ImageSpec{ + Image: testImageName, + }, + Command: test.command, + Args: test.args, + Windows: &runtime.WindowsContainerConfig{}, + } + runtimeSpec, err := c.containerSpec(testID, testSandboxID, testPid, nsPath, testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, config.Runtime{}) + assert.NoError(t, err) + assert.NotNil(t, runtimeSpec) + + // check the runtime spec for expected commandline and args + actualCommandLine := runtimeSpec.Process.CommandLine + actualArgs := runtimeSpec.Process.Args + + require.Equal(t, actualArgs, test.expectedArgs) + require.Equal(t, actualCommandLine, test.expectedCommandLine) + }) + } +} diff --git a/pkg/cri/server/container_execsync.go b/pkg/cri/server/container_execsync.go index 23885471e..b22281545 100644 --- a/pkg/cri/server/container_execsync.go +++ b/pkg/cri/server/container_execsync.go @@ -146,6 +146,8 @@ func (c *criService) execInternal(ctx context.Context, container containerd.Cont } pspec.Args = opts.cmd + // CommandLine may already be set on the container's spec, but we want to only use Args here. + pspec.CommandLine = "" if opts.stdout == nil { opts.stdout = cio.NewDiscardLogger()