v2: Content-Type: application/octet-stream header always added

[Carlos Konstanski]

File: common/http.py
Class: _BaseHTTPClient
Method: _set_common_request_kwargs

This method wants to set the header Content-Type: application/octet-stream if no existing Content-Type header exists.

The bug: it cannot find the existing header because the call to headers.get() fails. Therefore we end up with two Content-Type headers.

The cause: the strings in headers are converted from unicode sequences of type <str> to utf-8 sequences of type <bytes>. This happens in oslo_utils/encodeutils.py:66, method safe_encode().

Proposed fix: change the headers.get() call to look like the following:

content_type = headers.get(b'Content-Type', b'application/octet-stream')

[Carlos Konstanski (ckonstanski)]

I will submit a patch. A good first bug fix in openstack.

[Carlos Konstanski (ckonstanski)]

https://review.openstack.org/#/c/396816/ submitted.

[Joshua Randall (jcrandall)]

We were seeing occasional failures when attempting to make an update to an existing image, which occurred both using the glance cli or using the SDK `update` method. We believe we have tracked the underlying cause down to this issue.

To assist others in finding this, the user-facing error message we were getting was an HTTP exception from the server:

```
415 Unsupported Media Type: The server could not comply with the request since it is either malformed or otherwise incorrect.
```

This occurs when images.update() is called (which in turn results in a call to `patch`).

The server is expecting a content-type of 'application/openstack-images-v2.1-json-patch' but because of this bug, sometimes it gets the default content-type of application/octet-stream instead. I believe what is happening is that the client is sending both Content-Type headers with a nondeterministic ordering, and depending on the order the server either parses the patch correctly or returns the 415 status code.

We observe this behaviour in release versions 2.1.0 - 2.6.0, but not in release 2.0.0 (which pre-dates the commits that appear to cause this bug).

[Colin Nolan (colin.nolan)]

To follow up on Josh's comment, we have noticed the failure only occurs with Python 3 (tested with 3.5.2 and 3.6.0) but not Python 2.7.10.

Looking further into the issue, we found that the client is not sending both of the Content-Type headers. Instead the (non-deterministic) iteration through the headers by the underlying `requests` library, which converts the header name to a string, leads to the last Content-Type value in the iteration being used:
https://github.com/kennethreitz/requests/blob/master/requests/models.py#L445

[Carlos Konstanski (ckonstanski)]

See https://review.openstack.org/#/c/396816/

This issue has a fix up for review. The fix itself is super simple. See
http.py. The hold-up for merging is the process around getting a
satisfactory unit test written.

Here's the issue: that encode_headers() call in the diff for http.py
will change all the headers from strings (u'a string') to byte arrays
(b'a byte array'). In python2 there is no distinction; both objects are
equal. But in python3 they are not equal. Later when the code looks for
an existing "Content-Type" dict key, it's really looking for a
u'Content-Type' key, but none exists because it has been converted to
b'Content-Type'. So the new key-value pair gets added and we have both.

Eventually this doubly-laden dict makes its way into the requests
library where only one of the Content-Type entries will be chosen at
random. That is outside our reach; we cannot fix it there. The
randomness makes it impossible to write a functional test that always
passes or always fails.

The fix for this problem is to not call encode_headers() until all other
processing of the dict is done. encode_headers() even contains a comment
to this effect. It was not obeyed. This review brings the codebase into
conformance with this requierement.

I can't predict when this review will make it through the process. In
the meantime it is very easy to hack http.py yourself if you are
desperate.

Carlos

Colin Nolan <email address hidden> writes:

> To follow up on Josh's comment, we have noticed the failure only occurs
> with Python 3 (tested with 3.5.2 and 3.6.0) but not Python 2.7.10.
>
> Looking further into the issue, we found that the client is not
> sending both of the Content-Type headers. Instead the
> (non-deterministic) iteration through the headers by the underlying
> `requests` library, which converts the header name to a string, leads
> to the last Content-Type value in the iteration being used:
> https://github.com/kennethreitz/requests/blob/master/requests/models.py#L445

[Colin Nolan (colin.nolan)]

Thanks Carlos. The fix we made was exactly the same as yours:
```
--- a/glanceclient/common/http.py
+++ b/glanceclient/common/http.py
@@ -316,9 +316,10 @@ class SessionClient(adapter.Adapter, _BaseHTTPClient):
         super(SessionClient, self).__init__(session, **kwargs)

     def request(self, url, method, **kwargs):
- headers = encode_headers(kwargs.pop('headers', {}))
+ headers = kwargs.pop('headers', {})
         kwargs['raise_exc'] = False
         data = self._set_common_request_kwargs(headers, kwargs)
+ headers = encode_headers(headers)

         try:
             resp = super(SessionClient, self).request(url,
```

It would be great if your patches get merged as quickly as possible so that other real-world users don't have to endure this issue!

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/396816
Committed: https://git.openstack.org/cgit/openstack/python-glanceclient/commit/?id=03900522d4816fe5dc2958fa1eb30ab447cc8ee5
Submitter: Jenkins
Branch: master

commit 03900522d4816fe5dc2958fa1eb30ab447cc8ee5
Author: ckonstanski <email address hidden>
Date: Fri Nov 11 18:39:34 2016 -0700

    v2: Content-Type: application/octet-stream header always added

    The bug: any existing Content-Type header cannot be found because the
    call to headers.get() fails. Therefore we end up with two Content-Type
    headers because a new one (applicaion/octet-stream) gets added
    unconditionally. The cause: the strings (keys and values) in the headers
    dict are converted from unicode sequences of type <str> to utf-8
    sequences of type <bytes>. This happens in safe_encode()
    (oslo_utils/encodeutils.py:66). <str> != <bytes> even if they appear to
    have the same characters.

    Hence, for python 3.x, _set_common_request_kwargs() adds content-type
    to header even if custom content-type is set in the request.

    This results in unsupported media type exception when glance client
    is used with keystoneauth and python 3.x

    The fix: follow the directions in encode_headers().
    It says to do this just before sending the request. Honor this principle;
    do not encode headers and then perform more business logic on them.

    Change-Id: Idf6079b32f70bc171f5016467048e917d42f296d
    Closes-bug: #1641239
    Co-Authored-By: Pushkar Umaranikar <email address hidden>

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/python-glanceclient 2.7.0 release.