feat: add CustomData/protectedSettings size guards for hotfix PRs - #9182
feat: add CustomData/protectedSettings size guards for hotfix PRs#9182Abigail Liang (abigailliang-aks-sig-node) wants to merge 4 commits into
Conversation
Add two Go-test guards for the scriptless NBC CSE path so an oversized hotfix cannot silently break node provisioning: - Guard 1 (TestCustomDataSizeWithHotfix): asserts the slim Mode B CustomData (platform P + hotfix-injected scripts H) stays under MaxCustomDataLength (87,380). Runs in hotfix-generate.yml after the python injection, so the go:embed'd template carries the real H. - Guard 2 (TestProtectedSettingsSizeWithWorstCaseCerts): asserts the Mode B protectedSettings CSE command carrying worst-case customer input R (RP count-max certs: 10 CA trust + 20-cert proxy bundle) stays under the ~65,535 CRP protectedSettings limit, and warns once it crosses a 60,000 soft margin. Key finding: in Mode B, H lives in CustomData while R (certs) is moved to protectedSettings, so the two limits need two guards. RP validates cert COUNT only (customcatrustvalidator=10, httpproxyconfigvalidator=20), not byte size, and does not validate Linux protectedSettings size at all. The realistic worst case (all 4096-bit certs) renders to ~64,736 bytes, only ~800 bytes below the CRP hard limit -- flagged as a Sev3 platform risk to escalate to the CRP/RP team. Worst-case certs are pre-generated static X.509 fixtures under pkg/agent/testdata/customdata_size_guard/ for fast, deterministic runs. Design doc updated with the two-guard model and empirical measurements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 240e2517-2ad6-4925-90fa-4377e01798e6
Windows Unit Test Results 3 files 12 suites 51s ⏱️ Results for commit f6e897c. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Adds automated size-guardrails for the scriptless NBC CSE flow to prevent hotfix PRs from accidentally exceeding Azure’s VMSS CustomData and CRP protectedSettings limits, and wires those checks into the hotfix generation workflow.
Changes:
- Added Go tests to guard Mode B slim CustomData size (P+H) and protectedSettings command size (R).
- Added worst-case certificate fixtures used to model RP count-max certificate inputs.
- Added a CI step in
hotfix-generate.ymlto run the guards post hotfix-injection, plus a design doc capturing the size model and rationale.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/agent/customdata_size_guard_test.go |
Introduces two size-guard Go tests for scriptless Mode B CustomData and protectedSettings. |
pkg/agent/testdata/customdata_size_guard/worst_case_ca_trust_certs.txt |
Provides worst-case Custom CA trust cert fixture data for the protectedSettings guard. |
pkg/agent/testdata/customdata_size_guard/worst_case_proxy_trusted_ca.txt |
Provides worst-case proxy TrustedCA bundle fixture data for the protectedSettings guard. |
.github/workflows/hotfix-generate.yml |
Runs the new guards after hotfix injection and before committing generated hotfix files. |
docs/design-customdata-size-guard.md |
Documents the two-limit model (CustomData vs protectedSettings) and why two guards are needed. |
Suppressed comments (1)
pkg/agent/customdata_size_guard_test.go:166
- The protectedSettings limit is on the serialized JSON payload, not just the
commandToExecutestring. Since this guard is intentionally close to the limit, it should measurelen(json.Marshal(map[string]any{"commandToExecute": cseCmd}))(and use that value for both the hard-limit and soft-margin checks).
cseCmd := templateGenerator.getNodeBootstrappingCmd(config)
if len(cseCmd) >= protectedSettingsMaxLength {
t.Fatalf("Mode B protectedSettings CSE command is %d bytes, must be < the CRP protectedSettings "+
"limit (%d). Worst-case customer certs (R: %d CA trust + %d-cert proxy bundle) overflow "+
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f39f002 to
da70995
Compare
aks-rp PR 16755346 (WI 39098291) adds a byte-size cap on both cert inputs, previously validated by count only: - Custom CA trust: sum of all cert strings <= 10 KiB (maxCustomCAEncodedContentSize = 10*1024) - HTTP proxy trustedCa: whole bundle string <= 10 KiB (maxTrustedCAEncodedContentSize = 10*1024), gated create-only via enforceContentSize = Existing().Cluster() == nil Both limits are TOTAL (not per-cert) and create-only, so new clusters are bounded (~20 KiB R total) and safe. Existing/grandfathered clusters plus a proposed toggle for the one ~55 KiB outlier customer keep the protectedSettings overflow path reachable, so Guard 2 stays the AgentBaker backstop and its fixture intentionally models that un-capped worst case (30x 4096-bit certs) rather than the new 10 KiB cap. Update design doc RP table, Sev3 callout, and open questions; update guard test comments/TODO to cite PR 16755346 / WI 39098291. Also flag that the CA-trust size check appears unconditional (unlike proxy) -- confirm its create-only scope with the PR author. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 240e2517-2ad6-4925-90fa-4377e01798e6
da70995 to
843b0c3
Compare
RP now validates cert byte size (aks-rp PR 16755346, 10 KiB cap for new clusters), so worst-case R can no longer overflow protectedSettings. Guard 2 and its ~60 KB cert test fixtures are no longer needed. Only Guard 1 (TestCustomDataSizeWithHotfix) remains.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/agent/customdata_size_guard_test.go:17
- PR description and design doc describe two size guards (including TestProtectedSettingsSizeWithWorstCaseCerts for protectedSettings), but this file only implements Guard 1 (TestCustomDataSizeWithHotfix). As-is, there is no automated protection for the protectedSettings (~65,535 bytes) constraint that Mode B relies on; either add the second guard (and any required fixtures) or update the PR/doc to remove the claim that it exists.
// CustomData size guard for the scriptless NBC CSE path.
//
// Background (see docs/design-customdata-size-guard.md):
//
// In Mode B (slim CustomData), the generated payload carries P (platform content) + H (hotfix-
// injected scripts). R (customer certs) is moved to protectedSettings and is now capped by RP
// (aks-rp PR 16755346: 10 KiB per field for new clusters), so only H can overflow CustomData.
//
// This guard asserts that the slim Mode B CustomData stays under MaxCustomDataLength (87,380).
docs/design-customdata-size-guard.md:122
- This doc states that Guard 2 (TestProtectedSettingsSizeWithWorstCaseCerts) is implemented in pkg/agent/customdata_size_guard_test.go and even references a protectedSettingsMaxLength constant, but neither the test nor the constant exists in this PR/repo state. This makes the design doc misleading; please either implement Guard 2 as described (and keep the doc accurate) or revise §5 to document only the guard(s) that actually exist.
## 5. Two Guards (Implemented)
Investigation of the Mode B fallback revealed that **H and R end up in different Azure fields**, so a single CustomData check is insufficient. Two independent guards are implemented in
`pkg/agent/customdata_size_guard_test.go` (plain `Test*` funcs so they run under `go test -run`).
### Guard 1 — `TestCustomDataSizeWithHotfix` (H vs CustomData)
- Builds a scriptless config and forces Mode B (`config.ScriptlessCSEProvisionMode = true`) so
`getScriptlessBoothook` early-returns the **slim** CustomData (`P + H`).
- Asserts `len(slim) < MaxCustomDataLength` (87,380).
- In the hotfix CI this runs **after** `hotfix_generate.py` injects the real scripts into
`nodecustomdata.yml`; because the template is `//go:embed`'d, `go test` recompiles and measures
the **actual injected H** — no synthetic H is needed.
### Guard 2 — `TestProtectedSettingsSizeWithWorstCaseCerts` (R vs protectedSettings)
- Builds a worst-case customer-input (`R`) config and renders the Mode B CSE command via
`getNodeBootstrappingCmd` (the string RP places into the extension `protectedSettings`).
- Asserts `len(cseCmd) < protectedSettingsMaxLength` (65,535, the CRP hard limit), and **warns**
(without failing) once it crosses a 60,000 soft margin.
.github/workflows/hotfix-generate.yml:92
- The PR description says CI should run two guards after hotfix injection (CustomData + protectedSettings), but this workflow only runs TestCustomDataSizeWithHotfix. If Guard 2 is intended, add it to the workflow (and ensure it exists in the code); otherwise, the PR description/design doc should be updated to match what CI actually enforces.
# Runs AFTER hotfix injection (so the //go:embed'd nodecustomdata.yml carries the injected
# scripts H) and BEFORE the commit step (so an oversized hotfix fails the PR before its
# content is committed). See docs/design-customdata-size-guard.md.
# Guard 1 (TestCustomDataSizeWithHotfix): slim Mode B CustomData (P + H) < 87,380.
- name: Set up Go
uses: actions/setup-go@v7
with:
go-version: '1.25'
- name: Validate CustomData size
run: |
go test ./pkg/agent/ -run TestCustomDataSizeWithHotfix -v -count=1
aks-rp PR 16755346 now bounds customer cert input R by byte size (10 KiB total per field, create-only), and the largest observed production R (54.64 KiB CA-trust outlier) renders to 48,152 B — ~26% under the 65,535 protectedSettings limit. So no reachable customer input overflows protectedSettings and Guard 2 covered no real risk. Update the design doc to describe Guard 2 as considered-and-removed rather than a retained backstop: - §1/§5: Guard 1 retained; Guard 2 + cert fixtures removed after eval - §6: reframe empirical table + callout as removal justification - §7: Q4 becomes "when to re-add a protectedSettings guard" - §8: drop deleted testdata row; Guard 1 only - §9: timeline reflects done/removed state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 240e2517-2ad6-4925-90fa-4377e01798e6
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
pkg/agent/customdata_size_guard_test.go:27
- 🔴 High Risk — 🖥️ Cross-OS: The guard renders only Ubuntu, but
getScriptlessBoothookuses a different ignition wrapper for ACL/Flatcar and the injectednodecustomdata.ymlblocks render different distro-specific script bodies. A hotfix whose ACL, Mariner, OSGuard, or Flatcar payload exceeds the limit can therefore pass this Ubuntu-only check. Make this table-driven over every distinct supported rendering class and enforce the limit for each payload.
agentPoolProfile := &datamodel.AgentPoolProfile{
Name: "nodepool1",
OSType: datamodel.Linux,
Distro: datamodel.AKSUbuntuContainerd2204Gen2,
}
docs/design-customdata-size-guard.md:129
- The PR title/summary promises CustomData and protectedSettings size guards, but this design and the diff retain only the CustomData guard. Update the PR metadata to describe a single CustomData guard, or restore the promised protectedSettings guard, so reviewers and release owners do not assume CI validates both Azure limits.
### Guard 2 — `TestProtectedSettingsSizeWithWorstCaseCerts` (R vs protectedSettings) — **removed**
Originally rendered a worst-case customer-input (`R`) config through `getNodeBootstrappingCmd` and
asserted `len(cseCmd) < protectedSettingsMaxLength` (65,535). **Removed** because the risk it covered
is no longer reachable — see §6 for the byte-size RP validation (PR 16755346) and the empirical
pkg/agent/customdata_size_guard_test.go:22
- 🟡 Medium Risk — 🧪 Test Coverage: A minimal public-cloud configuration does not reserve the largest platform-owned
P. Scriptless AKS custom-cloud configurations add the compressedinit-aks-cloud.shentry (as exercised inpkg/agent/baker_test.go:1753-1818), so a hotfix near the threshold can pass here and still overflow for those nodes. Measure representative maximum-Pconfigurations, rather than only the minimum fixture, or reserve their additional bytes explicitly.
This issue also appears on line 23 of the same file.
// newScriptlessGuardConfig builds a minimal scriptless NBC config.
func newScriptlessGuardConfig() *datamodel.NodeBootstrappingConfiguration {
docs/design-customdata-size-guard.md:36
- This formula contradicts the corrected Mode B model later in §6:
Ris carried in protectedSettings, not in CustomData, soP + H + R ≤ Cis not the constraint this guard enforces. State the independent Mode B constraints here (CustomData:P + H < C; protectedSettings: NBC command includingRbelow its CRP limit) to avoid basing future sizing work on the known-false model.
This issue also appears on line 125 of the same file.
Constraint: P + H + R ≤ C
Or equivalently: H ≤ C − max(P + R)
Summary
pkg/agent/customdata_size_guard_test.go) for the scriptless NBC CSE pathTestCustomDataSizeWithHotfix): asserts slim Mode B CustomData (P + H) < 87,380hotfix-generate.ymlto run guard after hotfix injection, before commitdocs/design-customdata-size-guard.mdmain. Newofficial/*branches fork frommainweekly, so they inherit the guard automatically. Olderofficial/*branches lack it, but those map to VHDs that either already bake the fix (post-0729) or are approaching end of 6-month support. Backporting is not necessary.Context
Bridge solution until #9101 (embed hotfix scripts in ANC binary) lands. See design doc for full analysis of encoding chain, Mode A/B architecture, and why only H (not R) can overflow Mode B CustomData.
Test plan