Refactor rebuild_proxies to separate resolution and auth handling #5924
Conversation
without stripping Proxy-Authorization header
There was a problem hiding this comment.
Hey @nateprewitt, thanks for this!
I added a few comments following up on our discussions.
| @@ -633,7 +620,10 @@ def send(self, request, **kwargs): | |||
| kwargs.setdefault('stream', self.stream) | |||
| kwargs.setdefault('verify', self.verify) | |||
| kwargs.setdefault('cert', self.cert) | |||
| kwargs.setdefault('proxies', self.rebuild_proxies(request, self.proxies)) | |||
| if 'proxies' not in kwargs: | |||
There was a problem hiding this comment.
This change should actually fix the performance regression in most cases (Session.request - which I assume is the most common flow - always sets proxies on kwargs).
There was a problem hiding this comment.
Yep, I think proxies will be the escape hatch when performance is a concern here :)
| @@ -633,7 +620,10 @@ def send(self, request, **kwargs): | |||
| kwargs.setdefault('stream', self.stream) | |||
| kwargs.setdefault('verify', self.verify) | |||
| kwargs.setdefault('cert', self.cert) | |||
| kwargs.setdefault('proxies', self.rebuild_proxies(request, self.proxies)) | |||
| if 'proxies' not in kwargs: | |||
| kwargs['proxies'] = resolve_proxies( | |||
There was a problem hiding this comment.
So now we're:
- No longer stripping the proxy-auth header. Sounds right to me.
- No longer setting the proxy-auth header at all. Is this the desired behavior? What if someone uses a proxy and passes the username + password in the url? (though I'm not sure if that flow is supported or not)
There was a problem hiding this comment.
For 2.) we were never doing that before this change and I don't think that was an intended byproduct. We should still support our normal proxy auth flows that were available prior to #5681.
| no_proxy = proxies.get('no_proxy') | ||
| new_proxies = proxies.copy() | ||
|
|
||
| bypass_proxy = should_bypass_proxies(url, no_proxy=no_proxy) |
There was a problem hiding this comment.
Didn't we want to unify 850 and 851 so we don't call should_bypass_proxies if trust_env is False?
There was a problem hiding this comment.
We do, I was going to have @dbaxa rebase their change onto whatever we merge so they can still have a commit in the repo.
|
When can we merge this @nateprewitt ? |
|
I believe we're waiting on either some consensus with this proposal or feedback for another direction. @sigmavirus24 or @sethmlarson, do you have any thoughts on how we proceed with #5888? |
This is a proposal for handling #5888 that's come out of discussion in an alternative PR (#5893). Given that we've stalled out on progress there for a few weeks, this PR is to help drive a conclusion. The approach taken here is to mitigate the incorrect behavior of stripping
Proxy-Authorizationheaders off all requests sent withSession.send. This will not address the performance concerns from #5891 as that's a more significant problem without a clear answer.The goal of driving this separately from #5891 is that one is an unintended breakage in basic behavior of the library and the second is an unfortunate performance regression. I'd like to address the former more quickly if possible instead of letting the problem fester.