[DO NOT REVIEW]feat(anc): verify and extract repository hotfix packages - #9375
[DO NOT REVIEW]feat(anc): verify and extract repository hotfix packages#9375Abigail Liang (abigailliang-aks-sig-node) wants to merge 20 commits into
Conversation
Retain package-manager installation and drop the unused artifact descriptor contract, which staged package bytes as executables. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused removal is internally consistent, backward-compatible with stale JSON fields, and adequately tested.
Pull request overview
Removes ANC’s unsafe direct-download path, retaining package-manager installation and binary staging.
Changes:
- Removes artifact descriptors, HTTP download logic, and test hooks.
- Ignores legacy
artifactsfields and removes them during pointer rewrites. - Updates tests to verify package-manager use and artifact cleanup.
File summaries
| File | Description |
|---|---|
aks-node-controller/hotfix.go |
Removes direct artifact downloads. |
aks-node-controller/hotfix_test.go |
Updates package-manager behavior tests. |
aks-node-controller/checkhotfix.go |
Removes artifacts from parsing and persistence. |
aks-node-controller/checkhotfix_test.go |
Verifies stale artifacts are dropped. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Windows Unit Test Results 3 files 14 suites 48s ⏱️ Results for commit 2bdbe0b. ♻️ This comment has been updated with latest results. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a6056365-ad1e-4247-a219-27dc88ebb6b2
…roken-anc-direct-download
There was a problem hiding this comment.
🔵 Needs a closer look
Azure Linux thin-image repository resolution, verifier timeout handling, and fallback timing reporting need correction.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
aks-node-controller/hotfix.go:114
- 🟡 Medium Risk — ⚡ Performance: This timer starts only after the repository attempt fails, but the new bootstrap scenario reports it as the hotfix path duration. A network or verifier timeout can consume 30–60 seconds before this line, and all of that provisioning latency is omitted from the reported fallback result. Start an overall timer before
tryRepositoryDownload(or emit a separate total-duration field) and have the E2E parser use that total.
aks-node-controller/repository_hotfix.go:408 - 🟡 Medium Risk — 🔄 Backward Compatibility: Every
gpgvfailure except “not found” is classified as an integrity violation, butrunRepositoryCommandreturnscontext.DeadlineExceededwhen the verifier times out. Under bootstrap CPU/IO contention, that operational timeout therefore skips the documented apt/dnf fallback and disarms an otherwise valid staged hotfix. ReserveintegrityErrorfor an actual*exec.ExitErrorfrom verification; treat timeout/cancellation or process-launch failures as operational so fallback remains available.
aks-node-controller/repository_hotfix.go:1085 - 🟡 Medium Risk — 🖥️ Cross-OS: Substituting the raw
VERSION_IDbreaks the fast path on Azure Container Linux 3 thin images. Those images report values such as3.0.20260809(vhdbuilder/release-notes/AKSAzureContainerLinux/gen2tl/latest.txt:161-168), while PMC is rooted at/azurelinux/3.0/(parts/common/components.json:977-982), so every direct request uses a nonexistent repository and falls back. Normalize Azure Linux 3 to its repository release (3.0) before expanding$releasever, and add a dated-VERSION_IDregression case.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The merge from main brought in the e2e refactors that decoupled scenarios from test control (70d6199) and moved them to a standalone runner (a071b10). Scenario.T and RunScenario are both gone, so this scenario no longer compiled. Register the two scenarios instead of declaring them as Test_ functions, and move them out of the _test.go file, matching how every other scenario is now declared. The parser unit test stays in _test.go. Validators can no longer skip, so an absent hotfix now logs and passes rather than skipping. That is the right outcome anyway: no hotfix configured means there is nothing to time, which is not a failure of the code under test.
There was a problem hiding this comment.
🟡 Changes recommended
Azure Linux repository resolution and bootstrap timing reporting contain correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Azure Linux thin-image repository resolution is incorrect, and fallback timing omits repository-attempt latency.
Review details
Suppressed comments (2)
aks-node-controller/repository_hotfix.go:1083
- 🟡 Medium Risk — Cross-OS: Substituting the raw
VERSION_IDbreaks the fast path on Azure Container Linux thin images. Those images report values such as3.0.20260809(vhdbuilder/release-notes/AKSAzureContainerLinux/gen2tl/latest.txt:161-168), while PMC publishes this repository underazurelinux/3.0(parts/common/components.json:973-978). The generated URL therefore 404s and every thin-image hotfix falls back to dnf. Resolve$releaseverusing the distro/package-manager release value (or normalize supported Azure Linux 3 variants to3.0) and cover the datedVERSION_IDin a plan test.
baseURL := strings.ReplaceAll(repository.BaseURL, "$releasever", info.VersionID)
baseURL = strings.ReplaceAll(baseURL, "${releasever}", info.VersionID)
aks-node-controller/hotfix.go:114
- 🟡 Medium Risk — Performance: This timer starts only after
tryRepositoryDownloadreturns. A package or metadata timeout can add tens of seconds before apt/dnf starts, yet the completion line—andvalidateHotfixBootstrapTiming—reports only the package-manager portion as the bootstrap hotfix duration. That hides the main fallback latency this measurement is intended to expose. Start an overall timer before the repository attempt, or emit a separate overall duration and have the scenario report it.
pmcStart := time.Now()
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Verifier timeouts are misclassified as integrity failures, performance measurements can be misleading, and formatting currently fails the repository formatter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
aks-node-controller/repository_hotfix.go:408
- 🟡 Medium Risk — 🏗️ Architecture:
runRepositoryCommandreturnscontext.DeadlineExceededwhen the 60-second verifier timeout fires, but this branch converts that operational timeout into an integrity failure. The caller then removes the staged hotfix and skips apt/dnf fallback even though no invalid signature was established. Preserve cancellation/deadline errors as ordinary operational errors, and reserveintegrityErrorfor an actual nonzero verification result.
aks-node-controller/hotfix.go:114
- 🟡 Medium Risk — ⚡ Performance: This timer starts only after
tryRepositoryDownloadhas failed, while the fast-path timer starts before its network work. A fallback caused by a 30–60 second timeout will therefore report only the subsequent apt/dnf time, soparseHotfixTimingunderreports bootstrap cost and the two route measurements are not comparable. Start an overall timer before the fast-path attempt (or emit separate overall and package-manager phase durations).
pmcStart := time.Now()
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
| if errors.Is(err, errNoHotfixCompletionLine) { | ||
| // Not a failure of the code under test: if no hotfix was configured for this run | ||
| // there is nothing to time. Log it plainly and pass, rather than failing on absent | ||
| // data or reporting a misleading zero. Validators no longer control test outcome | ||
| // (see 70d6199c3e), so this cannot skip the test from here. |
There was a problem hiding this comment.
🔵 Needs a closer look
Proxy-aware repository access and meaningful hotfix activation in the timing scenarios must be addressed.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
aks-node-controller/repository_hotfix.go:345
- 🟡 Medium Risk — ⚡ Performance:
NewBaseTransportdeliberately setsProxy: nil(aks-node-controller/common/httpclient.go:37-45). Reusing it here means every repository request bypasses the node'sHTTP(S)_PROXYsettings. On proxy-only clusters the new fast path waits for a direct-connect timeout before falling back, adding provisioning latency and never exercising the advertised fast path. Repository traffic should use a transport that honors the node proxy configuration (includingNO_PROXY) while retaining these timeouts and redirect checks.
e2e/scenario_hotfix_bootstrap_perf.go:90
- 🟡 Medium Risk — 🧪 Test Coverage: These scenarios are registered specifically to collect bootstrap timing, but they succeed when no hotfix ran. This branch does not stage a hotfix pointer by default, and the scenarios neither inject one nor enable the LPS refresh, so a normal run reports success without measuring either path. Make the scenario arrange an active, matching hotfix target and treat a missing completion line as a failed/explicitly skipped run; otherwise the new E2E coverage cannot detect that the fast path was never exercised.
if errors.Is(err, errNoHotfixCompletionLine) {
// Not a failure of the code under test: if no hotfix was configured for this run
// there is nothing to time. Log it plainly and pass, rather than failing on absent
// data or reporting a misleading zero. Validators no longer control test outcome
// (see 70d6199c3e), so this cannot skip the test from here.
s.Logger.Logf("NO BOOTSTRAP HOTFIX TIMING: %v (no hotfix ran on this node)", err)
return nil
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
RPM extraction lacks equivalent safety checks, and the new timing scenarios can pass without exercising a hotfix path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
e2e/scenario_hotfix_bootstrap_perf.go:90
- 🟡 Medium Risk — 🧪 Test Coverage: This makes both new timing scenarios pass when they collected no timing at all. Neither scenario injects
aks-node-controller-hotfix.jsonnor enables the LPS hotfix feature, and this tree has no generated hotfix JSON, so the launcher will not rundownload-hotfix; both scenarios can therefore succeed without exercising either new path. Provision a known published target (or a deterministic LPS fixture) and treat a missing completion line as failure.
if errors.Is(err, errNoHotfixCompletionLine) {
// Not a failure of the code under test: if no hotfix was configured for this run
// there is nothing to time. Log it plainly and pass, rather than failing on absent
// data or reporting a misleading zero. Validators no longer control test outcome
// (see 70d6199c3e), so this cannot skip the test from here.
s.Logger.Logf("NO BOOTSTRAP HOTFIX TIMING: %v (no hotfix ran on this node)", err)
return nil
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
| rpm2cpio := exec.CommandContext(commandCtx, "rpm2cpio", packagePath) | ||
| rpm2cpio.Stdout = writer | ||
| rpm2cpio.Stderr = os.Stderr | ||
| cpio := exec.CommandContext(commandCtx, "cpio", "-idmu", "--quiet", "./usr/bin/aks-node-controller") |
There was a problem hiding this comment.
🔵 Needs a closer look
RPM extraction is unbounded, stale binaries can remain active, and the E2E scenarios can pass without exercising a hotfix.
Review details
Suppressed comments (3)
aks-node-controller/repository_hotfix.go:627
- The RPM path bypasses
repositoryBinaryMaxBytes:cpiowrites the selected member directly to disk, andcopyBinaryAlongsidethen reads the entire result into memory. A highly compressible RPM can therefore expand a binary far beyond the 128 MiB limit enforced for.deb, exhausting disk or memory during provisioning. Stream the selected member through a bounded writer (and reject non-regular members) before staging it.
cpio := exec.CommandContext(commandCtx, "cpio", "-idmu", "--quiet", "./usr/bin/aks-node-controller")
aks-node-controller/hotfix.go:172
- This still permits the launcher to execute the stale hotfix after an integrity failure when both unlink and chmod fail. The examples in the comment—an immutable file or read-only mount—typically prevent both operations, and the launcher ignores
download-hotfix's exit status before selecting any executable hotfix. Use a durable disarm marker or make the launcher select the hotfix only after a successful download instead of relying solely on best-effort file mutation.
if chmodErr := os.Chmod(path, 0o600); chmodErr != nil {
slog.Error("stale hotfix binary remains executable after repository integrity failure",
"path", path, "removeError", err, "chmodError", chmodErr)
return
e2e/scenario_hotfix_bootstrap_perf.go:90
- These scenarios currently succeed without exercising a hotfix. Their configs neither inject the optional hotfix JSON nor enable
ENABLE_PROVISIONING_HOTFIX, and this branch does not ship the static pointer, so the expected completion line is absent and this branch returns success. Make the scenario arrange a deterministic hotfix target and treat a missing completion line as a failure; otherwise a green run records no bootstrap timing and cannot validate either route.
if errors.Is(err, errNoHotfixCompletionLine) {
// Not a failure of the code under test: if no hotfix was configured for this run
// there is nothing to time. Log it plainly and pass, rather than failing on absent
// data or reporting a misleading zero. Validators no longer control test outcome
// (see 70d6199c3e), so this cannot skip the test from here.
s.Logger.Logf("NO BOOTSTRAP HOTFIX TIMING: %v (no hotfix ran on this node)", err)
return nil
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
PR Title Lint Failed ❌Current Title: Your PR title doesn't follow the expected format. Please update your PR title to follow one of these patterns: Conventional Commits Format:
Guidelines:
Examples:
Please update your PR title and the lint check will run again automatically. |
Failed gate run
Detective summaryThe first concrete failure was Packer ARM template validation before VM provisioning. WestUS3 Likely causeSignature: Recommended owner/actionNode Lifecycle/VHD infrastructure owner: reduce concurrent DADSv5 demand in WestUS3 or request a quota increase; PR owner likely does not need code changes for this gate failure. Strongest alternativePR-caused ANC hotfix change increasing VHD resource usage is the strongest alternative; it is less likely because the failure occurs before provisioning on a fixed Packer VM SKU quota preflight, not in the PR's ANC hotfix package logic. Evidence
Wiki signature |
What this PR does
Keeps the LPS hotfix contract version-only and implements the ANC repository fast path on the node.
After the configured hotfix resolves to a strictly newer patch for the current ANC release, ANC now:
InReleaseand Azure Linux/Marinerrepomd.xmlsignatures using installed repository keys;.deb/.rpmSHA-256 against authenticated metadata;usr/bin/aks-node-controller, never package bytes;Repository traffic starts only after the existing hotfix version and newer-patch gates pass, so inactive or non-matching hotfix pointers do not download metadata or packages.
Mooncake Bootstrap Registry is not treated as PMC; unsupported repository layouts continue through the existing package-manager fallback.
Prerequisite: the hotfix must be published to PMC first
The fast path derives one deterministic URL from name+version+arch+codename, so it can only
find a build that has already been published to
packages.microsoft.com. This is aprecondition, not a new constraint: an unpublished version 404s, which is an operational
failure rather than an integrity one, so it falls through to the package manager -- which
cannot install that version either. Neither path can install what has not shipped.
Worth noting for Azure Linux/Mariner: the AzureLinux
ms-ossrepositories currently publishaks-node-controllerRPMs and the MarinerMicrosoftrepository does not, so Mariner willkeep taking the fallback until that changes, regardless of the discovery fix in this PR.
Tracks work item 39535914.
Performance
Joined-node measurements on a Ready Ubuntu 24.04 AKS node
(
aks-nodepool1-27955216-vmss000000), target ANC202608.26.2-ubuntu24.04u1, 5 iterationseach. Test plan.
apt-get update + reinstallScoped update alone only measures metadata refresh, not install/dpkg cost, so the end-to-end scoped apt row is the relevant comparison. The direct row is a serial shell approximation, but it includes the important non-download work (
dpkg-deb -xand binary staging) and is still much faster than scoped apt.Joined-node timings are expected to be faster than pre-join/bootstrap because the node is already settled: package caches/network paths may be warm, kubelet/containerd/image pull work is no longer competing for CPU/IO, and dpkg is less likely to be contending with other bootstrap operations. For real pre-join/bootstrap timing, use the marker PR (#9424) once merged into this branch and compare
route=direct-httpvsroute=package-managerterminal durations.Security trade-off
Bypassing
apt-get installalso bypasses what dpkg provides: dependency resolution, thepackage database, maintainer scripts, and file-conflict checks. The fast path is only sound
because ANC is a standalone binary with no maintainer-script setup; if that stops being
true, the fast path is no longer sufficient and the apt fallback is required.
The digest is not taken on trust from a config or an LPS response. It is derived at runtime
from PMC's signed metadata, so the trust model matches apt's:
A SHA-256 only proves the bytes match an expected digest; it cannot prove the digest is
authentic. Chaining to
InReleaseis what makes it authentic, and it is why the.debdigestmust still be checked: GPG protects the metadata, not the package. If PMC's signing chain
itself were compromised, apt would be equally affected — this path is no weaker than apt, and
no stronger.
Alongside that: the package path is derived deterministically and cross-checked against the
signed
Filename, decompression is bounded, onlyusr/bin/aks-node-controlleris extracted,and any validation failure falls back to scoped apt with a pinned version.
Testing
cd aks-node-controller && go build -mod=readonly ./...cd aks-node-controller && go test ./...cd aks-node-controller && go test ./... -raceFast-path behaviour is pinned by tests that fail against the pre-change code:
TestUbuntuFastPathPrefersCompressedPackagesIndex— assertsPackages.gzis fetched andthe plain index is not.
TestUbuntuFastPathFallsBackToPlainPackagesIndex— repositories without a.gzentry still work.TestUbuntuFastPathRejectsTamperedCompressedIndex— a checksum mismatch on the compressedindex is an integrity failure, and nothing is staged.
TestUbuntuFastPathRequestsExactlyTheExpectedURLs— asserts the complete request log(InRelease, one index, one
.deb), so any extra fetch fails rather than passing unnoticed.TestUbuntuRepositoryFastPathParallelSuccessExtractsBinary— both branches must startconcurrently or the test times out.
TestRepositoryFastPathCancelsPeerBranchOnFailure— a fast failure cancels its peer insteadof waiting out gpgv's 60 s timeout; fails in ~10 s against the pre-change code.