review comments 2

Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>

Author: rawahars

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

@@ -482,27 +482,36 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error {
 	return nil
 }
 
+// updateConfigForHostProcessContainer validates and adjusts a container spec
+// against the pod sandbox spec for HostProcess (HPC) scenarios:
+//   - Rejects a hypervisor-isolated HPC inside a non-HPC sandbox.
+//   - Rejects a non-HPC container inside a process-isolated HPC sandbox
+//     (all containers in such a pod must share the host job/network).
+//   - Propagates HPC pod-level annotations (e.g. inherit-user, rootfs
+//     location) onto each container, since CRI only delivers them at pod
+//     create time.
+//   - Forces HostProcessInheritUser=false on a non-privileged container in a
+//     privileged hypervisor-isolated sandbox so it can't inherit SYSTEM.
 func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error {
-	if !oci.IsIsolatedJobContainer(p.spec) && oci.IsIsolatedJobContainer(s) {
+	isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && !oci.IsIsolated(p.spec)
+	isHypervisorIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && oci.IsIsolated(p.spec)
+	isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s) && !oci.IsIsolated(s)
+	isHypervisorIsolatedPrivilegedContainer := oci.IsJobContainer(s) && oci.IsIsolated(s)
+
+	if !isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer {
 		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.
+			//
+			// If the pod sandbox is a process-isolated HPC then the container must also be a
+			// process-isolated HPC.
 			return errors.New("cannot create a normal process isolated container if the pod sandbox is a job container running on host")
 		}
 

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

@@ -151,7 +151,7 @@ func createContainer(
 
 	// 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) {
+	if oci.IsJobContainer(s) && !oci.IsIsolated(s) {
 		opts := jobcontainers.CreateOptions{WCOWLayers: wcowLayers}
 		container, resources, err = jobcontainers.Create(ctx, id, s, opts)
 		if err != nil {
@@ -216,7 +216,7 @@ func newHcsTask(
 
 	// 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) {
+	if oci.IsJobContainer(s) && oci.IsIsolated(s) {
 		handleProcessArgsForIsolatedJobContainer(s, s.Process)
 	}
 
@@ -372,7 +372,7 @@ 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) {
+	if oci.IsJobContainer(ht.taskSpec) && oci.IsIsolated(ht.taskSpec) {
 		handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec)
 	}
 
@@ -1027,6 +1027,23 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st
 	return ht.c.Modify(ctx, modification)
 }
 
+// handleProcessArgsForIsolatedJobContainer adjusts the process spec for a
+// HostProcess container running inside a hypervisor-isolated UVM.
+//
+// Why the "cmd /c" wrap:
+//   - For a regular WCOW container, the guest creates the workload process
+//     from within the container silo, so the silo's filesystem view (including
+//     the bind-mounted container rootfs) is in scope for path resolution.
+//   - For a HostProcess container the guest creates the process from outside
+//     the silo and only attaches it to the silo's job object after creation.
+//     Executable-name resolution therefore uses the launcher's view (System32,
+//     Windows, PATH) and cannot see binaries that live only inside the
+//     container rootfs, nor resolve shell builtins like `echo`, `dir`, `set`,
+//     redirection, etc. Anything not present as a real .exe on the standard
+//     search path would fail with ERROR_FILE_NOT_FOUND.
+//   - cmd.exe always exists under System32 (so the launcher can find it) and,
+//     once joined to the silo's job, runs inside the container's filesystem
+//     view where it can resolve builtins and any rootfs-relative binary.
 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)

internal/hcs/schema2/container.go

@@ -9,6 +9,19 @@
 
 package hcsschema
 
+// IsolationType describes the isolation mode of a container.
+type IsolationType string
+
+const (
+	// IsolationTypeProcess is a fully process-isolated, namespace-isolated
+	// Windows Server container.
+	IsolationTypeProcess IsolationType = "Process"
+
+	// IsolationTypeHostProcess is a privileged (Windows) HostProcess
+	// container that shares the host namespace.
+	IsolationTypeHostProcess IsolationType = "HostProcess"
+)
+
 type Container struct {
 	GuestOs *GuestOs `json:"GuestOs,omitempty"`
 
@@ -34,5 +47,5 @@ type Container struct {
 
 	AdditionalDeviceNamespace *ContainerDefinitionDevice `json:"AdditionalDeviceNamespace,omitempty"`
 
-	IsolationType string `json:"IsolationType,omitempty"`
+	IsolationType IsolationType `json:"IsolationType,omitempty"`
 }

internal/hcsoci/hcsdoc_wcow.go

@@ -501,8 +501,9 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
 		AddValues: registryAdd,
 	}
 
-	if oci.IsIsolatedJobContainer(coi.Spec) {
-		v2Container.IsolationType = "HostProcess"
+	if oci.IsJobContainer(coi.Spec) && oci.IsIsolated(coi.Spec) {
+		// Set the isolation type to host process.
+		v2Container.IsolationType = hcsschema.IsolationTypeHostProcess
 
 		// If the customer specified a custom rootfs path then use that instead of default c:\hpc.
 		if customRootFsPath, ok := coi.Spec.Annotations[annotations.HostProcessRootfsLocation]; ok {
@@ -511,6 +512,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
 				return nil, nil, err
 			}
 
+			// Set the path for container root filesystem.
 			v2Container.Storage.PrivilegedContainerRootPath = customRootFsPathSanitized
 		}
 	}

internal/oci/util.go

@@ -21,19 +21,6 @@ func IsIsolated(s *specs.Spec) bool {
 }
 
 // IsJobContainer checks if `s` is asking for a Windows job container.
-// This request is for a shim based process isolated HPCs.
 func IsJobContainer(s *specs.Spec) bool {
-	return s.Linux == nil &&
-		s.Windows != nil &&
-		s.Windows.HyperV == nil &&
-		s.Annotations[annotations.HostProcessContainer] == "true"
-}
-
-// IsIsolatedJobContainer checks if `s` is asking for a Windows job container.
-// This request is for running HPC within guest.
-func IsIsolatedJobContainer(s *specs.Spec) bool {
-	return s.Linux == nil &&
-		s.Windows != nil &&
-		s.Windows.HyperV != nil &&
-		s.Annotations[annotations.HostProcessContainer] == "true"
+	return IsWCOW(s) && s.Annotations[annotations.HostProcessContainer] == "true"
 }

internal/oci/util_test.go

@@ -188,14 +188,14 @@ func Test_IsJobContainer(t *testing.T) {
 			expected: false,
 		},
 		{
-			name: "WCOW Hyper-V isolated with HostProcess=true (not a JobContainer)",
+			name: "WCOW Hyper-V isolated with HostProcess=true",
 			spec: &specs.Spec{
 				Windows: &specs.Windows{
 					HyperV: &specs.WindowsHyperV{},
 				},
 				Annotations: map[string]string{annotations.HostProcessContainer: "true"},
 			},
-			expected: false,
+			expected: true,
 		},
 		{
 			name: "LCOW without Windows (not a JobContainer)",
@@ -218,77 +218,11 @@ func Test_IsJobContainer(t *testing.T) {
 }
 
 // -----------------------------------------------------------------------------
-// IsIsolatedJobContainer tests
-// -----------------------------------------------------------------------------
-
-func Test_IsIsolatedJobContainer(t *testing.T) {
-	tests := []struct {
-		name     string
-		spec     *specs.Spec
-		expected bool
-	}{
-		{
-			name: "WCOW Hyper-V isolated with HostProcess=true",
-			spec: &specs.Spec{
-				Windows: &specs.Windows{
-					HyperV: &specs.WindowsHyperV{},
-				},
-				Annotations: map[string]string{annotations.HostProcessContainer: "true"},
-			},
-			expected: true,
-		},
-		{
-			name: "WCOW Hyper-V isolated with HostProcess=false",
-			spec: &specs.Spec{
-				Windows: &specs.Windows{
-					HyperV: &specs.WindowsHyperV{},
-				},
-				Annotations: map[string]string{annotations.HostProcessContainer: "false"},
-			},
-			expected: false,
-		},
-		{
-			name: "WCOW Hyper-V isolated with HostProcess missing",
-			spec: &specs.Spec{
-				Windows: &specs.Windows{
-					HyperV: &specs.WindowsHyperV{},
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "WCOW process-isolated with HostProcess=true (not isolated job)",
-			spec: &specs.Spec{
-				Windows:     &specs.Windows{},
-				Annotations: map[string]string{annotations.HostProcessContainer: "true"},
-			},
-			expected: false,
-		},
-		{
-			name: "LCOW without Windows (not an isolated job container)",
-			spec: &specs.Spec{
-				Linux: &specs.Linux{},
-			},
-			expected: false,
-		},
-	}
-
-	for _, tt := range tests {
-		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			actual := IsIsolatedJobContainer(tt.spec)
-			if actual != tt.expected {
-				t.Fatalf("IsIsolatedJobContainer() = %v, expected %v", actual, tt.expected)
-			}
-		})
-	}
-}
-
-// -----------------------------------------------------------------------------
-// Cross-property tests (consistency / mutual exclusivity)
+// Cross-property tests: IsJobContainer combined with IsIsolated to differentiate
+// process-isolated HPC from hypervisor-isolated HPC.
 // -----------------------------------------------------------------------------
 
-func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) {
+func Test_JobContainer_IsolationDifferentiation(t *testing.T) {
 	// Process-isolated WCOW HostProcess=true
 	processJob := &specs.Spec{
 		Windows:     &specs.Windows{},
@@ -297,8 +231,8 @@ func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) {
 	if !IsJobContainer(processJob) {
 		t.Fatal("expected IsJobContainer to be true for process-isolated HostProcess=true")
 	}
-	if IsIsolatedJobContainer(processJob) {
-		t.Fatal("expected IsIsolatedJobContainer to be false for process-isolated HostProcess=true")
+	if IsIsolated(processJob) {
+		t.Fatal("expected IsIsolated to be false for process-isolated HostProcess=true")
 	}
 
 	// Hyper-V isolated WCOW HostProcess=true
@@ -308,10 +242,10 @@ func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) {
 		},
 		Annotations: map[string]string{annotations.HostProcessContainer: "true"},
 	}
-	if IsJobContainer(hyperVJob) {
-		t.Fatal("expected IsJobContainer to be false for Hyper-V isolated HostProcess=true")
+	if !IsJobContainer(hyperVJob) {
+		t.Fatal("expected IsJobContainer to be true for Hyper-V isolated HostProcess=true")
 	}
-	if !IsIsolatedJobContainer(hyperVJob) {
-		t.Fatal("expected IsIsolatedJobContainer to be true for Hyper-V isolated HostProcess=true")
+	if !IsIsolated(hyperVJob) {
+		t.Fatal("expected IsIsolated to be true for Hyper-V isolated HostProcess=true")
 	}
 }

test/internal/container/container.go

@@ -48,7 +48,7 @@ func Create(
 		tb.Fatalf("layer parsing failed: %s", err)
 	}
 
-	if oci.IsJobContainer(spec) {
+	if oci.IsJobContainer(spec) && !oci.IsIsolated(spec) {
 		c, r, err = jobcontainers.Create(ctx, name, spec, jobcontainers.CreateOptions{WCOWLayers: wcowLayers})
 	} else {
 		co := &hcsoci.CreateOptions{