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 <pushkar.umaranikar@intel.com>

4 files changed, 136 insertions, 8 deletions

diff --git a/glanceclient/common/http.py b/glanceclient/common/http.py
index f65da68..c46b89b 100644
--- a/glanceclient/common/http.py
+++ b/glanceclient/common/http.py
@@ -315,16 +315,18 @@ 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)
-
         try:
-            resp = super(SessionClient, self).request(url,
-                                                      method,
-                                                      headers=headers,
-                                                      data=data,
-                                                      **kwargs)
+            # NOTE(pumaranikar): To avoid bug #1641239, no modification of
+            # headers should be allowed after encode_headers() is called.
+            resp = super(SessionClient,
+                         self).request(url,
+                                       method,
+                                       headers=encode_headers(headers),
+                                       data=data,
+                                       **kwargs)
         except ksa_exc.ConnectTimeout as e:
             conn_url = self.get_endpoint(auth=kwargs.get('auth'))
             conn_url = "%s/%s" % (conn_url.rstrip('/'), url.lstrip('/'))
diff --git a/glanceclient/tests/functional/base.py b/glanceclient/tests/functional/base.py
index a6306bf..02da623 100644
--- a/glanceclient/tests/functional/base.py
+++ b/glanceclient/tests/functional/base.py
@@ -10,8 +10,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import glanceclient
+from keystoneauth1 import loading
+from keystoneauth1 import session
 import os
-
 import os_client_config
 from tempest.lib.cli import base
 
@@ -60,3 +62,30 @@ class ClientTestBase(base.ClientTestBase):
     def glance(self, *args, **kwargs):
         return self.clients.glance(*args,
                                    **kwargs)
+
+    def glance_pyclient(self):
+        ks_creds = dict(
+            auth_url=self.creds["auth_url"],
+            username=self.creds["username"],
+            password=self.creds["password"],
+            project_name=self.creds["project_name"])
+        keystoneclient = self.Keystone(**ks_creds)
+        return self.Glance(keystoneclient)
+
+    class Keystone(object):
+        def __init__(self, **kwargs):
+            loader = loading.get_plugin_loader("password")
+            auth = loader.load_from_options(**kwargs)
+            self.session = session.Session(auth=auth)
+
+    class Glance(object):
+        def __init__(self, keystone, version="2"):
+            self.glance = glanceclient.Client(
+                version,
+                session=keystone.session)
+
+        def find(self, image_name):
+            for image in self.glance.images.list():
+                if image.name == image_name:
+                    return image
+            return None
diff --git a/glanceclient/tests/functional/test_http_headers.py b/glanceclient/tests/functional/test_http_headers.py
new file mode 100644
index 0000000..1596444
--- /dev/null
+++ b/glanceclient/tests/functional/test_http_headers.py
@@ -0,0 +1,61 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from glanceclient.tests.functional import base
+import time
+
+
+IMAGE = {"protected": False,
+         "disk_format": "qcow2",
+         "name": "glance_functional_test_image.img",
+         "visibility": "private",
+         "container_format": "bare"}
+
+
+class HttpHeadersTest(base.ClientTestBase):
+    def test_encode_headers_python(self):
+        """Test proper handling of Content-Type headers.
+
+        encode_headers() must be called as late as possible before a
+        request is sent. If this principle is violated, and if any
+        changes are made to the headers between encode_headers() and the
+        actual request (for instance a call to
+        _set_common_request_kwargs()), and if you're trying to set a
+        Content-Type that is not equal to application/octet-stream (the
+        default), it is entirely possible that you'll end up with two
+        Content-Type headers defined (yours plus
+        application/octet-stream). The request will go out the door with
+        only one of them chosen seemingly at random.
+
+        This test uses a call to update() because it sets a header such
+        as the following (this example may be subject to change):
+        Content-Type: application/openstack-images-v2.1-json-patch
+
+        This situation only occurs in python3. This test will never fail
+        in python2.
+
+        There is no test against the CLI because it swallows the error.
+        """
+        # the failure is intermittent - try up to 6 times
+        for attempt in range(0, 6):
+            glanceclient = self.glance_pyclient()
+            image = glanceclient.find(IMAGE["name"])
+            if image:
+                glanceclient.glance.images.delete(image.id)
+            image = glanceclient.glance.images.create(name=IMAGE["name"])
+            self.assertTrue(image.status == "queued")
+            try:
+                image = glanceclient.glance.images.update(image.id,
+                                                          disk_format="qcow2")
+            except Exception as e:
+                self.assertFalse("415 Unsupported Media Type" in e.details)
+            time.sleep(5)
diff --git a/glanceclient/tests/unit/test_http.py b/glanceclient/tests/unit/test_http.py
index a806ce7..ee82cf8 100644
--- a/glanceclient/tests/unit/test_http.py
+++ b/glanceclient/tests/unit/test_http.py
@@ -20,6 +20,7 @@ import fixtures
 from keystoneauth1 import session
 from keystoneauth1 import token_endpoint
 import mock
+from oslo_utils import encodeutils
 import requests
 from requests_mock.contrib import fixture
 import six
@@ -207,6 +208,41 @@ class TestClient(testtools.TestCase):
         self.assertEqual(b"ni\xc3\xb1o", encoded[b"test"])
         self.assertNotIn("none-val", encoded)
 
+    @mock.patch('keystoneauth1.adapter.Adapter.request')
+    def test_http_duplicate_content_type_headers(self, mock_ksarq):
+        """Test proper handling of Content-Type headers.
+
+        encode_headers() must be called as late as possible before a
+        request is sent. If this principle is violated, and if any
+        changes are made to the headers between encode_headers() and the
+        actual request (for instance a call to
+        _set_common_request_kwargs()), and if you're trying to set a
+        Content-Type that is not equal to application/octet-stream (the
+        default), it is entirely possible that you'll end up with two
+        Content-Type headers defined (yours plus
+        application/octet-stream). The request will go out the door with
+        only one of them chosen seemingly at random.
+
+        This situation only occurs in python3. This test will never fail
+        in python2.
+        """
+        path = "/v2/images/my-image"
+        headers = {
+            "Content-Type": "application/openstack-images-v2.1-json-patch"
+        }
+        data = '[{"value": "qcow2", "path": "/disk_format", "op": "replace"}]'
+        self.mock.patch(self.endpoint + path)
+        sess_http_client = self._create_session_client()
+        sess_http_client.patch(path, headers=headers, data=data)
+        # Pull out the headers with which Adapter.request was invoked
+        ksarqh = mock_ksarq.call_args[1]['headers']
+        # Only one Content-Type header (of any text-type)
+        self.assertEqual(1, [encodeutils.safe_decode(key)
+                             for key in ksarqh.keys()].count(u'Content-Type'))
+        # And it's the one we set
+        self.assertEqual(b"application/openstack-images-v2.1-json-patch",
+                         ksarqh[b"Content-Type"])
+
     def test_raw_request(self):
         """Verify the path being used for HTTP requests reflects accurately."""
         headers = {"Content-Type": "text/plain"}