feat: honor http_proxy environment variables

[JoshuaMoelans]

Fixes #787

• add option
• add tests
• document what types of proxy strings work and which don't
• add to docs as to which variables we read (and what order they are read) (tracking on feat(native): http_proxy from environment variables sentry-docs#12424)

Adds option to read http_proxy from environment (also checks https_proxy and prioritizes this if it exists). If this flag is set to true, it will overwrite any previously set proxies with sentry_options_set_proxy if the environment variables have a value.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.65%. Comparing base (56198fc) to head (da3c91e).
Report is 1 commits behind head on master.

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     

[JoshuaMoelans]

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).

[supervacuus]

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.

[supervacuus]

👍

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 8411d8b

[JoshuaMoelans]

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.

[supervacuus]

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.

[JoshuaMoelans]

Seems like it is https://curl.se/libcurl/c/libcurl-env.html

[supervacuus]

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.

[JoshuaMoelans]

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-env but the env. variable is empty? -> expected is that if there was a manually set proxy, it gets used.
• Test whetherproxy-from-env takes precedence over manually set proxy if both are given. (it shouldn't)

[JoshuaMoelans]

We currently can't know if the event was sent over the proxy, or if it used a fallback on the existing connection. It might make sense to listen to the port the proxy is on & confirm it passes through there.

On the other hand, the port_correct kind of already checks this by seeing if the example reads the proxy from the env (which is now 'wrong' and doesn't point to the running proxy server) and therefor no event ends up in the httpserver.

[supervacuus]

After talking with you, I prefer to check the mitmdump output and count the logs there. We can do this at the end when proxy_process is terminated. This way, we don't have to guess whether another route by-passing the proxy allowed the request to succeed. This is also true for the other proxy tests.

Only thinking out aloud, maybe I missed a detail, but that would be the route I would choose first.

[JoshuaMoelans]

We now have a way to read the stdout after terminating mitmdump, and we can check it as such:

proxy_process.terminate()
proxy_process.wait()
stdout, stderr = proxy_process.communicate()
assert "POST" in stdout

It makes sense to have this check for all of our proxy tests, as this is a more rigorous check on what goes through/around our proxy server.