feat: make Windows worker CRI address configurable - #1241
Open
fseldow wants to merge 16 commits into
Open
Conversation
copyAndFillTemplateSpec now branches on the node's kubernetes.io/os and produces a Windows HostProcess pod spec for Windows nodes: - HostProcess pod security context (runAsUserName NT AUTHORITY\SYSTEM) and hostNetwork, replacing the Linux-only SharedSecurityContext (capabilities, seccompProfile, readOnlyRootFilesystem are invalid on Windows). - Omit the CRI hostPath volume/mount; a HostProcess pod reaches the containerd named pipe directly. - Rewrite the shared-data emptyDir mount path to its Windows form. Adds non build-tagged LinuxSharedDataPath/WindowsSharedDataPath constants and SharedDataPathForOS helper so the Linux-run manager can emit the Windows path, plus WindowsHostProcessPodSecurityContext. Includes unit tests for the Linux and Windows pod specs and node OS detection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Verified end-to-end on an AKS Windows node: the manager-emitted remover pod runs, connects to containerd over the named pipe, and removes an image. Testing surfaced three additional requirements for a Windows HostProcess worker pod, now handled in copyAndFillTemplateSpec: - Clear container resource requests/limits. HostProcess containers run on the host and do not support them; leaving them set fails the container with 'The system cannot execute the specified program'. - Set an explicit sandbox-relative command (%CONTAINER_SANDBOX_MOUNT_POINT%\\<name>.exe). The kubelet expands that env var in the pod command but not in a baked-in image ENTRYPOINT. - Generalize path translation from /run/eraser.sh to C:\\run\\eraser.sh for all container mount paths, args and command entries (covers both the shared-data emptyDir and the imagelist configmap mount + --imagelist arg). Adds LinuxEraserPath/WindowsEraserPath base constants and extends the unit tests to cover imagelist path/arg rewriting, command injection and resource clearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Set PodSpec.OS to windows for the Windows HostProcess pod. This is the official Kubernetes signal that the pod targets Windows, and it makes the apiserver enforce OS-field consistency (validateOSFields) - e.g. rejecting leftover Linux-only securityContext fields - so a future change that adds a Windows-unsupported field is caught at admission rather than silently producing a broken pod. Linux pods are unaffected (spec.OS stays unset). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Add an envtest guardrail that creates the manager-emitted Windows pod spec against a real kube-apiserver (dry-run). Because the spec declares spec.os.name=windows, the apiserver runs validateOSFields, so a future change that adds a Windows-unsupported field is caught at admission instead of only failing on a live Windows node. - Positive: the emitted Windows pod spec is accepted. - Negative: re-introducing a Linux-only container field (capabilities, what SharedSecurityContext sets) is rejected with an Invalid error. The test skips when KUBEBUILDER_ASSETS is unset so plain \go test\ still works; \make test\ provides the envtest binaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
The manager owns the launch command for Windows worker containers, so set it unconditionally instead of only when unset. This also corrects the case where a template carried a Linux-style command that would otherwise survive onto a Windows pod. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
The Windows container command is now always the manager-built %CONTAINER_SANDBOX_MOUNT_POINT%\\<name>.exe, which never contains an eraser.sh path, so translating it was a no-op. Args still need translation (e.g. --imagelist), so that loop stays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
The manager rewrites Windows paths through the general linuxToWindowsEraserPath translator, so this shared-data-only helper had no callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
A Windows memory limit is enforced job-wide via a Job Object. The remover's 30Mi Linux default is too small for the Go runtime to start there, crashing the container with STATUS_STACK_OVERFLOW (0xC00000FD) before any code runs. Rather than stripping all resources on Windows (which also dropped valid requests and CPU limits), only raise a configured memory limit up to a verified-safe minimum of 256Mi. A limit that is not set is left unset (no Job Object memory cap), and larger configured limits are preserved. Verified on Windows Server 2022 / containerd 1.7.20+azure: the emitted remover pod's memory limit is raised 30Mi -> 256Mi and the container starts and removes an image (previously it crashed at startup). Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
An explicit zero memory limit means no Job Object cap on Windows, so leave it as-is instead of raising it to 256Mi. Raising a zero limit would also make a pod invalid when the request is larger (e.g. the scanner's 500Mi request). Adds an IsZero guard and a zero-limit test case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Windows workers connect to containerd through a named pipe that cannot be hostPath-mounted like a Linux socket, so the endpoint was hardcoded in the worker. Add a configurable RuntimeSpec.WindowsAddress and propagate it to Windows workers via the ERASER_RUNTIME_ADDRESS environment variable, falling back to the default containerd named pipe when unset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xinhe Li <xinhl@microsoft.com>
fseldow
requested review from
ashnamehrotra,
pmengelbert and
sozercan
as code owners
August 28, 2026 20:16
There was a problem hiding this comment.
Pull request overview
Adds configurable Windows CRI endpoints and Windows HostProcess worker support while preserving Linux socket mounting.
Changes:
- Adds and validates
WindowsAddress, including API conversions. - Propagates the endpoint to Windows workers through an environment variable.
- Adds Linux/Windows pod-spec and endpoint tests.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/utils/utils.go |
Adds platform paths and runtime-address environment constant. |
pkg/utils/security_context.go |
Defines Windows HostProcess security context. |
pkg/utils/platform_windows.go |
Resolves the Windows CRI endpoint from the environment. |
pkg/utils/platform_windows_test.go |
Tests endpoint resolution. |
controllers/imagejob/imagejob_windows_validation_test.go |
Validates Windows pod specs against the API server. |
controllers/imagejob/imagejob_controller.go |
Builds platform-specific worker pod specifications. |
controllers/imagejob/imagejob_controller_test.go |
Tests Linux and Windows pod generation. |
api/v1alpha3/zz_generated.conversion.go |
Converts the Windows address field. |
api/v1alpha3/eraserconfig_types.go |
Adds Windows endpoint configuration and validation. |
api/unversioned/eraserconfig_types.go |
Adds the internal Windows endpoint field and validation. |
api/unversioned/eraserconfig_types_test.go |
Tests Windows endpoint defaults and validation. |
Files not reviewed (1)
- api/v1alpha3/zz_generated.conversion.go: Generated file
Suppressed comments (1)
api/v1alpha3/eraserconfig_types.go:175
tcpis accepted here, but Windows workers cannot use it:pkg/utils/platform_windows.go:35-40rejects every protocol exceptnpipe, so the collector/remover fail while creating their CRI client for any acceptedtcp://address. Either add a TCP dialer on Windows (with corresponding tests) or stop advertising/accepting TCP.
switch u.Scheme {
case "npipe", "tcp":
return addr, nil
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+173
to
+175
| switch u.Scheme { | ||
| case "npipe", "tcp": | ||
| return addr, nil |
Comment on lines
+150
to
+156
| // resolve the Windows runtime address, defaulting to the containerd named | ||
| // pipe when it is not explicitly configured. | ||
| winAddr, err := resolveWindowsRuntimeAddress(rs.WindowsAddress) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| r.WindowsAddress = winAddr |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Windows workers connect to containerd through a named pipe (
\\.\pipe\containerd-containerd), which — unlike a Linux unix socket — cannot behostPath-mounted into the worker. Because of that the Windows CRI endpoint was effectively hardcoded in the worker (pkg/utils/platform_windows.go) and the manager's Windows branch ignoredRuntimeSpecentirely, leaving no way for users (including BYO / non-default containerd setups) to configure it.This PR makes the Windows runtime address configurable and propagates it end-to-end:
RuntimeSpec.WindowsAddressconfig field (bothunversionedandv1alpha3), validated (npipe/tcpschemes) and defaulting to the containerd named pipe when unset.ERASER_RUNTIME_ADDRESSenv var (a named pipe can't be mounted, so env is the HostProcess-compatible channel). The Windows worker reads it with a fallback to the default pipe.Linux behavior is unchanged (it keeps using the hostPath socket mount).
Addresses reviewer feedback on #1236 (#1236 (comment)) — "add and propagate a Windows-specific runtime address".
Important
Stacked on #1236. This branch is based on
feat/windows-ready-imagejob-pods(the manager Windows pod-spec work), so the diff shown here includes those commits until #1236 merges. Please review/merge #1236 first; this PR should be rebased ontomainafterwards.Testing
api/unversioned: default / customnpipe/tcpaccepted / invalid scheme rejected.ERASER_RUNTIME_ADDRESS; no env injected when unset.resolveCRIPathhonors the env override and falls back to the default pipe.