Skip to content

feat: manager emits Windows ImageJob pods - #1236

Open
fseldow wants to merge 13 commits into
eraser-dev:mainfrom
fseldow:feat/windows-ready-imagejob-pods
Open

feat: manager emits Windows ImageJob pods#1236
fseldow wants to merge 13 commits into
eraser-dev:mainfrom
fseldow:feat/windows-ready-imagejob-pods

Conversation

@fseldow

@fseldow fseldow commented Aug 24, 2026

Copy link
Copy Markdown

What this PR does / why we need it:

Summary

copyAndFillTemplateSpec now branches on the node's kubernetes.io/os:

Windows nodes produce a HostProcess pod spec:

  • Pod-level HostProcess security context, runAsUserName: NT AUTHORITY\SYSTEM, hostNetwork: true.
  • Drops the Linux-only SharedSecurityContext (capabilities / seccompProfile / readOnlyRootFilesystem are invalid on Windows).
  • Omits the CRI hostPath volume + mount — a HostProcess pod reaches the containerd named pipe directly.
  • Rewrites the shared-data emptyDir mount to its Windows form (C:\run\eraser.sh\shared-data).

Linux nodes keep existing behavior (refactored into fillLinuxPodSpec).

Supporting changes

  • pkg/utils: non build-tagged LinuxSharedDataPath / WindowsSharedDataPath constants + SharedDataPathForOS helper so the Linux-run manager can emit the Windows path; WindowsHostProcessPodSecurityContext + WindowsHostProcessUserName.
  • New imagejob_controller_test.go covering Linux spec, Windows spec, and node OS detection.

Validation

  • GOOS=linux go build ./... / go vet ./... pass
  • go test ./controllers/imagejob/... ./pkg/utils/... pass
  • gofmt clean

Follow-ups (out of scope here)

  • Align the Windows shared-data path with the worker IPC constants (coordination with Charles).
  • Windows Dockerfile stage / PLATFORM extension.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

fseldow and others added 2 commits August 24, 2026 02:22
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>
@fseldow
fseldow force-pushed the feat/windows-ready-imagejob-pods branch from a10f870 to 741b7c9 Compare August 24, 2026 09:22
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>
@fseldow
fseldow force-pushed the feat/windows-ready-imagejob-pods branch from 12bc23f to 144bb7f Compare August 24, 2026 17:29
fseldow and others added 8 commits August 24, 2026 18:34
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>
// program").
c.Resources = corev1.ResourceRequirements{}

