Skip to content

Tolerate read-only EC2 resolver during deploy#62

Merged
protostatis merged 1 commit into
mainfrom
fix/deploy-readonly-resolvconf
Jun 1, 2026
Merged

Tolerate read-only EC2 resolver during deploy#62
protostatis merged 1 commit into
mainfrom
fix/deploy-readonly-resolvconf

Conversation

@protostatis

Copy link
Copy Markdown
Owner

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

@protostatis protostatis left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.conf now 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 dns and resolvectl domain commands will silently fail with 2>/dev/null if 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.

@protostatis
protostatis merged commit b7491e4 into main Jun 1, 2026
3 checks passed
@protostatis
protostatis deleted the fix/deploy-readonly-resolvconf branch June 1, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant