From 5ce9c7dc964be0b3e8f9f273169b77ada85cd8ec Mon Sep 17 00:00:00 2001 From: Jamie Lennox <jamielennox@redhat.com> Date: Tue, 25 Nov 2014 13:25:12 +1000 Subject: [PATCH] Make glanceclient accept a session object To make this work we create a different HTTPClient that extends the basic keystoneclient Adapter. The Adapter is a standard set of parameters that all clients should know how to use like region_name and user_agent. We extend this with the glance specific response manipulation like loading and sending iterables. Implements: bp session-objects Change-Id: Ie8eb4bbf7d1a037099a6d4b272cab70525fbfc85 --- glanceclient/client.py | 47 ++++--- glanceclient/common/http.py | 191 ++++++++++++++++++--------- glanceclient/common/utils.py | 8 ++ glanceclient/tests/unit/test_http.py | 36 ++++- glanceclient/v1/client.py | 7 +- glanceclient/v2/client.py | 8 +- test-requirements.txt | 1 + 7 files changed, 205 insertions(+), 93 deletions(-) diff --git a/glanceclient/client.py b/glanceclient/client.py index 4c1e7b8c..db2a4f79 100644 --- a/glanceclient/client.py +++ b/glanceclient/client.py @@ -18,31 +18,44 @@ import warnings from glanceclient.common import utils -def Client(version=None, endpoint=None, *args, **kwargs): +def Client(version=None, endpoint=None, session=None, *args, **kwargs): """Client for the OpenStack Images API. Generic client for the OpenStack Images API. See version classes for specific details. - :param string version: The version of API to use. Note this is - deprecated and should be passed as part of the URL - (http://$HOST:$PORT/v$VERSION_NUMBER). + :param string version: The version of API to use. + :param session: A keystoneclient session that should be used for transport. + :type session: keystoneclient.session.Session """ - if version is not None: - warnings.warn(("`version` keyword is being deprecated. Please pass the" - " version as part of the URL. " - "http://$HOST:$PORT/v$VERSION_NUMBER"), - DeprecationWarning) + # FIXME(jamielennox): Add a deprecation warning if no session is passed. + # Leaving it as an option until we can ensure nothing break when we switch. + if session: + if endpoint: + kwargs.setdefault('endpoint_override', endpoint) - endpoint, url_version = utils.strip_version(endpoint) + if not version: + __, version = utils.strip_version(endpoint) - if not url_version and not version: - msg = ("Please provide either the version or an url with the form " - "http://$HOST:$PORT/v$VERSION_NUMBER") - raise RuntimeError(msg) + if not version: + msg = ("You must provide a client version when using session") + raise RuntimeError(msg) - version = int(version or url_version) + else: + if version is not None: + warnings.warn(("`version` keyword is being deprecated. Please pass" + " the version as part of the URL. " + "http://$HOST:$PORT/v$VERSION_NUMBER"), + DeprecationWarning) - module = utils.import_versioned_module(version, 'client') + endpoint, url_version = utils.strip_version(endpoint) + version = version or url_version + + if not version: + msg = ("Please provide either the version or an url with the form " + "http://$HOST:$PORT/v$VERSION_NUMBER") + raise RuntimeError(msg) + + module = utils.import_versioned_module(int(version), 'client') client_class = getattr(module, 'Client') - return client_class(endpoint, *args, **kwargs) + return client_class(endpoint, *args, session=session, **kwargs) diff --git a/glanceclient/common/http.py b/glanceclient/common/http.py index f746db58..b5f37cb1 100644 --- a/glanceclient/common/http.py +++ b/glanceclient/common/http.py @@ -17,6 +17,8 @@ import copy import logging import socket +from keystoneclient import adapter +from keystoneclient import exceptions as ksc_exc from oslo_utils import importutils from oslo_utils import netutils import requests @@ -50,7 +52,71 @@ USER_AGENT = 'python-glanceclient' CHUNKSIZE = 1024 * 64 # 64kB -class HTTPClient(object): +class _BaseHTTPClient(object): + + @staticmethod + def _chunk_body(body): + chunk = body + while chunk: + chunk = body.read(CHUNKSIZE) + if chunk == '': + break + yield chunk + + def _set_common_request_kwargs(self, headers, kwargs): + """Handle the common parameters used to send the request.""" + + # Default Content-Type is octet-stream + content_type = headers.get('Content-Type', 'application/octet-stream') + + # NOTE(jamielennox): remove this later. Managers should pass json= if + # they want to send json data. + data = kwargs.pop("data", None) + if data is not None and not isinstance(data, six.string_types): + try: + data = json.dumps(data) + content_type = 'application/json' + except TypeError: + # Here we assume it's + # a file-like object + # and we'll chunk it + data = self._chunk_body(data) + + headers['Content-Type'] = content_type + kwargs['stream'] = content_type == 'application/octet-stream' + + return data + + def _handle_response(self, resp): + if not resp.ok: + LOG.debug("Request returned failure status %s." % resp.status_code) + raise exc.from_response(resp, resp.content) + elif resp.status_code == requests.codes.MULTIPLE_CHOICES: + raise exc.from_response(resp) + + content_type = resp.headers.get('Content-Type') + + # Read body into string if it isn't obviously image data + if content_type == 'application/octet-stream': + # Do not read all response in memory when downloading an image. + body_iter = _close_after_stream(resp, CHUNKSIZE) + else: + content = resp.text + if content_type and content_type.startswith('application/json'): + # Let's use requests json method, it should take care of + # response encoding + body_iter = resp.json() + else: + body_iter = six.StringIO(content) + try: + body_iter = json.loads(''.join([c for c in body_iter])) + except ValueError: + body_iter = None + + return resp, body_iter + + +class HTTPClient(_BaseHTTPClient): def __init__(self, endpoint, **kwargs): self.endpoint = endpoint @@ -123,15 +189,16 @@ class HTTPClient(object): LOG.debug(msg) @staticmethod - def log_http_response(resp, body=None): + def log_http_response(resp): status = (resp.raw.version / 10.0, resp.status_code, resp.reason) dump = ['\nHTTP/%.1f %s %s' % status] headers = resp.headers.items() dump.extend(['%s: %s' % safe_header(k, v) for k, v in headers]) dump.append('') - if body: - body = encodeutils.safe_decode(body) - dump.extend([body, '']) + content_type = resp.headers.get('Content-Type') + + if content_type != 'application/octet-stream': + dump.extend([resp.text, '']) LOG.debug('\n'.join([encodeutils.safe_decode(x, errors='ignore') for x in dump])) @@ -155,37 +222,13 @@ class HTTPClient(object): as setting headers and error handling. """ # Copy the kwargs so we can reuse the original in case of redirects - headers = kwargs.pop("headers", {}) - headers = headers and copy.deepcopy(headers) or {} + headers = copy.deepcopy(kwargs.pop('headers', {})) if self.identity_headers: for k, v in six.iteritems(self.identity_headers): headers.setdefault(k, v) - # Default Content-Type is octet-stream - content_type = headers.get('Content-Type', 'application/octet-stream') - - def chunk_body(body): - chunk = body - while chunk: - chunk = body.read(CHUNKSIZE) - if chunk == '': - break - yield chunk - - data = kwargs.pop("data", None) - if data is not None and not isinstance(data, six.string_types): - try: - data = json.dumps(data) - content_type = 'application/json' - except TypeError: - # Here we assume it's - # a file-like object - # and we'll chunk it - data = chunk_body(data) - - headers['Content-Type'] = content_type - stream = True if content_type == 'application/octet-stream' else False + data = self._set_common_request_kwargs(headers, kwargs) if osprofiler_web: headers.update(osprofiler_web.get_trace_id_headers()) @@ -195,20 +238,20 @@ class HTTPClient(object): # complain. headers = self.encode_headers(headers) + if self.endpoint.endswith("/") or url.startswith("/"): + conn_url = "%s%s" % (self.endpoint, url) + else: + conn_url = "%s/%s" % (self.endpoint, url) + self.log_curl_request(method, conn_url, headers, data, kwargs) + try: - if self.endpoint.endswith("/") or url.startswith("/"): - conn_url = "%s%s" % (self.endpoint, url) - else: - conn_url = "%s/%s" % (self.endpoint, url) - self.log_curl_request(method, conn_url, headers, data, kwargs) resp = self.session.request(method, conn_url, data=data, - stream=stream, headers=headers, **kwargs) except requests.exceptions.Timeout as e: - message = ("Error communicating with %(endpoint)s %(e)s" % + message = ("Error communicating with %(endpoint)s: %(e)s" % dict(url=conn_url, e=e)) raise exc.InvalidEndpoint(message=message) except (requests.exceptions.ConnectionError, ProtocolError) as e: @@ -225,34 +268,8 @@ class HTTPClient(object): {'endpoint': endpoint, 'e': e}) raise exc.CommunicationError(message=message) - if not resp.ok: - LOG.debug("Request returned failure status %s." % resp.status_code) - raise exc.from_response(resp, resp.text) - elif resp.status_code == requests.codes.MULTIPLE_CHOICES: - raise exc.from_response(resp) - - content_type = resp.headers.get('Content-Type') - - # Read body into string if it isn't obviously image data - if content_type == 'application/octet-stream': - # Do not read all response in memory when - # downloading an image. - body_iter = _close_after_stream(resp, CHUNKSIZE) - self.log_http_response(resp) - else: - content = resp.text - self.log_http_response(resp, content) - if content_type and content_type.startswith('application/json'): - # Let's use requests json method, - # it should take care of response - # encoding - body_iter = resp.json() - else: - body_iter = six.StringIO(content) - try: - body_iter = json.loads(''.join([c for c in body_iter])) - except ValueError: - body_iter = None + resp, body_iter = self._handle_response(resp) + self.log_http_response(resp) return resp, body_iter def head(self, url, **kwargs): @@ -283,3 +300,45 @@ def _close_after_stream(response, chunk_size): # This will return the connection to the HTTPConnectionPool in urllib3 # and ideally reduce the number of HTTPConnectionPool full warnings. response.close() + + +class SessionClient(adapter.Adapter, _BaseHTTPClient): + + def __init__(self, session, **kwargs): + kwargs.setdefault('user_agent', USER_AGENT) + kwargs.setdefault('service_type', 'image') + super(SessionClient, self).__init__(session, **kwargs) + + def request(self, url, method, **kwargs): + 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) + except ksc_exc.RequestTimeout as e: + message = ("Error communicating with %(endpoint)s %(e)s" % + dict(url=conn_url, e=e)) + raise exc.InvalidEndpoint(message=message) + except ksc_exc.ConnectionRefused as e: + conn_url = self.get_endpoint(auth=kwargs.get('auth')) + conn_url = "%s/%s" % (conn_url.rstrip('/'), url.lstrip('/')) + message = ("Error finding address for %(url)s: %(e)s" % + dict(url=conn_url, e=e)) + raise exc.CommunicationError(message=message) + + return self._handle_response(resp) + + +def get_http_client(endpoint=None, session=None, **kwargs): + if session: + return SessionClient(session, **kwargs) + elif endpoint: + return HTTPClient(endpoint, **kwargs) + else: + raise AttributeError('Constructing a client must contain either an ' + 'endpoint or a session') diff --git a/glanceclient/common/utils.py b/glanceclient/common/utils.py index b199bf35..9016860d 100644 --- a/glanceclient/common/utils.py +++ b/glanceclient/common/utils.py @@ -428,6 +428,14 @@ def safe_header(name, value): return name, value +def endpoint_version_from_url(endpoint, default_version=None): + if endpoint: + endpoint, version = strip_version(endpoint) + return endpoint, version or default_version + else: + return None, default_version + + class IterableWithLength(object): def __init__(self, iterable, length): self.iterable = iterable diff --git a/glanceclient/tests/unit/test_http.py b/glanceclient/tests/unit/test_http.py index e8bfaaa5..e6107169 100644 --- a/glanceclient/tests/unit/test_http.py +++ b/glanceclient/tests/unit/test_http.py @@ -12,13 +12,17 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import functools import json +from keystoneclient.auth import token_endpoint +from keystoneclient import session import mock import requests from requests_mock.contrib import fixture import six from six.moves.urllib import parse +from testscenarios import load_tests_apply_scenarios as load_tests # noqa import testtools from testtools import matchers import types @@ -30,15 +34,39 @@ from glanceclient import exc from glanceclient.tests import utils +def original_only(f): + @functools.wraps(f) + def wrapper(self, *args, **kwargs): + if not hasattr(self.client, 'log_curl_request'): + self.skipTest('Skip logging tests for session client') + + return f(self, *args, **kwargs) + + class TestClient(testtools.TestCase): + scenarios = [ + ('httpclient', {'create_client': '_create_http_client'}), + ('session', {'create_client': '_create_session_client'}) + ] + + def _create_http_client(self): + return http.HTTPClient(self.endpoint, token=self.token) + + def _create_session_client(self): + auth = token_endpoint.Token(self.endpoint, self.token) + sess = session.Session(auth=auth) + return http.SessionClient(sess) + def setUp(self): super(TestClient, self).setUp() self.mock = self.useFixture(fixture.Fixture()) self.endpoint = 'http://example.com:9292' self.ssl_endpoint = 'https://example.com:9292' - self.client = http.HTTPClient(self.endpoint, token=u'abc123') + self.token = u'abc123' + + self.client = getattr(self, self.create_client)() def test_identity_headers_and_token(self): identity_headers = { @@ -140,6 +168,9 @@ class TestClient(testtools.TestCase): self.assertEqual(text, resp.text) def test_headers_encoding(self): + if not hasattr(self.client, 'encode_headers'): + self.skipTest('Cannot do header encoding check on SessionClient') + value = u'ni\xf1o' headers = {"test": value, "none-val": None} encoded = self.client.encode_headers(headers) @@ -206,6 +237,7 @@ class TestClient(testtools.TestCase): self.assertTrue(isinstance(body, types.GeneratorType)) self.assertEqual([data], list(body)) + @original_only def test_log_http_response_with_non_ascii_char(self): try: response = 'Ok' @@ -216,6 +248,7 @@ class TestClient(testtools.TestCase): except UnicodeDecodeError as e: self.fail("Unexpected UnicodeDecodeError exception '%s'" % e) + @original_only def test_log_curl_request_with_non_ascii_char(self): try: headers = {'header1': 'value1\xa5\xa6'} @@ -225,6 +258,7 @@ class TestClient(testtools.TestCase): except UnicodeDecodeError as e: self.fail("Unexpected UnicodeDecodeError exception '%s'" % e) + @original_only @mock.patch('glanceclient.common.http.LOG.debug') def test_log_curl_request_with_body_and_header(self, mock_log): hd_name = 'header1' diff --git a/glanceclient/v1/client.py b/glanceclient/v1/client.py index 68c2a336..b36f3060 100644 --- a/glanceclient/v1/client.py +++ b/glanceclient/v1/client.py @@ -29,10 +29,9 @@ class Client(object): http requests. (optional) """ - def __init__(self, endpoint, **kwargs): + def __init__(self, endpoint=None, **kwargs): """Initialize a new client for the Images v1 API.""" - endpoint, version = utils.strip_version(endpoint) - self.version = version or 1.0 - self.http_client = http.HTTPClient(endpoint, **kwargs) + endpoint, self.version = utils.endpoint_version_from_url(endpoint, 1.0) + self.http_client = http.get_http_client(endpoint=endpoint, **kwargs) self.images = ImageManager(self.http_client) self.image_members = ImageMemberManager(self.http_client) diff --git a/glanceclient/v2/client.py b/glanceclient/v2/client.py index 428ba619..803673ba 100644 --- a/glanceclient/v2/client.py +++ b/glanceclient/v2/client.py @@ -34,11 +34,9 @@ class Client(object): http requests. (optional) """ - def __init__(self, endpoint, **kwargs): - endpoint, version = utils.strip_version(endpoint) - self.version = version or 2.0 - self.http_client = http.HTTPClient(endpoint, **kwargs) - + def __init__(self, endpoint=None, **kwargs): + endpoint, self.version = utils.endpoint_version_from_url(endpoint, 2.0) + self.http_client = http.get_http_client(endpoint=endpoint, **kwargs) self.schemas = schemas.Controller(self.http_client) self.images = images.Controller(self.http_client, self.schemas) diff --git a/test-requirements.txt b/test-requirements.txt index 99714eed..8b296b9d 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -10,6 +10,7 @@ oslosphinx>=2.5.0 # Apache-2.0 sphinx>=1.1.2,!=1.2.0,!=1.3b1,<1.3 testrepository>=0.0.18 testtools>=0.9.36,!=1.2.0 +testscenarios>=0.4 fixtures>=0.3.14 requests-mock>=0.6.0 # Apache-2.0 tempest-lib>=0.5.0