From bdbeb34a17f851fa7e6483e28cde951d26df7951 Mon Sep 17 00:00:00 2001 From: Tobias Urdin Date: Fri, 4 Feb 2022 15:01:36 +0100 Subject: [PATCH 1/2] Cleanup old resize instances dir before resize If there is a failed resize that also failed the cleanup process performed by _cleanup_remote_migration() the retry of the resize will fail because it cannot rename the current instances directory to _resize. This renames the _cleanup_failed_migration() that does the same logic we want to _cleanup_failed_instance_base() and uses it for both migration and resize cleanup of directory. It then simply calls _cleanup_failed_instances_base() with the resize dir path before trying a resize. Closes-Bug: 1960230 Change-Id: I7412b16be310632da59a6139df9f0913281b5d77 (cherry picked from commit 9111b99f739d41c092db8d01712a5aa72388b5fb) (cherry picked from commit 31179f62f1c832e0e894d04e9c9dd59978577cc0) --- nova/tests/unit/virt/libvirt/test_driver.py | 11 ++++++++--- nova/virt/libvirt/driver.py | 11 +++++++---- ...cleanup-instances-dir-resize-56282e1b436a4908.yaml | 6 ++++++ 3 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/bug-1960230-cleanup-instances-dir-resize-56282e1b436a4908.yaml diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 99d30c45903..7186fd7fec1 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -21471,6 +21471,8 @@ def test_migrate_disk_and_power_off_exception( context.get_admin_context(), ins_ref, '10.0.0.2', flavor_obj, None) + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.' + '_cleanup_failed_instance_base') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs') @mock.patch('nova.virt.libvirt.utils.save_and_migrate_vtpm_dir') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.' @@ -21487,7 +21489,7 @@ def _test_migrate_disk_and_power_off( self, ctxt, flavor_obj, mock_execute, mock_exists, mock_rename, mock_is_shared, mock_get_host_ip, mock_destroy, mock_get_disk_info, mock_vtpm, mock_unplug_vifs, - block_device_info=None, params_for_instance=None): + mock_cleanup, block_device_info=None, params_for_instance=None): """Test for nova.virt.libvirt.driver.LivirtConnection .migrate_disk_and_power_off. """ @@ -21502,6 +21504,8 @@ def _test_migrate_disk_and_power_off( ctxt, instance, '10.0.0.2', flavor_obj, None, block_device_info=block_device_info) + mock_cleanup.assert_called_once() + mock_cleanup.reset_mock() self.assertEqual(out, disk_info_text) mock_vtpm.assert_called_with( instance.uuid, mock.ANY, mock.ANY, '10.0.0.2', mock.ANY, mock.ANY) @@ -21512,6 +21516,7 @@ def _test_migrate_disk_and_power_off( ctxt, instance, '10.0.0.1', flavor_obj, None, block_device_info=block_device_info) + mock_cleanup.assert_called_once() self.assertEqual(out, disk_info_text) mock_vtpm.assert_called_with( instance.uuid, mock.ANY, mock.ANY, '10.0.0.1', mock.ANY, mock.ANY) @@ -22441,8 +22446,8 @@ def test_finish_revert_migration_snap_backend_image_does_not_exist(self): self.assertFalse(drvr.image_backend.remove_snap.called) @mock.patch.object(shutil, 'rmtree') - def test_cleanup_failed_migration(self, mock_rmtree): - self.drvr._cleanup_failed_migration('/fake/inst') + def test_cleanup_failed_instance_base(self, mock_rmtree): + self.drvr._cleanup_failed_instance_base('/fake/inst') mock_rmtree.assert_called_once_with('/fake/inst') @mock.patch.object(libvirt_driver.LibvirtDriver, '_cleanup_resize') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 8719d67af30..2d775c7fbe3 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -10851,6 +10851,9 @@ def migrate_disk_and_power_off(self, context, instance, dest, disk_info = self._get_instance_disk_info(instance, block_device_info) try: + # If cleanup failed in previous resize attempts we try to remedy + # that before a resize is tried again + self._cleanup_failed_instance_base(inst_base_resize) os.rename(inst_base, inst_base_resize) # if we are migrating the instance with shared instance path then # create the directory. If it is a remote node the directory @@ -11074,9 +11077,9 @@ def finish_migration( LOG.debug("finish_migration finished successfully.", instance=instance) - def _cleanup_failed_migration(self, inst_base): - """Make sure that a failed migrate doesn't prevent us from rolling - back in a revert. + def _cleanup_failed_instance_base(self, inst_base): + """Make sure that a failed migrate or resize doesn't prevent us from + rolling back in a revert or retrying a resize. """ try: shutil.rmtree(inst_base) @@ -11132,7 +11135,7 @@ def finish_revert_migration( # that would conflict. Also, don't fail on the rename if the # failure happened early. if os.path.exists(inst_base_resize): - self._cleanup_failed_migration(inst_base) + self._cleanup_failed_instance_base(inst_base) os.rename(inst_base_resize, inst_base) root_disk = self.image_backend.by_name(instance, 'disk') diff --git a/releasenotes/notes/bug-1960230-cleanup-instances-dir-resize-56282e1b436a4908.yaml b/releasenotes/notes/bug-1960230-cleanup-instances-dir-resize-56282e1b436a4908.yaml new file mode 100644 index 00000000000..7a89c66092f --- /dev/null +++ b/releasenotes/notes/bug-1960230-cleanup-instances-dir-resize-56282e1b436a4908.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed bug `1960230 `_ that + prevented resize of instances that had previously failed and not been + cleaned up. From 719a2a6089ec240d23caeb93ac25ed5259d8d9bc Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Thu, 10 Nov 2022 09:55:48 -0800 Subject: [PATCH 2/2] [stable-only][cve] Check VMDK create-type against an allowed list NOTE(sbauza): Stable policy allows us to proactively merge a backport without waiting for the parent patch to be merged (exception to rule #4 in [1]. Marking [stable-only] in order to silence nova-tox-validate-backport [1] https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes Related-Bug: #1996188 Change-Id: I5a399f1d3d702bfb76c067893e9c924904c8c360 --- nova/conf/compute.py | 9 ++++++ nova/tests/unit/virt/test_images.py | 46 +++++++++++++++++++++++++++++ nova/virt/images.py | 31 +++++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/nova/conf/compute.py b/nova/conf/compute.py index e787424977a..fb65d8dffd9 100644 --- a/nova/conf/compute.py +++ b/nova/conf/compute.py @@ -1006,6 +1006,15 @@ * ``[scheduler]query_placement_for_image_type_support`` - enables filtering computes based on supported image types, which is required to be enabled for this to take effect. +"""), + cfg.ListOpt('vmdk_allowed_types', + default=['streamOptimized', 'monolithicSparse'], + help=""" +A list of strings describing allowed VMDK "create-type" subformats +that will be allowed. This is recommended to only include +single-file-with-sparse-header variants to avoid potential host file +exposure due to processing named extents. If this list is empty, then no +form of VMDK image will be allowed. """), ] diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index 085b169db3c..563330b5414 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -16,6 +16,8 @@ import mock from oslo_concurrency import processutils +from oslo_serialization import jsonutils +from oslo_utils import imageutils from nova.compute import utils as compute_utils from nova import exception @@ -135,3 +137,47 @@ def test_convert_image_without_direct_io_support(self, mock_execute, '-O', 'out_format', '-f', 'in_format', 'source', 'dest') mock_disk_op_sema.__enter__.assert_called_once() self.assertTupleEqual(expected, mock_execute.call_args[0]) + + def test_convert_image_vmdk_allowed_list_checking(self): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + + # If the format is not in the allowed list, we should get an error + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With the format in the allowed list, no error + self.flags(vmdk_allowed_types=['streamOptimized', 'monolithicFlat', + 'monolithicSparse'], + group='compute') + images.check_vmdk_image('foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With an empty list, allow nothing + self.flags(vmdk_allowed_types=[], group='compute') + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + @mock.patch.object(images, 'fetch') + @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info') + def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + mock_info.return_value = jsonutils.dumps(info) + with mock.patch('os.path.exists', return_value=True): + e = self.assertRaises(exception.ImageUnacceptable, + images.fetch_to_raw, None, 'foo', 'anypath') + self.assertIn('Invalid VMDK create-type specified', str(e)) diff --git a/nova/virt/images.py b/nova/virt/images.py index 5358f3766ac..f13c8722909 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -110,6 +110,34 @@ def get_info(context, image_href): return IMAGE_API.get(context, image_href) +def check_vmdk_image(image_id, data): + # Check some rules about VMDK files. Specifically we want to make + # sure that the "create-type" of the image is one that we allow. + # Some types of VMDK files can reference files outside the disk + # image and we do not want to allow those for obvious reasons. + + types = CONF.compute.vmdk_allowed_types + + if not len(types): + LOG.warning('Refusing to allow VMDK image as vmdk_allowed_' + 'types is empty') + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + try: + create_type = data.format_specific['data']['create-type'] + except KeyError: + msg = _('Unable to determine VMDK create-type') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + if create_type not in CONF.compute.vmdk_allowed_types: + LOG.warning('Refusing to process VMDK file with create-type of %r ' + 'which is not in allowed set of: %s', create_type, + ','.join(CONF.compute.vmdk_allowed_types)) + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + def fetch_to_raw(context, image_href, path, trusted_certs=None): path_tmp = "%s.part" % path fetch(context, image_href, path_tmp, trusted_certs) @@ -129,6 +157,9 @@ def fetch_to_raw(context, image_href, path, trusted_certs=None): reason=(_("fmt=%(fmt)s backed by: %(backing_file)s") % {'fmt': fmt, 'backing_file': backing_file})) + if fmt == 'vmdk': + check_vmdk_image(image_href, data) + if fmt != "raw" and CONF.force_raw_images: staged = "%s.converted" % path LOG.debug("%s was %s, converting to raw", image_href, fmt)