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

@@ -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

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
+                        region_name=region_name)
-                except ks_exc.EndpointNotFound:
+                except ks_exc.CatalogException as exc:
-                    LOG.warning('Endpoint for service %s was not found, '
+                    LOG.error('%(iface)s endpoint for %(stype)s in region '
-                                'falling back to local host on port 5050',
+                              '%(region)s was not found in the service '
-                                service_type)
+                              '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)

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(
-        self.assertTrue(client.ClientV1(api_version='1.0')
+            api_version=1, session=self.session).server_api_versions())
-                        .server_api_versions())
+        self.assertTrue(client.ClientV1(
-        self.assertTrue(client.ClientV1(api_version=(1, 0))
+            api_version='1.0', session=self.session).server_api_versions())
-                        .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)

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(

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)

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)
 
 

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.