c.Command = []string{windowsSandboxMountEnv + `\` + c.Name + ".exe"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could tie binary name to container name. Right now the scanner container's name is hardcoded as trivy-scanner regardless of the configured image. Meaning the custom scanner must ship a binary literally called trivy-scanner.exe. which is going to be confusing.

Better in the dockerfile's ENTRYPOINT or configurable per component. The problem is we don't have the windows dockerfile yet....

Should we consider fix the hardcoded trivy-scanner in imagecollector_controller.go:421?

@fseldow fseldow Aug 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

will leave it in next pr.
For discussion, how do you think we could pass the scanner name?
though scanner filed in configmap?

@fseldow fseldow Aug 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

if it is through configmap. Windows and linux may need different configs. I may have two options:

  1. Still share one configmap and one controller. But add windows level config
components:
  scanner:
    image: {repo: .../remover, tag: v1.5}      #default
    windows:                                     # Windows override
      image: {repo: .../remover-win, tag: v1.5}
manager:
  runtime:
    address: unix:///...                         
    windowsAddress: npipe://./pipe/containerd-containerd

Advantage: one controller pod + support empty node filter
Disadvantage: more complex configmap exposed to customer

  1. We setup two controller, each controller uses its own configmap. one for linux and another for windows.
eraser-controller-manager-xxx (eraser-manager-config)
eraser-controller-manager-windows-xxx (eraser-manager-config-win)

Advantage: no complex config, less change on configmap structure
Disadvantage: if customer sets incorrect node filter in one of the configmaps. Windows/linux scanner may not work as expected

@charleswool

Copy link
Copy Markdown
Contributor

Also can we run a standalone windows e2e testing and add the testing results similar to the Standalone E2E test results sections in #1229 description?

@fseldow

fseldow commented Aug 26, 2026

Copy link
Copy Markdown
Author

@charleswool Ran a standalone Windows E2E against a real AKS Windows node (mirroring the harness style from #1229, since upstream has no Windows CI). Keeping the runner script out of this PR — it lives on a separate branch of my fork.

Unlike the #1229 transport spike, this drives the actual controller path this PR changes: ImageList -> imagejob controller -> copyAndFillTemplateSpec (Windows branch) -> a HostProcess pod on the Windows node -> the remover connecting to containerd over the named pipe and deleting the requested image. The remover is this repo''s own binary (cri.NewRemoverClient -> utils.GetConn -> gRPC over the pipe), not crictl.

Standalone E2E test results

Harness — where the tooling lives
Manual E2E runner hack/windows-e2e.ps1 (on my fork''s windows-e2e-harness branch, not part of this PR)

The runner deploys the PR-head manager with a Windows-only node filter, seeds an unused image on the Windows node via a one-shot pod, waits for containerd to reclaim the exited seed container, triggers an ImageList job, then asserts on the emitted pod shape and the remover''s result.

Environment
Cluster AKS 1.35 (kubelet v1.35.6)
Node Windows Server 2022 Datacenter, build 10.0.20348
Runtime containerd 1.7.20+azure
Pod HostProcess, base mcr.microsoft.com/windows/nanoserver:ltsc2022
Identity runAsUserName: NT AUTHORITY\SYSTEM
Endpoint npipe://./pipe/containerd-containerd
Live results — commit e32bb7d6, the head of this PR
=== code under test ===
e32bb7d6 Merge branch ''main'' into feat/windows-ready-imagejob-pods

=== target windows node ===
  node    : akswin000000
  osImage : Windows Server 2022 Datacenter
  runtime : containerd://1.7.20+azure

=== deploy manager from PR head (windows node filter) ===
  manager : <registry>/eraser-manager:e2e-e32bb7d6

=== seed an unused image on the windows node ===
  seeded  : mcr.microsoft.com/windows/servercore:ltsc2022 (waiting for the exited container to be reclaimed)

=== emitted pod shape (what this PR produces) ===
  hostNetwork    : true
  os.name        : windows
  hostProcess    : true
  runAsUserName  : NT AUTHORITY\SYSTEM
  command        : %CONTAINER_SANDBOX_MOUNT_POINT%\remover.exe
  imagelist arg  : --imagelist=C:\run\eraser.sh\imagelist\images
  has CRI mount  : False

=== remover result over the named pipe ===
remover starting     imageListPtr=C:\run\eraser.sh\imagelist\images  criPath=npipe://./pipe/containerd-containerd
CRI client created successfully
successfully parsed image list file  count=1
removed image  mcr.microsoft.com/windows/servercore:ltsc2022  (sha256:f02b623b...)
  pod phase : Succeeded (exit 0)

RESULT: PASS
Note

An image still referenced by a container record (e.g. right after the seed pod exits, before containerd reclaims it) is correctly skipped by the remover as in-use — the harness waits for that reclaim before triggering. This matches the remover''s existing "image is running" guard and is not Windows-specific.

I also separately verified an empty node filter on a mixed Linux+Windows cluster: a single ImageJob fans out to both nodes and both remover pods complete (exit 0) — the Linux node takes the untouched Linux path (hostPath CRI socket mount, /run/eraser.sh/...) and the Windows node takes the HostProcess path (npipe, C:\run\eraser.sh\...).

@fseldow
fseldow force-pushed the feat/windows-ready-imagejob-pods branch from c94ad07 to e32bb7d Compare August 26, 2026 09:23
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.47170% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/utils/security_context.go 0.00% 8 Missing ⚠️
controllers/imagejob/imagejob_controller.go 88.88% 3 Missing and 2 partials ⚠️
Flag Coverage Δ
unittests 5.97% <75.47%> (-8.87%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/utils/utils.go 19.07% <ø> (+7.24%) ⬆️
controllers/imagejob/imagejob_controller.go 14.16% <88.88%> (ø)
pkg/utils/security_context.go 0.00% <0.00%> (ø)

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

// support CPU/memory requests or limits; leaving them set makes the
// container fail to start ("The system cannot execute the specified
// program").
c.Resources = corev1.ResourceRequirements{}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

resource limits should be supported for windows as well. Could we confirm the Kubernetes/containerd version and the specific repro for the startup error?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

containerd 1.7.20+azure
K8S version 1.35.6

The issue only happens when memory limit is set 30mi.

State:          Terminated
      Reason:       Error
      Exit Code:    -1073741571
      Started:      Wed, 26 Aug 2026 21:36:03 -0700
      Finished:     Wed, 26 Aug 2026 21:36:03 -0700
    Ready:          False

sorry my bad, it is caused by too small memory limit in my test. Will remain the resource limits

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

update the logic. If found memory limit < 256Mi, then force raise it to 256Mi

Copilot AI balanced review requested due to automatic review settings August 27, 2026 05:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds Windows HostProcess pod generation for ImageJobs while preserving Linux behavior.

Changes:

  • Adds OS-specific pod shaping, paths, security contexts, and memory limits.
  • Adds Windows pod validation and controller tests.
  • Adds a manual AKS Windows E2E script.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/utils/utils.go Adds Linux and Windows path constants.
pkg/utils/security_context.go Adds HostProcess security context helpers.
controllers/imagejob/imagejob_controller.go Generates OS-specific worker pod specs.
controllers/imagejob/imagejob_controller_test.go Tests Linux and Windows pod generation.
controllers/imagejob/imagejob_windows_validation_test.go Tests Kubernetes API validation.
hack/windows-e2e.ps1 Exercises manual removal on an AKS Windows node.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// is below the verified-safe minimum; leave an unset limit untouched.
raiseWindowsMemoryLimit(c)

c.Command = []string{windowsSandboxMountEnv + `\` + c.Name + ".exe"}

@fseldow fseldow Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

will leave the scanner in next pr.
windows will not be able to use trivy as scanner. We will only support BYO scanner image for windows.

Comment thread hack/windows-e2e.ps1 Outdated
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>
@fseldow
fseldow force-pushed the feat/windows-ready-imagejob-pods branch from 2d6cbe2 to e2cbc80 Compare August 27, 2026 05:08
Copilot AI review requested due to automatic review settings August 27, 2026 05:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

controllers/imagejob/imagejob_controller.go:650

  • Deriving the executable from c.Name breaks the documented custom-scanner path. The image collector always names that container trivy-scanner even when components.scanner.image points to an arbitrary custom scanner, and Linux currently relies on the image's entrypoint. A valid Windows custom image whose executable is not trivy-scanner.exe will therefore start a nonexistent path. Preserve a HostProcess-compatible configured command/entrypoint or carry an explicit executable path instead of deriving it from the Kubernetes container name.
		c.Command = []string{windowsSandboxMountEnv + `\` + c.Name + ".exe"}

Comment thread controllers/imagejob/imagejob_controller_test.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Xinhe Li <xinhl@microsoft.com>
Copilot AI review requested due to automatic review settings August 27, 2026 06:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

controllers/imagejob/imagejob_controller.go:650

  • This replaces every configured image entrypoint with an executable name derived from the Kubernetes container name. The scanner image is explicitly configurable for custom scanners (config/manager/controller_manager_config.yaml:43-55), but the config exposes no command/path field (api/unversioned/eraserconfig_types.go:147-157), so a valid Windows BYO scanner whose executable is not trivy-scanner.exe will fail before startup. Since BYO images are the intended Windows scanner path, preserve an image-provided HostProcess-aware entrypoint or add an explicit Windows executable/command contract instead of inferring it from c.Name.
		c.Command = []string{windowsSandboxMountEnv + `\` + c.Name + ".exe"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants