prepare_headers patch not needed for requests in python3

Bug #1904551 reported by Peter Uittenbroek
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
python-swiftclient
New
Undecided
Unassigned

Bug Description

Currently there is a patch in place for `requests.models.PreparedRequest.prepare_headers` to prevent the old `name.encode('ascii')` from being applied.
The if statement is very specific for python 2 and applies to requests < 2.0.0, which makes sense seeing this change ( https://github.com/psf/requests/commit/f5775594ccc0e2947b965c1fcf40747301a93974 ).
But there is also an or statement for NOT python2, making it always apply.

Not sure when this was needed, but it appears to work fine in python 3.
Going through git history of `requests`, I honestly cannot find any trace when this would have broken in python 3. The aforementioned change introduced the `to_native_string` for header keys. And the implementation and usage of that hasn't changed since. For our local setup using swiftclient, disabling the patch it all appears to keep working.

Desired situation:
Change the monkeypatch to only apply for the required version of requests.

Tags: python3
Revision history for this message
Tim Burke (1-tim-z) wrote :

The reason was hinted-at in https://opendev.org/openstack/python-swiftclient/commit/7175069b that introduced the `or not six.PY2` -- basically, it allows users to continue doing things like

  $ swift post -m '☃:❄'

which swift has allowed ever since account/container metadata was introduced back in https://opendev.org/openstack/swift/commit/e8d3f260.

Now, setting metadata like that isn't a great idea. Doing it in my dev environment made `swift list` go from ~0.5s to ~100s (!) because py3's stdlib doesn't know how to handle the non-compliant header and aborts parsing -- see https://bugs.python.org/issue37093

So the *real* value at the moment comes from being able to *clear* the non-compliant metadata after wondering why everything got so slow after switching to python3. Without it, users would have to either switch back to py2 or just use curl.

Revision history for this message
Peter Uittenbroek (puittenbroek) wrote :

Okay, that reason sort of makes sense. Think the snow flake + snowman is an extreme example but does show unicode problems.

The problem that I have now is that due to the monkeypatch being applied this way, any other package using requests is affected.
In my current case, my headers (keys) are all byte strings and I end up sending 'Accept-Encoding' and 'Content-Length' twice due to python's http client not recognizing the byte-string version in it's if statement.

    > /home/uittenbroek/.pyenv/versions/3.6.10/lib/python3.6/http/client.py(1270)_send_request()
    ipdb> header_names
    frozenset({b'authorization', b'accept', b'content-length', 'content-length', b'accept-encoding', b'connection', b'user-agent'})
    ipdb> 'accept-encoding' in header_names
    False

The 'Accept-Encoding' values are:
Accept-Encoding: identity
Accept-Encoding: gzip, deflate

Guessing the first (or being send twice) causes problems for me at this instance.

So I think the monkeypatch being applied should be way more defensive, fix there what breaks for you.

Revision history for this message
Peter Uittenbroek (puittenbroek) wrote :

This keeps working but doesn't mess with other headers:

   $ swift post -m '☃:❄'

Revision history for this message
Aarni Koskela (akx) wrote (last edit ):

+1 from me.

I actually bumped into this since I have a project with a fairly stringent warning filter, and using `distutils.version.StrictVersion` raises a warning with new versions of `setuptools`: https://github.com/pypa/setuptools/commit/1701579e0827317d8888c2254a17b5786b6b5246

That warning turned into an exception, and here we are...

I think it's somewhat irresponsible of `swiftclient` to quietly patch the requests package on import, especially since the patched version doesn't have the header injection mitigations that have been in requests proper since 2016 (https://github.com/psf/requests/commit/2669ab797ce769ecedf5493b04cb976f33e37210).

Maybe swiftclient shouldn't use requests if it needs to speak non-standard HTTP...

EDIT: I submitted a patch for this: https://review.opendev.org/c/openstack/python-swiftclient/+/828821

Revision history for this message
Aarni Koskela (akx) wrote :
Revision history for this message
Aarni Koskela (akx) wrote :

This has been released in swiftclient 4.0.0.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.