diff --git a/.zuul.yaml b/.zuul.yaml index 9bf327b590..40222e08c6 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -209,6 +209,7 @@ - job: name: tempest-integrated-storage-import parent: tempest-integrated-storage + timeout: 10800 description: | The regular tempest-integrated-storage job but with glance metadata injection post-run: playbooks/post-check-metadata-injection.yaml @@ -241,13 +242,13 @@ test-config: "$TEMPEST_CONFIG": image: - image_caching_enabled: True + build_timeout: 600 disk_formats: qcow2,ari,aki,vhd,vmdk,raw,ami,vdi,iso,vhdx + image_caching_enabled: True - job: name: glance-multistore-cinder-import parent: tempest-integrated-storage-import - timeout: 10800 description: | The regular import workflow job to test with multiple cinder stores vars: @@ -256,11 +257,6 @@ GLANCE_ENABLE_MULTIPLE_STORES: True CINDER_ENABLED_BACKENDS: lvm:lvmdriver-1,lvm:lvmdriver-2 GLANCE_CINDER_DEFAULT_BACKEND: lvmdriver-1 - devstack_local_conf: - test-config: - "$TEMPEST_CONFIG": - image: - build_timeout: 600 - job: name: tempest-integrated-storage-import-standalone diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py index 2f52d0ac28..9aef9bf6e0 100644 --- a/glance/async_/flows/api_image_import.py +++ b/glance/async_/flows/api_image_import.py @@ -207,6 +207,10 @@ def image_locations(self): # should have moderated access like all the other things here. return copy.deepcopy(self._image.locations) + @property + def image_disk_format(self): + return self._image.disk_format + @property def image_status(self): return self._image.status diff --git a/glance/async_/flows/plugins/image_conversion.py b/glance/async_/flows/plugins/image_conversion.py index 91e8cf944e..90a846ace7 100644 --- a/glance/async_/flows/plugins/image_conversion.py +++ b/glance/async_/flows/plugins/image_conversion.py @@ -26,6 +26,7 @@ from taskflow import task from glance.async_ import utils +from glance.common import format_inspector from glance.i18n import _, _LI LOG = logging.getLogger(__name__) @@ -88,8 +89,40 @@ def _execute(self, action, file_path, **kwargs): 'target': target_format} self.dest_path = dest_path + source_format = action.image_disk_format + inspector_cls = format_inspector.get_inspector(source_format) + if not inspector_cls: + # We cannot convert from disk_format types that qemu-img doesn't + # support (like iso, ploop, etc). The ones it supports overlaps + # with the ones we have inspectors for, so reject conversion for + # any format we don't have an inspector for. + raise RuntimeError( + 'Unable to convert from format %s' % source_format) + + # Use our own cautious inspector module (if we have one for this + # format) to make sure a file is the format the submitter claimed + # it is and that it passes some basic safety checks _before_ we run + # qemu-img on it. + # See https://bugs.launchpad.net/nova/+bug/2059809 for details. + try: + inspector = inspector_cls.from_file(src_path) + if not inspector.safety_check(): + LOG.error('Image failed %s safety check; aborting conversion', + source_format) + raise RuntimeError('Image has disallowed configuration') + except RuntimeError: + raise + except format_inspector.ImageFormatError as e: + LOG.error('Image claimed to be %s format failed format ' + 'inspection: %s', source_format, e) + raise RuntimeError('Image format detection failed') + except Exception as e: + LOG.exception('Unknown error inspecting image format: %s', e) + raise RuntimeError('Unable to inspect image') + try: stdout, stderr = putils.trycmd("qemu-img", "info", + "-f", source_format, "--output=json", src_path, prlimit=utils.QEMU_IMG_PROC_LIMITS, @@ -106,13 +139,10 @@ def _execute(self, action, file_path, **kwargs): raise RuntimeError(stderr) metadata = json.loads(stdout) - try: - source_format = metadata['format'] - except KeyError: - msg = ("Failed to do introspection as part of image " - "conversion for %(iid)s: Source format not reported") - LOG.error(msg, {'iid': self.image_id}) - raise RuntimeError(msg) + if metadata.get('format') != source_format: + LOG.error('Image claiming to be %s reported as %s by qemu-img', + source_format, metadata.get('format', 'unknown')) + raise RuntimeError('Image metadata disagrees about format') virtual_size = metadata.get('virtual-size', 0) action.set_image_attribute(virtual_size=virtual_size) diff --git a/glance/common/format_inspector.py b/glance/common/format_inspector.py index a11ff1a5e0..65502d8892 100755 --- a/glance/common/format_inspector.py +++ b/glance/common/format_inspector.py @@ -366,6 +366,23 @@ def safety_check(self): not self.has_unknown_features) +class QEDInspector(FileInspector): + def __init__(self, tracing=False): + super().__init__(tracing) + self.new_region('header', CaptureRegion(0, 512)) + + @property + def format_match(self): + if not self.region('header').complete: + return False + return self.region('header').data.startswith(b'QED\x00') + + def safety_check(self): + # QED format is not supported by anyone, but we want to detect it + # and mark it as just always unsafe. + return False + + # The VHD (or VPC as QEMU calls it) format consists of a big-endian # 512-byte "footer" at the beginning of the file with various # information, most of which does not matter to us: @@ -866,19 +883,52 @@ def close(self): self._source.close() +ALL_FORMATS = { + 'raw': FileInspector, + 'qcow2': QcowInspector, + 'vhd': VHDInspector, + 'vhdx': VHDXInspector, + 'vmdk': VMDKInspector, + 'vdi': VDIInspector, + 'qed': QEDInspector, +} + + def get_inspector(format_name): """Returns a FormatInspector class based on the given name. :param format_name: The name of the disk_format (raw, qcow2, etc). :returns: A FormatInspector or None if unsupported. """ - formats = { - 'raw': FileInspector, - 'qcow2': QcowInspector, - 'vhd': VHDInspector, - 'vhdx': VHDXInspector, - 'vmdk': VMDKInspector, - 'vdi': VDIInspector, - } - - return formats.get(format_name) + + return ALL_FORMATS.get(format_name) + + +def detect_file_format(filename): + """Attempts to detect the format of a file. + + This runs through a file one time, running all the known inspectors in + parallel. It stops reading the file once one of them matches or all of + them are sure they don't match. + + Returns the FileInspector that matched, if any. None if 'raw'. + """ + inspectors = {k: v() for k, v in ALL_FORMATS.items()} + with open(filename, 'rb') as f: + for chunk in chunked_reader(f): + for format, inspector in list(inspectors.items()): + try: + inspector.eat_chunk(chunk) + except ImageFormatError: + # No match, so stop considering this format + inspectors.pop(format) + continue + if (inspector.format_match and inspector.complete and + format != 'raw'): + # First complete match (other than raw) wins + return inspector + if all(i.complete for i in inspectors.values()): + # If all the inspectors are sure they are not a match, avoid + # reading to the end of the file to settle on 'raw'. + break + return inspectors['raw'] diff --git a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py index 44ea5ac28d..e59749fa09 100644 --- a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py +++ b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import fixtures import json import os import sys @@ -25,6 +26,7 @@ import glance.async_.flows.api_image_import as import_flow import glance.async_.flows.plugins.image_conversion as image_conversion from glance.async_ import utils as async_utils +from glance.common import format_inspector from glance.common import utils from glance import domain from glance import gateway @@ -91,6 +93,11 @@ def setUp(self): self.image_id, self.task.task_id) + self.inspector_mock = mock.MagicMock() + self.useFixture(fixtures.MockPatch('glance.common.format_inspector.' + 'get_inspector', + self.inspector_mock)) + @mock.patch.object(os, 'stat') @mock.patch.object(os, 'remove') def test_image_convert_success(self, mock_os_remove, mock_os_stat): @@ -105,7 +112,7 @@ def test_image_convert_success(self, mock_os_remove, mock_os_stat): image = mock.MagicMock(image_id=self.image_id, virtual_size=None, extra_properties={ 'os_glance_import_task': self.task.task_id}, - disk_format='qcow2') + disk_format='raw') self.img_repo.get.return_value = image with mock.patch.object(processutils, 'execute') as exc_mock: @@ -127,7 +134,7 @@ def test_image_convert_success(self, mock_os_remove, mock_os_stat): self.assertEqual(456, image.virtual_size) self.assertEqual(123, image.size) - def _setup_image_convert_info_fail(self): + def _setup_image_convert_info_fail(self, disk_format='qcow2'): image_convert = image_conversion._ConvertImage(self.context, self.task.task_id, self.task_type, @@ -137,7 +144,7 @@ def _setup_image_convert_info_fail(self): image = mock.MagicMock(image_id=self.image_id, virtual_size=None, extra_properties={ 'os_glance_import_task': self.task.task_id}, - disk_format='qcow2') + disk_format=disk_format) self.img_repo.get.return_value = image return image_convert @@ -149,6 +156,7 @@ def test_image_convert_fails_inspection(self): convert.execute, 'file:///test/path.raw') exc_mock.assert_called_once_with( 'qemu-img', 'info', + '-f', 'qcow2', '--output=json', '/test/path.raw', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, @@ -165,6 +173,7 @@ def test_image_convert_inspection_reports_error(self): convert.execute, 'file:///test/path.raw') exc_mock.assert_called_once_with( 'qemu-img', 'info', + '-f', 'qcow2', '--output=json', '/test/path.raw', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, @@ -201,6 +210,36 @@ def test_image_convert_invalid_qcow_data_file(self): self.assertEqual('QCOW images with data-file set are not allowed', str(e)) + def test_image_convert_no_inspector_match(self): + convert = self._setup_image_convert_info_fail() + self.inspector_mock.return_value = None + self.assertRaisesRegex(RuntimeError, + 'Unable to convert from format', + convert.execute, 'file:///test/path.hpfs') + + def test_image_convert_fails_inspection_safety_check(self): + convert = self._setup_image_convert_info_fail() + inspector = self.inspector_mock.return_value.from_file.return_value + inspector.safety_check.return_value = False + self.assertRaisesRegex(RuntimeError, + 'Image has disallowed configuration', + convert.execute, 'file:///test/path.qcow') + + def test_image_convert_fails_inspection_format_check(self): + convert = self._setup_image_convert_info_fail() + self.inspector_mock.return_value.from_file.side_effect = ( + format_inspector.ImageFormatError()) + self.assertRaisesRegex(RuntimeError, + 'Image format detection failed', + convert.execute, 'file:///test/path.qcow') + + def test_image_convert_fails_inspection_error(self): + convert = self._setup_image_convert_info_fail() + self.inspector_mock.return_value.from_file.side_effect = ValueError + self.assertRaisesRegex(RuntimeError, + 'Unable to inspect image', + convert.execute, 'file:///test/path.qcow') + def _test_image_convert_invalid_vmdk(self): data = {'format': 'vmdk', 'format-specific': { @@ -208,7 +247,7 @@ def _test_image_convert_invalid_vmdk(self): 'create-type': 'monolithicFlat', }}} - convert = self._setup_image_convert_info_fail() + convert = self._setup_image_convert_info_fail(disk_format='vmdk') with mock.patch.object(processutils, 'execute') as exc_mock: exc_mock.return_value = json.dumps(data), '' convert.execute('file:///test/path.vmdk') @@ -237,7 +276,7 @@ def test_image_convert_valid_vmdk(self): self._test_image_convert_invalid_vmdk) def test_image_convert_fails(self): - convert = self._setup_image_convert_info_fail() + convert = self._setup_image_convert_info_fail(disk_format='raw') with mock.patch.object(processutils, 'execute') as exc_mock: exc_mock.side_effect = [('{"format":"raw"}', ''), OSError('convert_fail')] @@ -245,6 +284,7 @@ def test_image_convert_fails(self): convert.execute, 'file:///test/path.raw') exc_mock.assert_has_calls( [mock.call('qemu-img', 'info', + '-f', 'raw', '--output=json', '/test/path.raw', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, @@ -257,7 +297,7 @@ def test_image_convert_fails(self): self.img_repo.save.assert_not_called() def test_image_convert_reports_fail(self): - convert = self._setup_image_convert_info_fail() + convert = self._setup_image_convert_info_fail(disk_format='raw') with mock.patch.object(processutils, 'execute') as exc_mock: exc_mock.side_effect = [('{"format":"raw"}', ''), ('', 'some error')] @@ -265,6 +305,7 @@ def test_image_convert_reports_fail(self): convert.execute, 'file:///test/path.raw') exc_mock.assert_has_calls( [mock.call('qemu-img', 'info', + '-f', 'raw', '--output=json', '/test/path.raw', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, @@ -282,9 +323,10 @@ def test_image_convert_fails_source_format(self): exc_mock.return_value = ('{}', '') exc = self.assertRaises(RuntimeError, convert.execute, 'file:///test/path.raw') - self.assertIn('Source format not reported', str(exc)) + self.assertIn('Image metadata disagrees about format', str(exc)) exc_mock.assert_called_once_with( 'qemu-img', 'info', + '-f', 'qcow2', '--output=json', '/test/path.raw', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, @@ -302,6 +344,7 @@ def test_image_convert_same_format_does_nothing(self): # Make sure we only called qemu-img for inspection, not conversion exc_mock.assert_called_once_with( 'qemu-img', 'info', + '-f', 'qcow2', '--output=json', '/test/path.qcow', prlimit=async_utils.QEMU_IMG_PROC_LIMITS, diff --git a/glance/tests/unit/common/test_format_inspector.py b/glance/tests/unit/common/test_format_inspector.py index ceb0c3caab..59d828de57 100644 --- a/glance/tests/unit/common/test_format_inspector.py +++ b/glance/tests/unit/common/test_format_inspector.py @@ -179,6 +179,12 @@ def test_from_file_reads_minimum(self): # a local file. self.assertLess(fmt.actual_size, file_size) + def test_qed_always_unsafe(self): + img = self._create_img('qed', 10 * units.Mi) + fmt = format_inspector.get_inspector('qed').from_file(img) + self.assertTrue(fmt.format_match) + self.assertFalse(fmt.safety_check()) + def _test_vmdk_bad_descriptor_offset(self, subformat=None): format_name = 'vmdk' image_size = 10 * units.Mi @@ -309,62 +315,6 @@ def test_qcow2_feature_flag_checks(self): data[0x4F] = 0x80 self.assertTrue(inspector.has_unknown_features) - def test_vmdk_safety_checks(self): - region = format_inspector.CaptureRegion(0, 0) - inspector = format_inspector.VMDKInspector() - inspector.new_region('descriptor', region) - - # This should be a legit VMDK descriptor which comments, blank lines, - # an extent, some ddb content, and some header values. - legit_desc = ['# This is a comment', - '', - ' ', - 'createType=monolithicSparse', - 'RW 1234 SPARSE "foo.vmdk"', - 'ddb.adapterType = "MFM', - '# EOF'] - region.data = ('\n'.join(legit_desc)).encode('ascii') - region.length = len(region.data) - self.assertTrue(inspector.safety_check()) - - # Any of these lines should trigger an error indicating that there is - # something in the descriptor we don't understand - bad_lines = [ - '#\U0001F4A9', - 'header Name=foo', - 'foo bar', - 'WR 123 SPARSE "foo.vmdk"', - ] - - for bad_line in bad_lines: - # Encode as UTF-8 purely so we can test that anything non-ASCII - # will trigger the decode check - region.data = bad_line.encode('utf-8') - region.length = len(region.data) - self.assertRaisesRegex(format_inspector.ImageFormatError, - 'Invalid VMDK descriptor', - inspector.safety_check) - - # Extents with slashes in the name fail the safety check - region.data = b'RW 123 SPARSE "/etc/shadow"' - region.length = len(region.data) - self.assertFalse(inspector.safety_check()) - - # A descriptor that specifies no extents fails the safety check - region.data = b'# Nothing' - region.length = len(region.data) - self.assertFalse(inspector.safety_check()) - - def test_vmdk_reject_footer(self): - data = struct.pack('<4sIIQQQQIQQ', b'KDMV', 3, 0, 0, 0, 0, 1, 0, 0, - format_inspector.VMDKInspector.GD_AT_END) - inspector = format_inspector.VMDKInspector() - inspector.region('header').data = data - inspector.region('header').length = len(data) - self.assertRaisesRegex(format_inspector.ImageFormatError, - 'footer', - inspector.post_process) - def test_vdi(self): self._test_format('vdi')