Skip to content

Allow proxy configuration via HTTP_PROXY env var#2161

Merged
st0012 merged 5 commits intogetsentry:masterfrom
natikgadzhi:sentry-ruby/env-proxy-vars-support
Nov 23, 2023
Merged

Allow proxy configuration via HTTP_PROXY env var#2161
st0012 merged 5 commits intogetsentry:masterfrom
natikgadzhi:sentry-ruby/env-proxy-vars-support

Conversation

@natikgadzhi
Copy link
Copy Markdown
Contributor

@natikgadzhi natikgadzhi commented Nov 3, 2023 •
edited
Loading

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?

@natikgadzhi natikgadzhi mentioned this pull request Nov 4, 2023
Merged
st0012
st0012 reviewed Nov 4, 2023
View reviewed changes
@st0012 st0012 added the bug fix label Nov 4, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 4, 2023 •
edited
Loading

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

@natikgadzhi @st0012
Update sentry-ruby/spec/sentry/transport/http_transport_spec.rb
aef805e 
Co-authored-by: Stan Lo <stan001212@gmail.com>
@natikgadzhi
Copy link
Copy Markdown
Contributor Author

natikgadzhi commented Nov 4, 2023

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
Copy Markdown
Contributor Author

natikgadzhi commented Nov 17, 2023

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
Copy Markdown
Contributor Author

natikgadzhi commented Nov 23, 2023 •
edited
Loading

@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
Copy Markdown
Member

sl0thentr0py commented Nov 23, 2023

i'll look tomorrow, irl now

st0012
st0012 approved these changes Nov 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

natikgadzhi commented Nov 23, 2023

@st0012 TIL. Fixed!

@st0012
Copy link
Copy Markdown
Contributor

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
@natikgadzhi
Copy link
Copy Markdown
Contributor Author

natikgadzhi commented Nov 23, 2023

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

@uhlhosting
Copy link
Copy Markdown

uhlhosting commented Dec 3, 2023

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

@natikgadzhi
Copy link
Copy Markdown
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

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sentry ignore proxy system configuration

4 participants

@natikgadzhi @sl0thentr0py @st0012 @uhlhosting