Use residential proxy for Reddit access#58
Conversation
protostatis
left a comment
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR replaces the WireGuard host-level VPN with an app-level residential proxy for Reddit access. It simplifies the architecture considerably — moving from complex iptables/conntrack/SSH-tunnel routing to a straightforward HTTP proxy passed via environment variables. The domain matching now covers subdomains (old.reddit.com, www.reddit.com), proxy resolution adds RESIDENTIAL_PROXY as a fallback behind PROXY_URL, and the deploy script automatically disables WireGuard when a proxy is configured. Tests cover the fallback and precedence behavior. The changes look sound overall; a few suggestions below.
Verdict: Comment
Comments
- Typing: tests/test_fetcher_proxy.py uses
monkeypatchwithout a type annotation. Since the diff mentions running ruff, addmonkeypatch: pytest.MonkeyPatchto avoid FBT001 or similar lint warnings. - Missing test case: there is no test for the default path when neither PROXY_URL nor RESIDENTIAL_PROXY is set. This is the most common misconfiguration scenario — worth asserting that proxies is empty and _needs_proxy still identifies Reddit domains (so the caller can warn instead of silently failing).
- Positive: dropping the WireGuard + connmark + SSH tunnel + socat stack in favor of a simple HTTP proxy significantly reduces operational surface area. The deploy script auto-disabling wg-quick@wg0 when a proxy is configured is a nice safety net against conflicting routing.
- Positive: PROXY_URL taking precedence over RESIDENTIAL_PROXY is the right design — gives a clear override path if a different proxy is needed for this crawler specifically.
Reviewed by Sky — Unchained Sky engineering agent
| @@ -111,7 +115,11 @@ def __init__( | |||
| self.rate_limiters: dict[str, RateLimiter] = {} | |||
There was a problem hiding this comment.
Consider what should happen when neither PROXY_URL nor RESIDENTIAL_PROXY is set. Currently fetcher.proxies becomes an empty list — proxy calls won't happen for Reddit domains, which could silently cause 403s. Consider logging a warning when proxy is needed but no proxy is configured.
| @@ -162,7 +170,10 @@ def _get_next_proxy(self) -> str | None: | |||
| def _needs_proxy(self, url: str) -> bool: | |||
| """Check if URL requires proxy (blocked domains).""" | |||
| domain = self._get_domain(url) | |||
There was a problem hiding this comment.
If _get_domain returns an empty string (e.g. from a malformed URL where parsed.hostname is None), the equality check domain == required_domain matches and _needs_proxy returns True for an empty domain. Add and domain to guard against this edge case: return any(domain and (domain == required_domain or domain.endswith(f'.{required_domain}')) for ...).
| # Setup WireGuard VPN if configured | ||
| if grep -q WG_PRIVATE_KEY .env 2>/dev/null; then | ||
| echo "Setting up WireGuard VPN..." | ||
| # Prefer the app-level residential proxy for Reddit. WireGuard remains only as |
There was a problem hiding this comment.
Add set -euo pipefail at the top of this script (if not already present) for safety around the conditional logic here. Without it, the grep or systemctl failures could be silently ignored.
1 participant
Summary: routes Reddit domains through the app-level residential proxy, supports RESIDENTIAL_PROXY fallback, passes crawler env during deploy, and updates docs/tests. Tests: uv run pytest tests; uv run --with ruff ruff check crypto_sentiment_crawler/crawler/fetcher.py tests/test_fetcher_proxy.py; git diff --check.