Tolerate read-only EC2 resolver during deploy#62
Conversation
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR improves deploy resilience on EC2 by tolerating read-only /etc/resolv.conf. The changes replace blind DNS restoration with a conditional check, prefer systemd-resolved recovery over direct resolv.conf modification, and add fallback nameservers. The WireGuard shutdown sequence is now more thorough (stop → down → disable, vs disable --now). Overall solid infrastructure improvement with minor shell robustness issues to flag.
Verdict: Comment
Comments
- The fallback to
/etc/resolv.confnow includes three nameservers (EC2 VPC + Cloudflare + Google) versus the original single EC2 resolver. This is better for resilience, but adds dependency on external resolvers. If these are private subnets without egress to 8.8.8.8/1.1.1.1, the timeouts are harmless (2s each) but add ~6s to DNS recovery. Consider documenting the expected network topology. - The
resolvectl dnsandresolvectl domaincommands will silently fail with2>/dev/nullif the interface isn't managed by systemd-resolved (e.g., NetworkManager-managed). This is acceptable as the resolv.conf fallback covers it, but consider adding debug logging (|| echo 'WARN: resolvectl failed') to aid troubleshooting when deploy logs are inspected. - Good progression: conditional check → systemd-resolved recovery → resolv.conf fallback → final validation. This layered approach is much more robust than the original blind write.
- No security concerns — no secrets exposed, no new attack surface introduced. The changes are purely defensive DNS recovery logic.
Reviewed by Sky — Unchained Sky engineering agent
Inline Comments (could not attach to lines)
.github/workflows/deploy.yml:141 — Missing || true on getent hosts github.com. If deployment runs under set -e (common in CI), a transient DNS failure here would abort the entire deploy. Add || true to make this a diagnostic check rather than a hard guard, since the subsequent git fetch will fail anyway if DNS is truly broken.
.github/workflows/deploy.yml:142 — Missing || true on getent hosts ghcr.io. Same concern as the github.com check above — if DNS is still not resolved, let the deploy proceed to the git fetch which will surface the error with better context.
.github/workflows/deploy.yml:125 — The conditional if ! getent hosts github.com guards the entire DNS recovery block. This is efficient but means a stale DNS cache could skip recovery on first attempt while the cache hasn't expired yet. Consider adding a getent ahosts variant or a retry loop if DNS flakiness is common on these EC2 instances.
.github/workflows/deploy.yml:147 — The resolvectl dns command sets multiple fallback resolvers (EC2 VPC, Cloudflare, Google). If the host firewall blocks outbound 53/udp to non-EC2 resolvers, this is fine — resolvers are tried in order with timeout. But if DNSSEC validation is enabled, adding external resolvers alongside the VPC resolver could cause unexpected SERVFAIL. Confirm whether DNSSEC is active on this account.
.github/workflows/deploy.yml:122 — Adding sudo wg-quick down wg0 after systemctl stop is good — ensures the interface is torn down even if the systemd unit was in a bad state. One nit: wg-quick down itself calls systemctl stop internally, so you could skip the explicit systemctl stop and just rely on wg-quick down || true followed by systemctl disable. Either approach works, but the current order does a redundant stop.
Summary\n- stop WireGuard explicitly before deploy network operations\n- recover DNS via systemd-resolved before trying /etc/resolv.conf\n- make read-only /etc/resolv.conf a warning and keep final GitHub/GHCR DNS checks\n\n## Validation\n- git diff --check\n- ruby -e 'require "yaml"; YAML.load_file(".github/workflows/deploy.yml")'\n- actionlint unavailable locally