fix: improved error handling on windows scripts baked into VHD

[Tim Wright (timmy-wright)]

What this PR does / why we need it:

Improved error handling on scripts baked into windows VHDs.

Which issue(s) this PR fixes:

Fixes #

[github-actions]

Windows Unit Test Results

  3 files   13 suites   52s ⏱️
406 tests 406 ✅ 0 💤 0 ❌
409 runs  409 ✅ 0 💤 0 ❌

Results for commit cb24704.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

This PR tightens error handling in the Windows CSE PowerShell scripts that are cached on/baked into Windows VHDs, primarily by adding explicit exit-code checks and consolidating repeated nssm.exe invocation patterns behind a helper.

Changes:

• Introduces a shared Invoke-Nssm helper (new helpers.ps1) and refactors kubelet/kube-proxy/containerd/csi-proxy/hosts-config-agent service registration to use it.
• Adds explicit $LASTEXITCODE checks for sc.exe, reg.exe import, and icacls calls to fail fast with clearer errors.
• Updates/extends Pester tests around RegisterContainerDService, and improves Windows CSE README test instructions.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
staging/cse/windows/windowsciliumnetworkingfunc.ps1	Minor formatting cleanup in install args.
staging/cse/windows/README	Adds example Pester commands for running Windows unit tests locally.
staging/cse/windows/kubeletfunc.ps1	Refactors NSSM-based kubelet/kube-proxy service setup to use Invoke-Nssm; adds tighter error checking around DependOnService setup.
staging/cse/windows/helpers.ps1	Adds Invoke-Nssm helper that throws on non-zero nssm.exe exit codes.
staging/cse/windows/containerdfunc.tests.suites/config.toml	Removes an old containerd config fixture file.
staging/cse/windows/containerdfunc.tests.ps1	Adds tests for RegisterContainerDService behavior (service exists vs not, sc.exe delete failures).
staging/cse/windows/containerdfunc.ps1	Adds small testability helpers (Get-RootRegistryPath, Out-FileAscii), adds sc.exe delete exit-code check, and refactors service registration to use Invoke-Nssm.
staging/cse/windows/configfunc.tests.ps1	Formatting/whitespace adjustments in tests.
staging/cse/windows/configfunc.ps1	Adds exit-code checks for sc.exe failure, reg.exe import, and icacls; refactors NSSM calls to use Invoke-Nssm.

Suppressed comments (2)

staging/cse/windows/containerdfunc.ps1:45

• Invoke-Nssm is used here, but it is only defined in the new staging/cse/windows/helpers.ps1 and is not dot-sourced by parts/windows/kuberneteswindowssetup.ps1.template (which only sources configfunc.ps1/containerdfunc.ps1/kubeletfunc.ps1, etc.). As a result, this will fail at runtime with "Invoke-Nssm is not recognized" unless the helper is imported.

  Write-Log "Registering containerd as a service"
  # setup containerd
  Invoke-Nssm -KubeDir $KubeDir install containerd $global:Containerdbinary
  Invoke-Nssm -KubeDir $KubeDir set containerd AppDirectory $KubeDir

staging/cse/windows/configfunc.ps1:571

• Invoke-Nssm is referenced here, but it is defined in staging/cse/windows/helpers.ps1 and is not dot-sourced by the Windows CSE entrypoint. This will throw at runtime unless the helper is imported before first use.

    $HostsConfigParameters = [io.path]::Combine($KubeDir, "hostsconfigagent.ps1")

    Invoke-Nssm -KubeDir $KubeDir install hosts-config-agent C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe
    Invoke-Nssm -KubeDir $KubeDir set hosts-config-agent AppDirectory "$KubeDir"

[Copilot]

Pull request overview

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

Suppressed comments (6)

staging/cse/windows/kubeletfunc.ps1:365

• Invoke-Nssm is used to configure kubelet/kubeproxy services, but kubeletfunc.ps1 does not ensure helpers.ps1 (which defines Invoke-Nssm) is loaded when this file is dot-sourced. This can cause New-NSSMService to fail at runtime with "Invoke-Nssm is not recognized".

Dot-source helpers.ps1 (idempotently) before the first Invoke-Nssm call in this function.

    # setup kubelet
    Invoke-Nssm -KubeDir $KubeDir install Kubelet C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe

staging/cse/windows/configfunc.ps1:549

• New-CsiProxyService uses Invoke-Nssm but configfunc.ps1 does not ensure helpers.ps1 (which defines Invoke-Nssm) is loaded in the CSE runtime. This can cause service registration to fail with "Invoke-Nssm is not recognized" when this function runs.

