feat: honor http_proxy environment variables
#1111
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
- Coverage 82.68% 82.65% -0.03%
==========================================
Files 53 53
Lines 7930 7935 +5
Branches 1240 1242 +2
==========================================
+ Hits 6557 6559 +2
- Misses 1263 1265 +2
- Partials 110 111 +1 |
src/sentry_core.c
Outdated
| 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.
JoshuaMoelans
changed the title
http_proxy environment variables
|
tests/test_integration_http.py
Outdated
| if proxy_auth == ["on"]: | ||
| current_run_arg += "-auth" | ||
| if proxy_from_env == ["proxy-from-env"]: | ||
| current_run_arg = "proxy-from-env" # overwrite args if proxy-from-env is set (e.g. don't manually set) |
There was a problem hiding this comment.
In cURL transport we don't even need to call our proxy-from-env setter, since it will attempt to read the environment variable automatically https://everything.curl.dev/usingcurl/proxies/env.html .
This kind of makes this feature only really useful for Windows.
There was a problem hiding this comment.
Is this also true for the library (i.e., libcurl)? There are a couple of such features where the library only provides the mechanism, but the actual read is then implemented in the application (i.e., curl). The docs you link refer to the CLI utility, not the library.
There was a problem hiding this comment.
Seems like it is https://curl.se/libcurl/c/libcurl-env.html
There was a problem hiding this comment.
That means our default behavior should be to enable this since curl also enables it by default. There is no way to override reading from env (except for a build flag, which we don't control) except when the proxy is configured explicitly.
| @pytest.mark.parametrize("proxy_status", [(["off"]), (["on"])]) | ||
| def test_capture_proxy(cmake, httpserver, run_args, proxy_status): | ||
| @pytest.mark.parametrize("proxy_auth", [(["off"]), (["on"])]) | ||
| @pytest.mark.parametrize("proxy_IPv", [(["4"]), (["6"])]) | ||
| @pytest.mark.parametrize("proxy_from_env", [(["proxy-from-env"]), ([""])]) |
There was a problem hiding this comment.
This test is becoming exponentially larger. It might make sense to split apart some test cases to not cover the full matrix at once. There are still some untested cases:
- What if
proxy-from-envbut the env. variable is empty? -> expected is that if there was a manually set proxy, it gets used. - Test whether
proxy-from-envtakes precedence over manually set proxy if both are given.
Fixes #787
http_proxyfrom environment variables sentry-docs#12424)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_proxyif the environment variables have a value.