diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index 428f971f15..9086beb831 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -29,6 +29,7 @@ import six.moves.urllib.parse as urlparse from ironic.common import exception from ironic.common.i18n import _ +from ironic.common import keystone LOG = logging.getLogger(__name__) @@ -83,6 +84,9 @@ def import_versioned_module(version, submodule=None): def GlanceImageService(client=None, version=1, context=None): module = import_versioned_module(version, 'image_service') service_class = getattr(module, 'GlanceImageService') + if (context is not None and CONF.glance.auth_strategy == 'keystone' + and not context.auth_token): + context.auth_token = keystone.get_admin_auth_token() return service_class(client, version, context) diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 31b031c723..f437991cbb 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -51,7 +51,6 @@ import eventlet from eventlet import greenpool from oslo_concurrency import lockutils from oslo_config import cfg -from oslo_context import context as ironic_context from oslo_db import exception as db_exception from oslo_log import log import oslo_messaging as messaging @@ -69,7 +68,6 @@ from ironic.common.i18n import _LE from ironic.common.i18n import _LI from ironic.common.i18n import _LW from ironic.common import images -from ironic.common import keystone from ironic.common import rpc from ironic.common import states from ironic.common import swift @@ -1150,22 +1148,14 @@ class ConductorManager(periodic_task.PeriodicTasks): node_iter = self.iter_nodes(fields=['id', 'conductor_affinity'], filters=filters) - admin_context = None workers_count = 0 for node_uuid, driver, node_id, conductor_affinity in node_iter: if conductor_affinity == self.conductor.id: continue - # NOTE(lucasagomes): The context provided by the periodic task - # will make the glance client to fail with an 401 (Unauthorized) - # so we have to use the admin_context with an admin auth_token - if not admin_context: - admin_context = ironic_context.get_admin_context() - admin_context.auth_token = keystone.get_admin_auth_token() - # Node is mapped here, but not updated by this conductor last try: - with task_manager.acquire(admin_context, node_uuid) as task: + with task_manager.acquire(context, node_uuid) as task: # NOTE(deva): now that we have the lock, check again to # avoid racing with deletes and other state changes node = task.node diff --git a/ironic/drivers/modules/ilo/deploy.py b/ironic/drivers/modules/ilo/deploy.py index d5f905ce0a..b97a029493 100644 --- a/ironic/drivers/modules/ilo/deploy.py +++ b/ironic/drivers/modules/ilo/deploy.py @@ -31,7 +31,6 @@ from ironic.common.i18n import _LI from ironic.common.i18n import _LW from ironic.common import image_service from ironic.common import images -from ironic.common import keystone from ironic.common import states from ironic.common import swift from ironic.conductor import task_manager @@ -870,9 +869,6 @@ class VendorPassthru(agent_base_vendor.BaseAgentVendor): task, root_uuid=root_uuid, efi_system_part_uuid=efi_system_part_uuid) else: - # Agent vendorpassthru are made without auth token. - # We require auth_token to talk to glance while building boot iso. - task.context.auth_token = keystone.get_admin_auth_token() self._configure_vmedia_boot(task, root_uuid) # Set boot mode diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py index 381079385e..c003aea2c9 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/ironic/tests/conductor/test_manager.py @@ -23,7 +23,6 @@ import datetime import eventlet import mock from oslo_config import cfg -from oslo_context import context from oslo_db import exception as db_exception import oslo_messaging as messaging from oslo_utils import strutils @@ -33,7 +32,6 @@ from ironic.common import boot_devices from ironic.common import driver_factory from ironic.common import exception from ironic.common import images -from ironic.common import keystone from ironic.common import states from ironic.common import swift from ironic.conductor import manager @@ -3395,7 +3393,6 @@ class ManagerTestProperties(tests_db_base.DbTestCase): self.assertEqual(exception.DriverNotFound, exc.exc_info[0]) -@mock.patch.object(keystone, 'get_admin_auth_token') @mock.patch.object(task_manager, 'acquire') @mock.patch.object(manager.ConductorManager, '_mapped_to_this_conductor') @mock.patch.object(dbapi.IMPL, 'get_nodeinfo_list') @@ -3423,8 +3420,7 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): get_nodeinfo_mock.assert_called_once_with( columns=self.columns, filters=self.filters) - def test_not_mapped(self, get_nodeinfo_mock, mapped_mock, acquire_mock, - get_authtoken_mock): + def test_not_mapped(self, get_nodeinfo_mock, mapped_mock, acquire_mock): get_nodeinfo_mock.return_value = self._get_nodeinfo_list_response() mapped_mock.return_value = False @@ -3433,11 +3429,10 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): self._assert_get_nodeinfo_args(get_nodeinfo_mock) mapped_mock.assert_called_once_with(self.node.uuid, self.node.driver) self.assertFalse(acquire_mock.called) - self.assertFalse(get_authtoken_mock.called) self.service.ring_manager.reset.assert_called_once_with() def test_already_mapped(self, get_nodeinfo_mock, mapped_mock, - acquire_mock, get_authtoken_mock): + acquire_mock): # Node is already mapped to the conductor running the periodic task self.node.conductor_affinity = 123 self.service.conductor.id = 123 @@ -3450,13 +3445,9 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): self._assert_get_nodeinfo_args(get_nodeinfo_mock) mapped_mock.assert_called_once_with(self.node.uuid, self.node.driver) self.assertFalse(acquire_mock.called) - self.assertFalse(get_authtoken_mock.called) self.service.ring_manager.reset.assert_called_once_with() - @mock.patch.object(context, 'get_admin_context') - def test_good(self, get_ctx_mock, get_nodeinfo_mock, mapped_mock, - acquire_mock, get_authtoken_mock): - get_ctx_mock.return_value = self.context + def test_good(self, get_nodeinfo_mock, mapped_mock, acquire_mock): get_nodeinfo_mock.return_value = self._get_nodeinfo_list_response() mapped_mock.return_value = True acquire_mock.side_effect = self._get_acquire_side_effect(self.task) @@ -3465,17 +3456,14 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): self._assert_get_nodeinfo_args(get_nodeinfo_mock) mapped_mock.assert_called_once_with(self.node.uuid, self.node.driver) - get_authtoken_mock.assert_called_once_with() acquire_mock.assert_called_once_with(self.context, self.node.uuid) # assert spawn_after has been called self.task.spawn_after.assert_called_once_with( self.service._spawn_worker, self.service._do_takeover, self.task) - @mock.patch.object(context, 'get_admin_context') - def test_no_free_worker(self, get_ctx_mock, get_nodeinfo_mock, mapped_mock, - acquire_mock, get_authtoken_mock): - get_ctx_mock.return_value = self.context + def test_no_free_worker(self, get_nodeinfo_mock, mapped_mock, + acquire_mock): mapped_mock.return_value = True acquire_mock.side_effect = self._get_acquire_side_effect( [self.task] * 3) @@ -3503,18 +3491,12 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): expected = [mock.call(self.context, self.node.uuid)] * 2 self.assertEqual(expected, acquire_mock.call_args_list) - # Only one auth token needed for all runs - get_authtoken_mock.assert_called_once_with() - # assert spawn_after has been called twice expected = [mock.call(self.service._spawn_worker, self.service._do_takeover, self.task)] * 2 self.assertEqual(expected, self.task.spawn_after.call_args_list) - @mock.patch.object(context, 'get_admin_context') - def test_node_locked(self, get_ctx_mock, get_nodeinfo_mock, mapped_mock, - acquire_mock, get_authtoken_mock): - get_ctx_mock.return_value = self.context + def test_node_locked(self, get_nodeinfo_mock, mapped_mock, acquire_mock,): mapped_mock.return_value = True acquire_mock.side_effect = self._get_acquire_side_effect( [self.task, exception.NodeLocked('error'), self.task]) @@ -3536,20 +3518,14 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): expected = [mock.call(self.context, self.node.uuid)] * 3 self.assertEqual(expected, acquire_mock.call_args_list) - # Only one auth token needed for all runs - get_authtoken_mock.assert_called_once_with() - # assert spawn_after has been called only 2 times expected = [mock.call(self.service._spawn_worker, self.service._do_takeover, self.task)] * 2 self.assertEqual(expected, self.task.spawn_after.call_args_list) - @mock.patch.object(context, 'get_admin_context') - def test_worker_limit(self, get_ctx_mock, get_nodeinfo_mock, mapped_mock, - acquire_mock, get_authtoken_mock): + def test_worker_limit(self, get_nodeinfo_mock, mapped_mock, acquire_mock): # Limit to only 1 worker self.config(periodic_max_workers=1, group='conductor') - get_ctx_mock.return_value = self.context mapped_mock.return_value = True acquire_mock.side_effect = self._get_acquire_side_effect( [self.task] * 3) @@ -3570,9 +3546,6 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): # assert acquire() gets called only once because of the worker limit acquire_mock.assert_called_once_with(self.context, self.node.uuid) - # Only one auth token needed for all runs - get_authtoken_mock.assert_called_once_with() - # assert spawn_after has been called self.task.spawn_after.assert_called_once_with( self.service._spawn_worker, diff --git a/ironic/tests/drivers/ilo/test_deploy.py b/ironic/tests/drivers/ilo/test_deploy.py index 053a76228a..bf86419bc1 100644 --- a/ironic/tests/drivers/ilo/test_deploy.py +++ b/ironic/tests/drivers/ilo/test_deploy.py @@ -26,7 +26,6 @@ from ironic.common import exception from ironic.common.glance_service import service_utils from ironic.common import image_service from ironic.common import images -from ironic.common import keystone from ironic.common import states from ironic.common import swift from ironic.conductor import task_manager @@ -990,7 +989,6 @@ class VendorPassthruTestCase(db_base.DbTestCase): @mock.patch.object(ilo_deploy, '_update_secure_boot_mode', autospec=True) @mock.patch.object(ilo_common, 'update_boot_mode', autospec=True) - @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) @mock.patch.object(agent_base_vendor.BaseAgentVendor, 'reboot_and_finish_deploy', autospec=True) @mock.patch.object(ilo_deploy.VendorPassthru, '_configure_vmedia_boot', @@ -1001,7 +999,6 @@ class VendorPassthruTestCase(db_base.DbTestCase): do_agent_iscsi_deploy_mock, configure_vmedia_boot_mock, reboot_and_finish_deploy_mock, - keystone_mock, boot_mode_cap_mock, update_secure_boot_mock): self.node.provision_state = states.DEPLOYWAIT @@ -1009,7 +1006,6 @@ class VendorPassthruTestCase(db_base.DbTestCase): self.node.save() do_agent_iscsi_deploy_mock.return_value = { 'root uuid': 'some-root-uuid'} - keystone_mock.return_value = 'admin-token' with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.vendor.continue_deploy(task) @@ -1022,8 +1018,6 @@ class VendorPassthruTestCase(db_base.DbTestCase): update_secure_boot_mock.assert_called_once_with(task, True) reboot_and_finish_deploy_mock.assert_called_once_with( mock.ANY, task) - # Ensure that admin token is populated in task - self.assertEqual('admin-token', task.context.auth_token) @mock.patch.object(ilo_deploy, '_update_secure_boot_mode', autospec=True) @mock.patch.object(ilo_common, 'update_boot_mode', autospec=True) diff --git a/ironic/tests/drivers/test_pxe.py b/ironic/tests/drivers/test_pxe.py index 65a303be36..fc26cd2232 100644 --- a/ironic/tests/drivers/test_pxe.py +++ b/ironic/tests/drivers/test_pxe.py @@ -151,6 +151,7 @@ class PXEPrivateMethodsTestCase(db_base.DbTestCase): self.node.uuid, 'deploy_kernel'))} show_mock.return_value = properties + self.context.auth_token = 'fake' image_info = pxe._get_image_info(self.node, self.context) show_mock.assert_called_once_with(mock.ANY, 'glance://image_uuid', method='get') diff --git a/ironic/tests/test_glance_service.py b/ironic/tests/test_glance_service.py index 3bde77d1df..498409d047 100644 --- a/ironic/tests/test_glance_service.py +++ b/ironic/tests/test_glance_service.py @@ -646,6 +646,7 @@ class TestGlanceSwiftTempURL(base.TestCase): super(TestGlanceSwiftTempURL, self).setUp() client = stubs.StubGlanceClient() self.context = context.RequestContext() + self.context.auth_token = 'fake' self.service = service.GlanceImageService(client, 2, self.context) self.config(swift_temp_url_key='correcthorsebatterystaple', group='glance') diff --git a/ironic/tests/test_image_service.py b/ironic/tests/test_image_service.py index dac0773411..b887cbc550 100644 --- a/ironic/tests/test_image_service.py +++ b/ironic/tests/test_image_service.py @@ -22,6 +22,7 @@ import six.moves.builtins as __builtin__ from ironic.common import exception from ironic.common.glance_service.v1 import image_service as glance_v1_service from ironic.common import image_service +from ironic.common import keystone from ironic.tests import base if six.PY3: @@ -227,21 +228,57 @@ class FileImageServiceTestCase(base.TestCase): class ServiceGetterTestCase(base.TestCase): + @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) @mock.patch.object(glance_v1_service.GlanceImageService, '__init__', return_value=None, autospec=True) - def test_get_glance_image_service(self, glance_service_mock): + def test_get_glance_image_service(self, glance_service_mock, token_mock): image_href = 'image-uuid' + self.context.auth_token = 'fake' image_service.get_image_service(image_href, context=self.context) glance_service_mock.assert_called_once_with(mock.ANY, None, 1, self.context) + self.assertFalse(token_mock.called) + @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) @mock.patch.object(glance_v1_service.GlanceImageService, '__init__', return_value=None, autospec=True) - def test_get_glance_image_service_url(self, glance_service_mock): + def test_get_glance_image_service_url(self, glance_service_mock, + token_mock): image_href = 'glance://image-uuid' + self.context.auth_token = 'fake' image_service.get_image_service(image_href, context=self.context) glance_service_mock.assert_called_once_with(mock.ANY, None, 1, self.context) + self.assertFalse(token_mock.called) + + @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) + @mock.patch.object(glance_v1_service.GlanceImageService, '__init__', + return_value=None, autospec=True) + def test_get_glance_image_service_no_token(self, glance_service_mock, + token_mock): + image_href = 'image-uuid' + self.context.auth_token = None + token_mock.return_value = 'admin-token' + image_service.get_image_service(image_href, context=self.context) + glance_service_mock.assert_called_once_with(mock.ANY, None, 1, + self.context) + token_mock.assert_called_once_with() + self.assertEqual('admin-token', self.context.auth_token) + + @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) + @mock.patch.object(glance_v1_service.GlanceImageService, '__init__', + return_value=None, autospec=True) + def test_get_glance_image_service_token_not_needed(self, + glance_service_mock, + token_mock): + image_href = 'image-uuid' + self.context.auth_token = None + self.config(auth_strategy='noauth', group='glance') + image_service.get_image_service(image_href, context=self.context) + glance_service_mock.assert_called_once_with(mock.ANY, None, 1, + self.context) + self.assertFalse(token_mock.called) + self.assertIsNone(self.context.auth_token) @mock.patch.object(image_service.HttpImageService, '__init__', return_value=None, autospec=True)