From 303ac3f835f6741ac922abcdf5f153b7e9e1d07b Mon Sep 17 00:00:00 2001
From: Pavlo Shchelokovskyy <shchelokovskyy@gmail.com>
Date: Wed, 21 Jun 2017 07:48:35 +0000
Subject: [PATCH] Use adapters for inspectorclient
Inspector-client is a bit lacking behind other clients, as it does not
have Adapter-based SessionClient and thus does not support all
adapter-related options.
That's why we construct a session and an adapter from config section,
use adapter to resolve inspector API from service catalog
(or return the fixed endpoint_override one)
and then pass the session and inspector API endpoint to client.
This patch also deprecates `[inspector]service_url` in favor of
`[inspector]endpoint_override`.
As a side-effect, addressig inspector service now supports both regions
and interfaces to specify entry in service catalog.
Also, inspectorclient calls are now being made with the user token
(wrapped with a service token) when there is a token in the task's
request context.
Change-Id: I21836e712fa9764468ac2654525554b5b4f03741
Partial-Bug: #1699547
---
devstack/lib/ironic | 2 +-
etc/ironic/ironic.conf.sample | 45 ++++-
ironic/conf/inspector.py | 9 +-
ironic/drivers/modules/inspector.py | 36 ++--
.../unit/drivers/modules/test_inspector.py | 160 +++++++++---------
...cated-inspector-opts-b19a08339712cfd7.yaml | 17 ++
.../keystoneauth-config-1baa45a0a2dd93b4.yaml | 4 +
7 files changed, 173 insertions(+), 100 deletions(-)
create mode 100644 releasenotes/notes/deprecated-inspector-opts-b19a08339712cfd7.yaml
diff --git a/devstack/lib/ironic b/devstack/lib/ironic
index e044891304..ee8119624b 100644
--- a/devstack/lib/ironic
+++ b/devstack/lib/ironic
@@ -1113,7 +1113,7 @@ function configure_ironic_conductor {
# TODO(pas-ha) this block is for transition period only,
# after all clients are moved to use keystoneauth adapters,
# it will be deleted
- local sections_with_adapter="service_catalog glance cinder"
+ local sections_with_adapter="service_catalog glance cinder inspector"
for conf_section in $sections_with_adapter; do
configure_adapter_for $conf_section
done
diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample
index 99f1fed87f..a384d7ec90 100644
--- a/etc/ironic/ironic.conf.sample
+++ b/etc/ironic/ironic.conf.sample
@@ -1895,12 +1895,28 @@
# fake_inspector driver. (boolean value)
#enabled = false
+# Always use this endpoint URL for requests for this client.
+# (string value)
+#endpoint_override = <None>
+
# Verify HTTPS connections. (boolean value)
#insecure = false
# PEM encoded client certificate key file (string value)
#keyfile = <None>
+# The maximum major version of a given API, intended to be
+# used as the upper bound of a range with min_version.
+# Mutually exclusive with version. (string value)
+#max_version = <None>
+
+# The minimum major version of a given API, intended to be
+# used as the lower bound of a range with max_version.
+# Mutually exclusive with version. If min_version is given
+# with no max_version it is as if max version is "latest".
+# (string value)
+#min_version = <None>
+
# User's password (string value)
#password = <None>
@@ -1918,8 +1934,24 @@
# Deprecated group/name - [inspector]/tenant_name
#project_name = <None>
-# ironic-inspector HTTP endpoint. If this is not set, the
-# service catalog will be used. (string value)
+# The default region_name for endpoint URL discovery. (string
+# value)
+#region_name = <None>
+
+# The default service_name for endpoint URL discovery. (string
+# value)
+#service_name = <None>
+
+# The default service_type for endpoint URL discovery. (string
+# value)
+#service_type = baremetal-introspection
+
+# DEPRECATED: ironic-inspector HTTP endpoint. If this is not
+# set, the service catalog will be used. (string value)
+# This option is deprecated for removal.
+# Its value may be silently ignored in the future.
+# Reason: Use [inspector]/endpoint_override option instead to
+# set a specific ironic-inspector API URL to connect to.
#service_url = <None>
# period (in seconds) to check status of nodes on inspection
@@ -1951,6 +1983,15 @@
# Deprecated group/name - [inspector]/user_name
#username = <None>
+# List of interfaces, in order of preference, for endpoint
+# URL. (list value)
+#valid_interfaces = internal,public
+
+# Minimum Major API version within a given Major API version
+# for endpoint URL discovery. Mutually exclusive with
+# min_version and max_version (string value)
+#version = <None>
+
[ipmi]
diff --git a/ironic/conf/inspector.py b/ironic/conf/inspector.py
index 336f53d743..3f56ac999c 100644
--- a/ironic/conf/inspector.py
+++ b/ironic/conf/inspector.py
@@ -23,6 +23,10 @@ opts = [
'This option does not affect new-style dynamic drivers '
'and the fake_inspector driver.')),
cfg.StrOpt('service_url',
+ deprecated_for_removal=True,
+ deprecated_reason=_("Use [inspector]/endpoint_override option "
+ "instead to set a specific "
+ "ironic-inspector API URL to connect to."),
help=_('ironic-inspector HTTP endpoint. If this is not set, '
'the service catalog will be used.')),
cfg.IntOpt('status_check_period', default=60,
@@ -33,8 +37,9 @@ opts = [
def register_opts(conf):
conf.register_opts(opts, group='inspector')
- auth.register_auth_opts(conf, 'inspector')
+ auth.register_auth_opts(conf, 'inspector',
+ service_type='baremetal-introspection')
def list_opts():
- return auth.add_auth_opts(opts)
+ return auth.add_auth_opts(opts, service_type='baremetal-introspection')
diff --git a/ironic/drivers/modules/inspector.py b/ironic/drivers/modules/inspector.py
index 4edfd956ba..2642a83a66 100644
--- a/ironic/drivers/modules/inspector.py
+++ b/ironic/drivers/modules/inspector.py
@@ -39,22 +39,36 @@ INSPECTOR_API_VERSION = (1, 0)
_INSPECTOR_SESSION = None
-def _get_inspector_session():
- if CONF.auth_strategy != 'keystone':
- return
-
+def _get_inspector_session(**kwargs):
global _INSPECTOR_SESSION
if not _INSPECTOR_SESSION:
- _INSPECTOR_SESSION = keystone.get_session(
- 'inspector', auth=keystone.get_auth('inspector'))
+ _INSPECTOR_SESSION = keystone.get_session('inspector', **kwargs)
return _INSPECTOR_SESSION
-def _get_client():
+def _get_client(context):
"""Helper to get inspector client instance."""
+ # NOTE(pas-ha) remove in Rocky
+ if CONF.auth_strategy != 'keystone':
+ CONF.set_override('auth_type', 'none', group='inspector')
+ service_auth = keystone.get_auth('inspector')
+ session = _get_inspector_session(auth=service_auth)
+ adapter_params = {}
+ if CONF.inspector.service_url and not CONF.inspector.endpoint_override:
+ adapter_params['endpoint_override'] = CONF.inspector.service_url
+ adapter = keystone.get_adapter('inspector', session=session,
+ **adapter_params)
+ inspector_url = adapter.get_endpoint()
+ # TODO(pas-ha) investigate possibility of passing user context here,
+ # similar to what neutron/glance-related code does
+ # NOTE(pas-ha) ironic-inspector-client has no Adaper-based
+ # SessionClient, so we'll resolve inspector API form adapter loaded
+ # form config options
+ # TODO(pas-ha) rewrite when inspectorclient is based on ksa Adapter,
+ # also add global_request_id to the call
return client.ClientV1(api_version=INSPECTOR_API_VERSION,
- inspector_url=CONF.inspector.service_url,
- session=_get_inspector_session())
+ session=session,
+ inspector_url=inspector_url)
class Inspector(base.InspectInterface):
@@ -137,7 +151,7 @@ class Inspector(base.InspectInterface):
def _start_inspection(node_uuid, context):
"""Call to inspector to start inspection."""
try:
- _get_client().introspect(node_uuid)
+ _get_client(context).introspect(node_uuid)
except Exception as exc:
LOG.exception('Exception during contacting ironic-inspector '
'for inspection of node %(node)s: %(err)s',
@@ -166,7 +180,7 @@ def _check_status(task):
task.node.uuid)
try:
- status = _get_client().get_status(node.uuid)
+ status = _get_client(task.context).get_status(node.uuid)
except Exception:
# NOTE(dtantsur): get_status should not normally raise
# let's assume it's a transient failure and retry later
diff --git a/ironic/tests/unit/drivers/modules/test_inspector.py b/ironic/tests/unit/drivers/modules/test_inspector.py
index 73b1abcb07..010d04b95d 100644
--- a/ironic/tests/unit/drivers/modules/test_inspector.py
+++ b/ironic/tests/unit/drivers/modules/test_inspector.py
@@ -14,6 +14,7 @@ import eventlet
import ironic_inspector_client as client
import mock
+from ironic.common import context
from ironic.common import driver_factory
from ironic.common import exception
from ironic.common import states
@@ -52,6 +53,63 @@ class DisabledTestCase(db_base.DbTestCase):
inspector.Inspector()
+@mock.patch('ironic.common.keystone.get_adapter', autospec=True)
+@mock.patch('ironic.common.keystone.get_service_auth', autospec=True,
+ return_value=mock.sentinel.sauth)
+@mock.patch('ironic.common.keystone.get_auth', autospec=True,
+ return_value=mock.sentinel.auth)
+@mock.patch('ironic.common.keystone.get_session', autospec=True,
+ return_value=mock.sentinel.session)
+@mock.patch.object(client.ClientV1, '__init__', return_value=None)
+class GetClientTestCase(db_base.DbTestCase):
+
+ def setUp(self):
+ super(GetClientTestCase, self).setUp()
+ # NOTE(pas-ha) force-reset global inspector session object
+ inspector._INSPECTOR_SESSION = None
+ self.api_version = (1, 0)
+ self.context = context.RequestContext(global_request_id='global')
+
+ def test__get_client(self, mock_init, mock_session, mock_auth,
+ mock_sauth, mock_adapter):
+ mock_adapter.return_value.get_endpoint.return_value = 'inspector_url'
+ inspector._get_client(self.context)
+ mock_init.assert_called_once_with(
+ session=mock.sentinel.session,
+ api_version=self.api_version,
+ inspector_url='inspector_url')
+ self.assertEqual(0, mock_sauth.call_count)
+ self.assertEqual(1, mock_session.call_count)
+
+ def test__get_client_standalone(self, mock_init, mock_session, mock_auth,
+ mock_sauth, mock_adapter):
+ self.config(auth_strategy='noauth')
+ mock_adapter.return_value.get_endpoint.return_value = 'inspector_url'
+ inspector._get_client(self.context)
+ self.assertEqual('none', inspector.CONF.inspector.auth_type)
+ mock_init.assert_called_once_with(
+ session=mock.sentinel.session,
+ api_version=self.api_version,
+ inspector_url='inspector_url')
+ self.assertEqual(0, mock_sauth.call_count)
+ self.assertEqual(1, mock_session.call_count)
+
+ def test__get_client_url(self, mock_init, mock_session, mock_auth,
+ mock_sauth, mock_adapter):
+ self.config(service_url='meow', group='inspector')
+ mock_adapter.return_value.get_endpoint.return_value = 'meow'
+ inspector._get_client(self.context)
+ mock_init.assert_called_once_with(
+ session=mock.sentinel.session,
+ api_version=self.api_version,
+ inspector_url='meow')
+ mock_adapter.assert_called_once_with('inspector',
+ session=mock.sentinel.session,
+ endpoint_override='meow')
+ self.assertEqual(0, mock_sauth.call_count)
+ self.assertEqual(1, mock_session.call_count)
+
+
class BaseTestCase(db_base.DbTestCase):
def setUp(self):
super(BaseTestCase, self).setUp()
@@ -65,8 +123,6 @@ class BaseTestCase(db_base.DbTestCase):
self.task.node = self.node
self.task.driver = self.driver
self.api_version = (1, 0)
- # NOTE(pas-ha) force-reset global inspector session object
- inspector._INSPECTOR_SESSION = None
class CommonFunctionsTestCase(BaseTestCase):
@@ -89,35 +145,18 @@ class CommonFunctionsTestCase(BaseTestCase):
self.assertTrue(warn_mock.called)
-@mock.patch('ironic.common.keystone.get_auth',
- lambda _section, **kwargs: mock.sentinel.auth)
-@mock.patch('ironic.common.keystone.get_session',
- lambda _section, **kwargs: mock.sentinel.session)
@mock.patch.object(eventlet, 'spawn_n', lambda f, *a, **kw: f(*a, **kw))
-@mock.patch.object(client.ClientV1, 'introspect')
-@mock.patch.object(client.ClientV1, '__init__', return_value=None)
+@mock.patch('ironic.drivers.modules.inspector._get_client', autospec=True)
class InspectHardwareTestCase(BaseTestCase):
- def test_ok(self, mock_init, mock_introspect):
+ def test_ok(self, mock_client):
+ mock_introspect = mock_client.return_value.introspect
self.assertEqual(states.INSPECTING,
self.driver.inspect.inspect_hardware(self.task))
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url=None)
- mock_introspect.assert_called_once_with(self.node.uuid)
-
- def test_url(self, mock_init, mock_introspect):
- self.config(service_url='meow', group='inspector')
- self.assertEqual(states.INSPECTING,
- self.driver.inspect.inspect_hardware(self.task))
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url='meow')
mock_introspect.assert_called_once_with(self.node.uuid)
@mock.patch.object(task_manager, 'acquire', autospec=True)
- def test_error(self, mock_acquire, mock_init, mock_introspect):
+ def test_error(self, mock_acquire, mock_client):
+ mock_introspect = mock_client.return_value.introspect
mock_introspect.side_effect = RuntimeError('boom')
self.driver.inspect.inspect_hardware(self.task)
mock_introspect.assert_called_once_with(self.node.uuid)
@@ -125,97 +164,50 @@ class InspectHardwareTestCase(BaseTestCase):
self.assertIn('boom', task.node.last_error)
task.process_event.assert_called_once_with('fail')
- def test_is_standalone(self, mock_init, mock_introspect):
- self.config(auth_strategy='noauth')
- self.assertEqual(states.INSPECTING,
- self.driver.inspect.inspect_hardware(self.task))
- mock_init.assert_called_once_with(
- session=None,
- api_version=self.api_version,
- inspector_url=None)
- mock_introspect.assert_called_once_with(self.node.uuid)
-
-@mock.patch('ironic.common.keystone.get_auth',
- lambda _section, **kwargs: mock.sentinel.auth)
-@mock.patch('ironic.common.keystone.get_session',
- lambda _section, **kwargs: mock.sentinel.session)
-@mock.patch.object(client.ClientV1, 'get_status')
-@mock.patch.object(client.ClientV1, '__init__', return_value=None)
+@mock.patch('ironic.drivers.modules.inspector._get_client', autospec=True)
class CheckStatusTestCase(BaseTestCase):
def setUp(self):
super(CheckStatusTestCase, self).setUp()
self.node.provision_state = states.INSPECTING
- def test_not_inspecting(self, mock_init, mock_get):
+ def test_not_inspecting(self, mock_client):
+ mock_get = mock_client.return_value.get_status
self.node.provision_state = states.MANAGEABLE
inspector._check_status(self.task)
self.assertFalse(mock_get.called)
- def test_not_inspector(self, mock_init, mock_get):
+ def test_not_inspector(self, mock_client):
+ mock_get = mock_client.return_value.get_status
self.task.driver.inspect = object()
inspector._check_status(self.task)
self.assertFalse(mock_get.called)
- def test_not_finished(self, mock_init, mock_get):
+ def test_not_finished(self, mock_client):
+ mock_get = mock_client.return_value.get_status
mock_get.return_value = {}
inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url=None)
mock_get.assert_called_once_with(self.node.uuid)
self.assertFalse(self.task.process_event.called)
- def test_exception_ignored(self, mock_init, mock_get):
+ def test_exception_ignored(self, mock_client):
+ mock_get = mock_client.return_value.get_status
mock_get.side_effect = RuntimeError('boom')
inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url=None)
mock_get.assert_called_once_with(self.node.uuid)
self.assertFalse(self.task.process_event.called)
- def test_status_ok(self, mock_init, mock_get):
+ def test_status_ok(self, mock_client):
+ mock_get = mock_client.return_value.get_status
mock_get.return_value = {'finished': True}
inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url=None)
mock_get.assert_called_once_with(self.node.uuid)
self.task.process_event.assert_called_once_with('done')
- def test_status_error(self, mock_init, mock_get):
+ def test_status_error(self, mock_client):
+ mock_get = mock_client.return_value.get_status
mock_get.return_value = {'error': 'boom'}
inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url=None)
mock_get.assert_called_once_with(self.node.uuid)
self.task.process_event.assert_called_once_with('fail')
self.assertIn('boom', self.node.last_error)
-
- def test_service_url(self, mock_init, mock_get):
- self.config(service_url='meow', group='inspector')
- mock_get.return_value = {'finished': True}
- inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=mock.sentinel.session,
- api_version=self.api_version,
- inspector_url='meow')
- mock_get.assert_called_once_with(self.node.uuid)
- self.task.process_event.assert_called_once_with('done')
-
- def test_is_standalone(self, mock_init, mock_get):
- self.config(auth_strategy='noauth')
- mock_get.return_value = {'finished': True}
- inspector._check_status(self.task)
- mock_init.assert_called_once_with(
- session=None,
- api_version=self.api_version,
- inspector_url=None)
- mock_get.assert_called_once_with(self.node.uuid)
- self.task.process_event.assert_called_once_with('done')
diff --git a/releasenotes/notes/deprecated-inspector-opts-b19a08339712cfd7.yaml b/releasenotes/notes/deprecated-inspector-opts-b19a08339712cfd7.yaml
new file mode 100644
index 0000000000..b1603cc722
--- /dev/null
+++ b/releasenotes/notes/deprecated-inspector-opts-b19a08339712cfd7.yaml
@@ -0,0 +1,17 @@
+---
+deprecations:
+ - |
+ Configuration option ``[inspector]/service_url`` is deprecated
+ and will be ignored in the Rocky release.
+ Instead, use ``[inspector]/endpoint_override`` configuration option to set
+ specific ironic-inspector api address when automatic discovery of
+ inspector API endpoint from keystone catalog is not desired.
+ This new option has no default value (``None``) and must be set explicitly.
+
+ - |
+ Relying on the value of ``[DEFAULT]/auth_strategy`` configuration option to
+ configure usage of standalone mode for inspector client is deprecated and
+ will be impossible the the Rocky release.
+ Instead, set ``[inspector]/auth_type`` configuration option to ``none`` and
+ provide the inspector API address as ``[inspector]/endpoint_override``
+ configuration option.
diff --git a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml b/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml
index d9c4a7ee06..98f687c8a0 100644
--- a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml
+++ b/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml
@@ -8,3 +8,7 @@ features:
Adds the ability to set keystoneauth settings in the
``[cinder]`` configuration section for service automatic
discovery.
+ - |
+ Adds the ability to set keystoneauth settings in the
+ ``[inspector]`` configuration section for service automatic
+ discovery.