[Windows] Implement executor on Windows #3517

gabriel-samfira · 2023-01-18T17:50:41Z

(I apologize for the large wall of text bellow)

This change enables the containerd executor to run on Windows. It builds on the great work started by @TBBle .

Most of the changes are just a reshuffle of existing code in smaller platform specific functions, with Windows specific changes made.

Some things happening in this PR

resolvconf and hosts file are not applicable to Windows
Ensuring the CWD exists, requires info about the user that is supposed to own it, which means we need to fetch the SID of the user residing inside the container for Windows as well. More details on that endeavor and approach bellow.
- The CWD is normally set by the WORKDIR directive, which also creates the location before the executor gets a chance to run. We still need this if we want to call the executor explicitly in a particular CWD.
On Windows, the localmounter is only needed when ensuring the CWD. In all other cases, we need to pass the layers in as they are and containerd/hcsshim will handle the proper mounting. We cannot pass in a bind mount as a root filesystem.
I am unsure if we can have a readonly root FS on Windows. Will need to check, but I doubt it.

Fetching the SID of the user

Linux uniquely identifies users on a system, using their UID. This information is stored in /etc/passwd and can easily be parsed by simply mounting the root filesystem and reading that file.

On Windows, there is an equivalent to both /etc/passwd and /etc/group in the form of a registry hive called SAM. This file can be found on all Windows systems in C:\Windows\System32\config\SAM.

This hive holds all user information on a particular system, including the SID of the user we care about. The bad news is that the data structures in this hive are completely undocumented and there is no API we can call to load the security info inside an offline SAM hive. We can load it as a registry hive, but parsing the data structures it holds is not documented. It's not impossible to do, but in the absence of an officially supported API to do this for us, we risk that sometime in the future our parser will break.

An attempt to parse the hive was made here: #3248

The alternative is to execute a utility in the context of a container to fetch the SID. This means that we need to run a binary that needs to exist in the container, which will use Windows APIs to fetch the SID of the user. To do this, we defined a small utility as a reexec function in buildkitd, which allows us to mount os.Executable() in C:\Windows\System32\get-user-info.exe and get the SID.

There are shortcuts to get the SID of well known accounts/groups, including the default ContainerAdministrator and ContainerUser. As long as there is no explicit USER stanza that defines a manually created user (via RUN net user testuser /ADD /ACTIVE:yes), this code path will be bypassed.

At this stage, this is just a draft, as it needs a number of other changes to land in various projects. A (hopefully) complete list bellow. Will amend the list if more are needed:

Depends on: Mount snapshots on Windows containerd/containerd#8043
Depends on: [Windows] Add get-user-info utility #3516
Depends on: [Windows] Handle paths on Windows #3322
Depends on: Fix copy on windows plus tests tonistiigi/fsutil#145
Depends on: Implement proper renameFile on Windows tonistiigi/fsutil#146
Depends on: Fix follow symlinkResolver on Windows tonistiigi/fsutil#147

Partially addresses: microsoft/Windows-Containers#34

Signed-off-by: Gabriel Adrian Samfira gsamfira@cloudbasesolutions.com

TBBle · 2023-01-19T11:39:50Z

util/windows/utils_windows.go

