From 03900522d4816fe5dc2958fa1eb30ab447cc8ee5 Mon Sep 17 00:00:00 2001 From: ckonstanski Date: Fri, 11 Nov 2016 18:39:34 -0700 Subject: [PATCH] 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 to utf-8 sequences of type . This happens in safe_encode() (oslo_utils/encodeutils.py:66). != 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 --- glanceclient/common/http.py | 16 ++--- glanceclient/tests/functional/base.py | 31 +++++++++- .../tests/functional/test_http_headers.py | 61 +++++++++++++++++++ glanceclient/tests/unit/test_http.py | 36 +++++++++++ 4 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 glanceclient/tests/functional/test_http_headers.py diff --git a/glanceclient/common/http.py b/glanceclient/common/http.py index f65da682..c46b89b4 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 a6306bf5..02da6234 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 00000000..15964442 --- /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 a806ce79..ee82cf82 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"}