ovh / python-ovh

Thin wrapper around OVH's APIs. Handles all the hard work including credential creation and requests signing.
https://pypi.org/project/ovh/
Other
278 stars 76 forks source link

Allow to use requests package from environment #106

Closed samjy closed 2 years ago

samjy commented 2 years ago

Description

Allow to use requests package from environment, if installed. Fall back on vendors.requests if not there.

Why?

This is a work around for issues described in #105 #96 #98 with the included requests package in newer python versions.

Some collections imports are broken in newer python versions:

envs/py3/lib/python3.10/site-packages/ovh/client.py:49: in <module>
    from .vendor.requests import request, Session
envs/py3/lib/python3.10/site-packages/ovh/vendor/requests/__init__.py:58: in <module>
    from . import utils
envs/py3/lib/python3.10/site-packages/ovh/vendor/requests/utils.py:30: in <module>
    from .cookies import RequestsCookieJar, cookiejar_from_dict
envs/py3/lib/python3.10/site-packages/ovh/vendor/requests/cookies.py:164: in <module>
    class RequestsCookieJar(cookielib.CookieJar, collections.MutableMapping):
E   AttributeError: module 'collections' has no attribute 'MutableMapping'

Limits

Here are a few points that should be considered before accepting this PR

  1. The package will be using the requests package without explicitely depending on it. This could make it harder to debug issues.
  2. The users will have to think of installing requests in their python>=3.10 environment for things to work, instead of relying on pip to do it for them.

Alternatives

Here are a few alternatives to the approach suggested in this PR

  1. Upgrade the included vendor/requests. That will require modifying it to include requests' dependencies (charset_normalizer, idna, urllib3, certifi).
  2. Introduce a requirement on requests instead of including it in the package. This could break existing configurations.
Abolah commented 2 years ago

@rbeuque74 could you please merge this PR as soon as possible ? Some of my scripts are depending of this update.

Skunnyk commented 2 years ago

Why does an old "requests" (from 2015) lib is included in this module ? It date back from 2016 era https://github.com/ovh/python-ovh/commit/e7978b9ec0ef89c211d48cff90fae60951040e83#diff-a7747d5054b3cb5f6b9c9cb23a2c3f2e57392df3ca5834506b47ddfc65f7d21f , and I'm sure is full of vulnerabillities (and SNI is explicitly disable in client.py but that is another topic).

Can't it depends on requests ?

samjy commented 2 years ago

@Skunnyk I agree with you.

I'm looking for the fastest way of solving the issue, as we're relying on this library. I think that some projects rely on requests being included, and removing it would be a breaking change.

@rbeuque74 what is the way forward here? Could this be a first step, while the module is adapted to depend on the official requests? Would you need help with that?

lalmeras commented 2 years ago

This fix works for me. As I step into this issue testing this fix, be warned that you need to install manually both requests and pypopenssl.

If pyopenssl is not installed :

If this fix works for me, I don't think it is appropriate for the ovh use-case.

From my point of view, It seems that the vendors embedding of requests is done to allow external libraries to use requests and configure it with pyopenssl and SNI support, and to allow ovh to use its own request with pyopenssl unbound (because of a ssl pool issue). So it targets specifically installation where requests is already installed, and its purpose is not to override SNI settings.

As it seems to me that the original pyopenssl/requests issue may be fixed by this requests release https://docs.python-requests.org/en/latest/community/updates/#id40, so I think the right fix is to drop vendor requests, drop pyopenssl.extract_from_urllib3() workaround and update dependency to requests >= 2.11.0 ?

rbeuque74 commented 2 years ago

Closed through #108