HostProcess containers with Hypervisor Isolation

cmd/containerd-shim-runhcs-v1/pod.go

@@ -201,7 +201,6 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques
 			parent.Close()
 			return nil, err
 		}
-
 	} else if oci.IsJobContainer(s) {
 		// If we're making a job container fake a task (i.e reuse the wcowPodSandbox logic)
 		p.sandboxTask = newWcowPodSandboxTask(ctx, events, req.ID, req.Bundle, parent, "")
@@ -223,7 +222,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques
 			}); err != nil {
 			return nil, err
 		}
-		p.jobContainer = true
+
 		return &p, nil
 	} else if !isWCOW {
 		return nil, errors.Wrap(errdefs.ErrFailedPrecondition, "oci spec does not contain WCOW or LCOW spec")
@@ -338,11 +337,6 @@ type pod struct {
 	// It MUST be treated as read only in the lifetime of the pod.
 	host *uvm.UtilityVM
 
-	// jobContainer specifies whether this pod is for WCOW job containers only.
-	//
-	// It MUST be treated as read only in the lifetime of the pod.
-	jobContainer bool
-
 	// spec is the OCI runtime specification for the pod sandbox container.
 	spec *specs.Spec
 
@@ -367,24 +361,9 @@ func (p *pod) CreateTask(ctx context.Context, req *task.CreateTaskRequest, s *sp
 		return nil, errors.Wrapf(errdefs.ErrAlreadyExists, "task with id: '%s' already exists id pod: '%s'", req.ID, p.id)
 	}
 
-	if p.jobContainer {
-		// This is a short circuit to make sure that all containers in a pod will have
-		// the same IP address/be added to the same compartment.
-		//
-		// There will need to be OS work needed to support this scenario, so for now we need to block on
-		// this.
-		if !oci.IsJobContainer(s) {
-			return nil, errors.New("cannot create a normal process isolated container if the pod sandbox is a job container")
-		}
-		// Pass through some annotations from the pod spec that if specified will need to be made available
-		// to every container as well. Kubernetes only passes annotations to RunPodSandbox so there needs to be
-		// a way for individual containers to get access to these.
-		oci.SandboxAnnotationsPassThrough(
-			p.spec.Annotations,
-			s.Annotations,
-			annotations.HostProcessInheritUser,
-			annotations.HostProcessRootfsLocation,
-		)
+	err = p.updateConfigForHostProcessContainer(s)
+	if err != nil {
+		return nil, err
 	}
 
 	ct, sid, err := oci.GetSandboxTypeAndID(s.Annotations)
@@ -501,3 +480,49 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error {
 
 	return nil
 }
+
+func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error {
+	if !oci.IsIsolatedJobContainer(p.spec) && oci.IsIsolatedJobContainer(s) {
+		return errors.New("cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container")
+	}
+
+	isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec)
+	isHypervisorIsolatedPrivilegedSandbox := oci.IsIsolatedJobContainer(p.spec)
+	isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s)
+	isHypervisorIsolatedPrivilegedContainer := oci.IsIsolatedJobContainer(s)
+
+	if isProcessIsolatedPrivilegedSandbox || (isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer) {
+		if isProcessIsolatedPrivilegedSandbox && !isProcessIsolatedPrivilegedContainer {
+			// This is a short circuit to make sure that all containers in a pod will have
+			// the same IP address/be added to the same compartment.
+			//
+			// There will need to be OS work needed to support this scenario, so for now we need to block on
+			// this.
+
+			// IsJobContainer returns true if there was no hypervisor isolation and request was for HPCs.
+			// Therefore, in this request we check if the pod was a process isolated HPC pod but the
+			// container spec is not job container spec.
+			return errors.New("cannot create a normal process isolated container if the pod sandbox is a job container running on host")
+		}
+
+		// Pass through some annotations from the pod spec that if specified will need to be made available
+		// to every container as well. Kubernetes only passes annotations to RunPodSandbox so there needs to be
+		// a way for individual containers to get access to these.
+		oci.SandboxAnnotationsPassThrough(
+			p.spec.Annotations,
+			s.Annotations,
+			annotations.HostProcessInheritUser,
+			annotations.HostProcessRootfsLocation,
+		)
+	}
+
+	if isHypervisorIsolatedPrivilegedSandbox &&
+		!isHypervisorIsolatedPrivilegedContainer &&
+		s.Annotations[annotations.HostProcessInheritUser] != "" {
+		// If the hypervisor isolated sandbox was privileged but the container is non-privileged, then
+		// we will explicitly disable the annotation which allows privileged user with the exec.
+		s.Annotations[annotations.HostProcessInheritUser] = "false"
+	}
+
+	return nil
+}

cmd/containerd-shim-runhcs-v1/pod_test.go

@@ -6,10 +6,12 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
+	"reflect"
 	"strconv"
 	"sync"
 	"testing"
 
+	"github.com/Microsoft/hcsshim/pkg/annotations"
 	task "github.com/containerd/containerd/api/runtime/task/v2"
 	"github.com/containerd/errdefs"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
@@ -388,3 +390,157 @@ func Test_pod_DeleteTask_TaskID_Not_Created(t *testing.T) {
 	err := p.KillTask(context.Background(), strconv.Itoa(rand.Int()), "", 0xf, true)
 	verifyExpectedError(t, nil, err, errdefs.ErrNotFound)
 }
+
+func getWCOWSpecsWithAnnotations(annotation map[string]string, isIsolated bool, processUser string) *specs.Spec {
+	spec := &specs.Spec{
+		Windows:     &specs.Windows{},
+		Annotations: annotation,
+		Process: &specs.Process{
+			User: specs.User{Username: processUser},
+		},
+	}
+
+	if isIsolated {
+		spec.Windows.HyperV = &specs.WindowsHyperV{}
+	}
+	return spec
+}
+
+func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) {
+	testCases := []struct {
+		testName              string
+		podSpec               *specs.Spec
+		containerSpec         *specs.Spec
+		expectedContainerSpec *specs.Spec
+		expectedError         string
+	}{
+		{
+			testName: "privileged container in unprivileged pod (isolated hpc)",
+			podSpec:  getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
+			containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			expectedContainerSpec: nil,
+			expectedError:         "cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container",
+		},
+		{
+			testName: "privileged container in privileged pod (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			expectedError: "",
+		},
+		{
+			testName: "normal container in privileged pod (process hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, false, ""),
+			containerSpec:         getWCOWSpecsWithAnnotations(map[string]string{}, false, ""),
+			expectedContainerSpec: nil,
+			expectedError:         "cannot create a normal process isolated container if the pod sandbox is a job container running on host",
+		},
+		{
+			testName: "normal container in privileged pod (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			containerSpec:         getWCOWSpecsWithAnnotations(map[string]string{}, false, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, false, ""),
+			expectedError:         "",
+		},
+		{
+			testName: "annotations passthrough (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer:      "true",
+				annotations.HostProcessInheritUser:    "true",
+				annotations.HostProcessRootfsLocation: "C:\\test",
+			}, true, ""),
+			containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer:      "true",
+				annotations.HostProcessInheritUser:    "true",
+				annotations.HostProcessRootfsLocation: "C:\\test",
+			}, true, ""),
+			expectedError: "",
+		},
+		{
+			testName: "annotations passthrough (process hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer:      "true",
+				annotations.HostProcessInheritUser:    "true",
+				annotations.HostProcessRootfsLocation: "C:\\test",
+			}, false, ""),
+			containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, false, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer:      "true",
+				annotations.HostProcessInheritUser:    "true",
+				annotations.HostProcessRootfsLocation: "C:\\test",
+			}, false, ""),
+			expectedError: "",
+		},
+		{
+			testName: "no annotation passthrough for normal containers (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessInheritUser:    "true",
+				annotations.HostProcessRootfsLocation: "C:\\test",
+			}, true, ""),
+			containerSpec:         getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
+			expectedError:         "",
+		},
+		{
+			testName: "no changes in user process for normal containers (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer:   "true",
+				annotations.HostProcessInheritUser: "true",
+			}, true, ""),
+			containerSpec:         getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
+			expectedError:         "",
+		},
+		{
+			testName: "set inherit user annotation to false for normal containers (isolated hpc)",
+			podSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessContainer: "true",
+			}, true, ""),
+			containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessInheritUser: "true",
+			}, true, ""),
+			expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{
+				annotations.HostProcessInheritUser: "false",
+			}, true, ""),
+			expectedError: "",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.testName, func(t *testing.T) {
+			p := &pod{spec: tc.podSpec}
+
+			err := p.updateConfigForHostProcessContainer(tc.containerSpec)
+			if err == nil && tc.expectedError != "" {
+				t.Fatalf("should have failed with '%s'", tc.expectedError)
+			}
+			if err != nil && err.Error() != tc.expectedError {
+				t.Fatalf("should have failed with '%s', actual: '%s'", tc.expectedError, err.Error())
+			}
+
+			if tc.expectedContainerSpec != nil {
+				equal := reflect.DeepEqual(tc.containerSpec, tc.expectedContainerSpec)
+				if !equal {
+					t.Fatalf("containerSpec expected: %+v, got: %+v", tc.expectedContainerSpec, tc.containerSpec)
+				}
+			}
+		})
+	}
+}

