-
Notifications
You must be signed in to change notification settings - Fork 276
HostProcess containers with Hypervisor Isolation #2531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
HostProcess containers with Hypervisor Isolation #2531
Conversation
76a7277 to
b56ed8c
Compare
b56ed8c to
e1e1b82
Compare
|
|
||
| // 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we stick with the above. IsJobContainer returns true when Linux == nil && Windows != nil && annotation[HPC] == true
IsIsolated is already true above based on setting HyperV.
So why can't you just do if IsJobContainer(s) && IsIsolated(s) of you want both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just for being explicitness. It just increases the length of each if condition.
If you don't have major concerns, we can leave this be. It makes the code readability better IMO.
internal/hcsoci/hcsdoc_wcow.go
Outdated
|
|
||
| // 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 { | ||
| v2Container.Storage.PrivilegedContainerRootPath = customRootFsPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path needs to be sanitized right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a method to sanitize as well protect against mount attacks.
| taskSpec := &specs.Spec{Annotations: tt.taskAnnotations} | ||
| // Ensure initial Username is set if provided | ||
| if tt.initialUsername != "" { | ||
| tt.specs.User.Username = tt.initialUsername |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already set this in the process spec above for each test case. Why override it? I dont get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! I had initially planned on test cases to change the usernames via this parameter. I will remove this in the revision.
| // 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this part of it special for HPC? I dont get it, this is true for all Windows containers right? At this point its just another container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, if we do not pass the command line to HCS, the call fails.
HPCs don't follow the exact same path as normal Argon containers. Argons are created as full Silos and might have a default shell but HPCs are partial Silos and seemingly don't have one.
cmd/containerd-shim-runhcs-v1/pod.go
Outdated
| var parent *uvm.UtilityVM | ||
| var lopts *uvm.OptionsLCOW | ||
| if oci.IsIsolated(s) { | ||
| if oci.IsIsolated(s) || oci.IsIsolatedJobContainer(s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You dont need to add this. We always make a UVM for any "IsIsolated" so you dont need to also handle the case for IsIsolatedJobContainer. That also "IsIsolated"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. This was not needed per se as we will always go through the if condition. However, I added this for being explicit. I can remove it.
4eba3a9 to
6f6cae7
Compare
This commit adds support for running HostProcess containers with hypervisor isolated workloads. Therefore, these HPCs will run as privileged processes within the UVM. Some of the key aspects of this feature are- - A pod needs to be marked privileged by passing in annotation 'microsoft.com/hostprocess-container' - Any container which has 'microsoft.com/hostprocess-container' annotation will be launched as HostProcess Container. - Other containers will be launched as normal process-isolated containers within the UVM - HPC within UVM will not have access to any network since the sandbox (UVM) does not have any network. - GCS will create the HPCs based on the specs. The behaviour is similar to existing HPCs for process-isolated case. - Annotations for Inherit user will lead to the HPC running as System user within UVM. - Annotation for custom rootfs will lead to the container filesystem being virtualized in a directory with that custom name. If not provided, `C:\hpc` is used by default. Signed-off-by: Harsh Rawat <[email protected]>
This commit adds thorough unit tests for: - IsJobContainer - IsIsolatedJobContainer These tests explicitly verify the expected behavior for Windows HostProcess containers across isolation modes and OS targets. Signed-off-by: Harsh Rawat <[email protected]>
This commit adds thorough unit tests for the updateConfigForHostProcessContainer method. We test the following functionality- - Reject privileged container in unprivileged sandbox (isolated HPC) — return error when container requests microsoft.com/hostprocess-container=true but the pod lacks it. - Allow privileged container in privileged sandbox (isolated HPC) — ensure no unexpected mutations when no passthrough annotations are present. - Block normal process-isolated container in process-isolated HPC pod — enforce constraint that all containers in a process HPC pod must also be job containers (error on mixing). - Allow normal hypervisor-isolated container in hypervisor-isolated HPC pod. - Passthrough annotations to privileged containers — propagate HostProcessInheritUser and HostProcessRootfsLocation from pod → container for: hypervisor-isolated HPC pods with privileged containers, and process-isolated HPC pods with privileged containers. - No passthrough for normal containers — verify annotations aren’t propagated when the container is non-privileged. - Set SYSTEM user for hypervisor-isolated privileged containers — when HostProcessInheritUser=true, set Process.User.Username to NT AUTHORITY\SYSTEM. - Do not change user for normal containers — ensure container user remains unchanged even if the pod has HostProcessInheritUser=true. - Force HostProcessInheritUser to "false" for normal containers — in hypervisor-isolated privileged pods, if a non-privileged container sets the inherit annotation, flip it to "false". Signed-off-by: Harsh Rawat <[email protected]>
Signed-off-by: Harsh Rawat <[email protected]>
Signed-off-by: Harsh Rawat <[email protected]>
Signed-off-by: Harsh Rawat <[email protected]>
Signed-off-by: Harsh Rawat <[email protected]>
Signed-off-by: Harsh Rawat <[email protected]>
36a6efa to
056de18
Compare
Summary
This PR adds support for running HostProcess containers with hypervisor isolation. These HPCs will run as privileged processes within the UVM. Few key aspects of this feature are-
C:\hpcis used by default.Primary changes
updateConfigForHostProcessContainermethod for handling privileged container configuration and annotation passthrough. This method enforces correct sandbox/container combinations and properly sets user and annotation values for HostProcess containers.ContainerTypeandContainerRootPath) to support HostProcess container identification and custom root filesystem locations. These are set during container creation when an isolated job container is detected.handleProcessArgsForIsolatedJobContainerto ensure that isolated job containers have their process arguments correctly prefixed withcmd /cwhen necessary, supporting bothCommandLineandArgsfields. This logic is invoked during container and exec creation.Testing Enhancements