From bee0a5c54aed343ae5c11fd5f2adfcd71d30211d Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 19 Jul 2024 15:26:51 +0200 Subject: [PATCH] Use client token when talking to manila When a user asks nova to attach a manila share to the user's instance nova should use the user's keystone token to talk to manila so that manila can verify if the user has access to the share. Manila is the OpenStack Shared Filesystems service. These series of patches implement changes required in Nova to allow the shares provided by Manila to be associated with and attached to instances using virtiofs. Implements: blueprint libvirt-virtiofs-attach-manila-shares Change-Id: I793f0518dcfffd2286e7bc685cab76794aece91d --- nova/api/openstack/compute/baremetal_nodes.py | 1 + nova/cmd/manage.py | 2 +- nova/scheduler/client/report.py | 3 +- nova/share/manila.py | 14 ++- nova/tests/fixtures/nova.py | 4 +- nova/tests/unit/test_manila.py | 117 ++++++++++++++++-- nova/tests/unit/test_utils.py | 16 ++- nova/utils.py | 40 +++++- nova/virt/ironic/driver.py | 2 +- 9 files changed, 164 insertions(+), 35 deletions(-) diff --git a/nova/api/openstack/compute/baremetal_nodes.py b/nova/api/openstack/compute/baremetal_nodes.py index b68e2cf6a3..1aa7d752a6 100644 --- a/nova/api/openstack/compute/baremetal_nodes.py +++ b/nova/api/openstack/compute/baremetal_nodes.py @@ -56,6 +56,7 @@ class BareMetalNodeController(wsgi.Controller): # up to handle this and raise VirtDriverNotReady as appropriate. self._ironic_connection = utils.get_sdk_adapter( 'baremetal', + admin=True, check_service=True, ) return self._ironic_connection diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index efd8c0ffd5..8b9bb51a95 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -3753,7 +3753,7 @@ class LimitsCommands(): conf_utils.register_ksa_opts( CONF, 'keystone_authtoken', 'identity') keystone_api = utils.get_sdk_adapter( - 'identity', conf_group='keystone_authtoken') + 'identity', admin=True, conf_group='keystone_authtoken') # Service ID is required in unified limits APIs. service_id = keystone_api.find_service('nova').id diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index ec1edbc637..ba42277ebf 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -274,7 +274,8 @@ class SchedulerReportClient(object): """Create the HTTP session accessing the placement service.""" # Flush provider tree and associations so we start from a clean slate. self.clear_provider_cache(init=True) - client = self._adapter or utils.get_sdk_adapter('placement') + client = self._adapter or utils.get_sdk_adapter( + "placement", admin=True) # Set accept header on every request to ensure we notify placement # service of our response body media type preferences. client.additional_headers = {'accept': 'application/json'} diff --git a/nova/share/manila.py b/nova/share/manila.py index e0e8b55aa2..a2f37fb0f2 100644 --- a/nova/share/manila.py +++ b/nova/share/manila.py @@ -30,7 +30,7 @@ LOG = logging.getLogger(__name__) MIN_SHARE_FILE_SYSTEM_MICROVERSION = "2.82" -def manilaclient(context): +def _manilaclient(context, admin=False): """Constructs a manila client object for making API requests. :return: An openstack.proxy.Proxy object for the specified service_type. @@ -41,7 +41,9 @@ def manilaclient(context): return utils.get_sdk_adapter( "shared-file-system", + admin=admin, check_service=True, + context=context, shared_file_system_api_version=MIN_SHARE_FILE_SYSTEM_MICROVERSION, global_request_id=context.global_id ) @@ -208,7 +210,7 @@ class API(object): paths.append(export_location.path) return paths[0] - client = manilaclient(context) + client = _manilaclient(context, admin=False) LOG.debug("Get share id:'%s' data from manila", share_id) share = client.get_share(share_id) export_locations = client.export_locations(share.id) @@ -236,7 +238,8 @@ class API(object): LOG.debug("Get share access id for share id:'%s'", share_id) - access_list = manilaclient(context).access_rules(share_id) + access_list = _manilaclient( + context, admin=True).access_rules(share_id) for access in access_list: if ( @@ -279,7 +282,7 @@ class API(object): LOG.debug("Allow host access to share id:'%s'", share_id) - access = manilaclient(context).create_access_rule( + access = _manilaclient(context, admin=True).create_access_rule( share_id, access_type=access_type, access_to=access_to, @@ -311,8 +314,6 @@ class API(object): respond with a status code 202. """ - client = manilaclient(context) - access = self.get_access( context, share_id, @@ -321,6 +322,7 @@ class API(object): ) if access: + client = _manilaclient(context, admin=True) LOG.debug("Deny host access to share id:'%s'", share_id) resp = client.delete_access_rule(access.id, share_id) if resp.status_code != 202: diff --git a/nova/tests/fixtures/nova.py b/nova/tests/fixtures/nova.py index f0164a3902..56ee615104 100644 --- a/nova/tests/fixtures/nova.py +++ b/nova/tests/fixtures/nova.py @@ -2044,10 +2044,10 @@ class UnifiedLimitsFixture(fixtures.Fixture): self.mock_sdk_adapter = mock.Mock() real_get_sdk_adapter = utils.get_sdk_adapter - def fake_get_sdk_adapter(service_type, **kwargs): + def fake_get_sdk_adapter(service_type, admin, **kwargs): if service_type == 'identity': return self.mock_sdk_adapter - return real_get_sdk_adapter(service_type, **kwargs) + return real_get_sdk_adapter(service_type, admin, **kwargs) self.useFixture(fixtures.MockPatch( 'nova.utils.get_sdk_adapter', fake_get_sdk_adapter)) diff --git a/nova/tests/unit/test_manila.py b/nova/tests/unit/test_manila.py index 7dc9ad890d..9993dff8f4 100644 --- a/nova/tests/unit/test_manila.py +++ b/nova/tests/unit/test_manila.py @@ -13,6 +13,8 @@ from requests import Response import fixtures +from keystoneauth1 import loading as ks_loading + import nova.conf from nova import context as nova_context from nova import exception @@ -156,9 +158,9 @@ class BaseManilaTestCase(object): self.mock_get_confgrp = self.useFixture(fixtures.MockPatch( 'nova.utils._get_conf_group')).mock - self.mock_get_auth_sess = self.useFixture(fixtures.MockPatch( - 'nova.utils._get_auth_and_session')).mock - self.mock_get_auth_sess.return_value = (None, mock.sentinel.session) + self.mock_ks_loading = self.useFixture( + fixtures.MockPatchObject(ks_loading, 'load_auth_from_conf_options') + ).mock self.service_type = 'shared-file-system' self.mock_connection = self.useFixture( @@ -223,16 +225,39 @@ class BaseManilaTestCase(object): return FakeConnection() - def create_client(self, context): - return manila.manilaclient(context) - - def test_client(self): - client = self.create_client(self.context) + @mock.patch('nova.utils.get_sdk_adapter') + def test_client(self, mock_get_sdk_adapter): + client = manila._manilaclient(self.context) self.assertTrue(hasattr(client, 'get_share')) self.assertTrue(hasattr(client, 'export_locations')) self.assertTrue(hasattr(client, 'access_rules')) self.assertTrue(hasattr(client, 'create_access_rule')) self.assertTrue(hasattr(client, 'delete_access_rule')) + mock_get_sdk_adapter.assert_called_once_with( + "shared-file-system", + admin=False, + check_service=True, + context=self.context, + shared_file_system_api_version="2.82", + global_request_id=self.context.global_id, + ) + + @mock.patch('nova.utils.get_sdk_adapter') + def test_client_admin(self, mock_get_sdk_adapter): + client = manila._manilaclient(self.context, admin=True) + self.assertTrue(hasattr(client, 'get_share')) + self.assertTrue(hasattr(client, 'export_locations')) + self.assertTrue(hasattr(client, 'access_rules')) + self.assertTrue(hasattr(client, 'create_access_rule')) + self.assertTrue(hasattr(client, 'delete_access_rule')) + mock_get_sdk_adapter.assert_called_once_with( + "shared-file-system", + admin=True, + check_service=True, + context=self.context, + shared_file_system_api_version="2.82", + global_request_id=self.context.global_id, + ) class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): @@ -246,10 +271,20 @@ class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): self.assertIn("Share nonexisting could not be found.", exc.message) - def test_get_share(self): + @mock.patch( + 'nova.utils.get_sdk_adapter', side_effect=nova.utils.get_sdk_adapter) + def test_get_share(self, mock_get_sdk_adapter): """Tests that we manage to get a share. """ share = self.api.get(self.context, '1234') + mock_get_sdk_adapter.assert_called_once_with( + "shared-file-system", + admin=False, + check_service=True, + context=self.context, + shared_file_system_api_version="2.82", + global_request_id=self.context.global_id, + ) self.assertIsInstance(share, manila.Share) self.assertEqual('1234', share.id) self.assertEqual(1, share.size) @@ -304,12 +339,21 @@ class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): self.assertIn("Share nonexisting2 could not be found.", exc.message) - def test_get_access(self): + @mock.patch( + 'nova.utils.get_sdk_adapter', side_effect=nova.utils.get_sdk_adapter) + def test_get_access(self, mock_get_sdk_adapter): """Tests that we manage to get an access id based on access_type and access_to parameters. """ access = self.api.get_access(self.context, '1234', 'ip', '0.0.0.0/0') - + mock_get_sdk_adapter.assert_called_once_with( + "shared-file-system", + admin=True, + check_service=True, + context=self.context, + shared_file_system_api_version="2.82", + global_request_id=self.context.global_id, + ) self.assertEqual('a25b2df3-90bd-4add-afa6-5f0dbbd50452', access.id) self.assertEqual('rw', access.access_level) self.assertEqual('active', access.state) @@ -342,10 +386,20 @@ class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): self.assertIn("Share nonexisting could not be found.", exc.message) - def test_allow_access(self): + @mock.patch( + 'nova.utils.get_sdk_adapter', side_effect=nova.utils.get_sdk_adapter) + def test_allow_access(self, mock_get_sdk_adapter): """Tests that we manage to allow access to a share. """ access = self.api.allow(self.context, '1234', 'ip', '0.0.0.0/0', 'rw') + mock_get_sdk_adapter.assert_called_once_with( + "shared-file-system", + admin=True, + check_service=True, + context=self.context, + shared_file_system_api_version="2.82", + global_request_id=self.context.global_id, + ) self.assertEqual('a25b2df3-90bd-4add-afa6-5f0dbbd50452', access.id) self.assertEqual('rw', access.access_level) self.assertEqual('active', access.state) @@ -385,7 +439,9 @@ class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): self.assertIn("Share nonexisting could not be found.", exc.message) - def test_deny_access(self): + @mock.patch( + 'nova.utils.get_sdk_adapter', side_effect=nova.utils.get_sdk_adapter) + def test_deny_access(self, mock_get_sdk_adapter): """Tests that we manage to deny access to a share. """ self.api.deny( @@ -394,6 +450,41 @@ class ManilaTestCase(BaseManilaTestCase, test.NoDBTestCase): 'ip', '0.0.0.0/0' ) + self.assertEqual(2, mock_get_sdk_adapter.call_count) + + self.assertEqual( + mock_get_sdk_adapter.call_args_list[0].args, + ( + "shared-file-system", + ), + ) + self.assertEqual( + mock_get_sdk_adapter.call_args_list[0].kwargs, + { + "admin": True, + "check_service": True, + "context": self.context, + "shared_file_system_api_version": "2.82", + "global_request_id": self.context.global_id, + }, + ) + + self.assertEqual( + mock_get_sdk_adapter.call_args_list[1].args, + ( + "shared-file-system", + ), + ) + self.assertEqual( + mock_get_sdk_adapter.call_args_list[1].kwargs, + { + "admin": True, + "check_service": True, + "context": self.context, + "shared_file_system_api_version": "2.82", + "global_request_id": self.context.global_id, + }, + ) def test_deny_access_fails_id_missing(self): """Tests that we fail if something wrong happens calling deny method. diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index f6372ce450..1ee8ba9365 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -1176,8 +1176,10 @@ class TestGetSDKAdapter(test.NoDBTestCase): self.mock_conf = self.useFixture(fixtures.MockPatch( 'nova.utils.CONF')).mock - def _test_get_sdk_adapter(self, strict=False): - actual = utils.get_sdk_adapter(self.service_type, check_service=strict) + def _test_get_sdk_adapter(self, admin, strict=False): + actual = utils.get_sdk_adapter( + self.service_type, admin, check_service=strict + ) self.mock_get_confgrp.assert_called_once_with(self.service_type) self.mock_get_auth_sess.assert_called_once_with( @@ -1189,24 +1191,26 @@ class TestGetSDKAdapter(test.NoDBTestCase): return actual def test_get_sdk_adapter(self): - self.assertEqual(self._test_get_sdk_adapter(), mock.sentinel.proxy) + self.assertEqual( + self._test_get_sdk_adapter(admin=True), mock.sentinel.proxy + ) def test_get_sdk_adapter_strict(self): self.assertEqual( - self._test_get_sdk_adapter(strict=True), mock.sentinel.proxy) + self._test_get_sdk_adapter(True, strict=True), mock.sentinel.proxy) def test_get_sdk_adapter_strict_fail(self): self.mock_connection.side_effect = sdk_exc.ServiceDiscoveryException() self.assertRaises( exception.ServiceUnavailable, - self._test_get_sdk_adapter, strict=True) + self._test_get_sdk_adapter, True, strict=True) def test_get_sdk_adapter_conf_group_fail(self): self.mock_get_confgrp.side_effect = ( exception.ConfGroupForServiceTypeNotFound(stype=self.service_type)) self.assertRaises(exception.ConfGroupForServiceTypeNotFound, - utils.get_sdk_adapter, self.service_type) + utils.get_sdk_adapter, self.service_type, True) self.mock_get_confgrp.assert_called_once_with(self.service_type) self.mock_connection.assert_not_called() self.mock_get_auth_sess.assert_not_called() diff --git a/nova/utils.py b/nova/utils.py index b85cf0cdf7..b6b3785331 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -966,7 +966,8 @@ def get_ksa_adapter(service_type, ksa_auth=None, ksa_session=None, def get_sdk_adapter( - service_type, check_service=False, conf_group=None, **kwargs + service_type, admin, check_service=False, conf_group=None, + context=None, **kwargs ): """Construct an openstacksdk-brokered Adapter for a given service type. @@ -976,10 +977,13 @@ def get_sdk_adapter( :param service_type: String name of the service type for which the Adapter is to be constructed. + :param admin: If set to true, the service will use Nova's service user + and password; otherwise, it will use the user's token. :param check_service: If True, we will query the endpoint to make sure the service is alive, raising ServiceUnavailable if it is not. :param conf_group: String name of the conf group to use, otherwise the name of the service_type will be used. + :param context: Use to get user's token, if admin is set to False. :param kwargs: Additional arguments to pass to the Adapter constructor. Mainly used to pass microversion to a specific service, e.g. shared_file_system_api_version="2.82". @@ -989,11 +993,37 @@ def get_sdk_adapter( :raise: ServiceUnavailable if check_service is True and the service is down """ confgrp = conf_group or _get_conf_group(service_type) - sess = _get_auth_and_session(confgrp)[1] + try: - conn = connection.Connection( - session=sess, oslo_conf=CONF, service_types={service_type}, - strict_proxies=check_service, **kwargs) + if admin is False: + if context is None: + raise ValueError( + "If admin is set to False then context cannot be None.") + + # NOTE(gibi): this is only needed to make sure + # CONF.service_user.auth_url config is registered + ks_loading.load_auth_from_conf_options( + CONF, nova.conf.service_token.service_user.name) + + # Create a connection using the user's token instead of nova's + # service user/pass. + conn = connection.Connection( + token=context.auth_token, + auth_type="v3token", + project_id=context.project_id, + project_domain_id=context.project_domain_id, + auth_url=CONF.service_user.auth_url, + service_types={service_type}, + strict_proxies=check_service, + **kwargs, + ) + else: + # Create a connection based on nova's service user/pass + sess = _get_auth_and_session(confgrp)[1] + conn = connection.Connection( + session=sess, oslo_conf=CONF, service_types={service_type}, + strict_proxies=check_service, **kwargs) + except sdk_exc.ServiceDiscoveryException as e: raise exception.ServiceUnavailable( _("The %(service_type)s service is unavailable: %(error)s") % diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index d3c7d191bb..0040beeb1f 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -198,7 +198,7 @@ class IronicDriver(virt_driver.ComputeDriver): # service isn't ready yet. Consumers of ironic_connection are set # up to handle this and raise VirtDriverNotReady as appropriate. self._ironic_connection = utils.get_sdk_adapter( - 'baremetal', check_service=True) + 'baremetal', admin=True, check_service=True) return self._ironic_connection def _get_node(self, node_id):