From 7337fefd978b14d8a5e04314565e696e86879f21 Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Fri, 23 Jun 2017 12:03:16 +0000 Subject: [PATCH] Refactor common keystone methods - Split loading session and auth from config to separate functions, allow to override options loaded from config. This will lay ground to more efficiently load clients that allow passing in both session and auth plugin objects separately. - When resoving a service endpoint, either fetch the requested interface, or first try 'internal' and then 'public'. This is done due to our config lacking any options to set the default interface for service endpoints to use, and we've used 'internal' by default, although DevStack has no such endpoints for most services any more [0]. This will be changed again when gradually introducing usage of keystoneauth Adapters to ironic. - Remove get_admin_token method, it was used only in glance-related code once, and was simply moved there. [0] https://review.openstack.org/#/c/433272 Change-Id: I73b21098f15af4d0445f89fdd6ad4e4a42177df6 Partial-Bug: #1699547 --- ironic/common/cinder.py | 3 +- ironic/common/glance_service/service_utils.py | 3 +- ironic/common/image_service.py | 6 +- ironic/common/keystone.py | 76 +++++++++++-------- ironic/common/neutron.py | 7 +- ironic/common/swift.py | 3 +- ironic/drivers/modules/deploy_utils.py | 4 +- ironic/drivers/modules/inspector.py | 3 +- ironic/tests/unit/common/test_cinder.py | 9 ++- .../tests/unit/common/test_glance_service.py | 7 +- ironic/tests/unit/common/test_keystone.py | 76 +++++++++---------- .../unit/drivers/modules/test_inspector.py | 8 +- 12 files changed, 117 insertions(+), 88 deletions(-) diff --git a/ironic/common/cinder.py b/ironic/common/cinder.py index 48d8461cf2..66801bf604 100644 --- a/ironic/common/cinder.py +++ b/ironic/common/cinder.py @@ -35,7 +35,8 @@ _CINDER_SESSION = None def _get_cinder_session(): global _CINDER_SESSION if not _CINDER_SESSION: - _CINDER_SESSION = keystone.get_session('cinder') + auth = keystone.get_auth('cinder') + _CINDER_SESSION = keystone.get_session('cinder', auth=auth) return _CINDER_SESSION diff --git a/ironic/common/glance_service/service_utils.py b/ironic/common/glance_service/service_utils.py index 1320177f3b..d4771064f0 100644 --- a/ironic/common/glance_service/service_utils.py +++ b/ironic/common/glance_service/service_utils.py @@ -108,7 +108,8 @@ def _get_api_server_iterator(): api_servers = [] if not CONF.glance.glance_api_servers and not CONF.glance.glance_host: - session = keystone.get_session('service_catalog') + session = keystone.get_session('glance', + auth=keystone.get_auth('glance')) api_servers = [keystone.get_service_url(session, service_type='image', endpoint_type='public')] else: diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index 028020c6e7..c85c1f11c5 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -41,7 +41,8 @@ _GLANCE_SESSION = None def _get_glance_session(): global _GLANCE_SESSION if not _GLANCE_SESSION: - _GLANCE_SESSION = keystone.get_session('glance') + auth = keystone.get_auth('glance') + _GLANCE_SESSION = keystone.get_session('glance', auth=auth) return _GLANCE_SESSION @@ -55,8 +56,7 @@ def GlanceImageService(client=None, version=None, context=None): service_class = getattr(module, 'GlanceImageService') if (context is not None and CONF.glance.auth_strategy == 'keystone' and not context.auth_token): - session = _get_glance_session() - context.auth_token = keystone.get_admin_auth_token(session) + context.auth_token = _get_glance_session().get_token() return service_class(client, version, context) diff --git a/ironic/common/keystone.py b/ironic/common/keystone.py index 267df25dda..52045c9ada 100644 --- a/ironic/common/keystone.py +++ b/ironic/common/keystone.py @@ -50,44 +50,60 @@ def ks_exceptions(f): @ks_exceptions -def get_session(group): +def get_session(group, **session_kwargs): + """Loads session object from options in a configuration file section. + + The session_kwargs will be passed directly to keystoneauth1 Session + and will override the values loaded from config. + Consult keystoneauth1 docs for available options. + + :param group: name of the config section to load session options from + + """ + return kaloading.load_session_from_conf_options( + CONF, group, **session_kwargs) + + +@ks_exceptions +def get_auth(group, **auth_kwargs): + """Loads auth plugin from options in a configuration file section. + + The auth_kwargs will be passed directly to keystoneauth1 auth plugin + and will override the values loaded from config. + Note that the accepted kwargs will depend on auth plugin type as defined + by [group]auth_type option. + Consult keystoneauth1 docs for available auth plugins and their options. + + :param group: name of the config section to load auth plugin options from + + """ try: - auth = kaloading.load_auth_from_conf_options(CONF, group) + auth = kaloading.load_auth_from_conf_options(CONF, group, + **auth_kwargs) except kaexception.MissingRequiredOptions: LOG.error('Failed to load auth plugin from group %s', group) raise - session = kaloading.load_session_from_conf_options( - CONF, group, auth=auth) - return session + return auth -# TODO(pas-ha) we actually should barely need this at all: -# if we instantiate a identity.Token auth plugin from incoming -# request context we could build a session with it, and each client -# would know its service_type already, looking up the endpoint by itself +# NOTE(pas-ha) Used by neutronclient and resolving ironic API only +# FIXME(pas-ha) remove this while moving to kesytoneauth adapters @ks_exceptions -def get_service_url(session, service_type='baremetal', - endpoint_type='internal'): - """Wrapper for get service url from keystone service catalog. +def get_service_url(session, **kwargs): + """Find endpoint for given service in keystone catalog. - Given a service_type and an endpoint_type, this method queries - keystone service catalog and provides the url for the desired - endpoint. + If 'interrace' is provided, fetches service url of this interface. + Otherwise, first tries to fetch 'internal' endpoint, + and then the 'public' one. + + :param session: keystoneauth Session object + :param kwargs: any other arguments accepted by Session.get_endpoint method - :param service_type: the keystone service for which url is required. - :param endpoint_type: the type of endpoint for the service. - :returns: an http/https url for the desired endpoint. """ - return session.get_endpoint(service_type=service_type, - interface=endpoint_type, - region_name=CONF.keystone.region_name) - -# TODO(pas-ha) move all clients to sessions, then we do not need this -@ks_exceptions -def get_admin_auth_token(session): - """Get admin token. - - Currently used for inspector, glance and swift clients. - """ - return session.get_token() + if 'interface' in kwargs: + return session.get_endpoint(**kwargs) + try: + return session.get_endpoint(interface='internal', **kwargs) + except kaexception.EndpointNotFound: + return session.get_endpoint(interface='public', **kwargs) diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 8e5c27f17c..a60a658f63 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -30,7 +30,8 @@ _NEUTRON_SESSION = None def _get_neutron_session(): global _NEUTRON_SESSION if not _NEUTRON_SESSION: - _NEUTRON_SESSION = keystone.get_session('neutron') + auth = keystone.get_auth('neutron') + _NEUTRON_SESSION = keystone.get_session('neutron', auth=auth) return _NEUTRON_SESSION @@ -60,7 +61,9 @@ def get_client(token=None): else: params['token'] = token params['endpoint_url'] = url or keystone.get_service_url( - session, service_type='network') + session, + service_type='network', + region_name=CONF.keystone.region_name) params.update({ 'timeout': CONF.neutron.url_timeout or CONF.neutron.timeout, 'insecure': CONF.neutron.insecure, diff --git a/ironic/common/swift.py b/ironic/common/swift.py index 1203ed0732..8168870eef 100644 --- a/ironic/common/swift.py +++ b/ironic/common/swift.py @@ -31,7 +31,8 @@ _SWIFT_SESSION = None def _get_swift_session(): global _SWIFT_SESSION if not _SWIFT_SESSION: - _SWIFT_SESSION = keystone.get_session('swift') + auth = keystone.get_auth('swift') + _SWIFT_SESSION = keystone.get_session('swift', auth=auth) return _SWIFT_SESSION diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index ffdb53bd29..7d79403a49 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -77,7 +77,9 @@ _IRONIC_SESSION = None def _get_ironic_session(): global _IRONIC_SESSION if not _IRONIC_SESSION: - _IRONIC_SESSION = keystone.get_session('service_catalog') + auth = keystone.get_auth('service_catalog') + _IRONIC_SESSION = keystone.get_session('service_catalog', + auth=auth) return _IRONIC_SESSION diff --git a/ironic/drivers/modules/inspector.py b/ironic/drivers/modules/inspector.py index 49092dc399..4edfd956ba 100644 --- a/ironic/drivers/modules/inspector.py +++ b/ironic/drivers/modules/inspector.py @@ -45,7 +45,8 @@ def _get_inspector_session(): global _INSPECTOR_SESSION if not _INSPECTOR_SESSION: - _INSPECTOR_SESSION = keystone.get_session('inspector') + _INSPECTOR_SESSION = keystone.get_session( + 'inspector', auth=keystone.get_auth('inspector')) return _INSPECTOR_SESSION diff --git a/ironic/tests/unit/common/test_cinder.py b/ironic/tests/unit/common/test_cinder.py index ba598aaa63..b7fb3786b8 100644 --- a/ironic/tests/unit/common/test_cinder.py +++ b/ironic/tests/unit/common/test_cinder.py @@ -30,6 +30,7 @@ from ironic.tests.unit.db import base as db_base from ironic.tests.unit.objects import utils as object_utils +@mock.patch.object(keystone, 'get_auth', autospec=True) @mock.patch.object(keystone, 'get_session', autospec=True) class TestCinderSession(base.TestCase): @@ -39,17 +40,21 @@ class TestCinderSession(base.TestCase): retries=2, group='cinder') - def test__get_cinder_session(self, mock_keystone_session): + def test__get_cinder_session(self, mock_keystone_session, mock_auth): """Check establishing new session when no session exists.""" mock_keystone_session.return_value = 'session1' self.assertEqual('session1', cinder._get_cinder_session()) - mock_keystone_session.assert_called_once_with('cinder') + mock_keystone_session.assert_called_once_with( + 'cinder', auth=mock_auth.return_value) + mock_auth.assert_called_once_with('cinder') """Check if existing session is used.""" mock_keystone_session.reset_mock() + mock_auth.reset_mock() mock_keystone_session.return_value = 'session2' self.assertEqual('session1', cinder._get_cinder_session()) self.assertFalse(mock_keystone_session.called) + self.assertFalse(mock_auth.called) @mock.patch.object(cinder, '_get_cinder_session', autospec=True) diff --git a/ironic/tests/unit/common/test_glance_service.py b/ironic/tests/unit/common/test_glance_service.py index cd6793fc7e..b166e044e2 100644 --- a/ironic/tests/unit/common/test_glance_service.py +++ b/ironic/tests/unit/common/test_glance_service.py @@ -859,14 +859,17 @@ class TestGlanceAPIServers(base.TestCase): @mock.patch.object(service_utils.keystone, 'get_service_url', autospec=True) @mock.patch.object(service_utils.keystone, 'get_session', autospec=True) - def test__get_api_servers_with_keystone(self, mock_session, + @mock.patch.object(service_utils.keystone, 'get_auth', autospec=True) + def test__get_api_servers_with_keystone(self, mock_auth, mock_session, mock_service_url): mock_service_url.return_value = 'https://example.com' endpoint = service_utils._get_api_server() self.assertEqual('https://example.com', endpoint) - mock_session.assert_called_once_with('service_catalog') + mock_auth.assert_called_once_with('glance') + mock_session.assert_called_once_with('glance', + auth=mock_auth.return_value) mock_service_url.assert_called_once_with(mock_session.return_value, service_type='image', endpoint_type='public') diff --git a/ironic/tests/unit/common/test_keystone.py b/ironic/tests/unit/common/test_keystone.py index 2365dea0f1..1b1a420706 100644 --- a/ironic/tests/unit/common/test_keystone.py +++ b/ironic/tests/unit/common/test_keystone.py @@ -13,7 +13,6 @@ # under the License. from keystoneauth1 import exceptions as ksexception -from keystoneauth1 import identity as kaidentity from keystoneauth1 import loading as kaloading import mock from oslo_config import cfg @@ -52,52 +51,45 @@ class KeystoneTestCase(base.TestCase): self.cfg_fixture = self.useFixture(fixture.Config()) self.addCleanup(cfg.CONF.reset) - def test_get_url(self): - fake_url = 'http://127.0.0.1:6385' - mock_sess = mock.Mock() - mock_sess.get_endpoint.return_value = fake_url - res = keystone.get_service_url(mock_sess) - mock_sess.get_endpoint.assert_called_with( - interface='internal', region_name='fake_region', - service_type='baremetal') - self.assertEqual(fake_url, res) - - def test_get_url_failure(self): - exc_map = ( - (ksexception.Unauthorized, exception.KeystoneUnauthorized), - (ksexception.EndpointNotFound, exception.CatalogNotFound), - (ksexception.EmptyCatalog, exception.CatalogNotFound), - (ksexception.Unauthorized, exception.KeystoneUnauthorized), - ) - for kexc, irexc in exc_map: - mock_sess = mock.Mock() - mock_sess.get_endpoint.side_effect = kexc - self.assertRaises(irexc, keystone.get_service_url, mock_sess) - - def test_get_admin_auth_token(self): - mock_sess = mock.Mock() - mock_sess.get_token.return_value = 'fake_token' - self.assertEqual('fake_token', - keystone.get_admin_auth_token(mock_sess)) - - def test_get_admin_auth_token_failure(self): - mock_sess = mock.Mock() - mock_sess.get_token.side_effect = ksexception.Unauthorized - self.assertRaises(exception.KeystoneUnauthorized, - keystone.get_admin_auth_token, mock_sess) - def test_get_session(self): - session = keystone.get_session(self.test_group) - # NOTE(pas-ha) 'password' auth_plugin is used - self.assertIsInstance(session.auth, - kaidentity.generic.password.Password) - self.assertEqual('http://127.0.0.1:9898', session.auth.auth_url) + self.config(timeout=10, group=self.test_group) + session = keystone.get_session(self.test_group, timeout=20) + self.assertEqual(20, session.timeout) - def test_get_session_fail(self): + def test_get_auth(self): + auth = keystone.get_auth(self.test_group) + self.assertEqual('http://127.0.0.1:9898', auth.auth_url) + + def test_get_auth_fail(self): # NOTE(pas-ha) 'password' auth_plugin is used, # so when we set the required auth_url to None, # MissingOption is raised self.config(auth_url=None, group=self.test_group) self.assertRaises(exception.ConfigInvalid, - keystone.get_session, + keystone.get_auth, self.test_group) + + def test_get_service_url_with_interface(self): + session = mock.Mock() + session.get_endpoint.return_value = 'spam' + params = {'interface': 'admin', 'ham': 'eggs'} + self.assertEqual('spam', keystone.get_service_url(session, **params)) + session.get_endpoint.assert_called_once_with(**params) + + def test_get_service_url_internal(self): + session = mock.Mock() + session.get_endpoint.return_value = 'spam' + params = {'ham': 'eggs'} + self.assertEqual('spam', keystone.get_service_url(session, **params)) + session.get_endpoint.assert_called_once_with(interface='internal', + **params) + + def test_get_service_url_internal_fail(self): + session = mock.Mock() + session.get_endpoint.side_effect = [ksexception.EndpointNotFound(), + 'spam'] + params = {'ham': 'eggs'} + self.assertEqual('spam', keystone.get_service_url(session, **params)) + session.get_endpoint.assert_has_calls([ + mock.call(interface='internal', **params), + mock.call(interface='public', **params)]) diff --git a/ironic/tests/unit/drivers/modules/test_inspector.py b/ironic/tests/unit/drivers/modules/test_inspector.py index b843d7f5e2..bcd197b4c8 100644 --- a/ironic/tests/unit/drivers/modules/test_inspector.py +++ b/ironic/tests/unit/drivers/modules/test_inspector.py @@ -89,8 +89,10 @@ 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 _n: mock.sentinel.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) @@ -134,8 +136,10 @@ class InspectHardwareTestCase(BaseTestCase): 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 _n: mock.sentinel.session) + lambda _section, **kwargs: mock.sentinel.session) @mock.patch.object(client.ClientV1, 'get_status') @mock.patch.object(client.ClientV1, '__init__', return_value=None) class CheckStatusTestCase(BaseTestCase):