From e9bcb68bab4df80106960c0e43a12e1bf7da89a5 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 17 Nov 2017 13:16:58 +0100 Subject: [PATCH] Drop default URI, make either a session or inspector_url required While convenient sometimes, the previous default behavior of assuming 127.0.0.1 is too unrealistic for production and may cause confusion. Change-Id: I32496bc57f0127e680bbd1823401ab0211f93114 --- ironic_inspector_client/__init__.py | 2 +- ironic_inspector_client/common/http.py | 40 +++++++++++++---- ironic_inspector_client/test/functional.py | 27 +++++++----- .../test/test_common_http.py | 44 ++++++------------- ironic_inspector_client/test/test_init.py | 3 +- ironic_inspector_client/test/test_v1.py | 2 + .../no-default-uri-861f675ccb75e05d.yaml | 11 +++++ 7 files changed, 77 insertions(+), 52 deletions(-) create mode 100644 releasenotes/notes/no-default-uri-861f675ccb75e05d.yaml diff --git a/ironic_inspector_client/__init__.py b/ironic_inspector_client/__init__.py index 4c2d6f3..4087b4d 100644 --- a/ironic_inspector_client/__init__.py +++ b/ironic_inspector_client/__init__.py @@ -12,4 +12,4 @@ # limitations under the License. from .v1 import ClientV1, DEFAULT_API_VERSION, MAX_API_VERSION # noqa -from .common.http import ClientError, VersionNotSupported # noqa +from .common.http import ClientError, EndpointNotFound, VersionNotSupported # noqa diff --git a/ironic_inspector_client/common/http.py b/ironic_inspector_client/common/http.py index 78bd8ff..2434952 100644 --- a/ironic_inspector_client/common/http.py +++ b/ironic_inspector_client/common/http.py @@ -18,14 +18,12 @@ import logging from keystoneauth1 import exceptions as ks_exc from keystoneauth1 import session as ks_session -from oslo_utils import netutils import requests import six from ironic_inspector_client.common.i18n import _ -_DEFAULT_URL = 'http://' + netutils.get_my_ipv4() + ':5050' _ERROR_ENCODING = 'utf-8' LOG = logging.getLogger('ironic_inspector_client') @@ -85,6 +83,19 @@ class VersionNotSupported(Exception): super(Exception, self).__init__(msg) +class EndpointNotFound(Exception): + """Denotes that endpoint for the introspection service was not found. + + ivar service_type: requested service type + """ + + def __init__(self, service_type): + self.service_type = service_type + msg = _('Endpoint of type %s was not found in the service catalog ' + 'and was not provided explicitly') % service_type + super(Exception, self).__init__(msg) + + class BaseClient(object): """Base class for clients, provides common HTTP code.""" @@ -105,8 +116,11 @@ class BaseClient(object): :param interface: interface type (public, internal, etc) to use when looking up the URL :param region_name: region name to use when looking up the URL + :raises: EndpointNotFound if the introspection service endpoint + was not provided via inspector_url and was not found in the + service catalog. """ - self._base_url = inspector_url or _DEFAULT_URL + self._base_url = inspector_url if session is None: self._session = ks_session.Session(None) @@ -117,11 +131,21 @@ class BaseClient(object): self._base_url = session.get_endpoint( service_type=service_type, interface=interface, - region_name=region_name) or _DEFAULT_URL - except ks_exc.EndpointNotFound: - LOG.warning('Endpoint for service %s was not found, ' - 'falling back to local host on port 5050', - service_type) + region_name=region_name) + except ks_exc.CatalogException as exc: + LOG.error('%(iface)s endpoint for %(stype)s in region ' + '%(region)s was not found in the service ' + 'catalog: %(error)s', + {'iface': interface, + 'stype': service_type, + 'region': region_name, + 'error': exc}) + raise EndpointNotFound(service_type=service_type) + + if not self._base_url: + # This handles the case when session=None and no inspector_url is + # provided, as well as keystoneauth plugins that may return None. + raise EndpointNotFound(service_type=service_type) self._base_url = self._base_url.rstrip('/') self._api_version = self._check_api_version(api_version) diff --git a/ironic_inspector_client/test/functional.py b/ironic_inspector_client/test/functional.py index 5443cc4..d3d487a 100644 --- a/ironic_inspector_client/test/functional.py +++ b/ironic_inspector_client/test/functional.py @@ -25,17 +25,21 @@ import unittest from ironic_inspector.common import swift from ironic_inspector import introspection_state as istate from ironic_inspector.test import functional +from keystoneauth1 import session as ks_session +from keystoneauth1 import token_endpoint from oslo_concurrency import processutils import ironic_inspector_client as client -from ironic_inspector_client.common import http from ironic_inspector_client import shell class TestV1PythonAPI(functional.Base): def setUp(self): super(TestV1PythonAPI, self).setUp() - self.client = client.ClientV1() + self.auth = token_endpoint.Token(endpoint='http://127.0.0.1:5050', + token='token') + self.session = ks_session.Session(self.auth) + self.client = client.ClientV1(session=self.session) functional.cfg.CONF.set_override('store_data', 'none', 'processing') def my_status_index(self, statuses): @@ -192,15 +196,18 @@ class TestV1PythonAPI(functional.Base): def test_client_init(self): self.assertRaises(client.VersionNotSupported, - client.ClientV1, api_version=(1, 999)) + client.ClientV1, session=self.session, + api_version=(1, 999)) self.assertRaises(client.VersionNotSupported, - client.ClientV1, api_version=2) + client.ClientV1, session=self.session, + api_version=2) - self.assertTrue(client.ClientV1(api_version=1).server_api_versions()) - self.assertTrue(client.ClientV1(api_version='1.0') - .server_api_versions()) - self.assertTrue(client.ClientV1(api_version=(1, 0)) - .server_api_versions()) + self.assertTrue(client.ClientV1( + api_version=1, session=self.session).server_api_versions()) + self.assertTrue(client.ClientV1( + api_version='1.0', session=self.session).server_api_versions()) + self.assertTrue(client.ClientV1( + api_version=(1, 0), session=self.session).server_api_versions()) self.assertTrue( client.ClientV1(inspector_url='http://127.0.0.1:5050') @@ -455,6 +462,4 @@ class TestCLI(BaseCLITest): if __name__ == '__main__': with functional.mocked_server(): - # Make links predictable - http._DEFAULT_URL = 'http://127.0.0.1:5050' unittest.main(verbosity=2) diff --git a/ironic_inspector_client/test/test_common_http.py b/ironic_inspector_client/test/test_common_http.py index ccee82d..5a9bfad 100644 --- a/ironic_inspector_client/test/test_common_http.py +++ b/ironic_inspector_client/test/test_common_http.py @@ -25,7 +25,7 @@ class TestCheckVersion(unittest.TestCase): @mock.patch.object(http.BaseClient, 'server_api_versions', lambda *args, **kwargs: ((1, 0), (1, 99))) def _check(self, version): - cli = http.BaseClient(1) + cli = http.BaseClient(1, inspector_url='http://127.0.0.1:5050') return cli._check_api_version(version) def test_tuple(self): @@ -64,7 +64,9 @@ FAKE_HEADERS = { 'return_value.headers': FAKE_HEADERS}) class TestServerApiVersions(unittest.TestCase): def _check(self, current=1): - return http.BaseClient(api_version=current).server_api_versions() + return http.BaseClient( + api_version=current, + inspector_url='http://127.0.0.1:5050').server_api_versions() def test_no_headers(self, mock_get): mock_get.return_value.headers = {} @@ -102,7 +104,7 @@ class TestServerApiVersions(unittest.TestCase): class TestRequest(unittest.TestCase): - base_url = http._DEFAULT_URL + '/v1' + base_url = 'http://127.0.0.1:5050/v1' def setUp(self): super(TestRequest, self).setUp() @@ -131,24 +133,18 @@ class TestRequest(unittest.TestCase): service_type='baremetal-introspection', interface=None, region_name=None) - def test_ok_no_endpoint(self): + def test_no_endpoint(self): self.session.get_endpoint.return_value = None - res = self.get_client().request('get', '/foo/bar') + self.assertRaises(http.EndpointNotFound, self.get_client) - self.assertIs(self.req.return_value, res) - self.req.assert_called_once_with(self.base_url + '/foo/bar', 'get', - raise_exc=False, headers=self.headers) self.session.get_endpoint.assert_called_once_with( service_type='baremetal-introspection', interface=None, region_name=None) - def test_ok_endpoint_not_found(self): + def test_endpoint_not_found(self): self.session.get_endpoint.side_effect = exceptions.EndpointNotFound() - res = self.get_client().request('get', '/foo/bar') + self.assertRaises(http.EndpointNotFound, self.get_client) - self.assertIs(self.req.return_value, res) - self.req.assert_called_once_with(self.base_url + '/foo/bar', 'get', - raise_exc=False, headers=self.headers) self.session.get_endpoint.assert_called_once_with( service_type='baremetal-introspection', interface=None, region_name=None) @@ -156,11 +152,13 @@ class TestRequest(unittest.TestCase): @mock.patch.object(session.Session, 'request', autospec=True, **{'return_value.status_code': 200}) def test_ok_no_auth(self, mock_req): - res = self.get_client(use_session=False).request('get', '/foo/bar') + res = self.get_client( + use_session=False, + inspector_url='http://some/host').request('get', '/foo/bar') self.assertIs(mock_req.return_value, res) mock_req.assert_called_once_with(mock.ANY, - self.base_url + '/foo/bar', 'get', + 'http://some/host/v1/foo/bar', 'get', raise_exc=False, headers=self.headers) def test_explicit_version(self): @@ -171,22 +169,6 @@ class TestRequest(unittest.TestCase): self.req.assert_called_once_with(self.base_url + '/foo/bar', 'get', raise_exc=False, headers=self.headers) - def test_explicit_url(self): - res = self.get_client(inspector_url='http://host').request( - 'get', '/foo/bar') - - self.assertIs(self.req.return_value, res) - self.req.assert_called_once_with('http://host/v1/foo/bar', 'get', - raise_exc=False, headers=self.headers) - - def test_explicit_url_with_version(self): - res = self.get_client(inspector_url='http://host/v1').request( - 'get', '/foo/bar') - - self.assertIs(self.req.return_value, res) - self.req.assert_called_once_with('http://host/v1/foo/bar', 'get', - raise_exc=False, headers=self.headers) - def test_error(self): self.req.return_value.status_code = 400 self.req.return_value.content = json.dumps( diff --git a/ironic_inspector_client/test/test_init.py b/ironic_inspector_client/test/test_init.py index ccdb939..295ad7c 100644 --- a/ironic_inspector_client/test/test_init.py +++ b/ironic_inspector_client/test/test_init.py @@ -23,6 +23,7 @@ class TestExposedAPI(unittest.TestCase): if not x.startswith('__') and not isinstance(getattr(ironic_inspector_client, x), types.ModuleType)} - self.assertEqual({'ClientV1', 'ClientError', 'VersionNotSupported', + self.assertEqual({'ClientV1', 'ClientError', 'EndpointNotFound', + 'VersionNotSupported', 'MAX_API_VERSION', 'DEFAULT_API_VERSION'}, exposed) diff --git a/ironic_inspector_client/test/test_v1.py b/ironic_inspector_client/test/test_v1.py index ac19e09..1795dfb 100644 --- a/ironic_inspector_client/test/test_v1.py +++ b/ironic_inspector_client/test/test_v1.py @@ -38,6 +38,7 @@ class TestInit(unittest.TestCase): my_ip = 'http://' + netutils.get_my_ipv4() + ':5050' def get_client(self, **kwargs): + kwargs.setdefault('inspector_url', self.my_ip) return ironic_inspector_client.ClientV1(**kwargs) def test_ok(self, mock_get): @@ -74,6 +75,7 @@ class BaseTest(unittest.TestCase): @mock.patch.object(http.BaseClient, 'server_api_versions', lambda self: ((1, 0), (1, 99))) def get_client(self, **kwargs): + kwargs.setdefault('inspector_url', self.my_ip) return ironic_inspector_client.ClientV1(**kwargs) diff --git a/releasenotes/notes/no-default-uri-861f675ccb75e05d.yaml b/releasenotes/notes/no-default-uri-861f675ccb75e05d.yaml new file mode 100644 index 0000000..f226440 --- /dev/null +++ b/releasenotes/notes/no-default-uri-861f675ccb75e05d.yaml @@ -0,0 +1,11 @@ +--- +upgrade: + - | + There is no longer a default introspection API endpoint. Previously, + if no endpoint was requested and no endpoint found in the service catalog, + ``127.0.0.1:5050`` was used by default. +other: + - | + The ``ClientV1`` constructor now raises a new ``EndpointNotFound`` + exception when no introspection API endpoint can be detected. Previously + this condition was ignored and ``127.0.0.1:5050`` was used as a fallback.