Add Windows Hostprocess flag #795

jsturtevant · 2022-02-08T18:30:39Z

Fixes: #518

Still needs:

docs
a bit more thought on how to specific user
test

cmd/nerdctl/run_windows.go

AkihiroSuda · 2022-02-10T18:14:43Z

README.md

@@ -370,6 +370,7 @@ Security flags:
 - :whale: `--cap-add=<CAP>`: Add Linux capabilities
 - :whale: `--cap-drop=<CAP>`: Drop Linux capabilities
 - :whale: `--privileged`: Give extended privileges to this container
+- :blue_square: :nerd_face: `--host-process`: Allow running Host Process containers on Windows. Inherits permissions from containerd process unless `--user` is specified then will start with user specified.  The user specified must be present on the host.  Requires Containerd 1.6+.


What about calling this --isolation=host

I like that idea, follows with what folks would be used to for process/hyper-v

this has grown on me overnight. I do have a concern on naming... Right now we call this hostprocess containers in kubernetes and job containers in hcsshim and we would be adding another slightly different way of referencing this concept. I think it is a minor difference but thought to bring it up. thoughts?

The 'job containers' name was supposed to be just for the devs on hcsshim as it was what we got accustomed to when referring to it internally, although it does kind of suck that the name leaks out in some error strings. That should maybe get addressed/cleaned up. If that was touched up would that alleviate some of the concern on the three way split @jsturtevant?

Feel like the UX is a bit improved if all the Windows folks need to remember is --isolation and its three options vs. --isolation and then an option off to the side that also dramatically reduces the amount of isolation you have. Also might simplify the case where both are specified (--isolation and --host-process) as ideally we should error out if they are as they're conflicting options.

It's a minor concern. Just wanted to call it out. Anytime we refer to the same thing multiple ways it adds complexity to understanding.

That said thinking about hostprocess as an --isolation resonates with me. and keeps it simple from the cli UX perspective.

This brings up one more thought, As of right now hostprocess doesn't not support hyper-v containers. Is it possible in the future it could? Does that even make sense?

That's a good question.. I could imagine a world where we could get it to work, but I'm not sure what use it would bring off the top of my head. I don't know if its worth putting too much brain power into thinking of at the moment though

Just want to make sure we don't back our selves into a naming thing. If it was a thing, would naming for isolation be something like hyperv-host 🤔

If we're able to specify --isolation twice (e.g. --isolation host --isolation hyper-v) would that suffice? We'd just need to short circuit on if someone specified process and a second option as that doesn't make sense. Or your idea of a fourth hyperv-host option.

sounds like we have some options. I don't think we need to solve it right now.

cmd/nerdctl/run.go

go.mod

cmd/nerdctl/run_windows.go

jsturtevant · 2022-02-11T16:09:58Z

The test failed with

   Command:  C:\Windows\system32\config\systemprofile\go\bin\nerdctl.exe --namespace=nerdctl-test run --rm --host-process -u "NT AUTHORITY\Local Service" gcr.io/k8s-staging-e2e-test-images/busybox:1.29-2 whoami
        ExitCode: 1
        Error:    exit status 1
        Stdout:   
        Stderr:   time="2022-02-10T18:18:12Z" level=fatal msg="failed to create user process token: failed to logon user: The user name or password is incorrect.: unknown"
        Failures:

I am guessing the containerd process isn't running with high enough privileges in the CI environment as this passes locally with containerd installed with a similar script. @dcantah do you know if there is another built in user we could use here?

dcantah · 2022-02-11T18:28:31Z

README.md

+Isolation flags:
+- :whale: :blue_square: :nerd_face: `--isolation=(default|host)`: Used on Windows to change process isolation level. Valid values are `default` and `host`. `default` runs process isolated containers.  `host` runs Host Process containers.  Host process containers inherit permissions from containerd process unless `--user` is specified then will start with user specified.  The user specified must be present on the host.  `host` requires Containerd 1.6+. Not implemented for Linux.
+


