From 8db72b99f2d580cb2930041c15c04b73731fda76 Mon Sep 17 00:00:00 2001 From: Dave McCowan Date: Mon, 7 Dec 2015 14:28:52 -0500 Subject: [PATCH] Check context before returning cached value The key manager caches the value of barbican client to be reused, saving an extra call to keystone. The cached value is only applicable to the current context, so the context must be checked before returning the cached value. Closes-Bug: #1523646 Change-Id: I7cd7f1ba8a749b230c611e4fb20ccf4127354c35 (cherry picked from commit 676a53ce44a5624a553e80bcff339300802d5494) --- nova/keymgr/barbican.py | 80 ++++++++++++++----------- nova/tests/unit/keymgr/test_barbican.py | 52 +++++++++++++++- 2 files changed, 96 insertions(+), 36 deletions(-) diff --git a/nova/keymgr/barbican.py b/nova/keymgr/barbican.py index 9f68b1b5360e..9579ea0623ff 100644 --- a/nova/keymgr/barbican.py +++ b/nova/keymgr/barbican.py @@ -62,6 +62,7 @@ class BarbicanKeyManager(key_mgr.KeyManager): def __init__(self): self._barbican_client = None + self._current_context = None self._base_url = None def _get_barbican_client(self, ctxt): @@ -72,47 +73,56 @@ class BarbicanKeyManager(key_mgr.KeyManager): :raises Forbidden: if the ctxt is None """ - if not self._barbican_client: - # Confirm context is provided, if not raise forbidden - if not ctxt: - msg = _("User is not authorized to use key manager.") - LOG.error(msg) - raise exception.Forbidden(msg) + # Confirm context is provided, if not raise forbidden + if not ctxt: + msg = _("User is not authorized to use key manager.") + LOG.error(msg) + raise exception.Forbidden(msg) - try: - _SESSION = session.Session.load_from_conf_options( - CONF, - BARBICAN_OPT_GROUP) + if not hasattr(ctxt, 'project_id') or ctxt.project_id is None: + msg = _("Unable to create Barbican Client without project_id.") + LOG.error(msg) + raise exception.KeyManagerError(msg) - auth = ctxt.get_auth_plugin() - service_type, service_name, interface = (CONF. - barbican. - catalog_info. - split(':')) - region_name = CONF.barbican.os_region_name - service_parameters = {'service_type': service_type, - 'service_name': service_name, - 'interface': interface, - 'region_name': region_name} + # If same context, return cached barbican client + if self._barbican_client and self._current_context == ctxt: + return self._barbican_client - if CONF.barbican.endpoint_template: - self._base_url = (CONF.barbican.endpoint_template % - ctxt.to_dict()) - else: - self._base_url = _SESSION.get_endpoint( - auth, **service_parameters) + try: + _SESSION = session.Session.load_from_conf_options( + CONF, + BARBICAN_OPT_GROUP) - # the barbican endpoint can't have the '/v1' on the end - self._barbican_endpoint = self._base_url.rpartition('/')[0] + auth = ctxt.get_auth_plugin() + service_type, service_name, interface = (CONF. + barbican. + catalog_info. + split(':')) + region_name = CONF.barbican.os_region_name + service_parameters = {'service_type': service_type, + 'service_name': service_name, + 'interface': interface, + 'region_name': region_name} - sess = session.Session(auth=auth) - self._barbican_client = barbican_client.Client( - session=sess, - endpoint=self._barbican_endpoint) + if CONF.barbican.endpoint_template: + self._base_url = (CONF.barbican.endpoint_template % + ctxt.to_dict()) + else: + self._base_url = _SESSION.get_endpoint( + auth, **service_parameters) - except Exception as e: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Error creating Barbican client: %s"), e) + # the barbican endpoint can't have the '/v1' on the end + self._barbican_endpoint = self._base_url.rpartition('/')[0] + + sess = session.Session(auth=auth) + self._barbican_client = barbican_client.Client( + session=sess, + endpoint=self._barbican_endpoint) + self._current_context = ctxt + + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.error(_LE("Error creating Barbican client: %s"), e) return self._barbican_client diff --git a/nova/tests/unit/keymgr/test_barbican.py b/nova/tests/unit/keymgr/test_barbican.py index 36901f36601d..a1c32cfeca32 100644 --- a/nova/tests/unit/keymgr/test_barbican.py +++ b/nova/tests/unit/keymgr/test_barbican.py @@ -37,8 +37,9 @@ class BarbicanKeyManagerTestCase(test_key_mgr.KeyManagerTestCase): super(BarbicanKeyManagerTestCase, self).setUp() # Create fake auth_token - self.ctxt = mock.Mock() + self.ctxt = mock.MagicMock() self.ctxt.auth_token = "fake_token" + self.ctxt.project = "fake_project" # Create mock barbican client self._build_mock_barbican() @@ -49,6 +50,7 @@ class BarbicanKeyManagerTestCase(test_key_mgr.KeyManagerTestCase): self.pre_hex = "AIDxQp2++uAbKaTVDMXFYIu8PIugJGqkK0JLqkU0rhY=" self.hex = ("0080f1429dbefae01b29a4d50cc5c5608bbc3c8ba0246aa42b424baa4" "534ae16") + self.key_mgr._current_context = self.ctxt self.key_mgr._base_url = "http://host:9311/v1" self.addCleanup(self._restore) @@ -221,3 +223,51 @@ class BarbicanKeyManagerTestCase(test_key_mgr.KeyManagerTestCase): self.key_mgr._barbican_client = None self.assertRaises(exception.Forbidden, self.key_mgr.store_key, None, None) + + @mock.patch('keystoneclient.session.Session') + @mock.patch('barbicanclient.client.Client') + def test_get_barbican_client_new(self, mock_barbican, mock_keystone): + manager = self._create_key_manager() + manager._get_barbican_client(self.ctxt) + self.assertEqual(mock_keystone.call_count, 1) + self.assertEqual(mock_barbican.call_count, 1) + + @mock.patch('keystoneclient.session.Session') + @mock.patch('barbicanclient.client.Client') + def test_get_barbican_client_reused(self, mock_barbican, mock_keystone): + manager = self._create_key_manager() + manager._get_barbican_client(self.ctxt) + self.assertEqual(mock_keystone.call_count, 1) + self.assertEqual(mock_barbican.call_count, 1) + manager._get_barbican_client(self.ctxt) + self.assertEqual(mock_keystone.call_count, 1) + self.assertEqual(mock_barbican.call_count, 1) + + @mock.patch('keystoneclient.session.Session') + @mock.patch('barbicanclient.client.Client') + def test_get_barbican_client_not_reused(self, mock_barbican, + mock_keystone): + manager = self._create_key_manager() + manager._get_barbican_client(self.ctxt) + self.assertEqual(mock_keystone.call_count, 1) + self.assertEqual(mock_barbican.call_count, 1) + ctxt2 = mock.MagicMock() + ctxt2.auth_token = "fake_token2" + ctxt2.project = "fake_project2" + manager._get_barbican_client(ctxt2) + self.assertEqual(mock_keystone.call_count, 2) + self.assertEqual(mock_barbican.call_count, 2) + + def test_get_barbican_client_null_context(self): + self.assertRaises(exception.Forbidden, + self.key_mgr._get_barbican_client, None) + + def test_get_barbican_client_missing_project(self): + del(self.ctxt.project_id) + self.assertRaises(exception.KeyManagerError, + self.key_mgr._get_barbican_client, self.ctxt) + + def test_get_barbican_client_none_project(self): + self.ctxt.project_id = None + self.assertRaises(exception.KeyManagerError, + self.key_mgr._get_barbican_client, self.ctxt)