gh-149746: Handle CIDRs in NO_PROXY env var - #156336
Open
turettn wants to merge 4 commits into
Open
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Copy link
Copy Markdown
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
Copy link
Copy Markdown
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead. |
Sorry, something went wrong.
Copy link
Copy Markdown
Documentation build overview2 files changed ± library/urllib.request.html ± whatsnew/changelog.html |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The change
This PR allows IPv4 & IPv6 CIDRs to be specified in NO_PROXY. This is a fairly common convention, as described in #149746.
With this change, if the following env var is set:
the proxy will not be used for any IP address in that range (e.g. 192.168.4.4).
Caching
Since parsing NO_PROXY involves a non-trivial amount of string manipulation, object allocation, and attempts to parse, it turned out significantly more expensive for long NO_PROXY strings than I expected. Since I would expect the environment to change very infrequently, I added a lru_cache around the repeated parsing of the same NO_PROXY string. In the overwhelming majority of production instances, I would expect the size of the cache to sit at 1, and the number of cache hits to be high (e.g. one per request).
Tightly coupled bug
Along the way, I discovered a bug in urllib.parse.splitport, which is deprecated but still used heavily internally:
>>> urllib.parse.splitport('::1') <python-input-4>:1: DeprecationWarning: urllib.parse.splitport() is deprecated as of 3.8, use urllib.parse.urlparse() instead (':', '1')I tried to work around this issue throughout my PR, but it kept getting uglier and I decided to bundle the fix with this change together. If you'd like a different approach (e.g. 2 stacked PRs) just let me know and I'm happy to refactor.
Existing Issue & PR
I foolishly wrote this PR before looking for existing issues (it started as a monkey-patch to fix an urgent issue in prod). When I found the issue, I also found an existing PR #149744.
The big difference in our approach seems to be that I focused solely on the environment variable, and not on environment-specific settings. It seems like Windows doesn't support CIDRs in the registry value, and I couldn't find a clear indicator that MacOS supports it either.
I went back and forth about posting this, but figured if its not helpful the close button is right there :).
Final notes
This is my first attempt to upstream code to Python. If I missed anything important or violated convention somewhere along the way, please let me know - I'm happy to amend my PR as needed.