fix: remove host-wide WireGuard dependency - #102
Conversation
protostatis
left a comment
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR retires a legacy host-wide WireGuard full tunnel that was blocking outbound DNS (CoinGecko, Telegram, Reddit, on-chain collectors) after the peer stopped handshaking, and switches production to direct EC2 egress with the existing cookie-backed Unbrowser transport for Reddit. The change removes the obsolete setup tooling (deploy/setup-wireguard.sh), adds a defensive WireGuard-retirement block to both deploy.yml and push-to-ec2.sh, and reworks the manual deploy script to preserve runtime state (data/logs/backups/run/.env) across a full app-directory replacement via a persist-and-restore pattern with an EXIT trap. Overall this is a well-constructed, correctly-documented cleanup with sensible fail-fast checks and real production validation. The main substantive issue is that the 'direct egress' canary uses github.com, which the removed WireGuard config had explicitly carved out as a bypass route — meaning a host with a still-active full tunnel would pass the canary instead of failing. Secondary nits are the transient exposure of .env (secrets) in a world-readable /tmp dir and the persist phase running without a protecting trap. No blockers found.
Verdict: Comment
Comments
- restore_runtime_state installs the EXIT trap, then is also invoked manually before 'trap - EXIT'. This double-invocation is correct for the success path but worth a comment/test: if the explicit restore's inner 'mv' fails under set -e, the exit path re-enters restore_runtime_state via the trap, and the non-sudo 'rm -rf $APP_DIR/$name' / 'mv' inside it assume the target dir is ec2-user-owned (which it is, after the chown). Low risk; a guard or set +e around the inner moves would make the failure path more predictable.
- The removal of setup-wireguard.sh is clean, but note that any other references to WG_* env vars (e.g. docker-compose.yml or crawler startup) are not touched in this diff. Confirm no lingering WG_PRIVATE_KEY/WG_* consumers remain, since the .env.docker.example removal only affects the template and not a live host's existing .env.
- Documentation changes are consistent and accurate; docs/EC2_INSTANCES.md correctly uses api.coingecko.com for its egress verification while the scripts diverge and use github.com — worth aligning.
Reviewed by Sky — Unchained Sky engineering agent
| ;; | ||
| esac | ||
| sudo rm -f /etc/wireguard/wg0.conf | ||
| if ! timeout 15 getent ahostsv4 github.com >/dev/null 2>&1; then |
There was a problem hiding this comment.
Weak egress canary: the removed deploy/setup-wireguard.sh added static bypass routes for GitHub ranges (140.82.112.0/20, 185.199.108.0/22 in PostUp), so github.com DNS + robots.txt were reachable via direct EC2 even while the full tunnel and VPN-only DNS were installed. A host with a still-active (dead-peer) wg0 would pass this check and silently deploy with broken DNS for the exact feeds this PR fixes. Use api.coingecko.com (as docs/EC2_INSTANCES.md already does) for both the DNS and HTTPS egress checks, since that host was carried over the VPN and is the true signal of direct egress.
| echo "ERROR: Direct EC2 DNS is unavailable after WireGuard cleanup" | ||
| exit 1 | ||
| fi | ||
| if ! curl -fsS --connect-timeout 5 --max-time 15 https://github.com/robots.txt >/dev/null; then |
There was a problem hiding this comment.
Same as the DNS check above: github.com/robots.txt is not a meaningful direct-egress canary because GitHub was explicitly bypass-routed in the retired WireGuard config. Prefer a host like api.coingecko.com that actually traversed the VPN tunnel.
| ;; | ||
| esac | ||
| sudo rm -f /etc/wireguard/wg0.conf | ||
| if ! timeout 15 getent ahostsv4 github.com >/dev/null 2>&1; then |
There was a problem hiding this comment.
Mirrors the deploy.yml canary gap: getent/curl on github.com won't detect a lingering full tunnel because GitHub was a hardcoded bypass route in the old PostUp. Switch the canary to api.coingecko.com to actually verify direct EC2 egress.
| fi | ||
|
|
||
| # Preserve runtime state before replacing application files. | ||
| rm -rf "$PERSIST_DIR" |
There was a problem hiding this comment.
PERSIST_DIR lives under /tmp (typically mode 1777), and the loop moves .env there with its original ownership/permissions intact. For the duration of the deploy, the API keys and UNBROWSER_COOKIE_SERVICE_TOKEN in .env are readable by any local user. Prefer a persist location outside /tmp (e.g. /var/tmp/crypto-sentiment-persist or a root-owned dir) and/or chmod 600 the staged .env after the mv.
| mkdir -p "$PERSIST_DIR" | ||
| for name in data logs backups run .env; do | ||
| if [ -e "$APP_DIR/$name" ]; then | ||
| mv "$APP_DIR/$name" "$PERSIST_DIR/$name" |
There was a problem hiding this comment.
The persist-out phase (rm -rf $PERSIST_DIR through the for-loop mv) runs before the EXIT trap is installed, so an interruption mid-move leaves runtime state split between /opt and /tmp with no automatic restore. Consider installing the trap (or a targeted cleanup) before the move-out loop so a failed move cannot strand data.
1 participant
Summary
.envduring legacy manual deploymentsIncident context
The expired WireGuard peer stopped handshaking while its full-tunnel route and VPN-only DNS remained installed. Containers stayed running, but CoinGecko, Telegram, Reddit, macro, and on-chain collectors could no longer resolve external hosts. Direct EC2 egress and the existing cookie-backed Unbrowser transport were verified in production, including fresh price and Reddit writes.
Validation
git diff --checkbash -n deploy/push-to-ec2.sh.github/workflows/deploy.ymlanddocker-compose.ymlwith PyYAMLuv run pytest tests/test_unbrowser_reddit.py tests/test_reddit_cookie_solver.py -q(11 passed)Notes