From 22dcfc65eebb6dcafee8bf3afe5749cf4e7c7f20 Mon Sep 17 00:00:00 2001 From: Galyna Zholtkevych Date: Fri, 16 Sep 2016 11:00:29 +0300 Subject: [PATCH] Validate the generated swift temp url During deployment, a swift temporary URL may be generated, to be used for getting the image for an instance. This validates the generated URL to make sure that it can be accessed. If validation fails, an exception is raised and the error is logged. This is helpful when diagnosing the problem with a deployment. Change-Id: I1f62bc9b5646d173ec6a3beb6282a5e6002c2030 Closes-Bug: 1512186 --- ironic/common/image_service.py | 9 ++++-- ironic/drivers/modules/deploy_utils.py | 29 +++++++++++++------ .../tests/unit/common/test_image_service.py | 11 +++++++ .../unit/drivers/modules/test_deploy_utils.py | 24 ++++++++++----- ...e-url-wnen-deploying-8820f4398ea9de9f.yaml | 4 +++ 5 files changed, 58 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/validate-image-url-wnen-deploying-8820f4398ea9de9f.yaml diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index 6016ac11e9..d54afa8b42 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -100,23 +100,26 @@ class BaseImageService(object): class HttpImageService(BaseImageService): """Provides retrieval of disk images using HTTP.""" - def validate_href(self, image_href): + def validate_href(self, image_href, secret=False): """Validate HTTP image reference. :param image_href: Image reference. + :param secret: Specify if image_href being validated should not be + shown in exception message. :raises: exception.ImageRefValidationFailed if HEAD request failed or returned response code not equal to 200. :returns: Response to HEAD request. """ + output_url = 'secreturl' if secret else image_href try: response = requests.head(image_href) if response.status_code != http_client.OK: raise exception.ImageRefValidationFailed( - image_href=image_href, + image_href=output_url, reason=_("Got HTTP code %s instead of 200 in response to " "HEAD request.") % response.status_code) except requests.RequestException as e: - raise exception.ImageRefValidationFailed(image_href=image_href, + raise exception.ImageRefValidationFailed(image_href=output_url, reason=e) return response diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 40f8d56d93..9d7679fc85 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -1272,6 +1272,23 @@ def build_instance_info_for_deploy(task): :raises: exception.ImageRefValidationFailed if image_source is not Glance href and is not HTTP(S) URL. """ + def validate_image_url(url, secret=False): + """Validates image URL through the HEAD request. + + :param url: URL to be validated + :param secret: if URL is secret (e.g. swift temp url), + it will not be shown in logs. + """ + try: + image_service.HttpImageService().validate_href(url, secret) + except exception.ImageRefValidationFailed as e: + with excutils.save_and_reraise_exception(): + LOG.error(_LE("Agent deploy supports only HTTP(S) URLs as " + "instance_info['image_source'] or swift " + "temporary URL. Either the specified URL is not " + "a valid HTTP(S) URL or is not reachable " + "for node %(node)s. Error: %(msg)s"), + {'node': node.uuid, 'msg': e}) node = task.node instance_info = node.instance_info iwdi = node.driver_internal_info.get('is_whole_disk_image') @@ -1280,9 +1297,10 @@ def build_instance_info_for_deploy(task): glance = image_service.GlanceImageService(version=2, context=task.context) image_info = glance.show(image_source) - swift_temp_url = glance.swift_temp_url(image_info) LOG.debug('Got image info: %(info)s for node %(node)s.', {'info': image_info, 'node': node.uuid}) + swift_temp_url = glance.swift_temp_url(image_info) + validate_image_url(swift_temp_url, secret=True) instance_info['image_url'] = swift_temp_url instance_info['image_checksum'] = image_info['checksum'] instance_info['image_disk_format'] = image_info['disk_format'] @@ -1295,14 +1313,7 @@ def build_instance_info_for_deploy(task): instance_info['kernel'] = image_info['properties']['kernel_id'] instance_info['ramdisk'] = image_info['properties']['ramdisk_id'] else: - try: - image_service.HttpImageService().validate_href(image_source) - except exception.ImageRefValidationFailed: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Agent deploy supports only HTTP(S) URLs as " - "instance_info['image_source']. Either %s " - "is not a valid HTTP(S) URL or " - "is not reachable."), image_source) + validate_image_url(image_source) instance_info['image_url'] = image_source if not iwdi: diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index 57ac3be832..fb81604218 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -67,6 +67,17 @@ class HttpImageServiceTestCase(base.TestCase): self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href) + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_error_with_secret_parameter(self, head_mock): + head_mock.return_value.status_code = 204 + e = self.assertRaises(exception.ImageRefValidationFailed, + self.service.validate_href, + self.href, + True) + self.assertIn('secreturl', six.text_type(e)) + self.assertNotIn(self.href, six.text_type(e)) + head_mock.assert_called_once_with(self.href) + @mock.patch.object(requests, 'head', autospec=True) def _test_show(self, head_mock, mtime, mtime_date): head_mock.return_value.status_code = http_client.OK diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 0aa1763b51..d0f49da62e 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -2617,8 +2617,11 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node = obj_utils.create_test_node(self.context, driver='fake_agent') + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) @mock.patch.object(image_service, 'GlanceImageService', autospec=True) - def test_build_instance_info_for_deploy_glance_image(self, glance_mock): + def test_build_instance_info_for_deploy_glance_image(self, glance_mock, + validate_mock): i_info = self.node.instance_info i_info['image_source'] = '733d1c44-a2ea-414b-aca7-69decf20d810' driver_internal_info = self.node.driver_internal_info @@ -2631,7 +2634,8 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): 'container_format': 'bare', 'properties': {}} glance_mock.return_value.show = mock.MagicMock(spec_set=[], return_value=image_info) - + glance_mock.return_value.swift_temp_url.return_value = ( + 'http://temp-url') mgr_utils.mock_the_extension_manager(driver='fake_agent') with task_manager.acquire( self.context, self.node.uuid, shared=False) as task: @@ -2644,11 +2648,15 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.instance_info['image_source']) glance_mock.return_value.swift_temp_url.assert_called_once_with( image_info) + validate_mock.assert_called_once_with(mock.ANY, 'http://temp-url', + secret=True) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) @mock.patch.object(utils, 'parse_instance_info', autospec=True) @mock.patch.object(image_service, 'GlanceImageService', autospec=True) def test_build_instance_info_for_deploy_glance_partition_image( - self, glance_mock, parse_instance_info_mock): + self, glance_mock, parse_instance_info_mock, validate_mock): i_info = {} i_info['image_source'] = '733d1c44-a2ea-414b-aca7-69decf20d810' i_info['kernel'] = '13ce5a56-1de3-4916-b8b2-be778645d003' @@ -2671,7 +2679,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): glance_mock.return_value.show = mock.MagicMock(spec_set=[], return_value=image_info) glance_obj_mock = glance_mock.return_value - glance_obj_mock.swift_temp_url.return_value = 'temp-url' + glance_obj_mock.swift_temp_url.return_value = 'http://temp-url' parse_instance_info_mock.return_value = {'swap_mb': 4} image_source = '733d1c44-a2ea-414b-aca7-69decf20d810' expected_i_info = {'root_gb': 5, @@ -2680,7 +2688,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): 'ephemeral_format': None, 'configdrive': 'configdrive', 'image_source': image_source, - 'image_url': 'temp-url', + 'image_url': 'http://temp-url', 'kernel': 'kernel', 'ramdisk': 'ramdisk', 'image_type': 'partition', @@ -2702,6 +2710,8 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.instance_info['image_source']) glance_mock.return_value.swift_temp_url.assert_called_once_with( image_info) + validate_mock.assert_called_once_with( + mock.ANY, 'http://temp-url', secret=True) image_type = task.node.instance_info['image_type'] self.assertEqual('partition', image_type) self.assertEqual('kernel', info['kernel']) @@ -2733,7 +2743,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.assertEqual(self.node.instance_info['image_source'], info['image_url']) validate_href_mock.assert_called_once_with( - mock.ANY, 'http://image-ref') + mock.ANY, 'http://image-ref', False) @mock.patch.object(utils, 'parse_instance_info', autospec=True) @mock.patch.object(image_service.HttpImageService, 'validate_href', @@ -2775,7 +2785,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.assertEqual(self.node.instance_info['image_source'], info['image_url']) validate_href_mock.assert_called_once_with( - mock.ANY, 'http://image-ref') + mock.ANY, 'http://image-ref', False) self.assertEqual('partition', info['image_type']) self.assertEqual(expected_i_info, info) parse_instance_info_mock.assert_called_once_with(task.node) diff --git a/releasenotes/notes/validate-image-url-wnen-deploying-8820f4398ea9de9f.yaml b/releasenotes/notes/validate-image-url-wnen-deploying-8820f4398ea9de9f.yaml new file mode 100644 index 0000000000..950950cc3d --- /dev/null +++ b/releasenotes/notes/validate-image-url-wnen-deploying-8820f4398ea9de9f.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Ironic now validates any swift temporary URL + when preparing for deployment of nodes.