Wasn't the Docker option for process isolated "process"? I think for Docker Desktop it defaulted to hyper-v if you didn't supply anything, and on server it defaulted to process. Regardless of that we should probably stick to the same naming as docker (unless I missed the discussion for naming this default 😆)

yes, I should probably have process as an explicit support flag but since hyper-v isn't really support fully in containerd we can default that.

The default flag uses whatever was set on the docker daemon via --exec-opt isolation=hyperv option: https://docs.docker.com/engine/reference/commandline/run/#specify-isolation-technology-for-container---isolation

I don't think containerd has this setting but the equivalent would be default_runtime_name?

I've updated the logic here. I am not sure if I got it right... There seems to be a couple way to get hyper-v, one by setting the oci spec field hyper-v to not nil. Another is to set an option on the shimoptions

AkihiroSuda · 2022-02-17T03:47:26Z

needs rebase

jsturtevant · 2022-02-18T16:49:48Z

Added a logging statement for the who the hostprocess container to see what it thinks it is running as and it is coming back as

run_windows_test.go:35: whoami nt authority\system

@dcantah I would think that since it is system it should be able to start Local Service?

dcantah · 2022-02-18T19:37:29Z

Added a logging statement for the who the hostprocess container to see what it thinks it is running as and it is coming back as

run_windows_test.go:35: whoami nt authority\system

@dcantah I would think that since it is system it should be able to start Local Service?

Yea this is really odd.. The only thing I can think of is there's something blocking logging in users in this environment. The cri-integration tests in Containerd have tests that login as different service accounts https://github.com/containerd/containerd/blob/5162238c7da04d6bdd771d0aa43f8e34c91ea2ca/integration/windows_hostprocess_test.go#L38

AkihiroSuda · 2022-07-07T05:38:42Z

What's current status?

jsturtevant · 2022-11-10T23:31:50Z

I refreshed the PR. It is still failing when running in CI though it works locally, I thought it might be a Cirrus vs github actions thing but doesn't look like it.

I will need to dig deeper, as it is working locally when I test it.

jsturtevant · 2023-01-05T18:19:53Z

debugging a different problem and got some insight from @marosset, It might be because I am not passing the user name info through on the config. going to try that and see I can get this working. better late than never 🙈

marosset · 2023-01-05T18:26:34Z

debugging a different problem and got some insight from @marosset, It might be because I am not passing the user name info through on the config. going to try that and see I can get this working. better late than never 🙈

I think that will fix the issue

dcantah · 2023-01-05T19:55:44Z

Nice. We didn't allow a blank username for CRI either, right? That's probably the right way.. defaulting to SYSTEM is kinda scary

dcantah · 2023-01-05T19:57:53Z

Hmm.. if the username string is empty we should error a bit earlier https://github.com/microsoft/hcsshim/blob/6547959343d65e0cf431dd9d589f45ee2b98172c/internal/jobcontainers/logon.go#L109. Maybe it's filling in container-user/container-admin from the image? 🤷‍♂️

jsturtevant · 2023-01-09T18:43:56Z

The windows tests are passing. Ends up the NT AUTHORITY must be upper case and I had some extra quotes when they were not needed in the test framework. I will clean up the commits and we should be good for a review.

jsturtevant · 2023-01-09T21:40:10Z

@AkihiroSuda this is finally ready for review. I did move the Windows CI from Cirrus to Github actions during my debugging, I can either drop that commit or move it to a separate PR but I think I prefer GitHub UI for debugging and it aligns better with rest of the tests.

dcantah · 2023-01-09T21:43:01Z

If Cirrus supports nested virt it might be worth staying on it, otherwise we won't be able to test hyper-v isolation on GHA (unless they made progress on offering this..)

cmd/nerdctl/run.go

cmd/nerdctl/run_windows.go

AkihiroSuda · 2023-01-09T22:59:39Z

README.md

@@ -395,6 +395,10 @@ Init process flags:
  Please make sure the binary exists in your `PATH`.
  - Default: `tini`

