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
Update Changelog #1847
Update Changelog #1847
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1847 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 2013 2013
=========================================
Hits 2013 2013
Continue to review full report at Codecov.
|
@jalopezsilva I tried to explain the current state of HTTPS proxies support. Can you please take a look? |
So what I'm seeing is we're looking at what may potentially be released as a minor release (the only feature is I'm wondering if we should make a 1.25.9 release with everything on master except the HTTPS proxies and then allowing the HTTPS proxy features to be released all at once in 1.26. We can do this by creating a What are our thoughts here? |
That sounds good to me! |
I can handle that construction. :) |
That sounds good to me as well! If you're doing a minor release, it might be worth adding the warning that I added when people attempt to contact an HTTPS proxy through their configuration. You'll have to cherry-pick the change and don't mention the Right now, people can have a configuration like the following:
We currently ignore the HTTPS on the URL for the HTTPS proxy definition. When we close #1806, we'll start using it and potentially attempting to connect using TLS to a non-TLS port. The benefit of adding the warning now, is that we'll give users a chance to clean their configuration before we add the HTTPS proxy support. If we don't do it, we might end up trying to do a TLS connection to a proxy that doesn't support TLS and the error might be even more confusing for users. I noticed this misconfiguration happening a lot when internally patching our libraries. |
@jalopezsilva Sorry for taking so much time to reply here, I've been trying things out over the past few days. On the What I see on
With the change on
Ideally we want for the last scenario to be: I'm wondering if we need to have a warning at all for v1.25.9 if HTTPS proxies in urllib3 both don't work? I'm just unsure about adding a warning to a minor that will be going away with a proper fix in the next major. Warnings are a tricky subject when the library is so widespread.
This comment has me confused, I don't see urllib3 ignoring HTTPS proxies, just not working at all? Is that what you meant or did you mean "we" meaning your configuration/scripts? |
Hey @sethmlarson, sorry I should have been clearer. The issue arises with a misconfiguration by users on their proxies. If you apply the following local patch, you'll understand what I'm talking about: https://pastebin.com/raw/GZDcwPE7 The patch expands the With the
The last case with a HTTPS proxy with an HTTPS destination should fail but doesn't. That's because the proxy scheme is mostly ignored. It's possible for users to have a 'HTTPS://' on their configuration when they really mean 'HTTP://'. With the The question is how prevalent is this misconfiguration? I found around 82 cases in our monorepo across teams. I don't have a repro handy but I suspect this misconfiguration also affects requests: https://github.com/psf/requests/blob/master/requests/adapters.py#L193. I cleaned up our configurations internally so I'm not worried about us. I do suspect this misconfiguration is out on the wild though.. I'm worried that if we roll TLS in TLS in |
@joelverhagen Okay now I perfectly understand, thank you so much for the super-detailed write up (again). Sorry for all the confusion :) I'll create a PR against |
I've created the PR: #1851 |
Closing in favor of #1852, thanks all. |
@pquentin We'll need to update the changelog on master as well, people get confused about releases "missing" without it. :) |
Warning looks great. I've subscribed myself to 1850 to see how many people are affected. Hopefully not too many. Thanks everyone! |
@sethmlarson Thanks for updating the master branch, nice catch. 👍 |
No description provided.