From 3621535d6733a8493a39eb1a98994694ed8f16c9 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 3 Sep 2020 14:23:52 +0200 Subject: [PATCH] Stop requiring checksums with file images Neither deploy method requires checksums with file images, they're simply ignored. Deprecate providing them. Change-Id: Ia123c1d3c57cc2814e3f971209cbee3ab336f7bd --- metalsmith/sources.py | 28 +++++++++++-------- metalsmith/test/test_cmd.py | 13 --------- metalsmith/test/test_provisioner.py | 7 ++--- metalsmith/test/test_sources.py | 13 ++++----- .../notes/file-checksum-d19370a8fde00a81.yaml | 8 ++++++ 5 files changed, 32 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/file-checksum-d19370a8fde00a81.yaml diff --git a/metalsmith/sources.py b/metalsmith/sources.py index 8e36030..930c555 100644 --- a/metalsmith/sources.py +++ b/metalsmith/sources.py @@ -19,6 +19,7 @@ import abc import logging import os from urllib import parse as urlparse +import warnings import openstack.exceptions import requests @@ -196,24 +197,26 @@ class FileWholeDiskImage(_Source): available at the same location to all conductors in the same group. """ - def __init__(self, location, checksum): + def __init__(self, location, checksum=None): """Create a local file source. :param location: Location of the image, optionally starting with ``file://``. - :param checksum: MD5 checksum of the image. + :param checksum: MD5 checksum of the image. DEPRECATED: checksums do + not actually work with file images. """ if not location.startswith('file://'): location = 'file://' + location self.location = location self.checksum = checksum + if self.checksum: + warnings.warn("Checksums cannot be used with file images", + DeprecationWarning) def _node_updates(self, connection): - LOG.debug('Image: %(image)s, checksum %(checksum)s', - {'image': self.location, 'checksum': self.checksum}) + LOG.debug('Image: %s', self.location) return { 'image_source': self.location, - 'image_checksum': self.checksum, } @@ -227,7 +230,8 @@ class FilePartitionImage(FileWholeDiskImage): available at the same location to all conductors in the same group. """ - def __init__(self, location, kernel_location, ramdisk_location, checksum): + def __init__(self, location, kernel_location, ramdisk_location, + checksum=None): """Create a local file source. :param location: Location of the image, optionally starting with @@ -236,7 +240,8 @@ class FilePartitionImage(FileWholeDiskImage): optionally starting with ``file://``. :param ramdisk_location: Location of the ramdisk of the image, optionally starting with ``file://``. - :param checksum: MD5 checksum of the image. + :param checksum: MD5 checksum of the image. DEPRECATED: checksums do + not actually work with file images. """ super(FilePartitionImage, self).__init__(location, checksum) if not kernel_location.startswith('file://'): @@ -289,14 +294,13 @@ def detect(image, kernel=None, ramdisk=None, checksum=None): kernel_type = _link_type(kernel) ramdisk_type = _link_type(ramdisk) - if not checksum: - raise ValueError('checksum is required for HTTP and file images') + if image_type == 'http' and not checksum: + raise ValueError('checksum is required for HTTP images') if image_type == 'file': if (kernel_type not in (None, 'file') - or ramdisk_type not in (None, 'file') - or checksum_type == 'http'): - raise ValueError('kernal, ramdisk and checksum can only be files ' + or ramdisk_type not in (None, 'file')): + raise ValueError('kernel and ramdisk can only be files ' 'for file images') if kernel or ramdisk: diff --git a/metalsmith/test/test_cmd.py b/metalsmith/test/test_cmd.py index 3841b07..d8e7349 100644 --- a/metalsmith/test/test_cmd.py +++ b/metalsmith/test/test_cmd.py @@ -463,20 +463,17 @@ class TestDeploy(Base): def test_args_file_whole_disk_image(self, mock_pr): args = ['deploy', '--image', 'file:///var/lib/ironic/image.img', - '--image-checksum', '95e750180c7921ea0d545c7165db66b8', '--network', 'mynet', '--resource-class', 'compute'] self._check(mock_pr, args, {}, {'image': mock.ANY}) source = mock_pr.return_value.provision_node.call_args[1]['image'] self.assertIsInstance(source, sources.FileWholeDiskImage) self.assertEqual('file:///var/lib/ironic/image.img', source.location) - self.assertEqual('95e750180c7921ea0d545c7165db66b8', source.checksum) def test_args_file_partition_disk_image(self, mock_pr): args = ['deploy', '--image', 'file:///var/lib/ironic/image.img', '--image-kernel', 'file:///var/lib/ironic/image.vmlinuz', '--image-ramdisk', 'file:///var/lib/ironic/image.initrd', - '--image-checksum', '95e750180c7921ea0d545c7165db66b8', '--network', 'mynet', '--resource-class', 'compute'] self._check(mock_pr, args, {}, {'image': mock.ANY}) @@ -487,16 +484,6 @@ class TestDeploy(Base): source.kernel_location) self.assertEqual('file:///var/lib/ironic/image.initrd', source.ramdisk_location) - self.assertEqual('95e750180c7921ea0d545c7165db66b8', source.checksum) - - @mock.patch.object(_cmd.LOG, 'critical', autospec=True) - def test_args_file_image_without_checksum(self, mock_log, mock_pr): - args = ['deploy', '--image', 'file:///var/lib/ironic/image.img', - '--resource-class', 'compute'] - self.assertRaises(SystemExit, _cmd.main, args) - self.assertTrue(mock_log.called) - self.assertFalse(mock_pr.return_value.reserve_node.called) - self.assertFalse(mock_pr.return_value.provision_node.called) @mock.patch.object(_cmd.LOG, 'critical', autospec=True) def test_args_file_image_with_incorrect_kernel(self, mock_log, mock_pr): diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index 089c9b0..08b4fe2 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -1102,13 +1102,12 @@ abcd image def test_with_file_whole_disk(self): self.instance_info['image_source'] = 'file:///foo/img' - self.instance_info['image_checksum'] = 'abcd' del self.instance_info['kernel'] del self.instance_info['ramdisk'] inst = self.pr.provision_node( self.node, - sources.FileWholeDiskImage('file:///foo/img', checksum='abcd'), + sources.FileWholeDiskImage('file:///foo/img'), [{'network': 'network'}]) self.assertEqual(inst.uuid, self.node.id) @@ -1132,7 +1131,6 @@ abcd image def test_with_file_partition(self): self.instance_info['image_source'] = 'file:///foo/img' - self.instance_info['image_checksum'] = 'abcd' self.instance_info['kernel'] = 'file:///foo/vmlinuz' self.instance_info['ramdisk'] = 'file:///foo/initrd' @@ -1140,8 +1138,7 @@ abcd image self.node, sources.FilePartitionImage('/foo/img', '/foo/vmlinuz', - '/foo/initrd', - checksum='abcd'), + '/foo/initrd'), [{'network': 'network'}]) self.assertEqual(inst.uuid, self.node.id) diff --git a/metalsmith/test/test_sources.py b/metalsmith/test/test_sources.py index 353d5c5..7b0645f 100644 --- a/metalsmith/test/test_sources.py +++ b/metalsmith/test/test_sources.py @@ -64,25 +64,25 @@ class TestDetect(unittest.TestCase): sources.detect, 'foobar', **kwargs) def test_checksum_required(self): - for tp in ('file', 'http', 'https'): + for tp in ('http', 'https'): self.assertRaisesRegex(ValueError, 'checksum is required', sources.detect, '%s://foo' % tp) def test_file_whole_disk(self): - source = sources.detect('file:///image', checksum='abcd') + source = sources.detect('file:///image') self.assertIs(source.__class__, sources.FileWholeDiskImage) self.assertEqual(source.location, 'file:///image') - self.assertEqual(source.checksum, 'abcd') + self.assertIsNone(source.checksum) source._validate(mock.Mock(), None) def test_file_partition_disk(self): - source = sources.detect('file:///image', checksum='abcd', + source = sources.detect('file:///image', kernel='file:///kernel', ramdisk='file:///ramdisk') self.assertIs(source.__class__, sources.FilePartitionImage) self.assertEqual(source.location, 'file:///image') - self.assertEqual(source.checksum, 'abcd') + self.assertIsNone(source.checksum) self.assertEqual(source.kernel_location, 'file:///kernel') self.assertEqual(source.ramdisk_location, 'file:///ramdisk') @@ -99,8 +99,7 @@ class TestDetect(unittest.TestCase): for kwargs in [{'kernel': 'foo'}, {'ramdisk': 'foo'}, {'kernel': 'http://foo'}, - {'ramdisk': 'http://foo'}, - {'checksum': 'http://foo'}]: + {'ramdisk': 'http://foo'}]: kwargs.setdefault('checksum', 'abcd') self.assertRaisesRegex(ValueError, 'can only be files', sources.detect, 'file:///image', **kwargs) diff --git a/releasenotes/notes/file-checksum-d19370a8fde00a81.yaml b/releasenotes/notes/file-checksum-d19370a8fde00a81.yaml new file mode 100644 index 0000000..0ae574d --- /dev/null +++ b/releasenotes/notes/file-checksum-d19370a8fde00a81.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Checksums are no longer required (nor used) with file images. +deprecations: + - | + Providing checksums for file images is deprecated. No deploy implementation + actually supports them.