refactor: export proxy configuration directly - #9387
refactor: export proxy configuration directly#9387Martin Heberling (Bickor) wants to merge 3 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Windows Unit Test Results 3 files 13 suites 45s ⏱️ Results for commit fe92717. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
The proxy compatibility regression and ineffective ordering assertions must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors Linux provisioning to export proxy variables directly instead of generating PROXY_VARS.
Changes:
- Exports uppercase and lowercase proxy variables directly.
- Removes legacy
PROXY_VARSgeneration. - Updates proxy documentation and test coverage.
Blocking findings remain: removing PROXY_VARS breaks compatibility when the ANC script overlay fails on older VHDs, and the ordering test can pass when no proxy export is found.
File summaries
| File | Description |
|---|---|
spec/parts/linux/cloud-init/artifacts/cse_main_spec.sh |
Adds proxy export and ordering tests; ordering assertions need strengthening. |
pkg/agent/variables.go |
Removes proxy command generation. |
pkg/agent/baker.go |
Removes the proxy template helper. |
parts/linux/cloud-init/artifacts/cse_main.sh |
Exports proxy variables directly; introduces an older-VHD compatibility risk. |
parts/linux/cloud-init/artifacts/cse_cmd.sh |
Stops rendering PROXY_VARS. |
aks-node-controller/proto/README.md |
Updates proxy output documentation. |
aks-node-controller/parser/parser.go |
Removes PROXY_VARS from controller environments. |
aks-node-controller/parser/helper.go |
Removes proxy command generation. |
aks-node-controller/app.go |
Removes obsolete comparison commentary. |
aks-node-controller/app_test.go |
Generalizes quoted-value parsing coverage. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| END { | ||
| print proxy_exports < configure_etc_environment | ||
| print proxy_exports < outbound_check | ||
| } |
There was a problem hiding this comment.
Fixed in fe927172d0. The ordering assertion now explicitly requires the export, package-resolution, and outbound-check line numbers to all be greater than zero before comparing their order. If any expected line disappears, the test prints false and fails rather than treating awk's default zero as a valid earlier line.
|
Scope note: this PR is being narrowed to the three reported legacy AgentBaker locations only: The scriptless path under The narrowing follow-up is staged locally and will update the PR diff after the signed commit is pushed. |
There was a problem hiding this comment.
🟡 Changes recommended
Removing PROXY_VARS breaks compatibility with supported older VHDs, and the stated proxy regression coverage is missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| func getProxyVariables(_ *datamodel.NodeBootstrappingConfiguration) string { | ||
| return "" |
There was a problem hiding this comment.
Fixed in fe927172d0. getProxyVariables once again emits a compatibility payload for getScriptlessNBCCmd, so a newly generated scriptless command continues to work with older VHD scripts that expect PROXY_VARS. The payload contains only constant trusted logic and reads customer values through quoted environment-variable references. A new Go test exercises getScriptlessConfiguration, decodes the generated NBC command, runs it with the old eval $PROXY_VARS behavior, verifies exact proxy round-tripping, and confirms hostile $(), backticks, quotes, semicolons, globs, and backslashes do not execute.
There was a problem hiding this comment.
🟡 Changes recommended
PROXY_VARS differs between provisioning paths, causing false migration parity mismatches.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| HTTP_PROXY_URLS={{GetVariable "httpProxyShellQuoted"}} | ||
| HTTPS_PROXY_URLS={{GetVariable "httpsProxyShellQuoted"}} | ||
| NO_PROXY_URLS={{GetVariable "noProxyShellQuoted"}} | ||
| PROXY_VARS='{{GetProxyVariables}}' |
There was a problem hiding this comment.
Thanks, this can produce a false differs: PROXY_VARS parity event when both inputs are supplied. It does not affect provisioning: NBCCmd is selected and executed first, and compareEnvs runs afterward as best-effort telemetry (app.go:673-677) without changing the command result or CSE exit code.
This PR intentionally limits the production fix to the reported AgentBaker path. The separate aks-node-controller/parser implementation, including alignment of its compatibility payload and its migration comparison behavior, remains follow-up work. We are accepting this temporary telemetry-only difference rather than expanding this security fix back into the parser path.
| // Older VHDs evaluate PROXY_VARS. Keep this payload free of customer-controlled values; | ||
| // those values are shell-quoted separately and referenced only through variables here. | ||
| return `if [ -n "${HTTP_PROXY_URLS}" ]; then export HTTP_PROXY="${HTTP_PROXY_URLS}" http_proxy="${HTTP_PROXY_URLS}"; fi; ` + | ||
| `if [ -n "${HTTPS_PROXY_URLS}" ]; then export HTTPS_PROXY="${HTTPS_PROXY_URLS}" https_proxy="${HTTPS_PROXY_URLS}"; fi; ` + | ||
| `if [ -n "${NO_PROXY_URLS}" ]; then export NO_PROXY="${NO_PROXY_URLS}" no_proxy="${NO_PROXY_URLS}"; fi` |
There was a problem hiding this comment.
Agreed that the two paths now intentionally produce different PROXY_VARS strings and can emit a false parity mismatch. This is telemetry-only: compareEnvs runs after the selected NBC command has completed and is explicitly best-effort, so it cannot fail or alter node provisioning.
We are leaving the separate aks-node-controller/parser producer unchanged in this PR by design. Aligning that producer with the trusted compatibility payload, or excluding this compatibility-only key from migration comparison, will be handled as follow-up work rather than widening the current AgentBaker security fix.
|
|
||
| // Older VHDs evaluate PROXY_VARS. Keep this payload free of customer-controlled values; | ||
| // those values are shell-quoted separately and referenced only through variables here. | ||
| return `if [ -n "${HTTP_PROXY_URLS}" ]; then export HTTP_PROXY="${HTTP_PROXY_URLS}" http_proxy="${HTTP_PROXY_URLS}"; fi; ` + |
There was a problem hiding this comment.
why not use the values from nbc.HTTPProxyConfig?
is the assumption that HTTP_PROXY_URLS is available and exported already here ?
one return uses nbc.HTTPProxyConfig while the other return uses ENV VARs
Summary
PROXY_VARSas fixed trusted compatibility code for older baked VHD scripts, without embedding customer-controlled values in evaluated shell codecse_main.shand remove itseval $PROXY_VARScallsaks-node-controller/parserimplementation unchanged for follow-up workTesting
go test ./...cd aks-node-controller && go test ./parser$(), backticks, globs, and backslashes; values round-trip without executing marker commandseval $PROXY_VARSbehavior exercised using the generated compatibility payloadbash -n parts/linux/cloud-init/artifacts/cse_main.shgit diff --check