Honor http_proxy environment variables #1111
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
- Coverage 82.67% 82.53% -0.14%
==========================================
Files 53 53
Lines 7930 7947 +17
Branches 1240 1242 +2
==========================================
+ Hits 6556 6559 +3
- Misses 1263 1277 +14
Partials 111 111 |
| if (options->read_proxy_from_environment) { | ||
| sentry__set_proxy_from_environment(options); | ||
| } |
There was a problem hiding this comment.
With this new option, I'm not sure we still need to support no_proxy; if users want to use the environment variable value in their own code, they can just not set this flag to true (which is the default anyway).
There was a problem hiding this comment.
Yes and no. You always have to consider that this is a feature that the users of our users require from them.
So, for instance, if your application ignores the http_proxy env-vars, your users don't care about the topic and accept that they have to manually configure the proxy config for that application (or even have no proxy config at all).
At some point, you might provide a UI to your users that does something like the following:
( ) direct HTTP connection.
( ) manually specify HTTP proxy: _________ .
( * ) read proxy from the environment.
You can route this directly to our proposed options interface. However, once your users have configured the app to read from the environment, they will want to stay with it because it allows them to have a centrally managed proxy configuration rather than defining it separately in every application.
This is where no_proxy enters the picture. So, yes, resetting the proxy config of that app solves the problem, but then the users of our users are essentially back to square 1. Again, this is not a requirement for an initial implementation of that feature but a likely follow-up.
| void | ||
| sentry__set_proxy_from_environment(sentry_options_t *opts) | ||
| { | ||
| const char *https_proxy = getenv("https_proxy"); |
There was a problem hiding this comment.
Should we support upper-case HTTP_PROXY and HTTPS_PROXY too? What happens if HTTP_PROXY and http_proxy are defined?
There was a problem hiding this comment.
I am fine with only accepting the lower-case versions, but since this is a "thing", we should clearly document the behavior.
| // ensure the proxy starts with `http://`, otherwise ignore it | ||
| if (opts->proxy && strstr(opts->proxy, "http://") == opts->proxy) { | ||
| const char *ptr = opts->proxy + 7; |
There was a problem hiding this comment.
Apparently our Windows transport doesn't take https:// proxies. This should be documented, since we now also read the HTTPS_PROXY environment variable, which on this platform would get ignored.
Fixes #787
Adds option to read
http_proxyfrom environment (also checkshttps_proxyand prioritizes this if it exists). If this flag is set to true, it will overwrite any previously set proxies withsentry_options_set_proxy.