Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Windows HostProcess Support #5131

Merged
merged 1 commit into from
Aug 20, 2021
Merged

Add Windows HostProcess Support #5131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/cri/annotations/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,7 @@ const (

// PodAnnotations are the annotations of the pod
PodAnnotations = "io.kubernetes.cri.pod-annotations"

// WindowsHostProcess is used by hcsshim to identify windows pods that are running HostProcesses
WindowsHostProcess = "microsoft.com/hostprocess-container"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be com.microsoft.xxx as like other annotations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotation was following the Kubernetes guidelines https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set. This is what it's set to/k8s checks against so there's not much we could do here at the moment in terms of changing it. As of right now in 1.22, Kubernetes is setting this annotation explicitly if the HostProcess field is filled in so that containerd doesn't need to be aware of the cri fields (even though that's what this PR is adding now that the 1.22 deps are vendored in 😆). In 1.23 of k8s I believe they're removing this explicit addition of the annotation if this feature goes to beta so we can change the annotation name then, as the only other change would just be renaming it in our windows containerd-shim to match once k8s no longer needs to care how it's named.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know that.

https://github.com/kubernetes/kubernetes/blob/7705b300e2085c3864bb1e49a7302bf17f080219/pkg/kubelet/kuberuntime/labels.go#L46-L50

Copy link
Member

@kevpar kevpar Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation is actually a bit weird right now. k8s states annotations should be in the form foo.com/bar-baz, while OCI states they should be in the form com.foo.bar-baz.

Probably the most technically correct thing to do would be for the CRI plugin to translate from the k8s form to the OCI form, but it doesn't do that today.

)
3 changes: 3 additions & 0 deletions pkg/cri/server/container_create_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package server

import (
"strconv"

"github.com/containerd/containerd/oci"
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
Expand Down Expand Up @@ -118,6 +120,7 @@ func (c *criService) containerSpec(
customopts.WithAnnotation(annotations.SandboxName, sandboxConfig.GetMetadata().GetName()),
customopts.WithAnnotation(annotations.ContainerName, containerName),
customopts.WithAnnotation(annotations.ImageName, imageName),
customopts.WithAnnotation(annotations.WindowsHostProcess, strconv.FormatBool(sandboxConfig.GetWindows().GetSecurityContext().GetHostProcess())),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need a check like this to disallow creating a HostProcess container in a non-HostProcess pod?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I saw it was merged already :) Might be worth addressing in a follow-up PR though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually handle this in the shim right now, but wouldnt hurt here also if we remove that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right place for it

)
return c.runtimeSpec(id, ociRuntime.BaseRuntimeSpec, specOpts...)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/cri/server/container_create_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox
SecurityContext: &runtime.WindowsContainerSecurityContext{
RunAsUsername: "test-user",
CredentialSpec: "{\"test\": \"spec\"}",
HostProcess: false,
},
},
}
Expand Down Expand Up @@ -130,6 +131,9 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox

assert.Contains(t, spec.Annotations, annotations.SandboxName)
assert.EqualValues(t, spec.Annotations[annotations.SandboxName], "test-sandbox-name")

assert.Contains(t, spec.Annotations, annotations.WindowsHostProcess)
assert.EqualValues(t, spec.Annotations[annotations.WindowsHostProcess], "false")
}
return config, sandboxConfig, imageConfig, specCheck
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/cri/server/restart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,9 @@ func (c *criService) loadSandbox(ctx context.Context, cntr containerd.Container)
// Don't need to load netns for host network sandbox.
return sandbox, nil
}
if goruntime.GOOS == "windows" && meta.Config.GetWindows().GetSecurityContext().GetHostProcess() {
return sandbox, nil
}
sandbox.NetNS = netns.LoadNetNS(meta.NetNSPath)

// It doesn't matter whether task is running or not. If it is running, sandbox
Expand Down
8 changes: 7 additions & 1 deletion pkg/cri/server/sandbox_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,18 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
log.G(ctx).Debugf("Use OCI %+v for sandbox %q", ociRuntime, id)

podNetwork := true
// Pod network is always needed on windows.

if goruntime.GOOS != "windows" &&
config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetNetwork() == runtime.NamespaceMode_NODE {
// Pod network is not needed on linux with host network.
podNetwork = false
}
if goruntime.GOOS == "windows" &&
config.GetWindows().GetSecurityContext().GetHostProcess() {
//Windows HostProcess pods can only run on the host network
podNetwork = false
}

if podNetwork {
// If it is not in host network namespace then create a namespace and set the sandbox
// handle. NetNSPath in sandbox metadata and NetNS is non empty only for non host network
Expand Down
3 changes: 3 additions & 0 deletions pkg/cri/server/sandbox_run_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package server

import (
"strconv"

"github.com/containerd/containerd"
"github.com/containerd/containerd/oci"
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
Expand Down Expand Up @@ -65,6 +67,7 @@ func (c *criService) sandboxContainerSpec(id string, config *runtime.PodSandboxC
customopts.WithAnnotation(annotations.SandboxNamespace, config.GetMetadata().GetNamespace()),
customopts.WithAnnotation(annotations.SandboxName, config.GetMetadata().GetName()),
customopts.WithAnnotation(annotations.SandboxLogDir, config.GetLogDirectory()),
customopts.WithAnnotation(annotations.WindowsHostProcess, strconv.FormatBool(config.GetWindows().GetSecurityContext().GetHostProcess())),
)

return c.runtimeSpec(id, "", specOpts...)
Expand Down
10 changes: 10 additions & 0 deletions pkg/cri/server/sandbox_run_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ func getRunPodSandboxTestData() (*runtime.PodSandboxConfig, *imagespec.ImageConf
LogDirectory: "test-log-directory",
Labels: map[string]string{"a": "b"},
Annotations: map[string]string{"c": "d"},
Windows: &runtime.WindowsPodSandboxConfig{
SecurityContext: &runtime.WindowsSandboxSecurityContext{
RunAsUsername: "test-user",
CredentialSpec: "{\"test\": \"spec\"}",
HostProcess: false,
},
},
}
imageConfig := &imagespec.ImageConfig{
Env: []string{"a=b", "c=d"},
Expand Down Expand Up @@ -70,6 +77,9 @@ func getRunPodSandboxTestData() (*runtime.PodSandboxConfig, *imagespec.ImageConf

assert.Contains(t, spec.Annotations, annotations.SandboxLogDir)
assert.EqualValues(t, spec.Annotations[annotations.SandboxLogDir], "test-log-directory")

assert.Contains(t, spec.Annotations, annotations.WindowsHostProcess)
assert.EqualValues(t, spec.Annotations[annotations.WindowsHostProcess], "false")
}
return config, imageConfig, specCheck
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/cri/server/sandbox_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ func (c *criService) getIPs(sandbox sandboxstore.Sandbox) (string, []string, err
// responsible for reporting the IP.
return "", nil, nil
}
if goruntime.GOOS == "windows" && config.GetWindows().GetSecurityContext().GetHostProcess() {
return "", nil, nil
}

if closed, err := sandbox.NetNS.Closed(); err != nil {
return "", nil, errors.Wrap(err, "check network namespace closed")
Expand Down