cmd/containerd-shim-runhcs-v1/task_hcs.go

@@ -148,6 +148,8 @@ func createContainer(
 		return nil, nil, err
 	}
 
+	// For Job Container which runs on host, we create the same using shim logic.
+	// If the request was for job container within UVM, then the request is passed along to GCS.
 	if oci.IsJobContainer(s) {
 		opts := jobcontainers.CreateOptions{WCOWLayers: wcowLayers}
 		container, resources, err = jobcontainers.Create(ctx, id, s, opts)
@@ -209,6 +211,12 @@ func newHcsTask(
 		shimOpts = v.(*runhcsopts.Options)
 	}
 
+	// For Isolated Job containers, we need special handling wherein if the command line
+	// is not specified, then we add 'cmd /C' by default.
+	if oci.IsIsolatedJobContainer(s) {
+		handleProcessArgsForIsolatedJobContainer(s, s.Process)
+	}
+
 	// Default to an infinite timeout (zero value)
 	var ioRetryTimeout time.Duration
 	if shimOpts != nil {
@@ -361,6 +369,10 @@ func (ht *hcsTask) CreateExec(ctx context.Context, req *task.ExecProcessRequest,
 		return errors.Wrapf(errdefs.ErrFailedPrecondition, "exec: '' in task: '%s' must be running to create additional execs", ht.id)
 	}
 
+	if oci.IsIsolatedJobContainer(ht.taskSpec) {
+		handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec)
+	}
+
 	io, err := cmd.NewUpstreamIO(ctx, req.ID, req.Stdout, req.Stderr, req.Stdin, req.Terminal, ht.ioRetryTimeout)
 	if err != nil {
 		return err
@@ -981,6 +993,22 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st
 	return ht.c.Modify(ctx, modification)
 }
 
+func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, specs *specs.Process) {
+	if specs.CommandLine != "" && !strings.HasPrefix(strings.TrimSpace(strings.ToLower(specs.CommandLine)), "cmd") {
+		specs.CommandLine = fmt.Sprintf("cmd /c %s", specs.CommandLine)
+	}
+	if len(specs.Args) > 0 && strings.TrimSpace(strings.ToLower(specs.Args[0])) != "cmd" {
+		specs.Args = append([]string{"cmd", "/c"}, specs.Args...)
+	}
+
+	// HostProcessInheritUser is set to explicit true or false during container create.
+	if taskSpec.Annotations[annotations.HostProcessInheritUser] == "true" {
+		// For privileged containers within the sandbox, if the annotation to inherit user is set
+		// we will set the user to NT AUTHORITY\SYSTEM.
+		specs.User.Username = `NT AUTHORITY\SYSTEM`
+	}
+}
+
 func isMountTypeSupported(hostPath, mountType string) bool {
 	// currently we only support mounting of host volumes/directories
 	switch mountType {