fix: reject empty resolver state during LocalDNS startup - #9361
fix: reject empty resolver state during LocalDNS startup#9361Saewon Kwak (saewoni) wants to merge 1 commit into
Conversation
Windows Unit Test Results 3 files 13 suites 49s ⏱️ Results for commit c3c511b. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Restores upstream DNS after LocalDNS cleanup by reapplying network configuration to the live interface.
Changes:
- Reconfigures the default-route interface after network reload.
- Adds success and failure ShellSpec coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
parts/linux/cloud-init/artifacts/localdns.sh |
Reapplies DNS configuration during cleanup. |
spec/parts/linux/cloud-init/artifacts/localdns_spec.sh |
Tests reconfiguration success and failure. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
don't merge yet — the e2e gate is red because of this pr, not flake. networkctl reconfigure on the default-route link wipes resolv.conf instead of reapplying upstream dns, so localdns hits the same exit 216 one step later. details inline on localdns.sh:720.
| scenario | this pr | 9362 | 9328 |
|---|---|---|---|
| localdns hosts plugin ubuntu 2404 | ❌ 216 | ✅ | ✅ |
| localdns hosts plugin azure linux v3 | ❌ 216 | ✅ | ✅ |
| localdns hosts plugin ACL | ❌ 216 | ✅ | ✅ |
| localdns hosts plugin ubuntu 2204 / 2604 minimal | ✅ | ✅ | ✅ |
Test_DCGM_Exporter_Compatibility/Ubuntu2404 shows the same empty-dns trace, so this hits any scenario with localdns on, not just the localdns tests. 2204/2604 only pass because they win the race.
the wait is also wrong, and this pr is what exposes it. wait_for_localdns_removed_from_resolv_conf (localdns.sh:607) only checks that 169.254.10.10 is absent. an empty nameserver list satisfies that, so it returns success and logs "DNS configuration refreshed successfully" on an empty resolver, then replace_azurednsip_in_corefile finds nothing and exits 216. it should require a non-empty upstream list that does not contain the listener ip. worth fixing regardless of what happens to this pr.
the repro in the description is circular — the mock was wired to write the corrected resolver file, then the test observed the corrected resolver file. the real reconfigure does the opposite.
suggested direction: if networkctl reload really isn't converging on some nodes, fix the wait (require a non-empty upstream list, extend the 5s budget) instead of reconfiguring the default-route link. please get a live ubuntu 24.04 repro first to confirm reload-only convergence is actually the problem.
nit: commit body says (cherry picked from abcca13) — link the source pr in the description.
dad5b69 to
c3c511b
Compare
|
Replying to Ye Wang [msft] (@yewmsft)'s review #pullrequestreview-5083056681 — all points addressed as of
Fixed —
Fixed exactly as suggested — if [ -n "$current_dns" ] && ! grep -qwF "$LOCALDNS_NODE_LISTENER_IP" <<< "$current_dns"; thenNow requires a non-empty upstream that does not contain the listener IP. Empty / comment-only / missing
Fixed — the circular mocked repro was removed from the description. It's replaced with a real live before/after (below).
Done. Validated live on an Ubuntu 24.04 AKS node — shipped-VHD baseline vs the PR's
Full before/after tables (node-level and pod-level) are in the PR description.
Fixed — no cherry-pick line remains in the commit body, and the description now has a ## Source section linking #9360. Re-review appreciated 🙏 |
Failed gate run
Detective summaryUbuntu 20.04 FIPS VHD failed while enabling Ubuntu Pro FIPS updates. The first failing step was Likely causeSignature: Recommended owner/actionNode Lifecycle/VHD owner: track as the existing Ubuntu Pro machine-token/fips-updates flake; no PR action unless this reproduces consistently outside Ubuntu Pro service failures. Strongest alternativePR-change-caused LocalDNS regression is the strongest alternative because PR 9361 changes Evidence
Wiki signature |
Failed gate run
Detective summaryVHDCaching scenarios failed while creating VMSS from custom gallery images. The first failing step was VMSS PUT for Ubuntu2604Minimal/Ubuntu2404 VHDCaching; Compute returned repeated Likely causeSignature: Recommended owner/actionNode Lifecycle/E2E owner: track image-version availability before VHDCaching VMSS create, or add targeted wait/retry around gallery image availability. Strongest alternativePR-change-caused LocalDNS regression is the strongest alternative because PR 9361 changes LocalDNS startup; it is less likely because the VMSS never reaches LocalDNS/node validation and fails on Compute gallery image lookup. Evidence
Wiki signature |
Problem
When LocalDNS exits, cleanup removes the
70-localdns.confdrop-in and runsnetworkctl reload. During that reload the active resolver state (/run/systemd/resolve/resolv.conf) can transiently contain no nameservers at all — or still contain the LocalDNS listener169.254.10.10while the listener is already gone.The LocalDNS startup path waits for the LocalDNS listener address to disappear from
resolv.confbefore reading the upstream VNET DNS servers into the Corefile. The old guard only checked that169.254.10.10was absent:An empty nameserver list satisfies that condition. So
wait_for_localdns_removed_from_resolv_confreturned success on an empty resolver, logged "DNS configuration refreshed successfully" against an empty upstream, and startup proceeded.replace_azurednsip_in_corefilethen found no upstream to substitute and exited 216 (ERR_LOCALDNS_FAIL).Because LocalDNS serves both listeners — the node listener
169.254.10.10and the pod/cluster listener169.254.10.11— from a single Corefile, this aborted startup before either listener was configured. Any scenario with LocalDNS enabled was affected (node and pod resolution), not just the LocalDNS tests; distros only differed by whether they won the timing race.Change
wait_for_localdns_removed_from_resolv_confnow requires both:If the resolver is empty, comment-only, or missing, the function keeps polling until a real upstream appears (or times out) instead of falsely reporting success. The existing asynchronous
networkctl reloadbehavior is retained — this PR does not callnetworkctl reconfigure, which live testing showed clearsresolv.confand reproduces the same exit-216 one step later.cleanup_iptables_and_dnsalso now re-initializesDEFAULT_ROUTE_INTERFACEalongside the drop-in path variables, so cleanup invoked from a trap or watchdog restart has a complete network context.Behavior
resolv.confstate during startupwaitreturns 0, logs "refreshed successfully" on empty upstream →replaceexits 216waitkeeps waiting / times out; startup does not proceed on empty upstream# nameserver ...)waitreturns 0 (false success) → 216waitkeeps waiting / times outwaitreturns 0 (false success) → 216waitkeeps waiting / times outwaitreturns 0,replacesucceedswaitreturns 0,replacesucceeds (unchanged)Validation
bash -nonlocaldns.shandlocaldns_spec.sh;git diff --check.Live AKS before/after
Validated on a live Ubuntu 24.04 AKS cluster (the distro where the failure reproduces), running the shipped VHD
localdns.shas the baseline and the PRlocaldns.shapplied to a node and restarted viasystemctl restart localdns.service.The shipped node's
localdns.shhas no non-empty guard (grep -c 'n "$current_dns"' = 0), confirming the baseline is the buggy version.Startup wait — before vs after (empty-resolver repro)
Running the real
wait_for_localdns_removed_from_resolv_confandreplace_azurednsip_in_corefilefunctions from each script against the injected resolver states:resolv.conf)waitrcwaitrcreplace→ rc=1 "No Upstream VNET DNS servers found" → exit 216168.63.129.16replacerc=0replacerc=0 (unchanged)Baseline, Case A (verbatim):
Fixed, Case A (verbatim):
Node-level and pod-level resolvers — after the fix
The PR script was installed on the patched node and
localdns.servicerestarted. Both listeners come up healthy (adig health-check.localdns.localagainst each IP succeeds):Because both the node listener (
169.254.10.10) and the pod/cluster listener (169.254.10.11) are served from the one Corefile that startup was previously aborting before writing, the fix restores both node-level and pod-level resolution. The patched node was restored to the shipped baseline afterward (localdns.serviceactive, both listeners OK); the cluster was left in its original state.Source
The
networkctl reconfigureapproach was explored in the earlier #9360 branch but is intentionally not included here — reviewer E2E showed it caused the same empty-resolver failure on Ubuntu 24.04, Azure Linux V3, and ACL. This PR is based onmainand contains only the resolver-wait correction.