libvirt: Do not destroy volume secrets during _hard_reboot

Ia2007bc63ef09931ea0197cef29d6a5614ed821a unfortunately missed that
resume_state_on_host_boot calls down into _hard_reboot always removing
volume secrets rendering that change useless.

This change seeks to address this by using the destroy_secrets kwarg
introduced by I856268b371f7ba712b02189db3c927cd762a4dc3 within the
_hard_reboot method of the libvirt driver to ensure secrets are not
removed during a hard reboot.

This resolves the original issue in bug #1905701 *and* allows admins to
hard reboot a users instance when that instance has encrypted volumes
attached with secrets stored in Barbican. This latter use case being
something we can easily test within tempest unlike the compute reboot in
bug #1905701.

This change is kept small as it should ideally be backported alongside
Ia2007bc63ef09931ea0197cef29d6a5614ed821a to stable/queens. Follow up
changes on master will improve formatting, doc text and introduce
functional tests to further validate this new behaviour of hard reboot
within the libvirt driver.

Closes-Bug: #1905701
Change-Id: I3d1b21ba6eb3f5eb728693197c24b4b315eef821

Author: lyarwood

nova/tests/unit/virt/libvirt/test_driver.py

@@ -9049,6 +9049,19 @@ def test_disconnect_volume_luks(self, mock_get_volume_encryptor):
                                                          uuids.volume_id)
         mock_encryptor.detach_volume.assert_not_called()
 
+        # assert that no attempt to remove the secret is made when
+        # destroy_secrets=False
+        drvr._host.find_secret.reset_mock()
+        drvr._host.delete_secret.reset_mock()
+        drvr._disconnect_volume(
+            self.context,
+            connection_info,
+            instance,
+            encryption=encryption,
+            destroy_secrets=False
+        )
+        drvr._host.delete_secret.assert_not_called()
+
         # assert that the encryptor is used if no secret is found
         drvr._host.find_secret.reset_mock()
         drvr._host.delete_secret.reset_mock()
@@ -10112,6 +10125,36 @@ def test_detach_encryptor_native_luks_device_path_secret_missing(
         mock_find_secret.assert_called_once_with('volume', uuids.volume_id)
         mock_get_encryptor.assert_not_called()
 
+    @mock.patch('nova.virt.libvirt.host.Host.delete_secret')
+    @mock.patch('nova.virt.libvirt.host.Host.find_secret', new=mock.Mock())
+    def test_detach_encryptor_skip_secret_removal(self, mock_delete_secret):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        drvr._detach_encryptor(
+            self.context,
+            {
+                'data': {
+                    'volume_id': uuids.volume_id
+                }
+            },
+            None,
+            destroy_secrets=False
+        )
+        # Assert that no attempt is made to delete the volume secert
+        mock_delete_secret.assert_not_called()
+
+        drvr._detach_encryptor(
+            self.context,
+            {
+                'data': {
+                    'volume_id': uuids.volume_id
+                }
+            },
+            None,
+            destroy_secrets=True
+        )
+        # Assert that volume secert is deleted
+        mock_delete_secret.assert_called_once_with('volume', uuids.volume_id)
+
     def test_allow_native_luksv1(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
         self.assertFalse(drvr._allow_native_luksv1({}))
@@ -15752,7 +15795,8 @@ def destroy_side_effect(*args, **kwargs):
             mock_domain_destroy.assert_called_once_with()
             mock_teardown_container.assert_called_once_with(instance)
             mock_cleanup.assert_called_once_with(self.context, instance,
-                                                 network_info, None, False)
+                                                 network_info, None, False,
+                                                 destroy_secrets=True)
 
     @mock.patch.object(libvirt_driver.LibvirtDriver, 'cleanup')
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_teardown_container')
@@ -15772,7 +15816,8 @@ def test_destroy_lxc_calls_teardown_container_when_no_domain(self,
                                           mock.call(instance)])
         mock_teardown_container.assert_called_once_with(instance)
         mock_cleanup.assert_called_once_with(self.context, instance,
-                                             network_info, None, False)
+                                             network_info, None, False,
+                                             destroy_secrets=True)
 
     @mock.patch.object(host.Host, 'get_guest')
     def test_reboot_different_ids(self, mock_get):