Dot-source helpers.ps1 (idempotently) before the first Invoke-Nssm call in this function.

    del $tempdir -Recurse

    Invoke-Nssm -KubeDir $KubeDir install csi-proxy "$KubeDir\csi-proxy.exe"

staging/cse/windows/containerdfunc.ps1:8

• Typo in comment: "overriden" should be "overridden".

# Function so it can be overriden in tests.

staging/cse/windows/containerdfunc.ps1:46

• Invoke-Nssm is called below but this script does not ensure helpers.ps1 (which defines Invoke-Nssm) is dot-sourced in the CSE runtime. As a result, RegisterContainerDService can fail at runtime with "Invoke-Nssm is not recognized" depending on load order.

Load helpers.ps1 (idempotently) before the first Invoke-Nssm call so the function is always available when this file is dot-sourced from c:\AzureData\windows\containerdfunc.ps1.

  Write-Log "Registering containerd as a service"
  # setup containerd
  Invoke-Nssm -KubeDir $KubeDir install containerd $global:Containerdbinary

staging/cse/windows/configfunc.ps1:572

• New-HostsConfigService also relies on Invoke-Nssm but there is no guarantee New-CsiProxyService ran first (or at all) to load helpers.ps1. If this is called independently, it can fail with "Invoke-Nssm is not recognized".

Dot-source helpers.ps1 (idempotently) before the first Invoke-Nssm call in this function too (or load it at file scope).

    $HostsConfigParameters = [io.path]::Combine($KubeDir, "hostsconfigagent.ps1")

    Invoke-Nssm -KubeDir $KubeDir install hosts-config-agent C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe

staging/cse/windows/helpers.ps1:13

• Invoke-Nssm relies only on $LASTEXITCODE to detect failures. If the nssm.exe invocation itself fails to start (e.g. path not found), $LASTEXITCODE may be unchanged (potentially 0) and this helper won't throw even though the command failed.

Also capture $LASTEXITCODE into a local variable before using it in the error message to avoid accidental mutation by subsequent commands.

    & "$KubeDir\nssm.exe" @NssmArguments | RemoveNulls
    if ($LASTEXITCODE -ne 0)
    {
        throw "nssm.exe $( $NssmArguments -join ' ' ) failed (exit code $LASTEXITCODE)"
    }

[Copilot]

Pull request overview

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

Suppressed comments (6)

staging/cse/windows/configfunc.ps1:549

• Invoke-Nssm is used here but it is defined in staging/cse/windows/helpers.ps1, which is not dot-sourced by the main entrypoint (parts/windows/kuberneteswindowssetup.ps1.template) nor by this script. That will cause runtime failures like "Invoke-Nssm is not recognized" when New-CsiProxyService runs. Consider dot-sourcing helpers.ps1 (guarded) before calling Invoke-Nssm.

    Invoke-Nssm -KubeDir $KubeDir install csi-proxy "$KubeDir\csi-proxy.exe"

staging/cse/windows/kubeletfunc.ps1:365

• New-NSSMService now calls Invoke-Nssm, but kubeletfunc.ps1 does not dot-source helpers.ps1 (where Invoke-Nssm is defined). Since the main entrypoint only dot-sources kubeletfunc.ps1 (and not helpers.ps1), this will fail at runtime when creating kubelet/kube-proxy services.

    Invoke-Nssm -KubeDir $KubeDir install Kubelet C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe

staging/cse/windows/containerdfunc.ps1:8

• Typo: "overriden" should be "overridden".

# Function so it can be overriden in tests.

staging/cse/windows/configfunc.ps1:572

• New-HostsConfigService also calls Invoke-Nssm but does not ensure helpers.ps1 has been dot-sourced. If New-HostsConfigService is invoked without a prior call that loads helpers.ps1, it will fail at runtime.

    Invoke-Nssm -KubeDir $KubeDir install hosts-config-agent C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe

staging/cse/windows/containerdfunc.ps1:46

• RegisterContainerDService now relies on Invoke-Nssm, but helpers.ps1 (where Invoke-Nssm is defined) is not dot-sourced by default when containerdfunc.ps1 is dot-sourced by kuberneteswindowssetup.ps1.template. This will break containerd service registration on real nodes unless helpers.ps1 is loaded.

  Invoke-Nssm -KubeDir $KubeDir install containerd $global:Containerdbinary

staging/cse/windows/kubeletfunc.ps1:389

