From f821db3a54f94b29d4d5b25de7b573b7ebc0ed60 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 25 Feb 2019 13:33:00 +0100 Subject: [PATCH] Allow image checksum to be a URL We allow image_source to be a URL, let us also support URLs for checksums. This change copies handling of multi-file checksum files from metalsmith. Change-Id: Ie4d7e5c79b76bdd72d50eeb384cf10519278a80c Story: #2005061 Task: #29605 --- ironic_python_agent/errors.py | 2 +- ironic_python_agent/extensions/standby.py | 83 +++++++---- .../tests/unit/extensions/test_standby.py | 130 ++++++++++++++++-- .../image-checksum-39b2ceef40933c28.yaml | 5 + 4 files changed, 186 insertions(+), 34 deletions(-) create mode 100644 releasenotes/notes/image-checksum-39b2ceef40933c28.yaml diff --git a/ironic_python_agent/errors.py b/ironic_python_agent/errors.py index 11f769f04..d17c0d122 100644 --- a/ironic_python_agent/errors.py +++ b/ironic_python_agent/errors.py @@ -142,7 +142,7 @@ class ImageDownloadError(RESTError): message = 'Error downloading image' def __init__(self, image_id, msg): - details = 'Download of image id {} failed: {}'.format(image_id, msg) + details = 'Download of image {} failed: {}'.format(image_id, msg) self.secondary_message = msg super(ImageDownloadError, self).__init__(details) diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index 36be96d3e..000ede5f2 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -23,6 +23,7 @@ from oslo_config import cfg from oslo_log import log import requests import six +from six.moves.urllib import parse as urlparse from ironic_python_agent import errors from ironic_python_agent.extensions import base @@ -54,6 +55,60 @@ def _path_to_script(script): return os.path.join(cwd, '..', script) +def _download_with_proxy(image_info, url, image_id): + """Opens a download stream for the given URL. + + :param image_info: Image information dictionary. + :param url: The URL string to request the image from. + :param image_id: Image ID or URL for logging. + + :raises: ImageDownloadError if the download stream was not started + properly. + """ + no_proxy = image_info.get('no_proxy') + if no_proxy: + os.environ['no_proxy'] = no_proxy + proxies = image_info.get('proxies', {}) + verify, cert = utils.get_ssl_client_options(CONF) + resp = requests.get(url, stream=True, proxies=proxies, + verify=verify, cert=cert) + if resp.status_code != 200: + msg = ('Received status code {} from {}, expected 200. Response ' + 'body: {}').format(resp.status_code, url, resp.text) + raise errors.ImageDownloadError(image_id, msg) + return resp + + +def _fetch_checksum(checksum, image_info): + """Fetch checksum from remote location, if needed.""" + if not (checksum.startswith('http://') or checksum.startswith('https://')): + # Not a remote checksum, return as it is. + return checksum + + LOG.debug('Downloading checksums file from %s', checksum) + resp = _download_with_proxy(image_info, checksum, checksum).text + lines = [line.strip() for line in resp.split('\n') if line.strip()] + if not lines: + raise errors.ImageDownloadError(checksum, "Empty checksum file") + elif len(lines) == 1: + # Special case - checksums file with only the checksum itself + if ' ' not in lines[0]: + return lines[0] + + # FIXME(dtantsur): can we assume the same name for all images? + expected_fname = os.path.basename( + urlparse.urlparse(image_info['urls'][0]).path) + for line in lines: + checksum, fname = line.strip().split(None, 1) + # The star symbol designates binary mode, which is the same as text + # mode on GNU systems. + if fname.strip().lstrip('*') == expected_fname: + return checksum.strip() + + raise errors.ImageDownloadError( + checksum, "Checksum file does not contain name %s" % expected_fname) + + def _write_partition_image(image, image_info, device): """Call disk_util to create partition and write the partition image. @@ -217,11 +272,15 @@ class ImageDownload(object): self._hash_algo = hashlib.md5() self._expected_hash_value = image_info['checksum'] + self._expected_hash_value = _fetch_checksum(self._expected_hash_value, + image_info) + details = [] for url in image_info['urls']: try: LOG.info("Attempting to download image from {}".format(url)) - self._request = self._download_file(image_info, url) + self._request = _download_with_proxy(image_info, url, + image_info['id']) except errors.ImageDownloadError as e: failtime = time.time() - self._time log_msg = ('URL: {}; time: {} ' @@ -236,28 +295,6 @@ class ImageDownload(object): details = '\n '.join(details) raise errors.ImageDownloadError(image_info['id'], details) - def _download_file(self, image_info, url): - """Opens a download stream for the given URL. - - :param image_info: Image information dictionary. - :param url: The URL string to request the image from. - - :raises: ImageDownloadError if the download stream was not started - properly. - """ - no_proxy = image_info.get('no_proxy') - if no_proxy: - os.environ['no_proxy'] = no_proxy - proxies = image_info.get('proxies', {}) - verify, cert = utils.get_ssl_client_options(CONF) - resp = requests.get(url, stream=True, proxies=proxies, - verify=verify, cert=cert) - if resp.status_code != 200: - msg = ('Received status code {} from {}, expected 200. Response ' - 'body: {}').format(resp.status_code, url, resp.text) - raise errors.ImageDownloadError(image_info['id'], msg) - return resp - def __iter__(self): """Downloads and returns the next chunk of the image. diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py index 2b5e7b66d..3c1c97dd0 100644 --- a/ironic_python_agent/tests/unit/extensions/test_standby.py +++ b/ironic_python_agent/tests/unit/extensions/test_standby.py @@ -23,13 +23,11 @@ from ironic_python_agent import hardware from ironic_python_agent.tests.unit import base -def _build_fake_image_info(): +def _build_fake_image_info(url='http://example.org'): return { 'id': 'fake_id', 'node_uuid': '1be26c0b-03f2-4d2e-ae87-c02d7f33c123', - 'urls': [ - 'http://example.org', - ], + 'urls': [url], 'checksum': 'abc123', 'image_type': 'whole-disk-image', } @@ -1119,10 +1117,10 @@ class TestStandbyExtension(base.IronicAgentTest): self.assertEqual(expected_msg, result_msg) +@mock.patch('hashlib.md5', autospec=True) +@mock.patch('requests.get', autospec=True) class TestImageDownload(base.IronicAgentTest): - @mock.patch('hashlib.md5', autospec=True) - @mock.patch('requests.get', autospec=True) def test_download_image(self, requests_mock, md5_mock): content = ['SpongeBob', 'SquarePants'] response = requests_mock.return_value @@ -1140,16 +1138,14 @@ class TestImageDownload(base.IronicAgentTest): self.assertEqual(image_info['checksum'], image_download._hash_algo.hexdigest()) - @mock.patch('time.time', autospec=True) - @mock.patch('requests.get', autospec=True) def test_download_image_fail(self, requests_mock, time_mock): response = requests_mock.return_value response.status_code = 401 response.text = 'Unauthorized' time_mock.return_value = 0.0 image_info = _build_fake_image_info() - msg = ('Error downloading image: Download of image id fake_id failed: ' - 'URL: http://example.org; time: 0.0 seconds. Error: ' + msg = ('Error downloading image: Download of image fake_id failed: ' + 'URL: http://example.org; time: .* seconds. Error: ' 'Received status code 401 from http://example.org, expected ' '200. Response body: Unauthorized') self.assertRaisesRegex(errors.ImageDownloadError, msg, @@ -1157,3 +1153,117 @@ class TestImageDownload(base.IronicAgentTest): requests_mock.assert_called_once_with(image_info['urls'][0], cert=None, verify=True, stream=True, proxies={}) + + def test_download_image_and_checksum(self, requests_mock, md5_mock): + content = ['SpongeBob', 'SquarePants'] + fake_cs = "019fe036425da1c562f2e9f5299820bf" + cs_response = mock.Mock() + cs_response.status_code = 200 + cs_response.text = fake_cs + '\n' + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info() + image_info['checksum'] = 'http://example.com/checksum' + md5_mock.return_value.hexdigest.return_value = fake_cs + image_download = standby.ImageDownload(image_info) + + self.assertEqual(content, list(image_download)) + requests_mock.assert_has_calls([ + mock.call('http://example.com/checksum', cert=None, verify=True, + stream=True, proxies={}), + mock.call(image_info['urls'][0], cert=None, verify=True, + stream=True, proxies={}), + ]) + self.assertEqual(fake_cs, image_download._hash_algo.hexdigest()) + + def test_download_image_and_checksum_multiple(self, requests_mock, + md5_mock): + content = ['SpongeBob', 'SquarePants'] + fake_cs = "019fe036425da1c562f2e9f5299820bf" + cs_response = mock.Mock() + cs_response.status_code = 200 + cs_response.text = """ +foobar irrelevant file.img +%s image.img +""" % fake_cs + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info( + 'http://example.com/path/image.img') + image_info['checksum'] = 'http://example.com/checksum' + md5_mock.return_value.hexdigest.return_value = fake_cs + image_download = standby.ImageDownload(image_info) + + self.assertEqual(content, list(image_download)) + requests_mock.assert_has_calls([ + mock.call('http://example.com/checksum', cert=None, verify=True, + stream=True, proxies={}), + mock.call(image_info['urls'][0], cert=None, verify=True, + stream=True, proxies={}), + ]) + self.assertEqual(fake_cs, image_download._hash_algo.hexdigest()) + + def test_download_image_and_checksum_unknown_file(self, requests_mock, + md5_mock): + content = ['SpongeBob', 'SquarePants'] + fake_cs = "019fe036425da1c562f2e9f5299820bf" + cs_response = mock.Mock() + cs_response.status_code = 200 + cs_response.text = """ +foobar irrelevant file.img +%s not-my-image.img +""" % fake_cs + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info( + 'http://example.com/path/image.img') + image_info['checksum'] = 'http://example.com/checksum' + md5_mock.return_value.hexdigest.return_value = fake_cs + self.assertRaisesRegex(errors.ImageDownloadError, + 'Checksum file does not contain name image.img', + standby.ImageDownload, image_info) + + def test_download_image_and_checksum_empty_file(self, requests_mock, + md5_mock): + content = ['SpongeBob', 'SquarePants'] + cs_response = mock.Mock() + cs_response.status_code = 200 + cs_response.text = " " + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info( + 'http://example.com/path/image.img') + image_info['checksum'] = 'http://example.com/checksum' + self.assertRaisesRegex(errors.ImageDownloadError, + 'Empty checksum file', + standby.ImageDownload, image_info) + + def test_download_image_and_checksum_failed(self, requests_mock, md5_mock): + content = ['SpongeBob', 'SquarePants'] + cs_response = mock.Mock() + cs_response.status_code = 400 + cs_response.text = " " + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info( + 'http://example.com/path/image.img') + image_info['checksum'] = 'http://example.com/checksum' + self.assertRaisesRegex(errors.ImageDownloadError, + 'Received status code 400 from ' + 'http://example.com/checksum', + standby.ImageDownload, image_info) diff --git a/releasenotes/notes/image-checksum-39b2ceef40933c28.yaml b/releasenotes/notes/image-checksum-39b2ceef40933c28.yaml new file mode 100644 index 000000000..fe5624ee6 --- /dev/null +++ b/releasenotes/notes/image-checksum-39b2ceef40933c28.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + Allows image checksum to be a URL pointing at a file with the image + checksum or several checksums.