feat(anc): gate check-hotfix on enable_provisioning_hotfix contract field#8717
feat(anc): gate check-hotfix on enable_provisioning_hotfix contract field#8717Devinwong wants to merge 11 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
2d4b37d to
59b7cef
Compare
59b7cef to
d80ae7d
Compare
|
Acknowledged - no action needed. This is the automated Buf CI status, and it reports Build, Format, Lint, and Breaking all passing for the additive optional field |
d80ae7d to
6854cfa
Compare
f842590 to
3ebabf0
Compare
297282e to
d552a0a
Compare
66c6968 to
a928ad4
Compare
Address review feedback: sourcing the feature-flag file via . executed arbitrary shell, so a malformed file could run commands or �xit and abort the wrapper - undermining fail-open. Parse KEY=VALUE lines instead and never execute the file; blank lines, comments, and non-identifier keys are skipped. The file is fully producer-controlled, so any valid identifier=value is accepted (not a fixed key list). Producer contract is unchanged (boothook writes the literal unquoted line ENABLE_PROVISIONING_HOTFIX=true). Adds a shellspec case proving arbitrary lines (exit, touch) are never executed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
673ebf0 to
d5bedcf
Compare
a928ad4 to
980ed53
Compare
Address Copilot review feedback: expand the FEATURES_PATH comment to document the on-node delivery mechanism explicitly - the cloud-init boothook (producer, #8717) is the sole trusted writer, running as root, and the file lands 0644 root-owned under /opt/azure/containers. Clarifies for reviewers/operators how ENABLE_PROVISIONING_HOTFIX is actually set, since there is no systemd env-var delivery path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The KEY=VALUE parse loop dropped the last line when the file had no trailing newline (read returns non-zero at EOF). Add the standard '|| [ -n "$_key" ]' guard so the final flag is honored, and add a shellspec regression covering a file with no trailing newline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
980ed53 to
7fcf159
Compare
7fcf159 to
45bf399
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the AKSNodeConfig contract with a new boolean field intended to control whether aks-node-controller check-hotfix performs provisioning-time hotfix pointer refresh work, and updates supporting code/tests around the wrapper/bootstrapping path.
Changes:
- Add
enable_provisioning_hotfixto theaksnodeconfig/v1proto contract and regenerate Go bindings. - Update
nodeconfigutils.CustomData()boothook generation to optionally write/opt/azure/containers/enabled_features.shwhen the field is enabled. - Adjust ShellSpec coverage for the wrapper’s feature-flag behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/parts/linux/cloud-init/artifacts/aks_node_controller_wrapper_spec.sh | Updates wrapper ShellSpec expectations/coverage around feature-flag parsing behavior. |
| aks-node-controller/proto/aksnodeconfig/v1/config.proto | Adds the enable_provisioning_hotfix contract field (tag 45) and documents intended semantics. |
| aks-node-controller/pkg/nodeconfigutils/utils.go | Adds enabled-features file path + boothook emission of an enabled-features write block. |
| aks-node-controller/pkg/nodeconfigutils/utils_test.go | Adds unit tests asserting boothook emission/omission behavior for enabled-features output. |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/config.pb.go | Regenerated protobuf Go output including the new field + accessor. |
Files not reviewed (1)
- aks-node-controller/pkg/gen/aksnodeconfig/v1/config.pb.go: Generated file
45bf399 to
3d1a949
Compare
| // Enables provisioning-time hotfix checking. When true, the cloud-boothook writes | ||
| // an enabled_features.sh file that the aks-node-controller wrapper parses to gate | ||
| // the check-hotfix subcommand; when false or unset no file is written and the | ||
| // wrapper skips check-hotfix entirely. Default-off, fail-open. |
| require.Contains(t, on, "\nENABLE_PROVISIONING_HOTFIX=true\n") | ||
| require.Contains(t, on, "chmod 0644 '/opt/azure/containers/enabled_features.sh'") | ||
|
|
||
| // The features file must be written BEFORE the service is started, so the wrapper can source it. |
| // This path is a shared contract with the aks-node-controller wrapper's FEATURES_PATH default. | ||
| // If it changes here it must change there too, or the wrapper will never source the file. | ||
| require.Equal(t, "/opt/azure/containers/enabled_features.sh", EnabledFeaturesFilePath) |
3d1a949 to
c9f66d4
Compare
| // Enables provisioning-time hotfix checking. When true, the cloud-boothook writes | ||
| // an enabled_features.sh file that the aks-node-controller wrapper parses to gate | ||
| // the check-hotfix subcommand; when false or unset no file is written and the | ||
| // wrapper skips check-hotfix entirely. Default-off, fail-open. | ||
| bool enable_provisioning_hotfix = 45; |
| // The features file must be written BEFORE the service is started, so the wrapper can source it. | ||
| featuresIdx := strings.Index(on, "enabled_features.sh") | ||
| startIdx := strings.Index(on, "systemctl start --no-block aks-node-controller.service") | ||
| require.NotEqual(t, -1, featuresIdx) | ||
| require.NotEqual(t, -1, startIdx) | ||
| require.Less(t, featuresIdx, startIdx, "enabled_features.sh must be written before the service starts") |
| func TestEnabledFeaturesFilePathMatchesWrapperContract(t *testing.T) { | ||
| // This path is a shared contract with the aks-node-controller wrapper's FEATURES_PATH default. | ||
| // If it changes here it must change there too, or the wrapper will never source the file. | ||
| require.Equal(t, "/opt/azure/containers/enabled_features.sh", EnabledFeaturesFilePath) | ||
| } |
Adds the producer half of the provisioning-hotfix gate. The AKSNodeConfig contract field `enable_provisioning_hotfix` (tag 45) now drives an on-node feature-flag file: when true, the cloud-boothook writes /opt/azure/containers/enabled_features.sh containing the literal line `ENABLE_PROVISIONING_HOTFIX=true` (quoted heredoc, chmod 0600) before the aks-node-controller service is started. The 2.1c wrapper sources that file and keeps the existing gate, so this file is the single delivery channel. When the field is false/unset, nothing is written and the generated custom data is byte-identical to before this feature existed (6-month VHD backward-compat). Flatcar/Ignition path does not write enabled_features.sh in M1; provisioning- hotfix is a no-op on Flatcar (follow-up if product needs it). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c9f66d4 to
9102831
Compare
| # flags like ENABLE_PROVISIONING_HOTFIX (there is no systemd environment-variable delivery). | ||
| # Writer: the cloud-init boothook (producer side, PR #8717), running as root at provision time, | ||
| # writes it ONLY when the corresponding aks-rp toggle is on. It lands under /opt/azure/containers | ||
| # (0644, root-owned) like the other provisioning artifacts, so only root can populate it and the |
| # $HOTFIX_JSON) that download-hotfix reads below, so it must run first. Gated default-off | ||
| # behind ENABLE_PROVISIONING_HOTFIX (only the literal "true" enables it) - the on-node | ||
| # terminal of the EnableProvisioningHotfix aks-rp region toggle. Wrapped defensively: it is | ||
| # fail-open, but an older ANC binary predating the subcommand exits non-zero. | ||
| if [ "${ENABLE_PROVISIONING_HOTFIX:-}" = "true" ]; then |
| return fmt.Sprintf(`cat <<'EOF' >%[1]s | ||
| ENABLE_PROVISIONING_HOTFIX=true | ||
| EOF | ||
| chmod 0600 %[1]s | ||
| `, EnabledFeaturesFilePath) |
| The variable thirdArg should eq "" | ||
| End | ||
|
|
||
| It 'does not call check-hotfix when ENABLE_PROVISIONING_HOTFIX is unset' |
| require.Contains(t, on, "\nENABLE_PROVISIONING_HOTFIX=true\n") | ||
| require.Contains(t, on, "chmod 0600 /opt/azure/containers/enabled_features.sh") | ||
|
|
||
| // The features file must be written BEFORE the service is started, so the wrapper can source it. |
|
|
||
| func TestEnabledFeaturesFilePathMatchesWrapperContract(t *testing.T) { | ||
| // This path is a shared contract with the aks-node-controller wrapper's FEATURES_PATH default. | ||
| // If it changes here it must change there too, or the wrapper will never source the file. |
|
Closing 2.1d as no-longer-needed — enablement delivery has been re-architected. DecisionPer the design review on the 2.1c wrapper session, the on-node gate for Why this PR's approach didn't reach production anywayThis PR's boothook write lived in Disposition
Closing. |
AgentBaker Linux gate detectiveRun: 171764413 Likely cause/signature: VMSS create returned ARM 409 Conflict / OperationNotAllowed: WestUS3 standardDDSv5Family cores quota was fully consumed (1300/1300) and the test needed 2 additional cores. Evidence: build metadata, timeline, failed E2E log 538, failed test results, and same-window quota failures across unrelated PRs. |
2.1d - gate check-hotfix on the enable_provisioning_hotfix contract field
POC / M1 draft. AgentBaker / Node SIG side only.
This is the final layer of the provisioning-hotfix stack. It makes the AKSNodeConfig
contract field the single source of truth for whether
aks-node-controller check-hotfixdoes any work, and relaxes the env gate added in 2.1c.
What changed
bool enable_provisioning_hotfix = 45;toaksnodeconfig/v1/config.proto(next free tag aftercse_timeout = 44) and regeneratethe Go bindings.
check-hotfixreads the field at the very top ofcheckHotfix()viaApp.provisioningHotfixEnabled()(reads the node-config JSON that is already on disk andcalls
GetEnableProvisioningHotfix()). When the field is not true (false, unset, or theconfig cannot be read/parsed) it returns the new telemetry outcome
disabledand exits 0WITHOUT any remote hotfix call. Fail-open everywhere.
aks-node-controller-wrapper.shnow callscheck-hotfixUNCONDITIONALLY (still wrapped defensively so it can never block provisioning). The
Go binary self-gates on the contract field.
Read channel
The hotfix-pointer READ CHANNEL is moving from a kube-system ConfigMap (read with a bootstrap
token) to the live-patching-service (LPS) IMDS-attested endpoint, validated reachable pre-kubelet
in e2e. That fetch/auth rewrite lives in #8696; this PR is channel-agnostic. The
enable_provisioning_hotfixcontract field and the Go self-gate decide WHETHERcheck-hotfixruns at all, independent of which channel it then uses, so the proto field is intentionally
channel-neutral and unchanged by the pivot.
Supersedes the env-delivery approach
An earlier revision of this PR delivered the toggle as an env var via a
cse_cmd.shtemplate var plus a systemd drop-in (
Environment="ENABLE_PROVISIONING_HOTFIX=...") onaks-node-controller.service, mirroring the IMDS-restriction pattern. That approach wasdropped because:
check-hotfixalready parses the AKSNodeConfig for its own connection details, so a realcontract field is available to the binary with zero new plumbing -
no template var, no drop-in, no env var.
env/drop-in written during provisioning would only take effect on the NEXT boot. Reading the
contract field directly avoids that activation-timing problem - it works on the same boot
because the config JSON is on disk before the service starts.
This also means absvc sets ONE field (the contract bool), not an env var plus a field.
Relaxes the 2.1c env gate
This PR relaxes the
ENABLE_PROVISIONING_HOTFIXenv gate introduced in #8715 (2.1c); gatingnow lives in the Go binary via the
enable_provisioning_hotfixcontract field - single sourceof truth, so absvc sets ONE field, not an env var plus a field. The 2.1c env gate is
intentionally added-then-relaxed across the stack so each PR stays reviewable on its own.
Default-off and fail-open
When
enable_provisioning_hotfixis false or unset, behavior is byte-identical to before thisstack:
check-hotfixmakes no remote call and provisioning proceeds unchanged. Any read orparse error is treated as off. This preserves the 6-month VHD support window in both directions
(older VHD + newer config, and newer VHD + older binary are both safe).
Before / after
check-hotfixreturns outcome=disabled, no remote call, exit 0check-hotfixreads the hotfix pointer from the LPS (live-patching-service)endpoint and stages it (the read channel itself is owned by feat(anc): add check-hotfix subcommand to read hotfix pointer from LPS #8696)
Stack
The aks-rp region toggle that sets the field is in a different repo and is the only remaining
out-of-repo piece. With the field settable on a node, the on-node PoC e2e tests (fail-open and
multi-base) become runnable.
Tests
go test ./...inaks-node-controller: all check-hotfix tests pass, including new gatetests (disabled -> outcome=disabled and the injected fetcher is never called; enabled ->
fetch path runs). Pre-existing Windows-only failures (CRLF goldens, file locks, os-release
message text) are unrelated and also fail on the base branch.