• Using Invoke-Expression here reintroduces quoting issues (e.g., if $KubeDir contains spaces) and expands the attack surface unnecessarily. You can avoid Invoke-Expression and still pass multiple DependOnService values by splatting an argument array, keeping proper quoting and the call operator.

    $LASTEXITCODE = 0
    Invoke-Expression "$KubeDir\nssm.exe set Kubelet DependOnService $kubeletDependOnServices | RemoveNulls"
    if (-not $?) { throw "Invoke-Expression failed to invoke before calling nssm.exe (PowerShell invocation failed - exit code $LASTEXITCODE)" }
    if ($LASTEXITCODE -ne 0) { throw "nssm.exe failed to set Kubelet DependOnService (exit code $LASTEXITCODE)" }

[Copilot]

Pull request overview

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

Suppressed comments (6)

staging/cse/windows/containerdfunc.ps1:10

• Typo in comment: "overriden" → "overridden".

# Function so it can be overriden in tests.

staging/cse/windows/containerdfunc.ps1:1

• Dot-sourcing helpers.ps1 via an absolute path (c:\AzureData\windows\helpers.ps1) will fail in the Windows unit-test workflow (which runs Pester directly from the repo without provisioning that path). Since these scripts are located next to helpers.ps1 both in-repo and when baked on the VHD, use $PSScriptRoot to dot-source the helper file instead.

. c:\AzureData\windows\helpers.ps1

staging/cse/windows/kubeletfunc.ps1:1

• Dot-sourcing helpers.ps1 via an absolute path (c:\AzureData\windows\helpers.ps1) will fail in the Windows unit-test workflow (which runs Pester directly from the repo without provisioning that path). Since kubeletfunc.ps1 lives next to helpers.ps1 both in-repo and when baked on the VHD, dot-source it via $PSScriptRoot.

. c:\AzureData\windows\helpers.ps1

staging/cse/windows/configfunc.ps1:1

• Dot-sourcing helpers.ps1 via an absolute path (c:\AzureData\windows\helpers.ps1) will fail in the Windows unit-test workflow (which runs Pester directly from the repo without provisioning that path). Since configfunc.ps1 lives next to helpers.ps1 both in-repo and when baked on the VHD, dot-source it via $PSScriptRoot.

. c:\AzureData\windows\helpers.ps1

staging/cse/windows/all.ps1:3

• all.ps1 is in staging/cse/windows and is also used from c:\AzureData\windows on the VHD. Dot-sourcing helpers.ps1 via an absolute path makes running it from the repo (and any Pester test that might dot-source it) fail. Use $PSScriptRoot to dot-source helpers.ps1 so it works both in-repo and on the VHD.

. c:\AzureData\windows\helpers.ps1

staging/cse/windows/containerdfunc.ps1:230

• Typo in log message: "avalaible" → "available".

    Write-Log "Containerd hyperv logging script not avalaible"

[Tim Wright (timmy-wright)]

Review: improved error handling on Windows CSE scripts

Reviewed with multiple angle passes (architecture/simplification, security, reliability, regression, performance) plus a targeted pass on idempotency risk from the new fatal error paths. Security and performance came back clean — no material findings. Two issues below should block merge; one is a non-blocking design note.

🔴 Blocking: new absolute dot-source breaks CI/local unit tests

configfunc.ps1:1, containerdfunc.ps1:1, kubeletfunc.ps1:1, and all.ps1:3 all add . c:\AzureData\windows\helpers.ps1 (hardcoded absolute path). That path only exists once bootstrapped on a real node — it doesn't exist when Pester loads these scripts directly from the repo.

Repro:

pwsh -NoProfile -NonInteractive -Command "$ErrorActionPreference='Stop'; . 'staging/cse/windows/configfunc.ps1'"

fails immediately at line 1 because the path is absent. Running the actual Pester suite for configfunc.tests.ps1 produces 0/17 passing for the same reason. containerdfunc.tests.ps1 was patched to pre-load helpers via . $PSScriptRoot\helpers.ps1, but that doesn't help — containerdfunc.ps1's own absolute-path line still fires and throws regardless. configfunc.tests.ps1 and kubeletfunc.tests.ps1 weren't patched at all. This matches the PR's own CI comment (124/393 tests failing, 127 failed runs) exactly.

