Skip to content
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

Allow proxy configuration via HTTP_PROXY env var #2161

Merged

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Nov 3, 2023

Summary

This PR allows sentry-ruby to use proxy config passed via environment variables, i.e. HTTP_PROXY. Closes #2131. The actual configuration settings is still respected over environment variables.

Changes

  • As @sl0thentr0py suggests, just removes the nil passed into ::Net::HTTP, and allows it to do all the work for us.
  • Adds a changelog entry.

TODOs

Review

  • @stephanie-anderson, with this PR, config.transport_configuration.proxy is preferred over the environment values:
    • If the config.transport_configuration.proxy is present, but malformed (does not have the minimally required uri key), it will still be used over HTTP_PROXY, even if that has the correct setup.
    • We should probably be consistent with how our other SDKs work with env vars — should we generally prefer the config passed in code, or the env vars?

@st0012 st0012 added the bug fix label Nov 4, 2023
Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #2161 (a706d3f) into master (7a65d3f) will increase coverage by 0.00%.
Report is 14 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2161   +/-   ##
=======================================
  Coverage   97.33%   97.34%           
=======================================
  Files          97       99    +2     
  Lines        3638     3687   +49     
=======================================
+ Hits         3541     3589   +48     
- Misses         97       98    +1     
Components Coverage Δ
sentry-ruby 98.03% <100.00%> (+<0.01%) ⬆️
sentry-rails 94.98% <ø> (ø)
sentry-sidekiq 94.50% <ø> (+0.79%) ⬆️
sentry-resque 93.65% <ø> (ø)
sentry-delayed_job 94.44% <ø> (+0.07%) ⬆️
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-ruby/lib/sentry/transport/configuration.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transport/http_transport.rb 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

Co-authored-by: Stan Lo <stan001212@gmail.com>
@natikgadzhi
Copy link
Contributor Author

Curiously, it fails for Ruby 2.4: https://github.com/getsentry/sentry-ruby/actions/runs/6755752783/job/18365644960?pr=2161#step:6:1523 I'll try it out locally and make a workaround.

@natikgadzhi
Copy link
Contributor Author

I'm still on this — proved to be difficult to get to locally (2.4 doesn't easily install on M1 with rbenv, so I'll docker this tomorrow). I have no clue why this works with Ruby 2.4 on Rack 2, but not on Ruby 2.4 and Rack 3 =/

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Nov 23, 2023

@sl0thentr0py if you're in the mood to merge things — can you help me out with this one? I was hitting a wall on that Ruby 2.4 test.

@sl0thentr0py
Copy link
Member

i'll look tomorrow, irl now

Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Sorry for the delay

@st0012
Copy link
Collaborator

st0012 commented Nov 23, 2023

The CI fails because Ruby didn't support getting user/pass from http_proxy until 2.5 with this commit. I think you need to add Ruby version condition on both proxy_user and proxy_pass assertions.

@natikgadzhi
Copy link
Contributor Author

@st0012 TIL. Fixed!

@st0012
Copy link
Collaborator

st0012 commented Nov 23, 2023

Thank you for the fix as well as the new documentation!!

@st0012 st0012 merged commit c4e4797 into getsentry:master Nov 23, 2023
97 checks passed
@natikgadzhi
Copy link
Contributor Author

I'll make a follow up docs PR when we ship this out.

@uhlhosting
Copy link

@natikgadzhi any idea when this fix will be pushed in self hosted dev release?

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Dec 3, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentry ignore proxy system configuration
4 participants