Add SOCKS support to proxy configuration parameter #1861
Conversation
Both HTTP and SOCKS proxy URL can be read from either the 'mirror.proxy' configuration option or <PROTO>_PROXY environment variables.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1861 +/- ##
==========================================
+ Coverage 79.69% 86.35% +6.65%
==========================================
Files 31 35 +4
Lines 4324 4440 +116
Branches 780 467 -313
==========================================
+ Hits 3446 3834 +388
+ Misses 721 429 -292
- Partials 157 177 +20 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
All looks great. Many thanks!
Just wondering if we should always log when we're applying proxy config and maybe test IPv6 URLs (they should) work ...
If you don't have time to test IPv6 and don't agree we should log cause somewhere else already does, just say so, that's fine :)
src/bandersnatch/config/proxy.py
Outdated
| logger.debug("using SOCKS ProxyConnector for %s", proxy_url) | ||
| return {"connector": ProxyConnector.from_url(proxy_url)} | ||
|
|
||
| if lowered.startswith("http"): | ||
| logger.debug("using HTTP proxy address %s", proxy_url) |
There was a problem hiding this comment.
| logger.debug("using SOCKS ProxyConnector for %s", proxy_url) | |
| return {"connector": ProxyConnector.from_url(proxy_url)} | |
| if lowered.startswith("http"): | |
| logger.debug("using HTTP proxy address %s", proxy_url) | |
| logger.info("using SOCKS ProxyConnector for %s", proxy_url) | |
| return {"connector": ProxyConnector.from_url(proxy_url)} | |
| if lowered.startswith("http"): | |
| logger.info("using HTTP proxy address %s", proxy_url) |
Wonder if we want to always tell people we're using a proxy?
There was a problem hiding this comment.
Very good point, if someone configured a proxy (...or didn't but one is used because of an environment variable), it would be important to know.
I left the messages in bandersnatch.config.proxy at 'debug', and added an 'info' level message to the Master initializer, with the idea that it's a little less verbose by default but can still optionally be traced in more detail.
| "SOCKS4_PROXY": "socks4://192.0.2.113:1080", | ||
| "SOCKS5_PROXY": "socks5://192.0.2.114:1080", | ||
| }, | ||
| "socks5://192.0.2.114:1080", |
There was a problem hiding this comment.
Should we put in an IPv6 URL for good measure?
There was a problem hiding this comment.
I think that is a good idea - I was tripped up several times by ProxyConnector.from_url rejecting things I expected to be valid, so it's definitely worth verifying.
None of these new tests cover all the way through to when the aiohttp.ClientSession is constructed - do you think that is worth covering?
There was a problem hiding this comment.
No, I think there we should rely on aiohttp tests. But I just want to know we pass IPv6 correctly if the libraries support it (pretty sure they do).
I think these are both excellent points! Ideally both should be straightforward. Thanks for the fast review! |
- Add IPv6 addresses to test cases (excellent sanity check since aiohttp_socks does some url validation) - Always log an 'info' level message if proxy configuration is being used
This unifies proxy configuration handling for HTTP and SOCKS, so that:
mirror.proxyoptionmirror.proxyoption as well as environment variablesFixes #1674