-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(iroh-net)!: Implement http proxy support #2298
Conversation
e4226aa
to
dca6b0b
Compare
Is it a good idea for a library to read environment variables? I think application should do this, there are more variables like NO_PROXY and at least in GNOME proxy is expected to be controlled via |
|
Environment variables for proxy configuration are extremely common. Fundamentally I think it makes sense because it is an environment configuration, that is the same for every tool on the machine, in almost every case, and it doesn't seem valuable to make the user comprehend separate configurations for every app they use that needs to access the network if it isn't necessary. |
Environment variable configuration is fine, but I think it should not be done by the libraries. If every library that does networking implements proxy variable parsing, then you will get slightly different handling of these environment variables within a single application: some libraries parse only uppercase variables, some handle Environment variables also have a downside that they cannot be reconfigured in runtime, so in Android applications you probably want to have some hooks that detect network changes, on desktop Linux you want to hook into NetworkManager changes because proxy may be needed in a corporate VPN and not even accessible outside this VPN. This kind of hooks/reconfigurations better happens on the application side because Rust libraries may not even be able to access necessary Java APIs on Android. Parsing environment variables in |
I see your point. Maybe we should have a Then the Iroh CLI will configure the client with the It would actually be handy to have the |
dca6b0b
to
f856b2b
Compare
the environment now needs to be read explicitly in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach of the builder in Endpoint
. Only minor note I have is that we now have two pin-project crates...
Thanks a bunch for this work. ❤️ |
Headers are based on how `curl` and env variables on`reqwest`. Setting any of - `HTTP_PROXY` - `http_proxy` - `HTTPS_PROXY` - `https_proxy` will make all relay code use these to proxy outgoing connections. Closes #2295 ## Breaking Changes - Added `iroh_net::endpoint::Builder::proxy_url` - Added `iroh_net::endpoint::Builder::proxy_from_env` - Added `iroh_net::relay::http::ClientError::Proxy` enum variant ## TODOs - [x] config & parsing env variables - [x] the todos in the code - [x] https proxy - [x] testing: tested manually on two machines using `squid`
Headers are based on how
curl
and env variables onreqwest
.Setting any of
HTTP_PROXY
http_proxy
HTTPS_PROXY
https_proxy
will make all relay code use these to proxy outgoing connections.
Closes #2295
Breaking Changes
iroh_net::endpoint::Builder::proxy_url
iroh_net::endpoint::Builder::proxy_from_env
iroh_net::relay::http::ClientError::Proxy
enum variantTODOs
squid