You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: migrate Windows ACR credential provider to use Dalec packages - #9275
What this PR does / why we need it:
Windows nodes now install the out-of-tree ACR credential provider from the Dalec-built package resolved from components.json for k8s version ≥ 1.33. This aligns Windows with the Linux PMC migration (the Linux gate was lowered to 1.33 in #9205).
Manual validation
Used VHDs from build 186517398 to add Windows test node pools in westus3 through aksdev with custom-image headers. Inspected configfunc.ps1 and CustomDataSetupScript.log.
OS / Kubernetes
Observed result
Windows 2022 / 1.32.11
New CSE logged the below-1.33 gate and used cached legacy provider 1.32.8.
Windows 2022 / 1.33
Selected cached Dalec provider 1.33.19-1.
Windows 2025 / 1.37.1
Selected cached Dalec provider 1.37.0-1.
No provider re-download or installation-failure fallback was shown.
Scope: Validated CSE/provider selection from logs.
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the runtime scripts to look at what is in the cache rather than rely on component.json - that decouples our scripts from the structure in compoents.json and makes future changes easier.
🟡 Medium Risk — Script Logic: The unanchored pattern also treats /hotfix/cloud-provider-azure/v1.33.3/binaries/azure-acr-credential-provider-windows-amd64-v1.33.3.tar.gz on a stock host as a stock URL. On Kubernetes ≥1.33, a successful Dalec installation then bypasses the explicitly requested hotfix. Anchor the pattern with ^ so only the root stock path opts into Dalec, and add a regression case for a custom-prefixed path on a stock host.
Review: Windows Dalec credential provider migration
What is good
This is a thoughtful migration with a fast common path and explicit compatibility handling.
Cache-first operation: a matching cached ZIP avoids metadata parsing and network access. Extraction uses temporary storage without modifying the cached archive.
Correct version ordering: numeric patch and package-revision comparisons avoid lexical ordering mistakes. Strict matching excludes unrelated files, prereleases, backups, and unsupported architectures.
Controlled behavior changes: the Kubernetes 1.33 gate preserves older-version behavior. Custom URLs bypass automatic Dalec selection, and network-isolated provisioning retains its ORAS path.
Older-VHD compatibility: metadata resolution supports both the older PMC layout and the dedicated Windows Dalec component.
Safe template handling: URL tokens are substituted literally rather than evaluated as PowerShell. HTTPS, unresolved-token, and expected-filename checks reject malformed download URLs.
Useful diagnostics and tests: logs explain selection and fallback decisions. Tests exercise cache misses, numeric ordering, metadata compatibility, sovereign endpoints, custom URLs, retries, and extraction failures. All 67 focused credential-provider tests passed locally.
Scope and recommendation
Reviewed PR #9275, specifically staging/cse/windows/configfunc.ps1 and its directly relevant callers, helpers, metadata, and tests.
I would address comments 1 and 2 before merging. Comments 3 and 4 are non-blocking improvements; comment 5 is a nitpick. The overall design does not need a framework or broad refactor.
Review comments, highest importance first
1. Blocking: validate the extracted provider before treating Dalec as successful
Successful ZIP extraction does not establish that azure-acr-credential-provider.exe exists. A valid ZIP without that file leaves the Dalec try block successfully, so the legacy fallback is skipped. The later copies fail outside the fallback boundary.
I reproduced this using a real ZIP containing only a README and PowerShell Copy-Item semantics. With ErrorActionPreference=Continue, the function returned without throwing, without falling back, and without installing the provider. With Stop, it threw but still did not attempt fallback.
The copy statements are pre-existing; the issue in the new path is that its fallback-protected operation ends before establishing a usable provider. A bad package could leave a node unable to authenticate private ACR image pulls.
Requested change: check that the expected executable exists inside the Dalec try block, so a missing file triggers fallback. Make final installation-copy errors terminating. Add a regression test using a valid ZIP without the executable; the existing installation tests mock the copies.
Design basis: moderate evidence. Aligning the success boundary with the actual operation makes control flow easier to reason about; the failure itself was directly reproduced. The trade-off is a small amount of explicit validation, not a larger installer abstraction.
2. Blocking: bound the optional download before relying on fallback
On a Dalec cache miss, the new metadata download runs before the legacy helper checks for an available cached RP archive. No explicit request timeout is supplied. Five retries bound the attempt count, not the duration of an attempt, so a stalled request can prevent the cached fallback from being reached within the provisioning budget.
Even immediately failing attempts introduce 40 seconds of retry sleeps before fallback. The tests explicitly cover attempting the download before reusing an available legacy archive.
Requested change: give this optional download a finite timeout and a retry budget appropriate to provisioning. Preserve the preference for Dalec, but bound how long it can delay a usable legacy source. Test the configured budget and fallback behavior.
The absence of timeouts also exists in the shared legacy downloader. The newly introduced regression is placing an additional network dependency ahead of a legacy cache hit that previously needed no download.
3. Non-blocking: retain the shared downloader's operational safeguards
Category: performance / consistency.
Location:configfunc.ps1:703-712, compared with parts/windows/windowscsehelper.ps1:223-284.
The new helper repeats the HTTP retry operation but does not preserve the existing helper's progress suppression or download-duration telemetry. It also has no equivalent explicit TLS setup. This creates two download paths whose operational behavior can drift.
Suggestion: preserve progress suppression and useful timing diagnostics, and check whether the existing TLS setup is required on supported Windows images. Do not blindly call DownloadFileOverHttp: its exit-based failure handling would defeat the new fallback contract.
Design basis: weak evidence / practitioner guidance. One coherent download policy reduces maintenance drift. A shared throwing download core could eventually help, but changing the independently released helper boundary expands scope and compatibility risk. Matching necessary safeguards locally is reasonable for this PR. No Windows-specific performance regression was measured.
4. Non-blocking: consider sharing the package-version rule, not the whole resolver
The cache and metadata resolvers duplicate the package-version grammar, numeric comparison representation, and Kubernetes major/minor compatibility rule.
Suggestion: consider a small shared parser for that domain rule. Keep filename matching, cache enumeration, and metadata traversal specific to their sources.
Design basis: weak evidence / practitioner guidance. Sharing authoritative rules can prevent inconsistent future edits, but two similar blocks alone do not justify an abstraction. Retaining the duplication is acceptable if a helper would make the code harder to follow. A generic resolver framework would be unnecessary.
5. Nitpick: name the eligibility condition after its purpose
$isStockLegacyUrl is also true when the RP URL is empty. The name describes only one reason the condition permits Dalec selection.
Suggestion: rename it to $allowDalecSelection.
Design basis: moderate evidence. Names that describe actual behavior improve comprehension. This is a local clarity improvement with no intended behavior change; it should not hold up the PR.
Security assessment
No confirmed new exploitable vulnerability was found. Literal template substitution and protected metadata are positive controls.
The new path still relies on trusted cache contents and package distribution rather than adding hash or signature verification. That limitation also exists in the legacy path; it is not a newly demonstrated vulnerability. When the RP URL is empty, metadata determines the HTTPS download host without a hostname allowlist. A legacy fallback also means the migration does not guarantee that every node uses Dalec.
These are trust-model considerations, not additional blocking findings from this review.
Validation and limitations
All 67 focused credential-provider Pester tests passed locally.
A valid ZIP missing the expected executable reproduced comment 1 with both Continue and Stop error preferences.
Inspection confirmed five download attempts, four ten-second retry delays, no explicit request timeout, and inherited progress output.
Validation ran under PowerShell on Linux, not Windows PowerShell 5.1. No network-stall experiment, Windows timing benchmark, or live-node exploit validation was performed.
No production scripts were changed and no review comments were posted.
The reason will be displayed to describe this comment to others. Learn more.
Inline comments accompanying the configfunc.ps1 review report. Comments 1 and 2 are recommended merge blockers; comments 3 and 4 are non-blocking improvements; comment 5 is a nitpick. This is a comment-only review, not an approval or request-changes vote.
The reason will be displayed to describe this comment to others. Learn more.
2. Blocking: bound this optional download before relying on the legacy fallback.
On a Dalec cache miss, this runs before DownloadFileOverHttp can reuse an available legacy RP archive. There is no explicit request timeout: five retries bound the attempt count, not how long an attempt can stall. Even immediately failing attempts add 40 seconds of retry sleeps before fallback.
Please add a finite request timeout and a retry budget appropriate to provisioning, with a test covering the budget and fallback behavior. This can preserve the preference for Dalec without leaving the usable cached source behind an unbounded optional download.
The legacy downloader also lacks an explicit timeout. The new regression is adding a network dependency ahead of a legacy cache hit that previously needed no download. This finding is based on code and retry-policy inspection; no network-stall experiment or Windows timing benchmark was run.
The reason will be displayed to describe this comment to others. Learn more.
3. Non-blocking: retain the shared downloader's operational safeguards.
Unlike DownloadFileOverHttp, this path does not suppress progress output or record download-duration telemetry. It also has no equivalent explicit TLS setup. Consider preserving progress suppression and timing diagnostics, and checking whether the existing TLS setup is required on supported Windows images. No Windows-specific performance regression was measured.
The throwing contract here is useful: calling the existing exit-based helper directly would undermine fallback. Matching the necessary safeguards locally is reasonable for this PR; extracting a shared throwing download core can be assessed separately rather than expanding the independently released helper boundary now.
The reason will be displayed to describe this comment to others. Learn more.
4. Non-blocking: consider sharing the package-version rule, not the whole resolver.
This repeats the package-version grammar, four-part numeric comparison representation, and Kubernetes major/minor compatibility rule from the cache-selection loop above. A small shared parser could help prevent the two sources from drifting when the version format or matching rule changes.
This is a maintainability suggestion, not a correctness defect. Keep filename matching, cache enumeration, and metadata traversal source-specific. Two occurrences alone do not justify an abstraction, so retaining the duplication is acceptable if a helper would make the flow harder to follow; a generic resolver framework would be unnecessary.
The reason will be displayed to describe this comment to others. Learn more.
5. Nitpick: name this condition after its purpose.
$isStockLegacyUrl is also true when the URL is empty. $allowDalecSelection would describe both cases and make the following conditional read directly as policy. This is only a local clarity improvement and should not hold up the PR.
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
componentsThis pull request updates cached components on Linux or Windows VHDs
5 participants
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.
github-actions
Bot
added
the
components
Tim Wright (timmy-wright)
left a comment
What this PR does / why we need it:
Windows nodes now install the out-of-tree ACR credential provider from the Dalec-built package resolved from components.json for k8s version ≥ 1.33. This aligns Windows with the Linux PMC migration (the Linux gate was lowered to 1.33 in #9205).
Manual validation
Used VHDs from build 186517398 to add Windows test node pools in westus3 through aksdev with custom-image headers. Inspected configfunc.ps1 and CustomDataSetupScript.log.
No provider re-download or installation-failure fallback was shown.
Scope: Validated CSE/provider selection from logs.
Which issue(s) this PR fixes:
Fixes #