From 3c7266effc73ce93672d76b421913274d3c2787e Mon Sep 17 00:00:00 2001 From: Fabian Koller Date: Mon, 29 Jun 2026 13:11:20 +0200 Subject: [PATCH 1/3] volume: preserve manage_existing recovery stages Let drivers signal safe pre-relocate failures by setting volume status to 'error' so the manage_existing flow preserves that state on rollback instead of always reverting to 'error_managing'. Add recovery logging in ManageExistingTask when the DB update cannot be persisted after a driver has already adopted the backend object. Change-Id: I4d060160561fd8df4e1869194a14934edf40f060 --- .../volume/flows/test_manage_volume_flow.py | 36 +++++++++++++++++++ .../volume/flows/manager/manage_existing.py | 7 ++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/cinder/tests/unit/volume/flows/test_manage_volume_flow.py b/cinder/tests/unit/volume/flows/test_manage_volume_flow.py index ba576196114..fb0c5425190 100644 --- a/cinder/tests/unit/volume/flows/test_manage_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_manage_volume_flow.py @@ -166,6 +166,21 @@ def test_prepare_for_quota_reservation_task_revert(self): reason='Volume manage failed.', status='error_managing') + def test_prepare_for_quota_reservation_task_revert_preserves_error(self): + mock_db = mock.MagicMock() + mock_driver = mock.MagicMock() + mock_result = mock.MagicMock() + mock_flow_failures = mock.MagicMock() + mock_error_out = self.mock_object(flow_common, 'error_out') + volume_ref = self._stub_volume_object_get(self) + volume_ref.status = 'error' + task = manager.PrepareForQuotaReservationTask(mock_db, mock_driver) + + task.revert(self.ctxt, mock_result, mock_flow_failures, volume_ref) + mock_error_out.assert_called_once_with(volume_ref, + reason='Volume manage failed.', + status='error') + def test_prepare_for_quota_reservation_with_wrong_volume(self): """Test the class PrepareForQuotaReservationTas with wrong vol.""" mock_db = mock.MagicMock() @@ -182,6 +197,27 @@ def test_prepare_for_quota_reservation_with_wrong_volume(self): wrong_volume, mock_manage_existing_ref) + def test_prepare_for_quota_reservation_execute_preserves_error(self): + mock_db = mock.MagicMock() + mock_driver = mock.MagicMock() + mock_manage_existing_ref = mock.MagicMock() + mock_error_out = self.mock_object(flow_common, 'error_out') + volume_ref = self._stub_volume_object_get(self) + volume_ref.status = 'error' + mock_driver.manage_existing_get_size.side_effect = ( + exception.CinderException) + task = manager.PrepareForQuotaReservationTask(mock_db, mock_driver) + + self.assertRaises(exception.CinderException, + task.execute, + self.ctxt, + volume_ref, + mock_manage_existing_ref) + mock_error_out.assert_called_once_with( + volume_ref, + 'Volume driver MagicMock get exception.', + status='error') + def test_manage_existing_task(self): """Test the class ManageExistingTask.""" mock_db = mock.MagicMock() diff --git a/cinder/volume/flows/manager/manage_existing.py b/cinder/volume/flows/manager/manage_existing.py index 654d200ff87..2a4d896dae6 100644 --- a/cinder/volume/flows/manager/manage_existing.py +++ b/cinder/volume/flows/manager/manage_existing.py @@ -57,8 +57,10 @@ def execute(self, context, volume, manage_existing_ref): except Exception: with excutils.save_and_reraise_exception(): reason = _("Volume driver %s get exception.") % driver_name + status = ('error' if volume.status == 'error' + else 'error_managing') flow_common.error_out(volume, reason, - status='error_managing') + status=status) return {'size': size, 'volume_type_id': volume.volume_type_id, @@ -69,8 +71,9 @@ def execute(self, context, volume, manage_existing_ref): def revert(self, context, result, flow_failures, volume, **kwargs): reason = _('Volume manage failed.') + status = 'error' if volume.status == 'error' else 'error_managing' flow_common.error_out(volume, reason=reason, - status='error_managing') + status=status) LOG.error("Volume %s: manage failed.", volume.id) From 95ab088e5f81e4c807711fc76b344b991f0d2d48 Mon Sep 17 00:00:00 2001 From: Fabian Koller Date: Mon, 29 Jun 2026 13:11:26 +0200 Subject: [PATCH 2/3] vmware.fcd: adopt detached VMDKs as FCD volumes Add manage_existing and manage_existing_get_size overrides to VMwareVStorageObjectDriver. Parses the '[ds] path.vmdk' source-name ref, validates caller-provided size_gb, registers the VMDK as a First Class Disk via RegisterDisk, and relocates it to the target datastore when source and target moref differ. All pre-relocate failures mark the volume as 'error' (safe for Nova abort); relocate and post-relocate failures leave status at the flow-owned 'error_managing'. No error path calls delete_fcd or unregister_disk. Change-Id: Ic7faba0b8aefaff1472a5ff0d380547c856f1a63 --- .../unit/volume/drivers/vmware/test_fcd.py | 727 ++++++++++++++++++ .../drivers/vmware/test_vmware_volumeops.py | 17 + cinder/volume/drivers/vmware/fcd.py | 190 +++++ 3 files changed, 934 insertions(+) diff --git a/cinder/tests/unit/volume/drivers/vmware/test_fcd.py b/cinder/tests/unit/volume/drivers/vmware/test_fcd.py index 0748b206c91..c4af2a150d7 100644 --- a/cinder/tests/unit/volume/drivers/vmware/test_fcd.py +++ b/cinder/tests/unit/volume/drivers/vmware/test_fcd.py @@ -20,6 +20,7 @@ import ddt from oslo_utils import timeutils from oslo_utils import units +from oslo_vmware import exceptions as vexc from oslo_vmware import image_transfer from oslo_vmware.objects import datastore from oslo_vmware import vim_util @@ -853,3 +854,729 @@ def test_retype( ds_sel.get_profile_id.assert_called_once_with(new_profile) vops.update_fcd_policy.assert_called_once_with( fcd_loc, new_profile_id.uniqueId) + + def test_manage_existing_get_size(self): + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk', 'size_gb': 64} + size = self._driver.manage_existing_get_size( + mock.sentinel.volume, ref) + self.assertEqual(64, size) + + def test_manage_existing_get_size_string_coercion(self): + """size_gb as string is coerced to int.""" + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': '128'} + size = self._driver.manage_existing_get_size( + mock.sentinel.volume, ref) + self.assertEqual(128, size) + + def test_manage_existing_get_size_missing_size_gb(self): + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk'} + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver.manage_existing_get_size, + volume, ref) + self.assertEqual('error', volume.status) + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + def test_manage_existing_get_size_invalid_size_gb(self, mock_log): + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk', + 'size_gb': 'not-an-int'} + + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver.manage_existing_get_size, + volume, ref) + + self.assertEqual('error', volume.status) + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('not-an-int', warning_args) + + def test_parse_manage_existing_ref_valid(self): + ref = {'source-name': '[eph_ds02] uuid/uuid.vmdk', 'size_gb': 64} + ds_name, rel_path = self._driver._parse_manage_existing_ref(ref) + self.assertEqual('eph_ds02', ds_name) + self.assertEqual('uuid/uuid.vmdk', rel_path) + + def test_parse_manage_existing_ref_missing_source_name(self): + ref = {'size_gb': 64} + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver._parse_manage_existing_ref, ref) + + def test_parse_manage_existing_ref_invalid_format(self): + ref = {'source-name': 'no-brackets-path.vmdk', 'size_gb': 64} + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver._parse_manage_existing_ref, ref) + + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_same_datastore( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name): + """Same-datastore: no relocate, provider_location from source.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk', 'size_gb': 64} + + # Source datastore lookup + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + # DC and inventory path + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + # register_disk returns FcdLocation + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-id-123' + fcd_loc.ds_ref_val = 'datastore-101' + fcd_loc.provider_location.return_value = ( + 'fcd-id-123@datastore-101') + vops.register_disk.return_value = fcd_loc + + # Target datastore is same as source + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-101' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.return_value = 'fcd-id-123@ds1' + + result = self._driver.manage_existing(volume, ref) + + # Verify datastore lookup + ds_sel.get_ds_ref_by_name.assert_called_once_with('ds1') + vops.get_dc.assert_called_once_with(src_ds_ref) + vops.get_inventory_path.assert_called_once_with(dc_ref) + # Verify register_disk call with exact URL + vops.register_disk.assert_called_once() + reg_args = vops.register_disk.call_args + self.assertIn('vm-uuid/vm-uuid.vmdk', reg_args[0][0]) + self.assertEqual(volume.name, reg_args[0][1]) + self.assertEqual(src_ds_ref, reg_args[0][2]) + # Same datastore: no relocate + vops.relocate_fcd.assert_not_called() + # provider_location converted from source fcd_loc + prov_loc_to_name.assert_called_once_with( + 'fcd-id-123@datastore-101') + self.assertEqual({'provider_location': 'fcd-id-123@ds1'}, result) + + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_different_datastore_relocates( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name): + """Cross-datastore: relocate called, provider_location uses target.""" + volume = self._create_volume_obj() + ref = {'source-name': '[src_ds] vm-uuid/vm-uuid.vmdk', 'size_gb': 64} + + # Source datastore + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + # DC + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + # register_disk + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-123' + fcd_loc.provider_location.return_value = 'fcd-123@datastore-101' + vops.register_disk.return_value = fcd_loc + + # Target datastore is DIFFERENT + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.return_value = 'fcd-123@nfs_target' + + result = self._driver.manage_existing(volume, ref) + + vops.relocate_fcd.assert_called_once_with( + fcd_loc, target_ds_ref, volume.name) + # provider_location must use TARGET moref, not source + prov_loc_to_name.assert_called_once_with( + 'fcd-123@datastore-202') + self.assertEqual({'provider_location': 'fcd-123@nfs_target'}, result) + + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_nfs_different_datastore_relocates( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name): + """NFS source on different datastore still triggers relocate.""" + volume = self._create_volume_obj() + ref = {'source-name': '[nfs_ds] vm-uuid/vm-uuid.vmdk', 'size_gb': 32} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-nfs-1' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-456' + fcd_loc.provider_location.return_value = 'fcd-456@datastore-nfs-1' + vops.register_disk.return_value = fcd_loc + + # Different target + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-789' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.return_value = 'fcd-456@nfs_target' + + result = self._driver.manage_existing(volume, ref) + + vops.relocate_fcd.assert_called_once_with( + fcd_loc, target_ds_ref, volume.name) + # provider_location must use TARGET moref + prov_loc_to_name.assert_called_once_with( + 'fcd-456@datastore-789') + self.assertEqual({'provider_location': 'fcd-456@nfs_target'}, result) + + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_id_same_datastore_reuses_fcd( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name): + """source-id means the VMDK is already an FCD.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk', + 'source-id': 'existing-fcd-123', + 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + vops.get_vmdk_path_for_fcd.return_value = ( + '[ds1] vm-uuid/vm-uuid.vmdk') + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-101' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.return_value = 'existing-fcd-123@ds1' + + result = self._driver.manage_existing(volume, ref) + + vops.get_dc.assert_not_called() + vops.get_inventory_path.assert_not_called() + vops.register_disk.assert_not_called() + vops.rename_fcd.assert_called_once() + rename_args = vops.rename_fcd.call_args[0] + self.assertEqual('existing-fcd-123', rename_args[0].fcd_id) + self.assertEqual('datastore-101', rename_args[0].ds_ref_val) + self.assertEqual(src_ds_ref, rename_args[1]) + self.assertEqual(volume.name, rename_args[2]) + vops.relocate_fcd.assert_not_called() + prov_loc_to_name.assert_called_once_with( + 'existing-fcd-123@datastore-101') + self.assertEqual({'provider_location': 'existing-fcd-123@ds1'}, + result) + + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_id_different_datastore_relocates( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name): + """Existing FCDs are relocated like newly-registered FCDs.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm-uuid/vm-uuid.vmdk', + 'source-id': 'existing-fcd-456', + 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + vops.get_vmdk_path_for_fcd.return_value = ( + '[ds1] vm-uuid/vm-uuid.vmdk') + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.return_value = 'existing-fcd-456@nfs_target' + + result = self._driver.manage_existing(volume, ref) + + vops.register_disk.assert_not_called() + vops.rename_fcd.assert_called_once() + renamed_loc = vops.rename_fcd.call_args[0][0] + self.assertEqual('existing-fcd-456', renamed_loc.fcd_id) + self.assertEqual('datastore-101', renamed_loc.ds_ref_val) + vops.relocate_fcd.assert_called_once() + relocate_args = vops.relocate_fcd.call_args[0] + self.assertEqual('existing-fcd-456', relocate_args[0].fcd_id) + self.assertEqual('datastore-101', relocate_args[0].ds_ref_val) + self.assertEqual(target_ds_ref, relocate_args[1]) + self.assertEqual(volume.name, relocate_args[2]) + prov_loc_to_name.assert_called_once_with( + 'existing-fcd-456@datastore-202') + self.assertEqual({'provider_location': 'existing-fcd-456@nfs_target'}, + result) + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_id_rename_failure_no_cleanup( + self, ds_sel, vops, select_ds_fcd, mock_log): + """rename_fcd failure must not register, relocate, or delete.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', + 'source-id': 'existing-fcd-789', + 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + vops.get_vmdk_path_for_fcd.return_value = '[ds1] vm/vm.vmdk' + vops.rename_fcd.side_effect = vexc.VimException('rename failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + vops.register_disk.assert_not_called() + vops.relocate_fcd.assert_not_called() + vops.delete_fcd.assert_not_called() + select_ds_fcd.assert_not_called() + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('existing-fcd-789', warning_args) + self.assertIn('vm/vm.vmdk', warning_args) + self.assertIn('rename failed', warning_args) + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_id_path_mismatch_no_cleanup( + self, ds_sel, vops, select_ds_fcd, mock_log): + """source-id must refer to the source-name VMDK.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', + 'source-id': 'wrong-fcd-id', + 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + vops.get_vmdk_path_for_fcd.return_value = '[ds1] other/other.vmdk' + + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + vops.rename_fcd.assert_not_called() + vops.register_disk.assert_not_called() + vops.relocate_fcd.assert_not_called() + vops.delete_fcd.assert_not_called() + select_ds_fcd.assert_not_called() + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('wrong-fcd-id', warning_args) + self.assertIn('vm/vm.vmdk', warning_args) + self.assertIn('other/other.vmdk', warning_args) + + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_register_failure_no_cleanup( + self, ds_sel, vops, select_ds_fcd): + """register_disk failure must not call delete_fcd or unregister.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + vops.register_disk.side_effect = vexc.VimException( + 'RegisterDisk failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + vops.delete_fcd.assert_not_called() + select_ds_fcd.assert_not_called() + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_already_registered_requires_source_id( + self, ds_sel, vops, select_ds_fcd, mock_log): + """Already-registered VMDKs need source-id from Nova.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + vops.register_disk.side_effect = vexc.AlreadyExistsException( + 'Already exists') + + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + select_ds_fcd.assert_not_called() + vops.delete_fcd.assert_not_called() + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('source-id', warning_args) + self.assertIn('vm/vm.vmdk', warning_args) + + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_relocate_failure_preserves_fcd( + self, ds_sel, vops, select_ds_fcd): + """relocate_fcd failure must not delete the registered FCD.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.provider_location.return_value = 'fcd-123@datastore-101' + fcd_loc.fcd_id = 'fcd-123' + vops.register_disk.return_value = fcd_loc + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + + vops.relocate_fcd.side_effect = vexc.VimException('Relocate failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + vops.delete_fcd.assert_not_called() + + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_ds_not_found_exception( + self, ds_sel, vops, select_ds_fcd): + """Backend exception from datastore lookup stays retryable.""" + volume = self._create_volume_obj() + ref = {'source-name': '[unknown_ds] vm/vm.vmdk', 'size_gb': 64} + + ds_sel.get_ds_ref_by_name.side_effect = vexc.VimException( + 'vCenter unavailable') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + vops.register_disk.assert_not_called() + vops.delete_fcd.assert_not_called() + + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_ds_returns_none( + self, ds_sel, vops, select_ds_fcd): + """get_ds_ref_by_name returning None raises InvalidReference.""" + volume = self._create_volume_obj() + ref = {'source-name': '[gone_ds] vm/vm.vmdk', 'size_gb': 64} + + ds_sel.get_ds_ref_by_name.return_value = None + + self.assertRaises( + cinder_exceptions.ManageExistingInvalidReference, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + vops.register_disk.assert_not_called() + vops.delete_fcd.assert_not_called() + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_source_url_failure_logs_warning( + self, ds_sel, vops, select_ds_fcd, mock_log): + """Failure before RegisterDisk logs and marks volume error.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + vops.get_dc.side_effect = vexc.VimException('dc lookup failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('vm/vm.vmdk', warning_args) + self.assertIn('dc lookup failed', warning_args) + vops.register_disk.assert_not_called() + vops.delete_fcd.assert_not_called() + + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_select_ds_failure_after_register( + self, ds_sel, vops, select_ds_fcd): + """_select_ds_fcd failure after register must not cleanup FCD.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-888' + fcd_loc.provider_location.return_value = 'fcd-888@datastore-101' + vops.register_disk.return_value = fcd_loc + + select_ds_fcd.side_effect = vexc.VimException( + 'No suitable datastore') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + vops.register_disk.assert_called_once() + vops.delete_fcd.assert_not_called() + vops.relocate_fcd.assert_not_called() + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch('cinder.volume.drivers.vmware.fcd.vim_util.get_moref_value') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_moref_compare_failure_logs_warning( + self, ds_sel, vops, select_ds_fcd, get_moref_value, mock_log): + """Moref comparison failure after register logs and keeps FCD.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-333' + fcd_loc.provider_location.return_value = 'fcd-333@datastore-101' + vops.register_disk.return_value = fcd_loc + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + get_moref_value.side_effect = vexc.VimException('moref failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('error', volume.status) + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('fcd-333', warning_args) + self.assertIn('moref failed', warning_args) + vops.delete_fcd.assert_not_called() + vops.relocate_fcd.assert_not_called() + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, + '_provider_location_to_ds_name_location') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_post_relocate_provider_update_failure( + self, ds_sel, vops, select_ds_fcd, prov_loc_to_name, mock_log): + """Post-relocate provider update failure must log and preserve FCD.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-777' + fcd_loc.provider_location.return_value = 'fcd-777@datastore-101' + vops.register_disk.return_value = fcd_loc + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + + prov_loc_to_name.side_effect = vexc.VimException( + 'summary lookup failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + self.assertEqual('available', volume.status) + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('fcd-777', warning_args) + self.assertIn('datastore-202', warning_args) + vops.delete_fcd.assert_not_called() + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_register_failure_logs_warning( + self, ds_sel, vops, select_ds_fcd, mock_log): + """Register failure logs warning with source path and reason.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + vops.register_disk.side_effect = vexc.VimException( + 'RegisterDisk failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('vm/vm.vmdk', warning_args) + self.assertIn('ds1', warning_args) + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_relocate_failure_logs_warning( + self, ds_sel, vops, select_ds_fcd, mock_log): + """Relocate failure logs warning with FCD ID, target, and reason.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.provider_location.return_value = 'fcd-123@datastore-101' + fcd_loc.fcd_id = 'fcd-123' + vops.register_disk.return_value = fcd_loc + + target_ds_ref = mock.Mock() + target_ds_ref.value = 'datastore-202' + select_ds_fcd.return_value = target_ds_ref + + vops.relocate_fcd.side_effect = vexc.VimException('Relocate failed') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('fcd-123', warning_args) + self.assertIn('datastore-202', warning_args) + + @mock.patch('cinder.volume.drivers.vmware.fcd.LOG') + @mock.patch.object(FCD_DRIVER, '_select_ds_fcd') + @mock.patch.object(FCD_DRIVER, 'volumeops') + @mock.patch.object(FCD_DRIVER, 'ds_sel') + def test_manage_existing_select_ds_failure_logs_warning( + self, ds_sel, vops, select_ds_fcd, mock_log): + """Target datastore selection failure logs warning with FCD ID.""" + volume = self._create_volume_obj() + ref = {'source-name': '[ds1] vm/vm.vmdk', 'size_gb': 64} + + src_ds_ref = mock.Mock() + src_ds_ref.value = 'datastore-101' + ds_sel.get_ds_ref_by_name.return_value = src_ds_ref + + dc_ref = mock.Mock() + vops.get_dc.return_value = dc_ref + vops.get_inventory_path.return_value = '/dc1' + + fcd_loc = mock.Mock() + fcd_loc.fcd_id = 'fcd-999' + fcd_loc.provider_location.return_value = 'fcd-999@datastore-101' + vops.register_disk.return_value = fcd_loc + + select_ds_fcd.side_effect = vexc.VimException( + 'No suitable datastore') + + self.assertRaises( + vexc.VimException, + self._driver.manage_existing, volume, ref) + + mock_log.warning.assert_called_once() + warning_args = str(mock_log.warning.call_args) + self.assertIn('fcd-999', warning_args) + self.assertIn('vm/vm.vmdk', warning_args) diff --git a/cinder/tests/unit/volume/drivers/vmware/test_vmware_volumeops.py b/cinder/tests/unit/volume/drivers/vmware/test_vmware_volumeops.py index 15e88d27517..e644772ab58 100644 --- a/cinder/tests/unit/volume/drivers/vmware/test_vmware_volumeops.py +++ b/cinder/tests/unit/volume/drivers/vmware/test_vmware_volumeops.py @@ -2173,6 +2173,23 @@ def test_register_disk(self): path=vmdk_url, name=name) + def test_rename_fcd(self): + fcd_location = mock.Mock() + fcd_id = mock.sentinel.fcd_id + fcd_location.id.return_value = fcd_id + ds_ref = mock.sentinel.ds_ref + name = mock.sentinel.name + + self.vops.rename_fcd(fcd_location, ds_ref, name) + + self.session.invoke_api.assert_called_once_with( + self.session.vim, + 'RenameVStorageObject', + self.session.vim.service_content.vStorageObjectManager, + id=fcd_id, + datastore=ds_ref, + name=name) + @mock.patch('cinder.volume.drivers.vmware.volumeops.VMwareVolumeOps.' '_create_controller_config_spec') @mock.patch('cinder.volume.drivers.vmware.volumeops.VMwareVolumeOps.' diff --git a/cinder/volume/drivers/vmware/fcd.py b/cinder/volume/drivers/vmware/fcd.py index 579da2895d1..9fb8912dfdc 100644 --- a/cinder/volume/drivers/vmware/fcd.py +++ b/cinder/volume/drivers/vmware/fcd.py @@ -119,6 +119,196 @@ def _select_ds_fcd(self, volume): (host, rp, folder, summary) = self._select_ds_for_volume(volume) return summary.datastore + def _parse_manage_existing_ref(self, existing_ref): + """Parse and validate the manage-existing reference. + + :param existing_ref: dict with 'source-name' key containing + '[datastore_name] relative/path.vmdk' + :returns: tuple (datastore_name, relative_path) + :raises ManageExistingInvalidReference: if ref is missing or malformed + """ + source_name = existing_ref.get('source-name') + if not source_name: + reason = _("manage_existing requires 'source-name'") + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + try: + ds_path = datastore.DatastorePath.parse(source_name) + if not ds_path.datastore or not ds_path.rel_path: + raise ValueError("empty component") + except (IndexError, ValueError): + reason = _("source-name must be '[datastore] path.vmdk', " + "got: %s") % source_name + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + return ds_path.datastore, ds_path.rel_path + + def manage_existing_get_size(self, volume, existing_ref): + """Return size of volume to be managed by manage_existing. + + :param volume: Cinder volume to manage + :param existing_ref: dict with 'size_gb' key + :returns: size in GB + :raises ManageExistingInvalidReference: if size_gb is missing or + not an integer + """ + size_gb = existing_ref.get('size_gb') + if size_gb is None: + volume.status = 'error' + LOG.warning("manage_existing_get_size: missing size_gb in ref. " + "Reference: %(ref)s.", + {'ref': existing_ref}) + reason = _("manage_existing requires 'size_gb' in the ref") + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + try: + return int(size_gb) + except (TypeError, ValueError) as exc: + volume.status = 'error' + LOG.warning("manage_existing_get_size: invalid size_gb in ref. " + "Reference: %(ref)s. Reason: %(reason)s.", + {'ref': existing_ref, 'reason': exc}) + reason = _("manage_existing requires integer 'size_gb' in the ref") + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + + def _log_manage_existing_warning(self, message, ds_name=None, + rel_path=None, fcd_id=None, + target_ds=None, reason=None, + ref=None): + source = ref + if ds_name and rel_path: + source = "[%s] %s" % (ds_name, rel_path) + LOG.warning("manage_existing: %(message)s. " + "FCD ID: %(fcd_id)s. Source path: %(source)s. " + "Target datastore: %(target_ds)s. Reason: %(reason)s.", + {'message': message, + 'fcd_id': fcd_id or 'unknown', + 'source': source or 'unknown', + 'target_ds': target_ds or 'unknown', + 'reason': reason}) + + def manage_existing(self, volume, existing_ref): + """Adopt a detached VMDK as an FCD-backed Cinder volume. + + Registers the VMDK at 'source-name' as a First Class Disk, then + relocates it to the volume's target datastore if necessary. + Never cleans up on failure. The source VMDK must survive all + error paths. + + Note on failure states: pre-relocate failures mark the in-flight + volume as 'error' so the manage_existing flow preserves a Nova-safe + abort state. Relocate and post-relocate failures leave flow handling + at 'error_managing' because operator recovery is required. + + :param volume: Cinder volume object + :param existing_ref: dict with 'source-name' ('[ds] path.vmdk') + and 'size_gb' + :returns: dict with 'provider_location' + """ + ds_name = rel_path = target_val = None + fcd_id = None + try: + ds_name, rel_path = self._parse_manage_existing_ref(existing_ref) + + src_ds_ref = self.ds_sel.get_ds_ref_by_name(ds_name) + if not src_ds_ref: + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, + reason=_("Source datastore '%s' not found") % ds_name) + + source_fcd_id = existing_ref.get('source-id') + if source_fcd_id: + # Nova already knows the FCD id (from VirtualDisk.vDiskId). + # Validate its backing path matches source-name, then rename. + fcd_id = source_fcd_id + src_val = vim_util.get_moref_value(src_ds_ref) + fcd_loc = vops.FcdLocation(source_fcd_id, src_val) + + fcd_path = self.volumeops.get_vmdk_path_for_fcd( + fcd_loc=fcd_loc) + fcd_ds_path = datastore.DatastorePath.parse(fcd_path) + if (fcd_ds_path.datastore != ds_name or + fcd_ds_path.rel_path != rel_path): + reason = (_("source-id %(fcd_id)s points to %(fcd_path)s, " + "not [%(ds)s] %(path)s") % + {'fcd_id': source_fcd_id, + 'fcd_path': fcd_path, + 'ds': ds_name, + 'path': rel_path}) + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + + self.volumeops.rename_fcd(fcd_loc, src_ds_ref, volume.name) + else: + # No pre-existing FCD id: register the raw VMDK as an FCD. + dc_ref = self.volumeops.get_dc(src_ds_ref) + dc_path = self.volumeops.get_inventory_path(dc_ref) + vmdk_url = datastore.DatastoreURL( + 'https', self.configuration.vmware_host_ip, + rel_path, dc_path, ds_name) + + try: + fcd_loc = self.volumeops.register_disk( + str(vmdk_url), volume.name, src_ds_ref) + except vexc.AlreadyExistsException: + reason = _("VMDK is already registered as FCD; " + "pass source-id") + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=reason) + fcd_id = fcd_loc.fcd_id + + target_ds_ref = self._select_ds_fcd(volume) + + src_val = fcd_loc.ds_ref_val + target_val = vim_util.get_moref_value(target_ds_ref) + needs_relocate = src_val != target_val + except Exception as exc: + # Pre-relocate failure: mark 'error' so the manage_existing flow + # preserves it on rollback, letting Nova abort and reattach the + # original VMDK. See ManageExistingTask status handling. + volume.status = 'error' + self._log_manage_existing_warning( + "pre-relocate failure", + ds_name=ds_name, + rel_path=rel_path, + fcd_id=fcd_id, + target_ds=target_val, + reason=exc, + ref=existing_ref) + raise + + if needs_relocate: + try: + self.volumeops.relocate_fcd( + fcd_loc, target_ds_ref, volume.name) + except Exception as exc: + self._log_manage_existing_warning( + "failed to relocate FCD", + ds_name=ds_name, + rel_path=rel_path, + fcd_id=fcd_loc.fcd_id, + target_ds=target_val, + reason=exc) + raise + fcd_loc = vops.FcdLocation(fcd_loc.fcd_id, target_val) + + try: + provider_location = self._provider_location_to_ds_name_location( + fcd_loc.provider_location()) + except Exception as exc: + if not needs_relocate: + volume.status = 'error' + self._log_manage_existing_warning( + "failed to finalize provider_location", + ds_name=ds_name, + rel_path=rel_path, + fcd_id=fcd_loc.fcd_id, + target_ds=target_val, + reason=exc) + raise + return {'provider_location': provider_location} + def _get_temp_image_folder_from_volume(self, volume): (host_ref, _resource_pool, folder, summary) = self._select_ds_for_volume(volume) From 9e40b705ee582c77c1de86a91d56beb0a3e238c9 Mon Sep 17 00:00:00 2001 From: Fabian Koller Date: Mon, 6 Jul 2026 11:23:43 +0200 Subject: [PATCH 3/3] api: allow service-token callers to bypass manage_existing policy Problem: The volume_extension:volume_manage policy defaults to rule:admin_api, so only admin users can call the manage_existing API. When another service (e.g. Nova) calls manage_existing on behalf of a user, the request carries the user's token as primary (for correct volume ownership and quota attribution) plus the service's token as secondary. The non-admin user token is rejected by the policy. Solution: Before enforcing the admin-only policy, check whether the request carries a valid service token using the existing is_service_request() helper (checks ctxt.service_roles against the configured service_token_roles). If the request comes from a trusted service, skip the policy check. This mirrors the pattern already used in attachment_deletion_allowed() for service-initiated detach operations. The admin path and the non-admin-without-service-token path remain unchanged. Change-Id: I88b07712f399dae7654d238605f6f410a1b5a9ba --- cinder/context.py | 1 + cinder/policies/base.py | 26 ++++ cinder/policies/manageable_volumes.py | 2 +- cinder/service_token.py | 47 +++++++ .../unit/api/contrib/test_volume_manage.py | 124 ++++++++++++++++++ cinder/tests/unit/test_context.py | 12 ++ cinder/volume/api.py | 5 +- 7 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 cinder/service_token.py diff --git a/cinder/context.py b/cinder/context.py index 21454cb42fe..988610d5523 100644 --- a/cinder/context.py +++ b/cinder/context.py @@ -261,6 +261,7 @@ def to_policy_values(self) -> dict: policy = super(RequestContext, self).to_policy_values() policy['is_admin'] = self.is_admin + policy['service_roles'] = self.service_roles return policy diff --git a/cinder/policies/base.py b/cinder/policies/base.py index 13907a73f7e..8948780f0a0 100644 --- a/cinder/policies/base.py +++ b/cinder/policies/base.py @@ -18,6 +18,8 @@ from oslo_log import versionutils from oslo_policy import policy +from cinder import service_token + # General observations # -------------------- # - This file uses the three "default roles" provided by Keystone during @@ -145,6 +147,7 @@ # as RULE_ADMIN_API in Xena and won't be deprecated until Yoga development. # XENA_SYSTEM_ADMIN_ONLY = "rule:xena_system_admin_only" RULE_ADMIN_API = "rule:admin_api" +RULE_ADMIN_OR_SERVICE_API = "rule:admin_api or is_service_request:True" # TODO: xena rules to be removed in AA xena_rule_defaults = [ @@ -279,6 +282,29 @@ def __init__(self, ) +@policy.register('is_service_request') +class IsServiceRequestCheck(policy.Check): + """oslo.policy check: `is_service_request:True`. + + Evaluates to True when the request context carries a service token + with at least one role listed in + `[keystone_authtoken]/service_token_roles`. + + Usage in policy rules:: + + "volume_extension:volume_manage": + "rule:admin_api or is_service_request:True" + """ + + def __init__(self, kind, match): + self.expected = match.lower() == 'true' + super(IsServiceRequestCheck, self).__init__(kind, str(self.expected)) + + def __call__(self, target, creds, enforcer): + service_roles = creds.get('service_roles') or [] + return service_token.is_service_request(service_roles) == self.expected + + # This is used by the deprecated rules in the individual policy files # in Xena. # TODO: remove in Yoga diff --git a/cinder/policies/manageable_volumes.py b/cinder/policies/manageable_volumes.py index 0eecce7cf85..0f6fe32be0a 100644 --- a/cinder/policies/manageable_volumes.py +++ b/cinder/policies/manageable_volumes.py @@ -41,7 +41,7 @@ ]), policy.DocumentedRuleDefault( name=MANAGE_POLICY, - check_str=base.RULE_ADMIN_API, + check_str=base.RULE_ADMIN_OR_SERVICE_API, description="Manage existing volumes.", operations=[ { diff --git a/cinder/service_token.py b/cinder/service_token.py new file mode 100644 index 00000000000..e2273161867 --- /dev/null +++ b/cinder/service_token.py @@ -0,0 +1,47 @@ +# Copyright (c) 2026 SAP SE +# +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Utilities for verifying incoming service tokens. + +This module handles the *incoming* side of service-token authentication: +checking whether a request context carries a service token whose roles match +the operator-configured `[keystone_authtoken]/service_token_roles` list. + +See also `cinder/service_auth.py` for the *outgoing* side (attaching a +service token when Cinder calls other services). +""" + +from oslo_config import cfg + +CONF = cfg.CONF + + +def is_service_request(service_roles): + """Return True if *service_roles* satisfy the configured service roles. + + A request counts as a service request when it carries a secondary service + token (populated into `RequestContext.service_roles` by Keystone + middleware) and at least one of those roles is listed in + `[keystone_authtoken]/service_token_roles`. + + :param service_roles: list of role strings from the service token + (`RequestContext.service_roles`). + :returns: True when the request carries an accepted service token. + """ + if not service_roles: + return False + configured = set(CONF.keystone_authtoken.service_token_roles) + return bool(configured.intersection(service_roles)) diff --git a/cinder/tests/unit/api/contrib/test_volume_manage.py b/cinder/tests/unit/api/contrib/test_volume_manage.py index 80d7e07240c..6e5a6d29115 100644 --- a/cinder/tests/unit/api/contrib/test_volume_manage.py +++ b/cinder/tests/unit/api/contrib/test_volume_manage.py @@ -640,3 +640,127 @@ def test_manage_with_default_type(self, called_with['volume_type']['name']) self.assertEqual(default_vt['id'], called_with['volume_type']['id']) + + @mock.patch('cinder.volume.api.API.manage_existing', wraps=api_manage) + def test_manage_volume_service_token_bypasses_policy(self, + mock_api_manage): + """A valid service token allows non-admin users to manage volumes. + + When another service (e.g. Nova) calls manage_existing on behalf of a + user, the request carries a service token. The manage policy + (admin-only) should be bypassed in this case, following the same + pattern as attachment_deletion_allowed. + """ + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, + is_admin=False) + ctxt.service_roles = ['service'] + + body = {'volume': {'host': 'host_ok', + 'ref': 'fake_ref'}} + req = webob.Request.blank('/v3/%s/os-volume-manage' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.environ['cinder.context'] = ctxt + req.headers["OpenStack-API-Version"] = "volume 3.11" + req.api_version_request = api_version.APIVersionRequest('3.11') + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(app()) + self.assertEqual(HTTPStatus.ACCEPTED, res.status_int) + self.assertEqual(1, mock_api_manage.call_count) + + def test_manage_volume_non_admin_without_service_token_rejected(self): + """Non-admin users without a service token are still rejected. + + The admin-only policy must still be enforced when there is no service + token on the request. + """ + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, + is_admin=False) + # No service_roles set (or empty) + ctxt.service_roles = [] + + body = {'volume': {'host': 'host_ok', + 'ref': 'fake_ref'}} + req = webob.Request.blank('/v3/%s/os-volume-manage' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.environ['cinder.context'] = ctxt + req.headers["OpenStack-API-Version"] = "volume 3.11" + req.api_version_request = api_version.APIVersionRequest('3.11') + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(app()) + self.assertEqual(HTTPStatus.FORBIDDEN, res.status_int) + + @mock.patch('cinder.volume.api.API.manage_existing', wraps=api_manage) + def test_manage_volume_admin_without_service_token_still_works( + self, mock_api_manage): + """Admin users can still manage volumes without a service token. + + The existing admin path must remain unchanged. + """ + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, + is_admin=True) + + body = {'volume': {'host': 'host_ok', + 'ref': 'fake_ref'}} + req = webob.Request.blank('/v3/%s/os-volume-manage' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.environ['cinder.context'] = ctxt + req.headers["OpenStack-API-Version"] = "volume 3.11" + req.api_version_request = api_version.APIVersionRequest('3.11') + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(app()) + self.assertEqual(HTTPStatus.ACCEPTED, res.status_int) + self.assertEqual(1, mock_api_manage.call_count) + + @mock.patch('cinder.volume.api.API.manage_existing', wraps=api_manage) + def test_manage_volume_custom_service_role(self, mock_api_manage): + """Custom service_token_roles config is respected. + + If the operator configures a custom role name for service tokens, + the manage API should accept requests with that role. + """ + self.override_config('service_token_roles', + ['service', 'nova_service'], + group='keystone_authtoken') + + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, + is_admin=False) + ctxt.service_roles = ['nova_service'] + + body = {'volume': {'host': 'host_ok', + 'ref': 'fake_ref'}} + req = webob.Request.blank('/v3/%s/os-volume-manage' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.environ['cinder.context'] = ctxt + req.headers["OpenStack-API-Version"] = "volume 3.11" + req.api_version_request = api_version.APIVersionRequest('3.11') + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(app()) + self.assertEqual(HTTPStatus.ACCEPTED, res.status_int) + self.assertEqual(1, mock_api_manage.call_count) + + def test_manage_volume_unconfigured_service_role_rejected(self): + """A service token with an unconfigured role is still rejected. + + Only roles listed in [keystone_authtoken]/service_token_roles + should be accepted. An arbitrary role on the service token does + not grant manage access. + """ + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, + is_admin=False) + ctxt.service_roles = ['not_a_configured_service_role'] + + body = {'volume': {'host': 'host_ok', + 'ref': 'fake_ref'}} + req = webob.Request.blank('/v3/%s/os-volume-manage' % fake.PROJECT_ID) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.environ['cinder.context'] = ctxt + req.headers["OpenStack-API-Version"] = "volume 3.11" + req.api_version_request = api_version.APIVersionRequest('3.11') + req.body = jsonutils.dump_as_bytes(body) + res = req.get_response(app()) + self.assertEqual(HTTPStatus.FORBIDDEN, res.status_int) diff --git a/cinder/tests/unit/test_context.py b/cinder/tests/unit/test_context.py index dddb070292c..b1fb79492b9 100644 --- a/cinder/tests/unit/test_context.py +++ b/cinder/tests/unit/test_context.py @@ -133,6 +133,18 @@ def test_request_context_with_roles(self): roles=roles) self.assertEqual(roles, ctxt.roles) + def test_to_policy_values_includes_service_roles(self): + ctxt = context.RequestContext('111', '222') + ctxt.service_roles = ['service'] + values = ctxt.to_policy_values() + self.assertEqual(['service'], values['service_roles']) + + def test_to_policy_values_empty_service_roles(self): + ctxt = context.RequestContext('111', '222') + ctxt.service_roles = [] + values = ctxt.to_policy_values() + self.assertEqual([], values['service_roles']) + @ddt.ddt class ContextAuthorizeTestCase(test.TestCase): diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e4c53c2a07..6b091109f7c 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -64,6 +64,7 @@ from cinder import quota_utils from cinder.scheduler import host_manager from cinder.scheduler import rpcapi as scheduler_rpcapi +from cinder import service_token from cinder import utils from cinder.volume.flows.api import create_volume from cinder.volume.flows.api import manage_existing @@ -1068,9 +1069,7 @@ def is_service_request(ctxt: 'context.RequestContext') -> bool: `service_token_roles` configuration option in the `[keystone_authtoken]` section (defaults to `service`). """ - roles = ctxt.service_roles - service_roles = set(CONF.keystone_authtoken.service_token_roles) - return bool(roles and service_roles.intersection(roles)) + return service_token.is_service_request(ctxt.service_roles) def terminate_connection(self, context: context.RequestContext,