Fix: use . $PSScriptRoot\helpers.ps1 in all four files instead of the hardcoded absolute path. windows_package_cse.sh copies staging/cse/windows/* flat into the deployed package, so $PSScriptRoot resolves correctly both in-repo and on a provisioned node.

🔴 Conditional regression: re-invocation after a partial failure can now hard-fail where it previously succeeded

helpers.ps1's new Invoke-Nssm throws on any non-zero nssm.exe exit code — including nssm.exe install <service> when the service already exists. Previously that exit code was never checked, so a subsequent nssm.exe set ... would still reconfigure the already-existing service and provisioning proceeded. Affected installs (no idempotency/cleanup before install):

• configfunc.ps1:550 (New-CsiProxyService) and configfunc.ps1:573 (New-HostsConfigService)
• kubeletfunc.ps1:367 (Kubelet) and kubeletfunc.ps1:394 (Kubeproxy)
• containerdfunc.ps1:48, immediately following a best-effort sc.exe delete containerd at :43 — the delete failing/racing can still leave the service present when install runs

The template has no built-in CSE retry loop, but it only guards re-entry via $CSEResultFilePath (kuberneteswindowssetup.ps1.template:686-689,738-741) — so an external re-invocation after a partial BasePrep failure will revisit these functions against a VM that already has some services registered from the prior attempt, and would now hard-fail instead of proceeding. The new Pester tests mock Invoke-Nssm directly, so they don't exercise this install-conflict path.

Fix: make each service registration idempotent — best-effort remove/reconfigure an existing service before install (mirroring the existing sc.exe delete containerd exemption), or explicitly tolerate the "service already exists" case and continue to the set calls. Worth adding a test that asserts behavior when Invoke-Nssm install hits an already-registered service.

🟡 Non-blocking design note: Invoke-Nssm duplicates Invoke-Executable

windowscsehelper.ps1 already has Invoke-Executable (supports AllowedExitCodes, retries, and funnels failures through Set-ExitCode), and configfunc.ps1 already depends on it today pre-PR. Worth considering reuse instead of a parallel Invoke-Nssm/manual $LASTEXITCODE pattern — but it's not a trivial substitution: Invoke-Executable's failure path calls Set-ExitCode, which exits the process immediately rather than throwing, so call sites that currently catch Invoke-Nssm's throw to assign a specific granular exit code (e.g. the GMSA registry-import error) would need restructuring. Not a merge blocker, just a follow-up worth scoping separately.

[Tim Wright (timmy-wright)]

Update: both blocking issues addressed

Dot-source fix (already pushed separately): configfunc.ps1, containerdfunc.ps1, kubeletfunc.ps1, and all.ps1 now use . $PSScriptRoot\helpers.ps1 instead of the hardcoded absolute path, resolving the CI/Pester failures.

Idempotency regression fix (this push): Addressed the conditional regression where Invoke-Nssm now throws on "service already exists" for a service left over from a prior partial provisioning attempt:

• Added a shared Remove-ServiceIfExists helper in helpers.ps1, generalizing the best-effort sc.exe delete cleanup pattern already used in RegisterContainerDService (best-effort: tolerates non-zero exit, e.g. 1072 "marked for deletion").
• Refactored RegisterContainerDService to use the shared helper instead of its inline duplicate.
• Applied the same best-effort pre-delete before nssm.exe install in New-CsiProxyService, New-HostsConfigService (configfunc.ps1), and New-NSSMService for Kubelet/Kubeproxy (kubeletfunc.ps1) — the four call sites that previously lacked this protection.
• Added Pester regression tests mirroring the existing RegisterContainerDService "service already exists" tests for all four sites plus Remove-ServiceIfExists directly (helpers.tests.ps1, new file).

Validated locally via pwsh/Pester (7.6.4): compared pass/fail counts before and after each commit to confirm zero regressions — all pre-existing local failures are unrelated Linux-sandbox limitations (missing Windows-only cmdlets like Get-Service/Start-Service, and a Pester 6.0.1 Assert-MockCalled compatibility quirk that predates this PR).

Commits: 0caf58a, 4da773f, a6d8e7c, d39c42b, b2eea5b.

[Copilot]

Pull request overview

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

Suppressed comments (2)

staging/cse/windows/containerdfunc.ps1:13

• Spelling in the new comment: "overriden" should be "overridden".

# Function so it can be overriden in tests.
function Get-RootRegistryPath {
  return "C:\ProgramData\containerd\certs.d"
}

staging/cse/windows/kubeletfunc.ps1:386

• The comment mentions 'docker csi-proxy', but the actual dependency list here is built from containerd/csi-proxy/hosts-config-agent. This makes the rationale confusing when reading the code later.

    # Do not use Invoke-Nssm when calling DependOnService since 'docker csi-proxy'
    # is parsed as a single string instead of two separate strings
    $LASTEXITCODE = 0

[Copilot]

Pull request overview

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

Suppressed comments (2)

staging/cse/windows/kubeletfunc.tests.ps1:443

• This mock for Get-Service ignores the requested -Name and always returns an object named 'Kubelet'. That makes the test less representative and could still pass even if the production code queried the wrong service name. Mocking should reflect the input service name so the test fails if the wrong service is deleted.

        BeforeEach {
            $mockExistingSvc = [PSCustomObject]@{Name = 'Kubelet'; Status = 'Stopped'}
            Mock Get-Service -MockWith { return $mockExistingSvc }
        }

staging/cse/windows/kubeletfunc.ps1:389

• $LASTEXITCODE is reset and checked using an unscoped variable inside the function. In PowerShell, native commands update $global:LASTEXITCODE; assigning to $LASTEXITCODE here can create a local variable that shadows the global, so the subsequent checks may incorrectly see 0 even if nssm.exe fails.

    $LASTEXITCODE = 0
    Invoke-Expression "$KubeDir\nssm.exe set Kubelet DependOnService $kubeletDependOnServices | RemoveNulls"
    if (-not $?) { throw "Invoke-Expression failed to invoke before calling nssm.exe (PowerShell invocation failed - exit code $LASTEXITCODE)" }
    if ($LASTEXITCODE -ne 0) { throw "nssm.exe failed to set Kubelet DependOnService (exit code $LASTEXITCODE)" }

[Copilot]

Pull request overview

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

Suppressed comments (3)

staging/cse/windows/containerdfunc.ps1:10

• Typo in comment: “overriden” should be “overridden”.

# Function so it can be overriden in tests.

staging/cse/windows/helpers.ps1:17

• Remove-ServiceIfExists calls sc.exe delete but does not wait for the service to actually disappear. On Windows, deletes are asynchronous and a service can remain present/marked-for-deletion, causing the subsequent nssm.exe install to fail even though delete was attempted. Consider waiting (with a short timeout) for Get-Service to return null after the delete attempt, and log if it never clears.

    $svc = Get-Service -Name $ServiceName -ErrorAction SilentlyContinue
    if ($null -ne $svc) {
        sc.exe delete $ServiceName
        # sc.exe delete can legitimately return non-zero here (e.g. 1072 - service already marked for deletion)
        # since this is best-effort cleanup of a pre-existing service, don't treat that as fatal.

staging/cse/windows/kubeletfunc.ps1:389

• Using Invoke-Expression here is avoidable and makes the call harder to reason about (and potentially risky if inputs ever change). You can pass the dependency list to nssm.exe as discrete arguments without Invoke-Expression, which also makes the comment about space-delimited parsing unnecessary.

    # Do not use Invoke-Nssm when calling DependOnService since 'docker csi-proxy'
    # is parsed as a single string instead of two separate strings
    $LASTEXITCODE = 0
    Invoke-Expression "$KubeDir\nssm.exe set Kubelet DependOnService $kubeletDependOnServices | RemoveNulls"
    if (-not $?) { throw "Invoke-Expression failed to invoke before calling nssm.exe (PowerShell invocation failed - exit code $LASTEXITCODE)" }

[Copilot]

Pull request overview

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

Suppressed comments (3)

staging/cse/windows/containerdfunc.ps1:12

• Typo in the new comment: “overriden” should be “overridden”.

# Function so it can be overriden in tests.
function Get-RootRegistryPath {
  return "C:\ProgramData\containerd\certs.d"

staging/cse/windows/helpers.ps1:30

• Invoke-Nssm only checks $LASTEXITCODE. If PowerShell fails to invoke nssm.exe (missing file, permission, etc.), $LASTEXITCODE can retain a previous value and this function may not throw. Check $? after invocation so invocation failures are surfaced reliably.

    & "$KubeDir\nssm.exe" @NssmArguments | RemoveNulls
    if ($LASTEXITCODE -ne 0)
    {
        throw "nssm.exe $( $NssmArguments -join ' ' ) failed (exit code $LASTEXITCODE)"
    }

staging/cse/windows/kubeletfunc.ps1:389

• This block uses Invoke-Expression to call nssm.exe even though Invoke-Nssm exists, and the comment mentions "docker csi-proxy" even though $kubeletDependOnServices is built from containerd/csi-proxy/hosts-config-agent. Consider avoiding Invoke-Expression by splitting the dependency string into arguments and calling Invoke-Nssm, which also centralizes error handling.

    # Do not use Invoke-Nssm when calling DependOnService since 'docker csi-proxy'
    # is parsed as a single string instead of two separate strings
    $LASTEXITCODE = 0
    Invoke-Expression "$KubeDir\nssm.exe set Kubelet DependOnService $kubeletDependOnServices | RemoveNulls"
    if (-not $?) { throw "Invoke-Expression failed to invoke before calling nssm.exe (PowerShell invocation failed - exit code $LASTEXITCODE)" }