+Isolation flags:
+- :whale: :blue_square: :nerd_face: `--isolation=(default|process|host)`: Used on Windows to change process isolation level. `default` will use the runtime options configured in `default_runtime` in the [containerd configuration](https://github.com/containerd/containerd/blob/master/docs/cri/config.md#cri-plugin-config-guide) which is `process` in containerd by default. `process` runs process isolated containers.  `host` runs Host Process containers.  Host process containers inherit permissions from containerd process unless `--user` is specified then will start with user specified and the user specified must be present on the host.  `host` requires Containerd 1.6+. Not implemented for Linux.
+


No support for the Hyper-V mode?

I think this should be a separate change, reason being is the way Windows launches the VM in the containerd-shim is a bit different than how it was done for docker (in terms of the resources assigned to the VM). It'd be a bit more than just filling in the hyper-v field on the runtime spec I imagine

I was also thinking this would be a separate changeset as the hyper-v support in containerd was only recently added.

README.md

cmd/nerdctl/run.go

cmd/nerdctl/run_windows.go

marosset · 2023-01-09T23:44:00Z

I had a couple of suggestions for the usage but overall LGTM
Thanks @jsturtevant

dcantah · 2023-01-10T05:18:34Z

cmd/nerdctl/run_windows.go

+	switch isolation {
+	case "host":
+		hpAnnotations := map[string]string{}
+		hpAnnotations[hostProcessContainer] = "true"
+
+		// If user is set we will attempt to start container with that user (must be present on the host)
+		// Otherwise we will inherit permissions from the user that the containerd process is running as
+		user, err := cmd.Flags().GetString("user")
+		if err != nil {
+			return nil, err
+		}
+		if user == "" {
+			hpAnnotations[hostProcessInheritUser] = "true"
+		}
+
+		opts = append(opts, oci.WithAnnotations(hpAnnotations))
+	case "default":
+		// no op
+		// use containerd's default runtime options
+	case "process":
+		opts = append(opts, WithWindowsProcessIsolated())
+	default:
+		// no op
+		// use containerd's default
+	}


I think we should combine default and process cases and get rid of the WithWindowsProcessIsolated option as the spec is only going to have that filled in if we set it anyways which would seem like a bug.

Suggested change

switch isolation {

case "host":

hpAnnotations := map[string]string{}

hpAnnotations[hostProcessContainer] = "true"

// If user is set we will attempt to start container with that user (must be present on the host)

// Otherwise we will inherit permissions from the user that the containerd process is running as

user, err := cmd.Flags().GetString("user")

if err != nil {

return nil, err

}

if user == "" {

hpAnnotations[hostProcessInheritUser] = "true"

}

opts = append(opts, oci.WithAnnotations(hpAnnotations))

case "default":

// no op

// use containerd's default runtime options

case "process":

opts = append(opts, WithWindowsProcessIsolated())

default:

// no op

// use containerd's default

}

switch isolation {

case "host":

hpAnnotations := map[string]string{

hostProcessContainer: "true",

}

// If user is set we will attempt to start container with that user (must be present on the host)

// Otherwise we will inherit permissions from the user that the containerd process is running as

user, err := cmd.Flags().GetString("user")

if err != nil {

return nil, err

}

if user == "" {

hpAnnotations[hostProcessInheritUser] = "true"

}

opts = append(opts, oci.WithAnnotations(hpAnnotations))

case "default", "process":

// no op

// use containerd's default runtime options

default:

return nil, fmt.Errorf("unknown isolation value %q. valid values are 'host', 'process' or 'default'", isolation)

}

Also, on docker this flag was defined globally I believe and would error on Linux for an invalid value, should we do some validations for linux as well?

Ok, I remember why I did it this way now.

If the default_runtime is set in the config.toml to be hyper-v don't we need to override the default runtime settings? aka, blank out hyper-v part of the spec?

Maybe I don't quite understand it properly...

Not much we could do it seems as hyperv is set in the shim itself if that's the case https://github.com/microsoft/hcsshim/blob/b295b1a866b1263ddede4f00dddf7a129b96c114/cmd/containerd-shim-runhcs-v1/service_internal.go#L109-L116

#795 (comment)

Ahh, missed that 😬 But there's no point in clearing the spec I think, as if they have runhcs-hypervisor as the default runtime the shim will set the hyperv section back anyways. We're setting all of this up before the shims launched so it seems moot

But there's no point in clearing the spec I think, as if they have runhcs-hypervisor as the default runtime the shim will set the hyperv section back anyway

So this means if the default runtime is hyperv then you can't run process isolated? I not sure we can handle this from here. doing nothing when process and hyperv is the default runtime would be bad experience as well.

I've added this back... Let me know what you think

Wait now that I'm thinking about it, isn't the default_runtime_name a CRI field? That shouldn't matter or get read here right? @AkihiroSuda

You can change the runtime as a whole via --runtime (although process, hyperv and LCOW are all wrapped up in the same shim on Windows so it doesn't matter really). The shim options would have to change for this to matter, and it seems like the only runtime whose options are configured is runc https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/run_runtime.go#L34-L63. But regardless, nil'ing out hyperv is still not needed, as we're the ones controlling the spec here. If the shim ends up setting it for some reason (like here) it's irrelevant what we do here

If the shim ends up setting it for some reason (like here) it's irrelevant what we do here

So this means if the default runtime is hyperv and the user runs nerdctl --isolation process the user won't get a process isolated container? Or is it only if sim options are set?

jsturtevant · 2023-01-10T17:13:07Z

If Cirrus supports nested virt it might be worth staying on it, otherwise we won't be able to test hyper-v isolation on GHA (unless they made progress on offering this..)

great point, It does look like the compute_engine_instance (https://cirrus-ci.org/guide/custom-vms/) does support nested_virtualization: nested_virtualization: true # optional. Whether to enable Intel VT-x. Defaults to false.

I will drop this commit.

cmd/nerdctl/run_windows_test.go

dcantah

LGTM after those test renames (and assuming CI is green)

dcantah · 2023-01-11T00:24:52Z

It does look like the compute_engine_instance (https://cirrus-ci.org/guide/custom-vms/) does support nested_virtualization: nested_virtualization: true # optional. Whether to enable Intel VT-x. Defaults to false.

Sweet!

AkihiroSuda

Thanks, could you consider squashing commits

AkihiroSuda · 2023-01-11T06:49:07Z

hack/configure-windows-ci.ps1

 curl.exe -L https://github.com/containerd/containerd/releases/download/v$version/containerd-$version-windows-amd64.tar.gz -o containerd-windows-amd64.tar.gz
-tar xvf containerd-windows-amd64.tar.gz
+tar.exe xvf containerd-windows-amd64.tar.gz


Not strictly necessary but makes it explicit we are using the tar.exe shipped with Windows vs a powershell alias.

Signed-off-by: James Sturtevant <jsturtevant@gmail.com>

AkihiroSuda

Thanks

dcantah reviewed Feb 8, 2022

View reviewed changes

cmd/nerdctl/run_windows.go Outdated Show resolved Hide resolved

dcantah reviewed Feb 8, 2022

View reviewed changes

cmd/nerdctl/run_windows.go Outdated Show resolved Hide resolved

AkihiroSuda added enhancement New feature or request impact/major platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) labels Feb 9, 2022

jsturtevant force-pushed the hostprocess-containers branch 4 times, most recently from bacdd4c to 98a1c37 Compare February 10, 2022 16:52

jsturtevant marked this pull request as ready for review February 10, 2022 17:02

AkihiroSuda reviewed Feb 10, 2022

View reviewed changes

cmd/nerdctl/run.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 10, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 10, 2022

View reviewed changes

cmd/nerdctl/run_windows.go Outdated Show resolved Hide resolved

jsturtevant force-pushed the hostprocess-containers branch 2 times, most recently from 1d5b38f to 2d1613d Compare February 11, 2022 17:00

dcantah reviewed Feb 11, 2022

View reviewed changes

jsturtevant force-pushed the hostprocess-containers branch 2 times, most recently from ea27336 to 2437d14 Compare February 18, 2022 16:38

jsturtevant force-pushed the hostprocess-containers branch from 2437d14 to f3af71a Compare November 10, 2022 21:30

jsturtevant force-pushed the hostprocess-containers branch from 4f0eb6b to bebfc3f Compare January 9, 2023 18:59

AkihiroSuda added this to the v1.2.0 milestone Jan 9, 2023

AkihiroSuda reviewed Jan 9, 2023

View reviewed changes

cmd/nerdctl/run.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 9, 2023

View reviewed changes

cmd/nerdctl/run_windows.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 9, 2023

View reviewed changes

marosset reviewed Jan 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

marosset reviewed Jan 9, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

marosset reviewed Jan 9, 2023

View reviewed changes

cmd/nerdctl/run.go Outdated Show resolved Hide resolved

marosset reviewed Jan 9, 2023

View reviewed changes

cmd/nerdctl/run_windows.go Outdated Show resolved Hide resolved

dcantah reviewed Jan 10, 2023

View reviewed changes

jsturtevant force-pushed the hostprocess-containers branch from a2da3a3 to 8706e3d Compare January 10, 2023 19:10

dcantah reviewed Jan 10, 2023

View reviewed changes

cmd/nerdctl/run_windows_test.go Outdated Show resolved Hide resolved

dcantah reviewed Jan 10, 2023

View reviewed changes

cmd/nerdctl/run_windows_test.go Outdated Show resolved Hide resolved

dcantah reviewed Jan 10, 2023

View reviewed changes

cmd/nerdctl/run_windows_test.go Outdated Show resolved Hide resolved

dcantah reviewed Jan 10, 2023

View reviewed changes

cmd/nerdctl/run_windows_test.go Outdated Show resolved Hide resolved

dcantah approved these changes Jan 10, 2023

View reviewed changes

jsturtevant force-pushed the hostprocess-containers branch from 8706e3d to 2f3aa59 Compare January 10, 2023 19:20

AkihiroSuda approved these changes Jan 11, 2023

View reviewed changes

AkihiroSuda reviewed Jan 11, 2023

View reviewed changes

Add ability to run Hostprocess containers

391bb6a

Signed-off-by: James Sturtevant <jsturtevant@gmail.com>

jsturtevant force-pushed the hostprocess-containers branch from ee6d724 to 391bb6a Compare January 12, 2023 03:33

AkihiroSuda approved these changes Jan 13, 2023

View reviewed changes

AkihiroSuda merged commit 0d8e0bd into containerd:main Jan 13, 2023

jsturtevant mentioned this pull request Jan 13, 2023

Add Windows HyperV container mode #1848

Merged

		Isolation flags:
		- :whale: :blue_square: :nerd_face: `--isolation=(default\|host)`: Used on Windows to change process isolation level. Valid values are `default` and `host`. `default` runs process isolated containers. `host` runs Host Process containers. Host process containers inherit permissions from containerd process unless `--user` is specified then will start with user specified. The user specified must be present on the host. `host` requires Containerd 1.6+. Not implemented for Linux.

Add Windows Hostprocess flag #795

Add Windows Hostprocess flag #795

Conversation

jsturtevant commented Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Feb 17, 2022

jsturtevant commented Feb 18, 2022

dcantah commented Feb 18, 2022

AkihiroSuda commented Jul 7, 2022

jsturtevant commented Nov 10, 2022

jsturtevant commented Jan 5, 2023

marosset commented Jan 5, 2023

dcantah commented Jan 5, 2023

dcantah commented Jan 5, 2023

jsturtevant commented Jan 9, 2023

jsturtevant commented Jan 9, 2023

dcantah commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marosset commented Jan 9, 2023

Choose a reason for hiding this comment

dcantah Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcantah Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Jan 10, 2023

dcantah left a comment

Choose a reason for hiding this comment

dcantah commented Jan 11, 2023

AkihiroSuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

jsturtevant commented Feb 8, 2022 •

edited

Loading

dcantah Jan 10, 2023 •

edited

Loading

dcantah Jan 10, 2023 •

edited

Loading