From f99f667a96a357adc0070d75b5940e76726f9664 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 3 Mar 2021 12:33:49 +0000 Subject: [PATCH] libvirt: Simplify device_path check in _detach_encryptor Introduced by Id670f13a7f197e71c77dc91276fc2fba2fc5f314 to resolve bug #1821696 this check was put in place to ensure _detach_encryptor did not attempt to use the os-brick encryptors with an unsupported volume type after libvirt secrets had been removed outside the control of Nova. With the introduction of the [workarounds]disable_native_luksv1 via Ia500eb614cf575ab846f64f4b69c9068274c8c1f however the use of _allow_native_luksv1 as part of this check is no longer valid. As this helper was updated to return False when the workaround is enabled, regardless of the underlying volume being attached natively or not. If an admin had enabled the workaround after users had launched instances with natively attached encrypted volumes *and* the libvirt secrets had gone missing _detach_encryptor would attempt to use the os-brick encryptors. This would fail when the underlying volume type is unsupported, for example rbd. See bug #1917619 for an example. This change resolves this corner case by dropping the use of _allow_native_luksv1 from the check and just asserting that a device_path is present for an encrypted volume before allowing the use of the os-brick encryptors. As noted this is safe as calls to the encryptors are idempotent, ignoring failures to detach when the underlying volume type is supported. Closes-Bug: #1917619 Change-Id: Iba40c2df72228b461767d5734d5a62403d9f2cfa (cherry picked from commit 4908daed96ddda492ced6cbb084abe8f33a8b1f7) --- nova/tests/unit/virt/libvirt/test_driver.py | 43 +++++++++++++-------- nova/virt/libvirt/driver.py | 18 +++++---- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 4e052c48659f..faa166f903e4 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -10005,9 +10005,9 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('os_brick.encryptors.get_encryption_metadata') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') - def test_detach_encryptor_encrypted_volume_meta_missing(self, - mock_allow_native_luksv1, mock_get_encryptor, mock_get_metadata): + def test_detach_encryptor_encrypted_volume_meta_missing( + self, mock_get_encryptor, mock_get_metadata + ): """Assert that if missing the encryption metadata of an encrypted volume is fetched and then used to detach the encryptor for the volume. """ @@ -10016,8 +10016,12 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_get_encryptor.return_value = mock_encryptor encryption = {'provider': 'luks', 'control_location': 'front-end'} mock_get_metadata.return_value = encryption - connection_info = {'data': {'volume_id': uuids.volume_id}} - mock_allow_native_luksv1.return_value = False + connection_info = { + 'data': { + 'device_path': '/dev/foo', + 'volume_id': uuids.volume_id + } + } drvr._detach_encryptor(self.context, connection_info, None) @@ -10029,9 +10033,11 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('os_brick.encryptors.get_encryption_metadata') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') - def test_detach_encryptor_encrypted_volume_meta_provided(self, - mock_allow_native_luksv1, mock_get_encryptor, mock_get_metadata): + def test_detach_encryptor_encrypted_volume_meta_provided( + self, + mock_get_encryptor, + mock_get_metadata + ): """Assert that when provided there are no further attempts to fetch the encryption metadata for the volume and that the provided metadata is then used to detach the volume. @@ -10040,8 +10046,12 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_encryptor = mock.MagicMock() mock_get_encryptor.return_value = mock_encryptor encryption = {'provider': 'luks', 'control_location': 'front-end'} - connection_info = {'data': {'volume_id': uuids.volume_id}} - mock_allow_native_luksv1.return_value = False + connection_info = { + 'data': { + 'device_path': '/dev/foo', + 'volume_id': uuids.volume_id + } + } drvr._detach_encryptor(self.context, connection_info, encryption) @@ -10051,20 +10061,19 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_encryptor.detach_volume.assert_called_once_with(**encryption) @mock.patch('nova.virt.libvirt.host.Host.find_secret') - @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._allow_native_luksv1') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_encryptor') - def test_detach_encryptor_native_luks_device_path_secret_missing(self, - mock_get_encryptor, mock_allow_native_luksv1, mock_find_secret): - """Assert that the encryptor is not built when native LUKS is - available, the associated volume secret is missing and device_path is - also missing from the connection_info. + def test_detach_encryptor_native_luks_device_path_secret_missing( + self, mock_get_encryptor, mock_find_secret + ): + """Assert that the encryptor is not built when the associated volume + secret is missing and device_path is also missing from the + connection_info. """ drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) encryption = {'provider': 'luks', 'control_location': 'front-end', 'encryption_key_id': uuids.encryption_key_id} connection_info = {'data': {'volume_id': uuids.volume_id}} mock_find_secret.return_value = False - mock_allow_native_luksv1.return_value = True drvr._detach_encryptor(self.context, connection_info, encryption) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index b5eb984fef97..8ee8757e40cc 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1964,14 +1964,18 @@ class LibvirtDriver(driver.ComputeDriver): return self._host.delete_secret('volume', volume_id) if encryption is None: encryption = self._get_volume_encryption(context, connection_info) - # NOTE(lyarwood): Handle bug #1821696 where volume secrets have been - # removed manually by returning if a LUKS provider is being used - # and device_path is not present in the connection_info. This avoids - # VolumeEncryptionNotSupported being thrown when we incorrectly build - # the encryptor below due to the secrets not being present above. - if (encryption and self._allow_native_luksv1(encryption=encryption) and - not connection_info['data'].get('device_path')): + + # NOTE(lyarwood): Handle bugs #1821696 and #1917619 by avoiding the use + # of the os-brick encryptors if we don't have a device_path. The lack + # of a device_path here suggests the volume was natively attached to + # QEMU anyway as volumes without a device_path are not supported by + # os-brick encryptors. For volumes with a device_path the calls to + # the os-brick encryptors are safe as they are actually idempotent, + # ignoring any failures caused by the volumes actually being natively + # attached previously. + if (encryption and connection_info['data'].get('device_path') is None): return + if encryption: encryptor = self._get_volume_encryptor(connection_info, encryption)