-
Notifications
You must be signed in to change notification settings - Fork 982
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
Fix #1219 Net::HTTP still uses env proxy #1221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving this!
conn_options[:proxy] = 'http://google.co.uk' | ||
context 'when a proxy is provided as option' do | ||
before do | ||
conn_options[:proxy] = 'http://google.co.uk' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this value is a bit not-self-documenting? "Does Google act as a proxy? How?" the reader (me) might think.
|
||
it 'handles requests with proxy' do | ||
res = conn.public_send(http_method, '/') | ||
expect(res.status).to eq(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, which I woudl like: Separate the "acts" from the "assert" with a newline
|
||
it 'handles requests with proxy' do | ||
res = conn.public_send(http_method, '/') | ||
expect(res.status).to eq(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(A newline, please. Thanks!)
shared_examples 'proxy examples' do | ||
it 'handles requests with proxy' do | ||
res = conn.public_send(http_method, '/') | ||
expect(res.status).to eq(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A newline, yeah?
@@ -1,5 +1,18 @@ | |||
# frozen_string_literal: true | |||
|
|||
shared_examples 'proxy examples' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really, I like this shared one.
faraday is a transitive dependency for us, is the expectation now that faraday will ignore env variables always? we were depending on that behavior |
@walterking Not at all! The problem was that using Net::HTTP adapter and setting If you don't set that config (which defaults to false), Faraday will continue to automatically pick the proxy from the environment for you |
hm, we dont set that variable that i can see. we updated to 1.2 and started to get connection refused errors, which we usually see when the proxy isnt being used, and this pr seemed like the most likely to cause that change We expect the error since i dont have a proxy actually running locally:
|
Mmmh thanks for the example, I'll have a closer look and try to understand what's wrong, but yes it seems like 1.2 did change something... That said, http_proxy should be used for http requests, while https_proxy is the one for https, so I'll try a few combinations. Right now my guess is that Net::HTTP might have used them incorrectly 🤔 |
interesting. in our app we set both http and https, but when i switch my local to https it seems to do the right thing |
So, I did some extra testing and I believe the issue is with # ~ HTTP_PROXY=http://127.0.0.1:1234 irb
> require 'net/http'
> Net::HTTP.new('http://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))
> Net::HTTP.new('https://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234))
# ~ HTTPS_PROXY=http://127.0.0.1:1234 irb
> require 'net/http'
> Net::HTTP.new('http://www.google.com').get('/')
=> SocketError (Failed to open TCP connection to http://www.google.com:80 (getaddrinfo: nodename nor servname provided, or not known))
> Net::HTTP.new('https://www.google.com').get('/')
=> Errno::ECONNREFUSED (Failed to open TCP connection to 127.0.0.1:1234 (Connection refused - connect(2) for "127.0.0.1" port 1234)) So as you can see |
Description
Ensures that the
proxy_address
is passed asnil
toNet::HTTP
to avoid that it picks the one from the environment.Also, since Net::HTTP::Proxy is obsolete (https://ruby-doc.org/stdlib-2.3.2/libdoc/net/http/rdoc/Net/HTTP.html#method-c-Proxy), it has now been replaced with a different call to
.new
.Fixes #1219