Skip to content
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

Support urllib3 >= 2.0.0 #1290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support urllib3 >= 2.0.0 #1290

wants to merge 1 commit into from

Conversation

danigm
Copy link

@danigm danigm commented May 10, 2023

The urllib3.request.RequestMethods has been moved to urllib3._request_methods.RequestMethods.

This patch changes the usage to continue working with the latest release, but now it's a "private" class in a "private" module, so maybe it's worth to start to think about updating this code to use the public API.

https://urllib3.readthedocs.io/en/stable/changelog.html#removed

See #1283

@danigm danigm requested review from a team as code owners May 10, 2023 07:25
@google-cla
Copy link

google-cla bot commented May 10, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@danigm
Copy link
Author

danigm commented May 10, 2023

The tests are working with this change, but it looks like it also work if instead of using directly the RequestMethods class we use some of the classes that inherit from it, like HTTPConnectionPool: https://urllib3.readthedocs.io/en/stable/reference/urllib3.connectionpool.html#urllib3.HTTPConnectionPool

@clundin25
Copy link
Contributor

Thank you for the contribution! It will take some time to accept this patch. There are downstream dependencies that depend on this functionality.

@pquentin
Copy link

Hello, urllib3 maintainer here 👋

It's unfortunate that urllib3 has always documented its internals as if they were a public API, because it makes any changes difficult.

Can you please clarify what downstream dependencies depend on exactly?

@clundin25
Copy link
Contributor

Hi @pquentin,

We use PyOpenSSL to implement non-file based credentials w/ mTLS. We are investigating other approaches but until we have a solution in place we cannot migrate.

Deprecation notice here: urllib3/urllib3#2691

@pquentin
Copy link

pquentin commented May 15, 2023

OK, so RequestsMethod isn't really an issue, this is good to know. Others have reported that non-file TLS is the only the reason why they still use pyOpenSSL, and it's true that we don't have a good alternative today.

It's only deprecated, not removed, though. Is the issue the deprecation warning? I don't want to be pushy: this is your software. But I'm trying to better understand the blockers.

@clundin25
Copy link
Contributor

@danigm Let me double check. My assumption is that we do not want the deprecation warning to propagate.

I appreciate the effort / time to update this for us!

@arithmetic1728
Copy link
Contributor

RequestsMethod is one reason. Another reason (as @clundin25 mentioned) is that urllib3 will remove pyopenssl support in v2.1.0. For v2.0.0 it currently just raises a warning but that could bother the users, so we would like to keep urllib3 < 2.0 for now until we find a good way to handle both issues.

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request May 19, 2023
https://build.opensuse.org/request/show/1085995
by user dgarcia + dimstar_suse
- Add urllib3-2.patch to support newer urllib3 -- gh#googleapis/google-auth-library-python#1290
- Remove no-python3.patch
- Update to 2.17.3:
  * Add useEmailAzp claim for id token iam flow (#1270) (7a9c6f2)
- 2.17.2:
  * Do not create new JWT credentials if they make the same claims as
    the existing. (#1267) (eebb7b6)
- 2.17.1:
  * Print out reauth plugin error and raise if challenge output is
    None (#1265) (08d22fe)
- 2.17.0:
  * Experimental service account iam endpoint flow for id token
    (#1258) (8ff0de5)
  * Python: Remove aws url validation (#1254) (20a966b)
- 2.16.3:
  * Read both applicationId and relyingPartyId. (#1246) (e125dfe)
- 2.16.2:
  * Call gcloud config get project to get project for user cred
    (#1243) (c078a13)
  * Do not use hardcoded string 'python', when yo
@mgorny
Copy link

mgorny commented Jun 14, 2023

Thanks for the patch. I'm going to take it into Gentoo to let us unblock urllib3-2.

@pquentin
Copy link

@arithmetic1728 @clundin25 I have discussed this with the other urllib3 maintainers and we're going to undeprecate pyOpenSSL. We won't add the deprecation and warning back unless there's a good solution for this problem (like a third-party library or a fix in CPython itself).

Would that be enough for you to upgrade?

@arithmetic1728
Copy link
Contributor

@pquentin This is great news! Which release will pyopenssl support be back?

The only thing left to update urllib3 would be RequestsMethod, we might want let our AuthorizedHttp class inherit urllib3._request_methods.RequestMethods as in https://github.com/googleapis/google-auth-library-python/pull/1290/files. Is urllib3 planning to change urllib3._request_methods.RequestMethods in the future?

@sethmlarson
Copy link

sethmlarson commented Sep 16, 2023

@arithmetic1728 Happy to hear you're willing to migrate! 🥳 I opened an issue to remove the deprecation warning for pyOpenSSL.

The RequestMethods class is private, since it's really just a convenience class for us to not need to reimplement all the logic around .request() for all the different layers (PoolManager, ConnectionPool, ProxyManager). You could copy all the relevant code into AuthorizedHttp instead of subclassing? It's mostly shuffling headers and request bodies around.

@arithmetic1728
Copy link
Contributor

@sethmlarson Thank you for the suggestions. We will upgrade urllib3 once the depreciation message is removed.

@pquentin
Copy link

Hello! https://pypi.org/project/urllib3/2.0.5/ is now out without the deprecation warning.

@arithmetic1728
Copy link
Contributor

@pquentin awesome. Thank you!

@antoni-szych-rtbhouse
Copy link

Seems like this was partially done in #1389, but without fixing the docstrings. Maybe @danigm can rebase this PR to master in order to fix the outdated docstrings?

The urllib3.request.RequestMethods has been moved to
urllib3._request_methods.RequestMethods.

This patch changes the usage to continue working with the latest
release, but now it's a "private" class in a "private" module, so maybe
it's worth to start to think about updating this code to use the public
API.

https://urllib3.readthedocs.io/en/stable/changelog.html#removed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants