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 a58cd90958ec..cda881374b71 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -7127,6 +7127,40 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, # 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, 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 @@ class LibvirtConnTestCase(test.NoDBTestCase, '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 a631494c7ed9..a6ecf50f8242 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -1274,3 +1274,28 @@ class TestDriverBlockDeviceNewFlow(TestDriverBlockDevice): 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 ab5cd09f697a..4100928528ab 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 1f921c1fefd0..f9587f7d4817 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1274,8 +1274,8 @@ class LibvirtDriver(driver.ComputeDriver): """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 @@ class LibvirtDriver(driver.ComputeDriver): # 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 @@ class LibvirtDriver(driver.ComputeDriver): 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 @@ class LibvirtDriver(driver.ComputeDriver): 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 9de99e67f3c2..48145a9d3be5 100644 --- a/nova/virt/libvirt/volume/volume.py +++ b/nova/virt/libvirt/volume/volume.py @@ -22,6 +22,7 @@ from oslo_log import log as logging 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 @@ class LibvirtBaseVolumeDriver(object): # 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)