Skip to content

feat(anc): direct HTTP download for hotfix with SHA-256 verification - #9233

Open
Abigail Liang (abigailliang-aks-sig-node) wants to merge 5 commits into
mainfrom
abigailliang/anc-hotfix-http-download
Open

feat(anc): direct HTTP download for hotfix with SHA-256 verification#9233
Abigail Liang (abigailliang-aks-sig-node) wants to merge 5 commits into
mainfrom
abigailliang/anc-hotfix-http-download

Conversation

@abigailliang-aks-sig-node

Copy link
Copy Markdown
Contributor

Summary

  • Add artifacts field to hotfix config JSON for direct HTTP download URLs + SHA-256 checksums
  • ~25x faster than apt-get/dnf path (0.87s vs 10-22s)
  • URL restricted to HTTPS + PMC allowlist, cross-domain redirects rejected
  • Backward compatible: falls back to apt/dnf when no artifacts field or no OS/arch match

Fallback strategy

Condition Behavior
artifacts missing or no OS/arch match Fallback to apt/dnf
HTTP network error Fallback to apt/dnf
SHA-256 mismatch Hard fail, keep VHD-baked ANC
Invalid URL (non-HTTPS, non-PMC) Hard fail, keep VHD-baked ANC

Example hotfix config with artifacts

{
  "hotfixes": {
    "202607.02": "202607.02.2"
  },
  "artifacts": {
    "202607.02.2": {
      "ubuntu-22.04-amd64": {
        "url": "https://packages.microsoft.com/...deb",
        "sha256": "c5c29cd3..."
      }
    }
  }
}

Future work (separate PRs)

  • Descriptor signing (schemaVersion, expiresAt, keyId)
  • LPS/ABSvc/CRP changes to preserve artifacts field during serialization
  • Package content validation (version/arch/OS inside package)

Test plan

  • All existing hotfix tests pass
  • New tests: artifact HTTP success, SHA mismatch hard fail, HTTP error fallback, no artifacts fallback, invalid URL hard fail
  • buildArtifactKey tests for Ubuntu/AzureLinux
  • validateArtifactURL tests for HTTPS/host allowlist
  • E2E with real PMC artifact

…fication

Replace the slow apt-get/dnf package manager path (~10-22s) with direct
HTTP download (~0.87s) when an artifact descriptor is present in the
hotfix config. The new optional `artifacts` field in the hotfix JSON
maps version + OS/arch to a download URL and SHA-256 digest.

Fallback strategy:
- No artifacts or no OS/arch match: fallback to apt/dnf (backward compat)
- HTTP network error: fallback to apt/dnf
- SHA-256 mismatch or invalid URL: hard fail, keep VHD-baked ANC
Three places were discarding the new artifacts field:
1. LPS staging (line 212): only carried Hotfixes
2. Cold-start fallback (line 448): lenient struct lacked Artifacts
3. writeHotfixConfig serialization: anonymous struct omitted Artifacts

All three now pass Artifacts through so download-hotfix can use
the direct HTTP download path end-to-end.
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Windows Unit Test Results

  3 files   12 suites   43s ⏱️
389 tests 389 ✅ 0 💤 0 ❌
392 runs  392 ✅ 0 💤 0 ❌

Results for commit 4f17626.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

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.

Pull request overview

Adds a faster hotfix delivery path for aks-node-controller by allowing direct HTTPS artifact downloads (with SHA-256 verification) when the hotfix config includes an artifacts descriptor, while retaining the existing apt/dnf fallback behavior.

Changes:

  • Extend hotfix config parsing to support an artifacts map keyed by hotfix version and ID-VERSION_ID-GOARCH.
  • Implement direct HTTP download with URL allowlisting, redirect restrictions, and SHA-256 verification (with hard-fail on integrity violations).
  • Add unit tests covering artifact parsing, URL validation, key building, and download/fallback behaviors.

Reviewed changes

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

File Description
aks-node-controller/hotfix.go Adds artifact resolution + direct HTTP download/verify path and associated config structs/helpers.
aks-node-controller/hotfix_test.go Adds tests for artifacts parsing, artifact key derivation, URL validation, and download behavior.
aks-node-controller/checkhotfix.go Propagates/stages artifacts through check-hotfix (LPS + cold-start) into the shared pointer file.
aks-node-controller/app.go Adds an injectable httpDownload hook to allow unit tests to bypass real networking.

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

