Repository navigation
fix: reject LocalDNS listener upstreams - #9471
Saewon Kwak (saewoni) wants to merge 9 commits into
Conversation
Windows Unit Test Results 3 files 21 suites 1m 18s ⏱️ Results for commit 9f50a52. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
The new validation still accepts malformed non-address values that can break LocalDNS startup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens LocalDNS Corefile generation against self-referential upstream resolvers.
Changes:
- Validates upstream entries and rejects LocalDNS listener addresses.
- Adds ShellSpec coverage for listener-only and mixed upstream states.
File summaries
| File | Description |
|---|---|
parts/linux/cloud-init/artifacts/localdns.sh |
Adds upstream validation before Corefile replacement. |
spec/parts/linux/cloud-init/artifacts/localdns_spec.sh |
Tests listener rejection and Azure DNS behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The new whitelist still accepts malformed addresses that can break LocalDNS startup.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:188
- 🟡 Medium Risk — 🔧 Script Logic: This character whitelist is not IP-address validation: values such as
999.999.999.999,deadbeef, and:all match and are then inserted into the CoreDNSforwarddirective. A malformed resolver state can therefore still make LocalDNS fail at startup. Please enforce IPv4 octet ranges and valid IPv6 structure, with corresponding malformed-address tests.
if ! [[ "${upstream_dns_ip}" =~ ^[0-9a-fA-F.:]+$ ]]; then
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The validator still accepts syntactically invalid IP addresses that can break LocalDNS startup.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:193
- 🟡 Medium Risk — Script Logic: This checks only the character set, not whether the token is a syntactically valid IP address. Values such as
999.999.999.999,deadbeef,:, and1..2.3all pass and are then written into the CoreDNSforwarddirective, so malformed resolver state can still make LocalDNS fail at startup—the state this change is intended to reject. Validate each token as an actual IPv4/IPv6 address (including octet/group structure and ranges) before proceeding.
case "${upstream_dns_ip}" in
*[!0-9a-fA-F.:]*|"")
echo "Invalid upstream VNET DNS server '${upstream_dns_ip}' in ${RESOLV_CONF}."
return 1
;;
esac
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed review comment #9471 (comment) in commit 94fad76. Replaced the character whitelist with POSIX-compatible IPv4/IPv6 structural validation, including IPv4-mapped IPv6 support. Added regression cases for 999.999.999.999, deadbeef, :::, and ....; invalid values are rejected before Corefile replacement. |
There was a problem hiding this comment.
🟡 Changes recommended
The validator currently rejects every ordinary IPv4 upstream and also accepts several malformed IPv6 forms.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:213
- 🔴 High Risk — Script Logic: This branch runs before the plain-IPv4 check, so every normal IPv4 resolver (for example
10.0.0.1or Azure DNS168.63.129.16) contains a dot but does not match:[^:]+$and exits with failure here.replace_azurednsip_in_corefilewill therefore fail and LocalDNS startup exits 216 on the common IPv4 configuration. Handle colon-free IPv4 values before entering the embedded-IPv4/IPv6 path.
if (value ~ /\./) {
if (value !~ /:[^:]+$/) exit 1
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
AgentBaker Linux gate detectiveRun: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=180752543 Detective summary
Likely cause and signature
Root-cause challengeStrongest alternative is a PR-caused regression because this PR changes localdns.sh and its shell tests, but both failures happen in VHD build/provisioning infrastructure before any LocalDNS E2E behavior is exercised. Test-code/product regression, CSE, dependency/package, and Packer-template theories are less likely than shared infra capacity/image-version availability based on the error locations and changed-file scope. Recommended owner/actionAgentBaker VHD build infrastructure owners should restore WestUS3 standardDDSv5Family quota headroom and investigate why the AzureLinuxV3 kata SIG version was unavailable after the build step. PR author action is likely none unless a rerun still fails after infra capacity/image availability is restored. Evidence
|
|
Addressed review comment #9471 (comment) in commit The IPv6 validator now rejects triple colons and unmatched single leading/trailing colons while preserving the valid compressed unspecified address Added ShellSpec cases for:
|
There was a problem hiding this comment.
🟡 Changes recommended
The validator currently rejects ordinary IPv4 upstreams, causing LocalDNS startup to fail.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:217
- 🔴 High Risk — Script Logic: This branch handles every dotted value as an IPv4-embedded IPv6 address and requires a colon before the later pure-IPv4 validation can run. Standard upstreams such as
168.63.129.16and10.0.0.1therefore return failure, causing LocalDNS startup to exit 216. Limit this branch to dotted values that also contain a colon so line 223 validates ordinary IPv4 addresses.
if (value ~ /\./) {
if (value !~ /:[^:]+$/) exit 1
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Ye Wang (yewmsft)
left a comment
Ye Wang (yewmsft)
left a comment
There was a problem hiding this comment.
the direction changed since my last read, and the new validator breaks all ipv4 upstreams.
localdns.sh:216 — every ipv4 address has a . and no :, so it hits exit 1 at 217 before valid_ipv4() is ever consulted. that branch was written for ipv4-mapped ipv6 (::ffff:10.0.0.1) but it swallows bare ipv4 first. sourced the function at this head and ran it:
nameserver 168.63.129.16 -> rc=1 Invalid upstream VNET DNS server '168.63.129.16'
nameserver 10.0.0.1 -> rc=1 Invalid upstream VNET DNS server '10.0.0.1'
that is exit $ERR_LOCALDNS_FAIL -> systemctlEnableAndStart localdns 30 fails in cse_config.sh -> every node with localdns enabled fails to provision. shellspec is red on this head for the same reason.
stepping back — is this the right direction? RESOLV_CONF is /run/systemd/resolve/resolv.conf, written only by systemd-resolved, which serializes from its own dns server list and only ever emits canonical addresses. it cannot produce 1:::2 or .... or 999.999.999.999. the thing we actually need to reject is the two listener ips, and that is a string compare. we added 40 lines of hand-rolled ipv6 parsing on a day-0 provisioning path to guard inputs that can't occur, and it took out the one path that does. i'd revert to the simple form and keep the listener check.
also, all 8 new tests are negative ipv6 shapes. none cover the positive ipv4 path that just regressed, and none of these are still missing:
- listener first, ip second (
169.254.10.10then10.0.0.1) — the loop early-returns so it is order sensitive by construction, and every mixed test today puts the legit ip first. the original bug was about position in the list, pin both orders .10+.11together, no legit upstream168.63.129.16+10.0.0.1— the only shape that hits the replace branch while the list already contains AZURE_DNS_IP- single legit non-azure ip alone
localdns.sh:389 (not in the diff) — verify_default_route_interface does the same awk | paste -sd' ' read, but takes $1 only, checks .10 only, and .11 not at all. and if $1 is the listener we give up instead of trying the next server. follow-up pr is fine, but let's not keep two copies of this check.
Agentbaker E2E is skipping on this run. day-0 resolver path, please get a real run linked before merge.
AgentBaker Linux gate detectiveRun: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=180768632 Detective summaryThe gate is failing before image creation during Packer VM deployment validation in WestUS3. The first useful failure is ARM InvalidTemplateDeployment with inner QuotaExceeded for standardDDSv5Family cores: current usage 1396 of limit 1400, with 16 additional cores required and a minimum limit of 1412. CorroborationTimeline reports failed Build VHD tasks across the matrix, and the failed Build VHD logs show the same WestUS3 standardDDSv5Family quota exhaustion before Packer creates a VM. There are no failed E2E test records. Build metadata and changed files tie the run to LocalDNS shell/spec changes, but the failure occurs in shared regional capacity validation before LocalDNS behavior is exercised. Likely cause and signatureLikely root cause: shared AgentBaker VHD build infrastructure quota exhaustion in WestUS3 for standardDDSv5Family. Classification: VHD build / infra-capacity quota, not PR-change-caused. Confidence: high. Wiki signature: vhd-westus3-standardddsv5family-quota-exceeded. Root-cause challengeStrongest alternative is a PR-caused regression because this PR changes localdns.sh and its spec, but the failed steps are Packer ARM template validation and quota preflight before any LocalDNS content can affect provisioning. Product, test-code, CSE/provisioning, dependency/package, and Packer-template theories are less likely than shared regional quota exhaustion. Recommended owner/actionAgentBaker VHD build infrastructure owners should restore standardDDSv5Family quota headroom in WestUS3 or move affected build capacity to a region/SKU family with available cores. PR author action is likely none unless a rerun still fails after capacity is restored. Evidence
|
There was a problem hiding this comment.
🔵 Needs a closer look
The format check still accepts malformed addresses that can break LocalDNS startup.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:201
- 🟡 Medium Risk — 🔧 Script Logic: This character whitelist is not address validation. Malformed values such as
999.999.999.999,deadbeef,1:2:3, and...all pass and are then inserted into the CoreDNSforwarddirective, so a transient malformed resolver state can still break LocalDNS startup despite this PR's stated rejection guarantee. Parse each token as an actual IPv4/IPv6 literal (including IPv4 ranges and IPv6 structure), then reject unspecified/listener addresses before modifying the Corefile.
# systemd-resolved emits canonical address values; reject tokens outside that format.
for upstream_dns_ip in ${UPSTREAM_VNET_DNS_SERVERS}; do
case "${upstream_dns_ip}" in
"::"|*[!0-9a-fA-F.:]*|"")
echo "Invalid upstream VNET DNS server '${upstream_dns_ip}' in ${RESOLV_CONF}."
return 1
;;
esac
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The character allowlist still accepts malformed and non-address upstream values.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:197
- 🟡 Medium Risk — Script Logic: This is only a character allowlist, not address validation. Non-address or malformed values such as
abc,1.2.3,999.999.999.999, and2001:db8::1::2all bypass this branch and are written into the Corefile, so a bad resolver snapshot can still produce an unusable or self-resolving forwarder instead of being rejected. Please validate each token as a syntactically valid IPv4 or IPv6 address (and reject unusable unspecified addresses) before copying or editing the Corefile.
"::"|*[!0-9a-fA-F.:]*|"")
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Address validation remains incomplete, and the unchanged-Corefile test helper cannot detect mutations.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:198
- 🟡 Medium Risk — 🔧 Script Logic: This character whitelist does not validate an IP address. Inputs such as
999.999.999.999,abc, and1:2all pass and are then inserted into the CoreDNSforwarddirective, so malformed resolver state can still produce an unusable Corefile and abort LocalDNS startup. Please validate IPv4 octet ranges and complete IPv6 syntax before modifying the Corefile.
# systemd-resolved emits canonical address values; reject tokens outside that format.
for upstream_dns_ip in ${UPSTREAM_VNET_DNS_SERVERS}; do
case "${upstream_dns_ip}" in
"::"|*[!0-9a-fA-F.:]*|"")
echo "Invalid upstream VNET DNS server '${upstream_dns_ip}' in ${RESOLV_CONF}."
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| if ! assert_updated_localdns_corefile_unchanged; then | ||
| echo "Updated localdns corefile changed unexpectedly." | ||
| return 1 | ||
| fi | ||
| return "$status" |
AgentBaker Linux gate detectiveRun: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=180793366 Detective summaryThe E2E task failed after VMSS/node readiness during post-provisioning Linux validation. The first useful failure is Ubuntu2604Minimal_NPD_Basic reporting Critical kernel issues detected with a kernel call trace at boot-time timestamp 1.286683. The task then exited with status 1. CorroborationThe failed test record, the Run AgentBaker E2E timeline task, and the E2E log all point to the same scenario and kernel-call-trace validator failure. The log also shows E2E_FAILED_TESTS_RETRY_COUNT=0, so this run did not retry the scenario. A prior wiki row already tracks the same Ubuntu 26.04 Minimal kernel-critical-validation signature, while other adjacent scenarios completed kernel validation without critical issues. Likely cause and signatureLikely root cause: recurring Ubuntu 26.04 Minimal image/kernel validation flake or image issue surfaced by NPD_Basic, not a LocalDNS PR change. Classification: E2E/test kernel validation. Confidence: medium. Wiki signature: e2e-ubuntu2604minimal-kernel-critical-validation. Root-cause challengeStrongest alternative is a PR-caused LocalDNS regression because this PR changes localdns.sh and shell specs, but the failure is an NPD_Basic kernel call trace after readiness and does not fail at LocalDNS validation. Product/test-code regression and CSE/provisioning are less likely than shared image/kernel validation flake based on the existing matching signature and scenario scope. Recommended owner/actionTreat this as an AgentBaker E2E/image flake unless it reproduces after rerun. Node/VHD owners should inspect the Ubuntu 26.04 Minimal kernel log artifact for the call trace signature; PR author action is likely none unless rerun reproduces with a LocalDNS-specific failure. Evidence
|
|
comprehensive test result here: #9503 (comment) Conclusive Live Validation — All Three Problems ResolvedAll three PR problems were reproduced and confirmed fixed on a single fresh, LocalDNS-enabled Ubuntu 22.04 agent node, patched with the latest cherry-picked artifacts from the combined branch. Setup
PR #9439 — Deterministic terminal failure (previously inconclusive) — NOW CONFIRMEDFault: sabotaged the coredns binary path and drove repeated restarts to exhaust the budget. Result: the bounded budget lands LocalDNS deterministically in PR #9471 — Listener/invalid upstream rejection (previously inconclusive live) — NOW CONFIRMEDThe on-node branch No LocalDNS listener IP was ever written into the generated corefile in any reject case. Valid upstreams forward correctly. Confirmed live (8/8 cases). PR #9360 — Node DNS restoration after terminal exit — RE-CONFIRMEDTerminal dead-service test on the same patched node: Confirmed live. Final verdict
Based on the latest cherry-picked commits from PRs #9360, #9439, and #9471, all three problems are no longer present on the live cluster's LocalDNS-enabled agent node. The disposable node was deleted after testing. |
d0b0c49 to
738838b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Address validation remains incomplete, and the unchanged-Corefile test helper cannot detect mutations.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
spec/parts/linux/cloud-init/artifacts/localdns_spec.sh:88
- The new unchanged-file check cannot fail any of these tests: both the expected production failure and a
cmpmismatch return status 1, while every caller only assertsThe status should be failure. Consequently, a regression that modifies the Corefile before returning failure still passes. Make the wrapper distinguish “function failed and file stayed unchanged” from a comparison failure, and update callers to assert that exact result/status.
if ! assert_updated_localdns_corefile_unchanged; then
echo "Updated localdns corefile changed unexpectedly."
return 1
fi
return "$status"
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| # systemd-resolved emits canonical address values; reject tokens outside that format. | ||
| for upstream_dns_ip in ${UPSTREAM_VNET_DNS_SERVERS}; do | ||
| case "${upstream_dns_ip}" in | ||
| "::"|*[!0-9a-fA-F.:]*|"") | ||
| echo "Invalid upstream VNET DNS server '${upstream_dns_ip}' in ${RESOLV_CONF}." |
|
the listener check is right, but it's in the wrong place. 1. put both listeners in the wait condition instead: if [ -n "$current_dns" ] \
&& ! grep -qwF "$LOCALDNS_NODE_LISTENER_IP" <<< "$current_dns" \
&& ! grep -qwF "$LOCALDNS_CLUSTER_LISTENER_IP" <<< "$current_dns"; thenthen 2. if you want real validation, use something that parses: ip route get "${upstream_dns_ip}" >/dev/null 2>&1 || { echo "..."; return 1; }or drop the format check entirely and keep only the listener check — systemd-resolved doesn't emit garbage, and you say so yourself in the comment. 3. one loop, not two. 4. sed -e "s|${AZURE_DNS_IP}|${UPSTREAM_VNET_DNS_SERVERS}|g" \
"${LOCALDNS_CORE_FILE}" > "${UPDATED_LOCALDNS_CORE_FILE}" || {
echo "Failed to generate ${UPDATED_LOCALDNS_CORE_FILE}"
return 1
}one write instead of two, and no window where 5. tests cover reject paths. is there one for "wait succeeds after a mixed list clears"? that's the path that decides whether a node provisions or burns a restart slot. |
738838b to
9f50a52
Compare
|
|
|
Summary
Reject invalid upstream resolver state before generating the LocalDNS
Corefile.
Day-0 bug
During LocalDNS startup,
70-localdns.confis removed so DHCP/VNET DNSservers can be discovered and used as CoreDNS upstreams. The network reload is
asynchronous, so
resolv.confcan temporarily be empty, stale, or contain amix of a real upstream and a LocalDNS listener.
The existing code flattened all
nameserverentries into one space-joinedstring and compared that string with individual IP addresses. As a result,
states such as the following were not rejected:
The LocalDNS listener could then be inserted into the CoreDNS
forwarddirective, creating a self-referential forwarding path. An exact listener-only
state could also skip replacement and be persisted as the upstream. The cluster
listener
169.254.10.11was not checked at all.Change
169.254.10.10.169.254.10.11.168.63.129.16.replace_azurednsip_in_corefileso resolverchanges after the wait are surfaced rather than hidden by reusing an older
snapshot.
Empty resolver state is already handled by the non-empty wait guard from
#9361. This PR addresses the
follow-up listener and mixed-list validation described in
#9361 review discussion.
Testing
bash -npassed for the modified shell script and spec.git diff --checkpassed.should provide the full ShellSpec and ShellCheck validation.