+	procInfo := executor.ProcessInfo{
+		Meta: executor.Meta{
+			Args: []string{"get-user-info", userName},
+			User: "ContainerAdministrator",


Can this run as ContainerUser?

Returns Access Denied if I remember correctly. Will double check.

tonistiigi · 2023-01-29T02:37:43Z

At this stage, this is just a draft, as it needs a number of other changes to land in various projects.

Don't grow this into a very large single commit. We'll be happy to take windows-modified updates so single packages or groups of functions without the full thing being ready. Eg. #3544 is a nice example.

gabriel-samfira · 2023-01-29T10:25:37Z

At this stage, this is just a draft, as it needs a number of other changes to land in various projects.

Don't grow this into a very large single commit. We'll be happy to take windows-modified updates so single packages or groups of functions without the full thing being ready. Eg. #3544 is a nice example.

Indeed!

I wanted to first figure out all the needed bits to get buildkitd functioning on Windows. I spoke with @TBBle , and I'll rebase his work and propose the PRs on containerd and hcsshim (hopefully this week).

Once that is done I plan to circle back and split things up, respond to reviews and get everything in a state that is easy to review and merge.

This change splits the containerdexecutor.Run() function into smaller pieces and enables it to run on Windows. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira · 2023-08-15T10:16:59Z

@tonistiigi @AkihiroSuda

I rebased this PR and it's ready for review. Most of the changes are code shuffle in smaller functions and the addition of the Windows specific bits.

PTAL if you have the time.

gabriel-samfira · 2023-08-23T17:42:04Z

CC @crazy-max @jedevc if you have some time to review, I would appreciate it.

tonistiigi

Will wcow be containerd-worker only?

executor/containerdexecutor/executor.go

tonistiigi · 2023-08-28T22:36:43Z

executor/containerdexecutor/executor_unix.go

+	return releaseAll, resolvConf, hostsFile, nil
+}
+
+func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {


This can just be a method on containerState (jobDetails), same for cwd. Not sure if makes sense for others.

Added getTaskOpts to containerState.

tonistiigi · 2023-08-28T22:42:28Z

executor/containerdexecutor/executor_unix.go

+	}, nil
+}
+
+func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (func(), string, string, error) {


It isn't ideal that this is a mix of 2 things: preparing /etc files and setting up properties of details (otherwise I would call this prepareEtcPaths. If it is already a mix, then why is CWD different? If you don't have better ideas then rename the cwd to prepareCWD so it is consitent.

It isn't ideal that this is a mix of 2 things: preparing /etc files and setting up properties of details

I can add the /etc files as fields in details and keep things consistent. This way each platform can pick and choose what they need. Would that be better?

(otherwise I would call this prepareEtcPaths.

It also records the rootfsPath on Linux and rootMounts []mount.Mount on Windows. These are used later to ensure the CWD exists and so they can also be used later in Exec(), fixing the exiting TODO.

. If it is already a mix, then why is CWD different?

The CWD is a bit different because on Windows we're pretty much forced to boot up a container to get the SID (Security Identifier) of the owner of the CWD.

There is that whole song and dance in the windows version that calls into windows.ResolveUsernameToSID(), which passes in the executor so it can resolve the user SID without reimplementing the executor elsewhere. That's why it's attached to the executor. I can attach it to containerState{} and pass in the executor (and any other parameters it needs) as an arg.

How would you prefer?

executor/containerdexecutor/executor_unix.go

executor/oci/spec.go

gabriel-samfira

Will wcow be containerd-worker only?

At this point, yes.

gabriel-samfira · 2023-08-30T09:22:20Z

executor/containerdexecutor/executor_unix.go

+	}, nil
+}
+
+func (w *containerdExecutor) prepareExecutionEnv(ctx context.Context, rootMount executor.Mount, mounts []executor.Mount, meta executor.Meta, details *jobDetails) (func(), string, string, error) {


It isn't ideal that this is a mix of 2 things: preparing /etc files and setting up properties of details

I can add the /etc files as fields in details and keep things consistent. This way each platform can pick and choose what they need. Would that be better?

(otherwise I would call this prepareEtcPaths.

It also records the rootfsPath on Linux and rootMounts []mount.Mount on Windows. These are used later to ensure the CWD exists and so they can also be used later in Exec(), fixing the exiting TODO.

. If it is already a mix, then why is CWD different?

The CWD is a bit different because on Windows we're pretty much forced to boot up a container to get the SID (Security Identifier) of the owner of the CWD.

There is that whole song and dance in the windows version that calls into windows.ResolveUsernameToSID(), which passes in the executor so it can resolve the user SID without reimplementing the executor elsewhere. That's why it's attached to the executor. I can attach it to containerState{} and pass in the executor (and any other parameters it needs) as an arg.

How would you prefer?

executor/oci/spec.go

gabriel-samfira · 2023-08-30T09:36:09Z

executor/containerdexecutor/executor_unix.go

+	return releaseAll, resolvConf, hostsFile, nil
+}
+
+func (w *containerdExecutor) getTaskOpts(ctx context.Context, details *jobDetails) (containerd.NewTaskOpts, error) {


Added getTaskOpts to containerState.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira · 2023-08-30T11:01:02Z

Failure seems to be unrelated to the changes. May be worth triggering that job again.

crazy-max added the area/windows label Jan 18, 2023

gabriel-samfira force-pushed the enable-windows-executor branch 2 times, most recently from aaed23e to 43c1ff4 Compare January 18, 2023 22:27

gabriel-samfira mentioned this pull request Jan 18, 2023

Implement readUser on Windows #3518

Merged

TBBle reviewed Jan 19, 2023

View reviewed changes

gabriel-samfira mentioned this pull request Feb 23, 2023

As a Developer I want to be able to build Windows Containers with BuildKit microsoft/Windows-Containers#34

Open

gabriel-samfira force-pushed the enable-windows-executor branch 7 times, most recently from 4c6eaad to 85cd781 Compare August 15, 2023 09:17

Implement executor on Windows

b6b322b

This change splits the containerdexecutor.Run() function into smaller pieces and enables it to run on Windows. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira force-pushed the enable-windows-executor branch from 85cd781 to b6b322b Compare August 15, 2023 09:35

gabriel-samfira marked this pull request as ready for review August 15, 2023 10:12

tonistiigi reviewed Aug 28, 2023

View reviewed changes

gabriel-samfira commented Aug 30, 2023

View reviewed changes

Cleanup args, rename jobDetails

cc5657c

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira force-pushed the enable-windows-executor branch from ce9e636 to cc5657c Compare August 30, 2023 10:40

tonistiigi approved these changes Aug 30, 2023

View reviewed changes

AkihiroSuda approved these changes Aug 30, 2023

View reviewed changes

AkihiroSuda merged commit 05eb728 into moby:master Aug 30, 2023
56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Implement executor on Windows #3517

[Windows] Implement executor on Windows #3517

gabriel-samfira commented Jan 18, 2023 •

edited

Loading

TBBle Jan 19, 2023

gabriel-samfira Jan 19, 2023

tonistiigi commented Jan 29, 2023

gabriel-samfira commented Jan 29, 2023

gabriel-samfira commented Aug 15, 2023

gabriel-samfira commented Aug 23, 2023

tonistiigi left a comment

tonistiigi Aug 28, 2023

gabriel-samfira Aug 30, 2023

tonistiigi Aug 28, 2023

gabriel-samfira Aug 30, 2023

gabriel-samfira left a comment

gabriel-samfira Aug 30, 2023

gabriel-samfira Aug 30, 2023

gabriel-samfira commented Aug 30, 2023

[Windows] Implement executor on Windows #3517

[Windows] Implement executor on Windows #3517

Conversation

gabriel-samfira commented Jan 18, 2023 • edited Loading

Some things happening in this PR

Fetching the SID of the user

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Jan 29, 2023

gabriel-samfira commented Jan 29, 2023

gabriel-samfira commented Aug 15, 2023

gabriel-samfira commented Aug 23, 2023

tonistiigi left a 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

gabriel-samfira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriel-samfira commented Aug 30, 2023

gabriel-samfira commented Jan 18, 2023 •

edited

Loading