Merge pull request #8198 from kiashok/argsEscapedSupportInCri

Add ArgsEscaped support for CRI

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
 }
 

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"

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)
+}

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

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
+	}
+}

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
+}

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.

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{
+	var specOpts []oci.SpecOpts
-		customopts.WithProcessArgs(config, imageConfig),
+	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

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{
+	var specOpts []oci.SpecOpts
-		customopts.WithProcessArgs(config, imageConfig),
+	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

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{
+	sandboxConfig := getSandboxConfig()
-		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"},
-	}
 	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)
+		})
+	}
+}

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()