From cafe3d066ef7021c18961d4b239a10f61db23f2d Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 31 Jan 2018 19:06:46 -0500 Subject: [PATCH] libvirt: fix native luks encryption failure to find volume_id Not all volume types put a 'volume_id' entry in the connection_info['data'] dict. This change uses a new utility method to look up the volume_id in the connection_info data dict and if not found there, uses the 'serial' value from the connection_info, which we know at least gets set when the DriverVolumeBlockDevice code attaches the volume. This also has to update pre_live_migration since the connection_info dict doesn't have a 'volume_id' key in it. It's unclear what this code was expecting, or if it ever really worked, but since an attached volume represented by a BlockDeviceMapping here has a volume_id attribute, we can just use that. As that code path was never tested, this updates related unit tests and refactors the tests to actually use the type of DriverVolumeBlockDevice objects that the ComputeManager would be sending down to the driver pre_live_migration method. The hard-to-read squashed dicts in the tests are also re-formatted so a human can actually read them. Change-Id: Ie02d298cd92d5b5ebcbbcd2b0e8be01f197bfafb Closes-Bug: #1746609 --- nova/tests/unit/virt/libvirt/test_driver.py | 151 +++++++++++++++----- nova/tests/unit/virt/test_block_device.py | 25 ++++ nova/virt/block_device.py | 10 ++ nova/virt/libvirt/driver.py | 13 +- nova/virt/libvirt/volume/volume.py | 3 +- 5 files changed, 158 insertions(+), 44 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index a58cd90958e..cda881374b7 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -7127,6 +7127,40 @@ def test_attach_encryptor_encrypted_volume_meta_provided(self, mock_encryptor.attach_volume.assert_called_once_with(self.context, **encryption) + @mock.patch.object(key_manager, 'API') + @mock.patch('os_brick.encryptors.get_encryption_metadata') + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') + def test_attach_encryptor_encrypted_native_luks_serial(self, + mock_get_encryptor, mock_get_metadata, mock_get_key_mgr): + """Uses native luks encryption with a provider encryptor and the + connection_info has a serial but not volume_id in the 'data' + sub-dict. + """ + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + mock_encryptor = mock.MagicMock() + mock_get_encryptor.return_value = mock_encryptor + encryption = {'provider': 'luks', 'control_location': 'front-end', + 'encryption_key_id': uuids.encryption_key_id} + connection_info = {'serial': uuids.serial, 'data': {}} + # Mock out the key manager + key = u'3734363537333734' + key_encoded = binascii.unhexlify(key) + mock_key = mock.Mock() + mock_key_mgr = mock.Mock() + mock_get_key_mgr.return_value = mock_key_mgr + mock_key_mgr.get.return_value = mock_key + mock_key.get_encoded.return_value = key_encoded + + with mock.patch.object(drvr, '_use_native_luks', return_value=True): + with mock.patch.object(drvr._host, 'create_secret') as crt_scrt: + drvr._attach_encryptor(self.context, connection_info, + encryption, allow_native_luks=True) + + mock_get_metadata.assert_not_called() + mock_get_encryptor.assert_not_called() + crt_scrt.assert_called_once_with( + 'volume', uuids.serial, password=key) + @mock.patch('os_brick.encryptors.get_encryption_metadata') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') def test_detach_encryptor_connection_info_incomplete(self, @@ -10473,15 +10507,42 @@ def _test_pre_live_migration_works_correctly_mocked(self, target_ret=None, src_supports_native_luks=True, dest_supports_native_luks=True, allow_native_luks=True): # Creating testdata - vol = {'block_device_mapping': [ - {'connection_info': {'serial': '12345', u'data': - {'device_path': - u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.abc.12345.opst-lun-X'}}, - 'mount_device': '/dev/sda'}, - {'connection_info': {'serial': '67890', u'data': - {'device_path': - u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.cde.67890.opst-lun-Z'}}, - 'mount_device': '/dev/sdb'}]} + c = context.get_admin_context() + instance = objects.Instance(root_device_name='/dev/vda', + **self.test_instance) + bdms = objects.BlockDeviceMappingList(objects=[ + fake_block_device.fake_bdm_object(c, { + 'connection_info': jsonutils.dumps({ + 'serial': '12345', + 'data': { + 'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260' + '-iqn.abc.12345.opst-lun-X' + } + }), + 'device_name': '/dev/sda', + 'volume_id': uuids.volume1, + 'source_type': 'volume', + 'destination_type': 'volume' + }), + fake_block_device.fake_bdm_object(c, { + 'connection_info': jsonutils.dumps({ + 'serial': '67890', + 'data': { + 'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260' + '-iqn.cde.67890.opst-lun-Z' + } + }), + 'device_name': '/dev/sdb', + 'volume_id': uuids.volume2, + 'source_type': 'volume', + 'destination_type': 'volume' + }) + ]) + # We go through get_block_device_info to simulate what the + # ComputeManager sends to the driver (make sure we're using the + # correct type of BDM objects since there are many of them and + # they are super confusing). + block_device_info = driver.get_block_device_info(instance, bdms) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) @@ -10496,16 +10557,11 @@ def fake_none(*args, **kwargs): self.stubs.Set(drvr, '_is_native_luks_available', lambda: dest_supports_native_luks) - instance = objects.Instance(**self.test_instance) - c = context.get_admin_context() nw_info = FakeNetworkInfo() # Creating mocks - self.mox.StubOutWithMock(driver, "block_device_info_get_mapping") - driver.block_device_info_get_mapping(vol - ).AndReturn(vol['block_device_mapping']) self.mox.StubOutWithMock(drvr, "_connect_volume") - for v in vol['block_device_mapping']: + for v in block_device_info['block_device_mapping']: drvr._connect_volume(c, v['connection_info'], instance, allow_native_luks=allow_native_luks) self.mox.StubOutWithMock(drvr, 'plug_vifs') @@ -10526,14 +10582,14 @@ def fake_none(*args, **kwargs): migrate_data.src_supports_native_luks = True result = drvr.pre_live_migration( - c, instance, vol, nw_info, None, + c, instance, block_device_info, nw_info, None, migrate_data=migrate_data) if not target_ret: target_ret = self._generate_target_ret() self.assertEqual( + target_ret, result.to_legacy_dict( - pre_migration_result=True)['pre_live_migration_result'], - target_ret) + pre_migration_result=True)['pre_live_migration_result']) @mock.patch.object(os, 'mkdir') @mock.patch('nova.virt.libvirt.utils.get_instance_path_at_destination') @@ -10617,15 +10673,42 @@ def test_pre_live_migration_vol_backed_works_correctly_mocked(self): # Creating testdata, using temp dir. with utils.tempdir() as tmpdir: self.flags(instances_path=tmpdir) - vol = {'block_device_mapping': [ - {'connection_info': {'serial': '12345', u'data': - {'device_path': - u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.abc.12345.opst-lun-X'}}, - 'mount_device': '/dev/sda'}, - {'connection_info': {'serial': '67890', u'data': - {'device_path': - u'/dev/disk/by-path/ip-1.2.3.4:3260-iqn.cde.67890.opst-lun-Z'}}, - 'mount_device': '/dev/sdb'}]} + c = context.get_admin_context() + inst_ref = objects.Instance(root_device_name='/dev/vda', + **self.test_instance) + bdms = objects.BlockDeviceMappingList(objects=[ + fake_block_device.fake_bdm_object(c, { + 'connection_info': jsonutils.dumps({ + 'serial': '12345', + 'data': { + 'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260' + '-iqn.abc.12345.opst-lun-X' + } + }), + 'device_name': '/dev/sda', + 'volume_id': uuids.volume1, + 'source_type': 'volume', + 'destination_type': 'volume' + }), + fake_block_device.fake_bdm_object(c, { + 'connection_info': jsonutils.dumps({ + 'serial': '67890', + 'data': { + 'device_path': '/dev/disk/by-path/ip-1.2.3.4:3260' + '-iqn.cde.67890.opst-lun-Z' + } + }), + 'device_name': '/dev/sdb', + 'volume_id': uuids.volume2, + 'source_type': 'volume', + 'destination_type': 'volume' + }) + ]) + # We go through get_block_device_info to simulate what the + # ComputeManager sends to the driver (make sure we're using the + # correct type of BDM objects since there are many of them and + # they are super confusing). + block_device_info = driver.get_block_device_info(inst_ref, bdms) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) @@ -10638,12 +10721,10 @@ def fake_none(*args, **kwargs): class FakeNetworkInfo(object): def fixed_ips(self): return ["test_ip_addr"] - inst_ref = objects.Instance(**self.test_instance) - c = context.get_admin_context() nw_info = FakeNetworkInfo() # Creating mocks self.mox.StubOutWithMock(drvr, "_connect_volume") - for v in vol['block_device_mapping']: + for v in block_device_info['block_device_mapping']: drvr._connect_volume(c, v['connection_info'], inst_ref, allow_native_luks=True) self.mox.StubOutWithMock(drvr, 'plug_vifs') @@ -10661,9 +10742,9 @@ def fixed_ips(self): filename='foo', src_supports_native_luks=True, ) - ret = drvr.pre_live_migration(c, inst_ref, vol, nw_info, None, - migrate_data) - target_ret = { + ret = drvr.pre_live_migration( + c, inst_ref, block_device_info, nw_info, None, migrate_data) + expected_result = { 'graphics_listen_addrs': {'spice': None, 'vnc': None}, 'target_connect_addr': None, @@ -10682,8 +10763,8 @@ def fixed_ips(self): 'dev': 'sdb', 'type': 'disk'}}}} self.assertEqual( - ret.to_legacy_dict(True)['pre_live_migration_result'], - target_ret) + expected_result, + ret.to_legacy_dict(True)['pre_live_migration_result']) self.assertTrue(os.path.exists('%s/%s/' % (tmpdir, inst_ref['name']))) diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index a631494c7ed..a6ecf50f824 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -1274,3 +1274,28 @@ def test_volume_attach_multiattach_no_virt_driver_support(self): self.assertRaises(exception.MultiattachNotSupportedByVirtDriver, test_bdm.attach, self.context, instance, self.volume_api, self.virt_driver) + + +class TestGetVolumeId(test.NoDBTestCase): + + def test_get_volume_id_none_found(self): + self.assertIsNone(driver_block_device.get_volume_id(None)) + self.assertIsNone(driver_block_device.get_volume_id({})) + self.assertIsNone(driver_block_device.get_volume_id({'data': {}})) + + def test_get_volume_id_found_volume_id_no_serial(self): + self.assertEqual(uuids.volume_id, + driver_block_device.get_volume_id( + {'data': {'volume_id': uuids.volume_id}})) + + def test_get_volume_id_found_no_volume_id_serial(self): + self.assertEqual(uuids.serial, + driver_block_device.get_volume_id( + {'serial': uuids.serial})) + + def test_get_volume_id_found_both(self): + # volume_id is taken over serial + self.assertEqual(uuids.volume_id, + driver_block_device.get_volume_id( + {'serial': uuids.serial, + 'data': {'volume_id': uuids.volume_id}})) diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index ab5cd09f697..4100928528a 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -880,3 +880,13 @@ def is_block_device_mapping(bdm): return (bdm.source_type in ('image', 'volume', 'snapshot', 'blank') and bdm.destination_type == 'volume' and is_implemented(bdm)) + + +def get_volume_id(connection_info): + if connection_info: + # Check for volume_id in 'data' and if not there, fallback to + # the 'serial' that the DriverVolumeBlockDevice adds during attach. + volume_id = connection_info.get('data', {}).get('volume_id') + if not volume_id: + volume_id = connection_info.get('serial') + return volume_id diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 1f921c1fefd..f9587f7d481 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1274,8 +1274,8 @@ def _get_volume_encryption(self, context, connection_info): """Get the encryption metadata dict if it is not provided """ encryption = {} - if connection_info.get('data', {}).get('volume_id'): - volume_id = connection_info['data']['volume_id'] + volume_id = driver_block_device.get_volume_id(connection_info) + if volume_id: encryption = encryptors.get_encryption_metadata(context, self._volume_api, volume_id, connection_info) return encryption @@ -1323,7 +1323,7 @@ def _attach_encryptor(self, context, connection_info, encryption, # NOTE(lyarwood): Store the passphrase as a libvirt secret locally # on the compute node. This secret is used later when generating # the volume config. - volume_id = connection_info.get('data', {}).get('volume_id') + volume_id = driver_block_device.get_volume_id(connection_info) self._host.create_secret('volume', volume_id, password=passphrase) elif encryption: encryptor = self._get_volume_encryptor(connection_info, @@ -1340,7 +1340,7 @@ def _detach_encryptor(self, context, connection_info, encryption): If native LUKS decryption is enabled then delete previously created Libvirt volume secret from the host. """ - volume_id = connection_info.get('data', {}).get('volume_id') + volume_id = driver_block_device.get_volume_id(connection_info) if volume_id and self._host.find_secret('volume', volume_id): return self._host.delete_secret('volume', volume_id) if encryption is None: @@ -7427,10 +7427,7 @@ def pre_live_migration(self, context, instance, block_device_info, bdmi.type = disk_info['type'] bdmi.format = disk_info.get('format') bdmi.boot_index = disk_info.get('boot_index') - volume_id = connection_info.get('volume_id') - volume_secret = None - if volume_id: - volume_secret = self._host.find_secret('volume', volume_id) + volume_secret = self._host.find_secret('volume', vol.volume_id) if volume_secret: bdmi.encryption_secret_uuid = volume_secret.UUIDString() diff --git a/nova/virt/libvirt/volume/volume.py b/nova/virt/libvirt/volume/volume.py index 9de99e67f3c..48145a9d3be 100644 --- a/nova/virt/libvirt/volume/volume.py +++ b/nova/virt/libvirt/volume/volume.py @@ -22,6 +22,7 @@ import nova.conf from nova import exception from nova import profiler +from nova.virt import block_device as driver_block_device from nova.virt.libvirt import config as vconfig import nova.virt.libvirt.driver from nova.virt.libvirt import utils as libvirt_utils @@ -109,7 +110,7 @@ def get_config(self, connection_info, disk_info): # a shareable disk. conf.shareable = True - volume_id = connection_info.get('data', {}).get('volume_id') + volume_id = driver_block_device.get_volume_id(connection_info) volume_secret = None if volume_id: volume_secret = self.host.find_secret('volume', volume_id)