diff --git a/keystoneclient/exceptions.py b/keystoneclient/exceptions.py index 833b3925a..d3a7c3212 100644 --- a/keystoneclient/exceptions.py +++ b/keystoneclient/exceptions.py @@ -4,6 +4,8 @@ Exception definitions. """ +from keystoneclient.openstack.common import jsonutils + class CommandError(Exception): pass @@ -143,7 +145,7 @@ _code_map = dict((c.http_status, c) for c in [BadRequest, ServiceUnavailable]) -def from_response(response, body): +def from_response(response, body=None): """ Return an instance of an ClientException or subclass based on an requests response. @@ -155,6 +157,12 @@ def from_response(response, body): raise exception_from_response(resp, resp.text) """ cls = _code_map.get(response.status_code, ClientException) + if body is None: + try: + body = jsonutils.loads(response.text) + except Exception: + body = response.text + if body: if hasattr(body, 'keys'): error = body[body.keys()[0]] diff --git a/keystoneclient/httpclient.py b/keystoneclient/httpclient.py index 56fc4e7ec..e137bc184 100644 --- a/keystoneclient/httpclient.py +++ b/keystoneclient/httpclient.py @@ -39,9 +39,84 @@ from keystoneclient import exceptions _logger = logging.getLogger(__name__) -class HTTPClient(object): +USER_AGENT = 'python-keystoneclient' - USER_AGENT = 'python-keystoneclient' + +def request(url, method='GET', headers=None, original_ip=None, debug=False, + logger=None, **kwargs): + """Perform a http request with standard settings. + + A wrapper around requests.request that adds standard headers like + User-Agent and provides optional debug logging of the request. + + Arguments that are not handled are passed through to the requests library. + + :param string url: The url to make the request of. + :param string method: The http method to use. (eg. 'GET', 'POST') + :param dict headers: Headers to be included in the request. (optional) + :param string original_ip: Mark this request as forwarded for this ip. + (optional) + :param bool debug: Enable debug logging. (Defaults to False) + :param logging.Logger logger: A logger to output to. (optional) + + :raises exceptions.ClientException: For connection failure, or to indicate + an error response code. + + :returns: The response to the request. + """ + + if not headers: + headers = dict() + + if not logger: + logger = _logger + + headers.setdefault('User-Agent', USER_AGENT) + + if original_ip: + headers['Forwarded'] = "for=%s;by=%s" % (original_ip, USER_AGENT) + + if debug: + string_parts = ['curl -i'] + + if method: + string_parts.append(' -X %s' % method) + + string_parts.append(' %s' % url) + + if headers: + for header in headers.iteritems(): + string_parts.append(' -H "%s: %s"' % header) + + logger.debug("REQ: %s" % "".join(string_parts)) + + data = kwargs.get('data') + if data: + logger.debug("REQ BODY: %s\n" % data) + + try: + resp = requests.request( + method, + url, + headers=headers, + **kwargs) + except requests.ConnectionError: + msg = 'Unable to establish connection to %s' % url + raise exceptions.ClientException(msg) + + if debug: + logger.debug("RESP: [%s] %s\nRESP BODY: %s\n", + resp.status_code, resp.headers, resp.text) + + if resp.status_code >= 400: + logger.debug("Request returned failure status: %s", + resp.status_code) + raise exceptions.from_response(resp) + + return resp + + +class HTTPClient(object): def __init__(self, username=None, tenant_id=None, tenant_name=None, password=None, auth_url=None, region_name=None, timeout=None, @@ -483,33 +558,6 @@ class HTTPClient(object): """ raise NotImplementedError - def http_log_req(self, args, kwargs): - if not self.debug_log: - return - - string_parts = ['curl -i'] - for element in args: - if element in ('GET', 'POST'): - string_parts.append(' -X %s' % element) - else: - string_parts.append(' %s' % element) - - for element in kwargs['headers']: - header = ' -H "%s: %s"' % (element, kwargs['headers'][element]) - string_parts.append(header) - - _logger.debug("REQ: %s" % "".join(string_parts)) - if 'data' in kwargs: - _logger.debug("REQ BODY: %s\n" % (kwargs['data'])) - - def http_log_resp(self, resp): - if self.debug_log: - _logger.debug( - "RESP: [%s] %s\nRESP BODY: %s\n", - resp.status_code, - resp.headers, - resp.text) - def serialize(self, entity): return json.dumps(entity) @@ -522,7 +570,7 @@ class HTTPClient(object): """Returns True if this client provides a service catalog.""" return self.auth_ref.has_service_catalog() - def request(self, url, method, **kwargs): + def request(self, url, method, body=None, **kwargs): """Send an http request with the specified characteristics. Wrapper around requests.request to handle tasks such as @@ -531,54 +579,37 @@ class HTTPClient(object): # Copy the kwargs so we can reuse the original in case of redirects request_kwargs = copy.copy(kwargs) request_kwargs.setdefault('headers', kwargs.get('headers', {})) - request_kwargs['headers']['User-Agent'] = self.USER_AGENT - if self.original_ip: - request_kwargs['headers']['Forwarded'] = "for=%s;by=%s" % ( - self.original_ip, self.USER_AGENT) - if 'body' in kwargs: + + if body: request_kwargs['headers']['Content-Type'] = 'application/json' - request_kwargs['data'] = self.serialize(kwargs['body']) - del request_kwargs['body'] + request_kwargs['data'] = self.serialize(body) + if self.cert: - request_kwargs['cert'] = self.cert + request_kwargs.setdefault('cert', self.cert) if self.timeout is not None: request_kwargs.setdefault('timeout', self.timeout) - self.http_log_req((url, method,), request_kwargs) - - try: - resp = requests.request( - method, - url, - verify=self.verify_cert, - **request_kwargs) - except requests.ConnectionError: - msg = 'Unable to establish connection to %s' % url - raise exceptions.ClientException(msg) - - self.http_log_resp(resp) + resp = request(url, method, original_ip=self.original_ip, + verify=self.verify_cert, debug=self.debug_log, + **request_kwargs) if resp.text: try: - body = json.loads(resp.text) + body_resp = json.loads(resp.text) except (ValueError, TypeError): - body = None + body_resp = None _logger.debug("Could not decode JSON from body: %s" % resp.text) else: _logger.debug("No body was returned.") - body = None + body_resp = None - if resp.status_code >= 400: - _logger.debug( - "Request returned failure status: %s", - resp.status_code) - raise exceptions.from_response(resp, body or resp.text) - elif resp.status_code in (301, 302, 305): + if resp.status_code in (301, 302, 305): # Redirected. Reissue the request to the new location. - return self.request(resp.headers['location'], method, **kwargs) + return self.request(resp.headers['location'], method, body, + **request_kwargs) - return resp, body + return resp, body_resp def _cs_request(self, url, method, **kwargs): """Makes an authenticated request to keystone endpoint by diff --git a/test-requirements.txt b/test-requirements.txt index 0f61dc1b5..2525fa8a1 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -3,6 +3,7 @@ hacking>=0.5.6,<0.7 coverage>=3.6 discover fixtures>=0.3.12 +httpretty>=0.6.3 keyring>=1.6.1 mock>=0.8.0 mox>=0.5.3 diff --git a/tests/test_http.py b/tests/test_http.py index 8b6261ecb..9ab3dcced 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -1,7 +1,26 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2013 OpenStack LLC +# +# 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. + import json import mock +import httpretty import requests +import testtools +from testtools import matchers from keystoneclient import exceptions from keystoneclient import httpclient @@ -28,6 +47,18 @@ def get_authed_client(): return cl +class FakeLog(object): + def __init__(self): + self.warn_log = str() + self.debug_log = str() + + def warn(self, msg=None, *args, **kwargs): + self.warn_log = "%s\n%s" % (self.warn_log, (msg % args)) + + def debug(self, msg=None, *args, **kwargs): + self.debug_log = "%s\n%s" % (self.debug_log, (msg % args)) + + class ClientTest(utils.TestCase): def test_unauthorized_client_requests(self): @@ -44,7 +75,7 @@ class ClientTest(utils.TestCase): with mock.patch('time.time', mock.Mock(return_value=1234)): resp, body = cl.get("/hi") headers = {"X-Auth-Token": "token", - "User-Agent": cl.USER_AGENT} + "User-Agent": httpclient.USER_AGENT} MOCK_REQUEST.assert_called_with( "GET", "http://127.0.0.1:5000/hi", @@ -97,7 +128,7 @@ class ClientTest(utils.TestCase): headers = { "X-Auth-Token": "token", "Content-Type": "application/json", - "User-Agent": cl.USER_AGENT + "User-Agent": httpclient.USER_AGENT } MOCK_REQUEST.assert_called_with( "POST", @@ -117,5 +148,69 @@ class ClientTest(utils.TestCase): args, kwargs = MOCK_REQUEST.call_args self.assertIn( - ('Forwarded', "for=%s;by=%s" % (ORIGINAL_IP, cl.USER_AGENT)), + ('Forwarded', "for=%s;by=%s" % (ORIGINAL_IP, + httpclient.USER_AGENT)), kwargs['headers'].items()) + + +class BasicRequestTests(testtools.TestCase): + + url = 'http://keystone.test.com/' + + def setUp(self): + super(BasicRequestTests, self).setUp() + self.logger = FakeLog() + httpretty.enable() + + def tearDown(self): + httpretty.disable() + super(BasicRequestTests, self).tearDown() + + def request(self, method='GET', response='Test Response', status=200, + url=None, **kwargs): + if not url: + url = self.url + + httpretty.register_uri(method, url, body=response, status=status) + + return httpclient.request(url, method, debug=True, + logger=self.logger, **kwargs) + + @property + def last_request(self): + return httpretty.httpretty.last_request + + def test_basic_params(self): + method = 'GET' + response = 'Test Response' + status = 200 + + resp = self.request(method=method, status=status, response=response) + + self.assertEqual(self.last_request.method, method) + + self.assertThat(self.logger.debug_log, matchers.Contains('curl')) + self.assertThat(self.logger.debug_log, matchers.Contains('-X %s' % + method)) + self.assertThat(self.logger.debug_log, matchers.Contains(self.url)) + + self.assertThat(self.logger.debug_log, matchers.Contains(str(status))) + self.assertThat(self.logger.debug_log, matchers.Contains(response)) + + def test_headers(self): + headers = {'key': 'val', 'test': 'other'} + + self.request(headers=headers) + + for k, v in headers.iteritems(): + self.assertEqual(self.last_request.headers[k], v) + + for header in headers.iteritems(): + self.assertThat(self.logger.debug_log, + matchers.Contains('-H "%s: %s"' % header)) + + def test_body(self): + data = "BODY DATA" + resp = self.request(response=data) + self.assertThat(self.logger.debug_log, matchers.Contains('BODY:')) + self.assertThat(self.logger.debug_log, matchers.Contains(data)) diff --git a/tests/test_https.py b/tests/test_https.py index 4873a4cde..8d332899d 100644 --- a/tests/test_https.py +++ b/tests/test_https.py @@ -37,7 +37,7 @@ class ClientTest(utils.TestCase): with mock.patch('time.time', mock.Mock(return_value=1234)): resp, body = cl.get("/hi") headers = {"X-Auth-Token": "token", - "User-Agent": cl.USER_AGENT} + "User-Agent": httpclient.USER_AGENT} kwargs = copy.copy(self.TEST_REQUEST_BASE) kwargs['cert'] = ('cert.pem', 'key.pem') kwargs['verify'] = 'ca.pem' @@ -57,7 +57,7 @@ class ClientTest(utils.TestCase): headers = { "X-Auth-Token": "token", "Content-Type": "application/json", - "User-Agent": cl.USER_AGENT + "User-Agent": httpclient.USER_AGENT } kwargs = copy.copy(self.TEST_REQUEST_BASE) kwargs['cert'] = ('cert.pem', 'key.pem') @@ -81,7 +81,7 @@ class ClientTest(utils.TestCase): headers = { "X-Auth-Token": "token", "Content-Type": "application/json", - "User-Agent": cl.USER_AGENT + "User-Agent": httpclient.USER_AGENT } kwargs = copy.copy(self.TEST_REQUEST_BASE) kwargs['cert'] = ('cert.pem', 'key.pem')