Add validations for Windows HostProcess CRI configs

dcantah · dcantah · commit 978ff393d254 · 2022-05-27T21:17:07.000-07:00
HostProcess containers require every container in the pod to be a
host process container and have the corresponding field set. The Kubelet
usually enforces this so we'd error before even getting here but we recently
found a bug in this logic so better to be safe than sorry.

Signed-off-by: Daniel Canter &lt;dcanter@microsoft.com&gt;
diff --git a/integration/main_test.go b/integration/main_test.go
@@ -194,8 +194,8 @@ func PodSandboxConfigWithCleanup(t *testing.T, name, ns string, opts ...PodSandb
 	return sb, sbConfig
 }
 
-// Set Windows HostProcess.
-func WithWindowsHostProcess(p *runtime.PodSandboxConfig) { //nolint:unused
+// Set Windows HostProcess on the pod.
+func WithWindowsHostProcessPod(p *runtime.PodSandboxConfig) { //nolint:unused
 	if p.Windows == nil {
 		p.Windows = &runtime.WindowsPodSandboxConfig{}
 	}
@@ -252,6 +252,18 @@ func WithWindowsUsername(username string) ContainerOpts { //nolint:unused
 	}
 }
 
+func WithWindowsHostProcessContainer() ContainerOpts { //nolint:unused
+	return func(c *runtime.ContainerConfig) {
+		if c.Windows == nil {
+			c.Windows = &runtime.WindowsContainerConfig{}
+		}
+		if c.Windows.SecurityContext == nil {
+			c.Windows.SecurityContext = &runtime.WindowsContainerSecurityContext{}
+		}
+		c.Windows.SecurityContext.HostProcess = true
+	}
+}
+
 // Add container command.
 func WithCommand(cmd string, args ...string) ContainerOpts {
 	return func(c *runtime.ContainerConfig) {
diff --git a/integration/windows_hostprocess_test.go b/integration/windows_hostprocess_test.go
@@ -32,41 +32,42 @@ var (
 	defaultCommand       = WithCommand("Powershell", "/c", "$env:CONTAINER_SANDBOX_MOUNT_POINT/pause.exe")
 	localServiceUsername = WithWindowsUsername("NT AUTHORITY\\Local service")
 	localSystemUsername  = WithWindowsUsername("NT AUTHORITY\\System")
+	hpcContainerOpt      = WithWindowsHostProcessContainer()
 )
 
 // Tests to verify the Windows HostProcess
 func TestWindowsHostProcess(t *testing.T) {
 	EnsureImageExists(t, pauseImage)
 
 	t.Run("run as Local Service", func(t *testing.T) {
-		runHostProcess(t, false, pauseImage, localServiceUsername, defaultCommand)
+		runHostProcess(t, false, pauseImage, hpcContainerOpt, localServiceUsername, defaultCommand)
 	})
 	t.Run("run as Local System", func(t *testing.T) {
-		runHostProcess(t, false, pauseImage, localSystemUsername, defaultCommand)
+		runHostProcess(t, false, pauseImage, hpcContainerOpt, localSystemUsername, defaultCommand)
 	})
 	t.Run("run as unacceptable user", func(t *testing.T) {
-		runHostProcess(t, true, pauseImage, WithWindowsUsername("Guest"), defaultCommand)
+		runHostProcess(t, true, pauseImage, hpcContainerOpt, WithWindowsUsername("Guest"), defaultCommand)
 	})
 	t.Run("run command on host", func(t *testing.T) {
 		cmd := WithCommand("Powershell", "/c", "Get-Command containerd.exe")
-		runHostProcess(t, false, pauseImage, localServiceUsername, cmd)
+		runHostProcess(t, false, pauseImage, hpcContainerOpt, localServiceUsername, cmd)
 	})
 	t.Run("run withHostNetwork", func(t *testing.T) {
 		hostname, err := os.Hostname()
 		require.NoError(t, err)
 		cmd := WithCommand("Powershell", "/c", fmt.Sprintf("if ($env:COMPUTERNAME -ne %s) { exit -1 }", hostname))
-		runHostProcess(t, false, pauseImage, localServiceUsername, cmd)
+		runHostProcess(t, false, pauseImage, hpcContainerOpt, localServiceUsername, cmd)
 	})
 	t.Run("run with a different os.version image", func(t *testing.T) {
 		image := "docker.io/e2eteam/busybox:1.29-windows-amd64-1909"
 		EnsureImageExists(t, image)
-		runHostProcess(t, false, image, localServiceUsername, defaultCommand)
+		runHostProcess(t, false, image, hpcContainerOpt, localServiceUsername, defaultCommand)
 	})
 }
 
 func runHostProcess(t *testing.T, expectErr bool, image string, opts ...ContainerOpts) {
 	t.Logf("Create a pod config and run sandbox container")
-	sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox1", "hostprocess", WithWindowsHostProcess)
+	sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox1", "hostprocess", WithWindowsHostProcessPod)
 
 	t.Logf("Create a container config and run container in a pod")
 	containerConfig := ContainerConfig(
diff --git a/pkg/cri/server/container_create_windows.go b/pkg/cri/server/container_create_windows.go
@@ -17,6 +17,7 @@
 package server
 
 import (
+	"errors"
 	"strconv"
 
 	"github.com/containerd/containerd/oci"
@@ -50,6 +51,16 @@ func (c *criService) containerSpec(
 	specOpts := []oci.SpecOpts{
 		customopts.WithProcessArgs(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
+	// didn't have the field set. The only case that is valid is if these are the same value.
+	cntrHpc := config.GetWindows().GetSecurityContext().GetHostProcess()
+	sandboxHpc := sandboxConfig.GetWindows().GetSecurityContext().GetHostProcess()
+	if cntrHpc != sandboxHpc {
+		return nil, errors.New("pod spec and all containers inside must have the HostProcess field set to be valid")
+	}
+
 	if config.GetWorkingDir() != "" {
 		specOpts = append(specOpts, oci.WithProcessCwd(config.GetWorkingDir()))
 	} else if imageConfig.WorkingDir != "" {
@@ -120,7 +131,7 @@ func (c *criService) containerSpec(
 		customopts.WithAnnotation(annotations.SandboxName, sandboxConfig.GetMetadata().GetName()),
 		customopts.WithAnnotation(annotations.ContainerName, containerName),
 		customopts.WithAnnotation(annotations.ImageName, imageName),
-		customopts.WithAnnotation(annotations.WindowsHostProcess, strconv.FormatBool(sandboxConfig.GetWindows().GetSecurityContext().GetHostProcess())),
+		customopts.WithAnnotation(annotations.WindowsHostProcess, strconv.FormatBool(sandboxHpc)),
 	)
 	return c.runtimeSpec(id, ociRuntime.BaseRuntimeSpec, specOpts...)
 }
diff --git a/pkg/cri/server/container_create_windows_test.go b/pkg/cri/server/container_create_windows_test.go
@@ -83,6 +83,7 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox
 			Namespace: "test-sandbox-ns",
 			Attempt:   2,
 		},
+		Windows:     &runtime.WindowsPodSandboxConfig{},
 		Hostname:    "test-hostname",
 		Annotations: map[string]string{"c": "d"},
 	}
@@ -195,3 +196,52 @@ func TestMountNamedPipe(t *testing.T) {
 	specCheck(t, testID, testSandboxID, testPid, spec)
 	checkMount(t, spec.Mounts, `\\.\pipe\foo`, `\\.\pipe\foo`, "", []string{"rw"}, nil)
 }
+
+func TestHostProcessRequirements(t *testing.T) {
+	testID := "test-id"
+	testSandboxID := "sandbox-id"
+	testContainerName := "container-name"
+	testPid := uint32(1234)
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
+	c := newTestCRIService()
+	for desc, test := range map[string]struct {
+		containerHostProcess bool
+		sandboxHostProcess   bool
+		expectError          bool
+	}{
+		"hostprocess container in non-hostprocess sandbox should fail": {
+			containerHostProcess: true,
+			sandboxHostProcess:   false,
+			expectError:          true,
+		},
+		"hostprocess container in hostprocess sandbox should be fine": {
+			containerHostProcess: true,
+			sandboxHostProcess:   true,
+			expectError:          false,
+		},
+		"non-hostprocess container in hostprocess sandbox should fail": {
+			containerHostProcess: false,
+			sandboxHostProcess:   true,
+			expectError:          true,
+		},
+		"non-hostprocess container in non-hostprocess sandbox should be fine": {
+			containerHostProcess: false,
+			sandboxHostProcess:   false,
+			expectError:          false,
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			containerConfig.Windows.SecurityContext.HostProcess = test.containerHostProcess
+			sandboxConfig.Windows.SecurityContext = &runtime.WindowsSandboxSecurityContext{
+				HostProcess: test.sandboxHostProcess,
+			}
+			_, err := c.containerSpec(testID, testSandboxID, testPid, "", testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
+			if test.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