@@ -15993,7 +16038,8 @@ def test_hard_reboot(self, mock_get_mdev, mock_destroy, mock_get_disk_info,
         mock_get_mdev.assert_called_once_with(instance)
         mock_destroy.assert_called_once_with(self.context, instance,
                 network_info, destroy_disks=False,
-                block_device_info=block_device_info)
+                block_device_info=block_device_info,
+                destroy_secrets=False)
 
         mock_get_guest_xml.assert_called_once_with(self.context, instance,
             network_info, mock.ANY, mock.ANY,
@@ -19278,6 +19324,59 @@ def test_cleanup_instance_dir_with_rbd_workaround(self,
         self.assertTrue(instance.cleaned)
         save.assert_called_once_with()
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain',
+                new=mock.Mock())
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_vpmems',
+                new=mock.Mock(return_value=None))
+    def test_cleanup_destroy_secrets(self, mock_disconnect_volume):
+        block_device_info = {
+            'block_device_mapping': [
+                {
+                    'connection_info': mock.sentinel.connection_info
+                }
+            ]
+        }
+        instance = objects.Instance(self.context, **self.test_instance)
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+
+        # Pass destroy_vifs=False and destroy_disks=False as we only care about
+        # asserting the behaviour of destroy_secrets in this test.
+        drvr.cleanup(
+            self.context,
+            instance,
+            network_info={},
+            block_device_info=block_device_info,
+            destroy_vifs=False,
+            destroy_disks=False,
+            destroy_secrets=False
+        )
+        drvr.cleanup(
+            self.context,
+            instance,
+            network_info={},
+            block_device_info=block_device_info,
+            destroy_vifs=False,
+            destroy_disks=False,
+        )
+
+        # Assert that disconnect_volume is called with destroy_secrets=False
+        # and destroy_secrets=True by default
+        mock_disconnect_volume.assert_has_calls([
+            mock.call(
+                self.context,
+                mock.sentinel.connection_info,
+                instance,
+                destroy_secrets=False
+            ),
+            mock.call(
+                self.context,
+                mock.sentinel.connection_info,
+                instance,
+                destroy_secrets=True
+            )
+        ])
+
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_allow_native_luksv1')
     def test_swap_volume_native_luks_blocked(self, mock_allow_native_luksv1,

nova/virt/libvirt/driver.py

@@ -1411,7 +1411,7 @@ def destroy(self, context, instance, network_info, block_device_info=None,
         # unblock the waiting threads and clean up.
         self._device_event_handler.cleanup_waiters(instance.uuid)
         self.cleanup(context, instance, network_info, block_device_info,
-                     destroy_disks)
+                     destroy_disks, destroy_secrets=destroy_secrets)
 
     def _undefine_domain(self, instance):
         try:
@@ -1485,11 +1485,12 @@ def cleanup(self, context, instance, network_info, block_device_info=None,
                 block_device_info=block_device_info,
                 destroy_vifs=destroy_vifs,
                 cleanup_instance_dir=cleanup_instance_dir,
-                cleanup_instance_disks=cleanup_instance_disks)
+                cleanup_instance_disks=cleanup_instance_disks,
+                destroy_secrets=destroy_secrets)
 
     def _cleanup(self, context, instance, network_info, block_device_info=None,
                  destroy_vifs=True, cleanup_instance_dir=False,
-                 cleanup_instance_disks=False):
+                 cleanup_instance_disks=False, destroy_secrets=True):
         """Cleanup the domain and any attached resources from the host.
 
         This method cleans up any pmem devices, unplugs VIFs, disconnects
@@ -1530,7 +1531,9 @@ def _cleanup(self, context, instance, network_info, block_device_info=None,
                 continue
 
             try:
-                self._disconnect_volume(context, connection_info, instance)
+                self._disconnect_volume(
+                    context, connection_info, instance,
+                    destroy_secrets=destroy_secrets)
             except Exception as exc:
                 with excutils.save_and_reraise_exception() as ctxt:
                     if cleanup_instance_disks:
@@ -1847,8 +1850,13 @@ def _should_disconnect_target(self, context, instance, multiattach,
         return (False if connection_count > 1 else True)
 
     def _disconnect_volume(self, context, connection_info, instance,
-                           encryption=None):
-        self._detach_encryptor(context, connection_info, encryption=encryption)
+                           encryption=None, destroy_secrets=True):
+        self._detach_encryptor(
+            context,
+            connection_info,
+            encryption=encryption,
+            destroy_secrets=destroy_secrets
+        )
         vol_driver = self._get_volume_driver(connection_info)
         volume_id = driver_block_device.get_volume_id(connection_info)
         multiattach = connection_info.get('multiattach', False)
@@ -1961,7 +1969,8 @@ def _attach_encryptor(self, context, connection_info, encryption):
                                                    encryption)
             encryptor.attach_volume(context, **encryption)
 
-    def _detach_encryptor(self, context, connection_info, encryption):
+    def _detach_encryptor(self, context, connection_info, encryption,
+                          destroy_secrets=True):
         """Detach the frontend encryptor if one is required by the volume.
 
         The request context is only used when an encryption metadata dict is
@@ -1973,7 +1982,11 @@ def _detach_encryptor(self, context, connection_info, encryption):
         """
         volume_id = driver_block_device.get_volume_id(connection_info)
         if volume_id and self._host.find_secret('volume', volume_id):
+            if not destroy_secrets:
+                LOG.debug("Skipping volume secret destruction")
+                return
             return self._host.delete_secret('volume', volume_id)
+
         if encryption is None:
             encryption = self._get_volume_encryption(context, connection_info)
 
@@ -3729,7 +3742,8 @@ def _hard_reboot(self, context, instance, network_info,
         # we can here without losing data. This allows us to re-initialise from
         # scratch, and hopefully fix, most aspects of a non-functioning guest.
         self.destroy(context, instance, network_info, destroy_disks=False,
-                     block_device_info=block_device_info)
+                     block_device_info=block_device_info,
+                     destroy_secrets=False)
 
         # Convert the system metadata to image metadata
         # NOTE(mdbooth): This is a workaround for stateless Nova compute