[DO NOT REVIEW] feat(anc): verify and extract repository hotfix packages - #9375
Open
Abigail Liang (abigailliang-aks-sig-node) wants to merge 7 commits into
Open
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>
Abigail Liang (abigailliang-aks-sig-node)
requested review from
Devinwong,
Sri Harsha (SriHarsha001),
Abdul Asfari (aboodasfari),
Nishchay (awesomenix),
Ben Brady (benjamin-brady),
Calvin S. (calvin197),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
fcher,
Ganeshkumar Ashokavardhanan (ganeshkumarashok),
janenotjung-hue,
Karen Chen (karenychen),
lilypan26,
Mark Ibrahim (mxj220),
Peter Damianov (pdamianov-dev),
Patrick W. Healy (phealy),
r2k1,
Runzhen (runzhen),
Shun Lyu (sinmentis),
sulixu,
Tim Wright (timmy-wright),
Thibault Cohen (titilambert),
Xu Xue (xuexu6666) and
Zachary (zachary-bailey)
as code owners
September 2, 2026 18:29
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 2, 2026 18:29
View session
Contributor
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.
Contributor
Windows Unit Test Results 3 files 13 suites 55s ⏱️ Results for commit 5a3b17f. ♻️ 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
Abigail Liang (abigailliang-aks-sig-node)
changed the title
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 00:20
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Azure Linux release resolution, stale-binary handling, and failed-download cancellation need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
aks-node-controller/repository_hotfix.go:897
- 🟡 Medium Risk — 🖥️ Cross-OS:
$releaseveris expanded from the rawVERSION_ID. ACL/OS Guard images in this repository report values such as3.0.20260809(vhdbuilder/release-notes/AKSAzureContainerLinux/arm64gen2tl/latest.txt:166-168), while the Azure Linux repository layout uses/azurelinux/3.0/...(parts/common/components.json:981). This generates a nonexistent repository URL on those supported images, so the RPM fast path always falls back. Resolve$releaseverthe same way the package manager does, or normalize Azure Linux to its major/minor release before substitution.
baseURL := strings.ReplaceAll(repository.BaseURL, "$releasever", info.VersionID)
baseURL = strings.ReplaceAll(baseURL, "${releasever}", info.VersionID)
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
Extract helpers to bring parseOneLineAptRepository, parseDebPackageMetadata, and resolveRPMPackageMetadata under the gocognit threshold; hoist repeated arch and OS-ID literals into constants; reuse the outer err binding where govet flagged shadowing; wrap a long test fixture. No behavior change.
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 00:37
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Ubuntu metadata lookup uses an invalid Release-relative path, preventing the fast path and package-manager fallback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| assert.True(t, isIntegrityError(err)) | ||
| } | ||
|
|
||
| func TestResolveRPMPackageMetadata(t *testing.T) { |
golangci-lint-action runs `config verify` before linting, which fetches the JSONSchema from golangci-lint.run with no retry or cache. Each of the three matrix legs fetches independently, so an outage there fails the entire lint gate before a single linter runs -- three consecutive failures today, all `context deadline exceeded` on that fetch. The check only validates .golangci.yaml syntax. That file changes rarely and a mistake in it surfaces right away in the lint run itself, so the coverage lost is small next to a gate that depends on third-party availability.
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 00:44
View session
removeStaleHotfix logged unlink failures and moved on. The launcher selects the hotfix binary on `[ -x ]` alone and deliberately ignores download-hotfix's exit status (fail-open), so a stale binary that could not be unlinked stayed armed and ran on the very next boot after an integrity violation -- the cleanup was the only gate, and it was advisory. Clear the executable bits when unlink fails, closing the launcher's gate through a second, independent mechanism, and log at error level if even that does not work. Scope: the staged binary is only ever written by copyBinaryAlongside from a package-manager-verified install, so this contains a stale-but-authentic ANC rather than attacker-controlled code. Left the launcher's fail-open exit-status handling alone: making it fail-closed would also stop honoring an existing hotfix on unrelated transient errors, which is a larger behavior change than this fix.
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 00:52
View session
A Debian InRelease indexes its checksum entries relative to the suite directory that holds it -- "main/binary-amd64/Packages" -- not relative to the repository root. We passed the root-relative "dists/jammy/main/binary-amd64/Packages", which matches nothing on a real repository. parseReleaseSHA256 would then report no entry, resolveUbuntuPackageMetadata converts that to an integrity error, and downloadBinaryHotfixIfNeeded treats integrity errors as terminal: it removes the staged hotfix and returns without trying the package-manager fallback. Every Ubuntu hotfix would have failed this way. Derive the suite-relative path for the InRelease lookup and keep the root-relative one for the download URL. The tests passed because the fixtures wrote the root-relative path into the InRelease too, so they encoded the same wrong assumption as the code. Verified against the real https://packages.microsoft.com/ubuntu/22.04/prod InRelease, which publishes suite-relative entries, and corrected the fixtures to match; they now fail against the old lookup. The RPM path was checked and is unaffected -- repomd hrefs are genuinely repository-root-relative.
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 00:57
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Mariner repository discovery cannot select its Microsoft repository, so the new fast path never runs there.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
aks-node-controller/repository_hotfix.go:932
- 🟡 Medium Risk — 🖥️ Cross-OS: this plan is also used for
ID=mariner, butparseMSOSSRepositoryonly accepts sections or base URLs containingms-oss. Mariner config usesmariner-official-microsoft(seeparts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh:33) and RepoDepot only rewrites that existing URL (init-aks-cloud.sh:471-474), so Mariner always returns “unsupported” here and falls back to dnf instead of using the advertised repository fast path. Select the repository section/path based oninfo.IDand add a Mariner plan test.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The package download and metadata resolution ran concurrently but were both awaited unconditionally, so the slower branch was never cut short. A package 404 that returns immediately still waited out the metadata branch -- gpgv's 60s command timeout plus a 30s request -- before downloadBinaryHotfixIfNeeded could fall back to the package manager, adding that delay to node provisioning. Run both under a cancellable context and cancel on the first failure. Cancelling makes error classification load-bearing, which is the subtle part. A killed gpgv comes back from verifyRepoSignature as an integrityError, and integrity errors are terminal: they disarm the staged hotfix and skip the fallback. Reporting the peer's induced error would turn a benign 404 into apparent tampering. So the first failure is recorded with the branch it came from, and only that error is returned; the peer's is dropped. A dead caller context is reported ahead of either. Extracted the block into fetchPackageAndMetadata to stay under the funlen limit. The test asserts all three properties -- the peer's context is cancelled, the package error (not the cancellation) is reported, and the result is not classified as an integrity error. Against the pre-fix code it fails in 10.17s on the cancellation assertion; it passes in 0.01s now. Also verified under -race, since failBranch writes shared state from both goroutines.
Copilot started reviewing on behalf of
Abigail Liang (abigailliang-aks-sig-node)
September 3, 2026 01:06
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A gpgv timeout is incorrectly treated as an integrity failure, preventing the intended package-manager fallback.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
aks-node-controller/repository_hotfix.go:404
- 🟡 Medium Risk — The command timeout is returned as
context.DeadlineExceeded, but this branch classifies every non-ErrNotFoundresult as an integrity failure. A slow or wedged localgpgvtherefore removes the staged hotfix and skips apt/dnf fallback even though no integrity violation was established; treat cancellation/deadline errors as operational so the documented fallback remains available.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Abigail Liang (abigailliang-aks-sig-node)
changed the title
Contributor
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. |
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.
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.
Tracks work item 39535914.
Testing
cd aks-node-controller && go build -mod=readonly ./...cd aks-node-controller && go test ./...cd aks-node-controller && go test ./... -race