Comment thread aks-node-controller/hotfix.go Outdated
Comment thread aks-node-controller/hotfix.go Outdated
Comment thread aks-node-controller/hotfix.go
Comment thread aks-node-controller/hotfix_test.go Outdated
Comment thread aks-node-controller/hotfix_test.go Outdated
Comment thread aks-node-controller/checkhotfix.go
Add downloadDir field to App so tests can override the temp file
directory (CI lacks /opt/azure/containers/). Update test assertion
to expect the copyBinaryAlongside error (vhdBinaryPath not present)
while verifying package manager was not invoked.

This comment was marked as duplicate.

Copilot AI left a comment

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.

Pull request overview

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

Suppressed comments (3)

aks-node-controller/hotfix.go:124

  • On artifact integrity errors (invalid URL / SHA mismatch), this returns an error but does not remove any previously staged hotfix at /opt/azure/containers/aks-node-controller-hotfix. If a node had an older staged hotfix binary, the launcher will still prefer and execute it, which contradicts the stated behavior to “keep VHD-baked ANC” on integrity failure.
			if isIntegrityError(err) {
				// SHA mismatch or invalid descriptor: hard fail, do NOT fallback to package manager.
				return fmt.Errorf("artifact integrity check failed for %s: %w", hotfixVersion, err)
			}

aks-node-controller/hotfix.go:133

  • The direct-download path stages the downloaded artifact file directly as the executable hotfix binary via copyBinaryAlongside(tmpPath, hotfixBinaryPath, ...). However artifactInfo and the PR description show the URL is a package artifact (e.g. .deb/.rpm). The launcher selects and executes $BIN_PATH-hotfix when it is executable, so staging a package file here will result in an invalid executable and break provisioning. The direct-download path needs to either (a) download a raw aks-node-controller binary artifact, or (b) download the package and extract/install the aks-node-controller binary from it before staging.
			// Direct download succeeded — stage from temp file instead of pkgBinaryPath.
			if err := copyBinaryAlongside(tmpPath, hotfixBinaryPath, vhdBinaryPath); err != nil {
				os.Remove(tmpPath)
				return fmt.Errorf("stage hotfix binary from artifact: %w", err)
			}

aks-node-controller/hotfix.go:599

  • doHTTPDownload reads the entire response body with io.ReadAll without any size limit. Even with a host allowlist, an unexpectedly large response can cause excessive memory usage (and the content is then hashed in-memory), which is avoidable and could lead to OOM/restart loops during provisioning. Consider enforcing a reasonable maximum artifact size (and ideally streaming to disk while hashing).
	if resp.StatusCode != http.StatusOK {
		return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, artifactURL)
	}
	return io.ReadAll(resp.Body)

Copilot AI left a comment

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.

Pull request overview

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

Suppressed comments (3)

aks-node-controller/hotfix.go:446

  • The direct-download path stages the downloaded file as the executable hotfix binary via copyBinaryAlongside(). However the artifact descriptor (and tests/PR example) uses a *.deb URL; copying a Debian/RPM package to /opt/azure/containers/aks-node-controller-hotfix and marking it executable will cause the launcher to execute the package file (likely "exec format error") rather than installing/extracting it. The direct-download implementation needs to either (a) download a raw ANC binary artifact, or (b) download the package and install/extract the ANC binary from it before staging hotfixBinaryPath.
	if err := copyBinaryAlongside(tmpPath, hotfixBinaryPath, vhdBinaryPath); err != nil {
		os.Remove(tmpPath)
		// Staging failure after successful download+verify is a hard error — do not fallback
		// to package manager since we already verified the binary integrity.
		return newIntegrityError("stage hotfix binary from artifact: %v", err)

aks-node-controller/hotfix_test.go:741

  • This test name suggests a successful artifact-based hotfix download, but the test currently expects an error due to staging failure (missing vhdBinaryPath). Renaming the test to reflect what it actually verifies will avoid confusion when interpreting failures.
func TestDownloadHotfix_ArtifactHTTPSuccess(t *testing.T) {

aks-node-controller/checkhotfix.go:493

  • writeHotfixConfig intends to preserve existing artifacts when the incoming config has none, but it only checks for nil. If the incoming JSON includes "artifacts": {} (empty map), this will not be nil and will erase any existing artifacts written by cloud-init. Consider treating both nil and empty as "no artifacts" for preservation.
	if out.Artifacts == nil {
		out.Artifacts = existing.Artifacts
	}

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.

2 participants