From 5482045e88a526538df26265af0a54ee847917c8 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 9 Oct 2025 08:45:25 +0200 Subject: [PATCH] Remove bespoke logic for handling redirects while validating URLs This logic is broken in a few places when dealing with real world redirect cases, such as Debian Cloud images redirecting to mirrors. It seems that the code was written with an incorrect assumption that requests does not limit redirects by default. It does, the default max_redirects seems to be 30. We can change it on Session if we need. The anaconda case is now handled by checking the url field of the response. Closes-Bug: #2127154 Change-Id: I200d631e166075ceab80dcd4b0ff596d1860aa3b Signed-off-by: Dmitry Tantsur --- ironic/common/exception.py | 15 --- ironic/common/image_service.py | 28 +---- ironic/common/images.py | 19 +-- ironic/drivers/modules/deploy_utils.py | 96 ++++++--------- .../tests/unit/common/test_image_service.py | 112 +++++------------- ironic/tests/unit/common/test_images.py | 17 +-- .../unit/drivers/modules/test_deploy_utils.py | 41 ++++--- .../notes/redirect-f2f1bc4079763e7e.yaml | 5 + 8 files changed, 102 insertions(+), 231 deletions(-) create mode 100644 releasenotes/notes/redirect-f2f1bc4079763e7e.yaml diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 033d7ac0e4..bf0511cb79 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -953,21 +953,6 @@ class NodeVerifyFailure(IronicException): _msg_fmt = _("Failed to verify node %(node)s: %(reason)s") -class ImageRefIsARedirect(IronicException): - _msg_fmt = _("Received a URL redirect when attempting to evaluate " - "image reference %(image_ref)s pointing to " - "%(redirect_url)s. This may, or may not be recoverable.") - redirect_url = None - - def __init__(self, image_ref=None, redirect_url=None, msg=None): - self.redirect_url = redirect_url - # Kwargs are expected by IronicException to convert the message. - super(ImageRefIsARedirect, self).__init__( - message=msg, - image_ref=image_ref, - redirect_url=redirect_url) - - class ConcurrentActionLimit(TemporaryFailure): # NOTE(TheJulia): We explicitly don't report the concurrent # action limit configuration value as a security guard since diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index ae9d2b588a..4b90bc945e 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -153,9 +153,6 @@ class HttpImageService(BaseImageService): shown in exception message. :raises: exception.ImageRefValidationFailed if HEAD request failed or returned response code not equal to 200. - :raises: exception.ImageRefIsARedirect if the supplied URL is a - redirect to a different URL. The caller may be able to handle - this. :returns: Response to HEAD request. """ output_url = 'secreturl' if secret else image_href @@ -170,32 +167,9 @@ class HttpImageService(BaseImageService): auth = HttpImageService.gen_auth_from_conf_user_pass(image_href) # NOTE(TheJulia): Head requests do not work on things that are not # files, but they can be responded with redirects or a 200 OK.... - # We don't want to permit endless redirects either, thus not - # request an override to the requests default to try and resolve - # redirects as otherwise we might end up with something like - # HTTPForbidden or a list of files. Both should be okay to at - # least know things are okay in a limited fashion. response = requests.head(image_href, verify=verify, timeout=CONF.webserver_connection_timeout, - auth=auth) - if (response.status_code == http_client.MOVED_PERMANENTLY - or response.status_code == http_client.FOUND - or response.status_code == http_client.TEMPORARY_REDIRECT - or response.status_code == http_client.PERMANENT_REDIRECT): - # NOTE(TheJulia): In the event we receive a redirect, we need - # to notify the caller. Before this we would just fail, - # but a url which is missing a trailing slash results in a - # redirect to a target path, and the caller *may* actually - # care about that. - redirect = requests.Session().get_redirect_target(response) - - # Extra guard because this is pointless if there is no - # location in the field. Requests also properly formats - # our string for us, or gives us None. - if redirect: - raise exception.ImageRefIsARedirect( - image_ref=image_href, - redirect_url=redirect) + auth=auth, allow_redirects=True) if (response.status_code == http_client.FORBIDDEN and str(image_href).endswith('/')): diff --git a/ironic/common/images.py b/ironic/common/images.py index f98d3ffa2f..542801fa8a 100644 --- a/ironic/common/images.py +++ b/ironic/common/images.py @@ -790,24 +790,12 @@ def is_source_a_path(ctx, image_source): context=ctx) try: res = image_service.validate_href(image_source) - if 'headers' in dir(res): - # response/result is from the HTTP check path. - headers = res.headers - else: - # We have no headers. - headers = {} - except exception.ImageRefIsARedirect as e: - # Our exception handling formats this for us in this - # case. \o/ - LOG.debug(str(e)) - # Servers redirect to a proper folder ending in a slash if - # not supplied originally. - if e.redirect_url and e.redirect_url.endswith('/'): - return True + headers = getattr(res, 'headers', {}) except Exception: # NOTE(TheJulia): I don't really like this pattern, *but* # the wholedisk image support is similar. return + # NOTE(TheJulia): Files should have been caught almost exclusively # before with the Content-Length check. # When the ISO is mounted and the webserver mount point url is @@ -833,7 +821,8 @@ def is_source_a_path(ctx, image_source): # NOTE(TheJulia): Files on a webserver have a length which is returned # when headres are queried. return False - if image_source.endswith('/'): + # In case of redirects, URL may be different from image_source. + if res.url.endswith('/'): # If all else fails, looks like a URL, and the server didn't give # us any hints. return True diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index f3fcb01024..706e7b25a3 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -1210,7 +1210,10 @@ def _validate_image_url(node, url, secret=False, inspect_image=None, oci.set_image_auth(url, image_auth) oci.validate_href(url, secret) else: - image_service.HttpImageService().validate_href(url, secret) + resp = image_service.HttpImageService().validate_href(url, secret) + if resp and resp.url != url: + LOG.debug('URL %s redirected to %s', url, resp.url) + image_info['image_source'] = resp.url except exception.ImageRefValidationFailed as e: with excutils.save_and_reraise_exception(): LOG.error("The specified URL is not a valid HTTP(S) URL or is " @@ -1540,73 +1543,48 @@ def build_instance_info_for_deploy(task): # and not a file, or where a user may be supplying a full URL to # a remotely hosted image, we at a minimum need to check if the # url is valid, and address any redirects upfront. - try: - # NOTE(TheJulia): In the case we're here, we not doing an - # integrated image based deploy, but we may also be doing - # a path based anaconda base deploy, in which case we have - # no backing image, but we need to check for a URL - # redirection. So, if the source is a path (i.e. isap), - # we don't need to inspect the image as there is no image - # in the case for the deployment to drive. - validated_results = {} - if isap: - # This is if the source is a path url, such as one used by - # anaconda templates to to rely upon bootstrapping - # defaults. - _validate_image_url(node, image_source, - inspect_image=False) - else: - # When not isap, we can just let _validate_image_url make - # the required decision on if contents need to be sampled, - # or not. We try to pass the image_disk_format which may - # be declared by the user, and if not we set - # expected_format to None. - validate_results = _validate_image_url( - node, - image_source, - expected_format=instance_info.get( - 'image_disk_format', - None)) + + # NOTE(TheJulia): In the case we're here, we not doing an + # integrated image based deploy, but we may also be doing + # a path based anaconda base deploy, in which case we have + # no backing image, but we need to check for a URL + # redirection. So, if the source is a path (i.e. isap), + # we don't need to inspect the image as there is no image + # in the case for the deployment to drive. + if isap: + # This is if the source is a path url, such as one used by + # anaconda templates to to rely upon bootstrapping + # defaults. + validate_results = _validate_image_url(node, image_source, + inspect_image=False) + else: + # When not isap, we can just let _validate_image_url make + # the required decision on if contents need to be sampled, + # or not. We try to pass the image_disk_format which may + # be declared by the user, and if not we set + # expected_format to None. + validate_results = _validate_image_url( + node, + image_source, + expected_format=instance_info.get( + 'image_disk_format', + None)) # image_url is internal, and used by IPA and some boot # templates. In most cases, it needs to come from image_source # explicitly. - if 'disk_format' in validated_results: + if 'disk_format' in validate_results: # Ensure IPA has the value available, so write what we # detect, if anything. This is also an item which might be # needful with ansible deploy interface, when used in # standalone mode. instance_info['image_disk_format'] = \ validate_results.get('disk_format') - if not instance_info.get('image_url'): - instance_info['image_url'] = image_source - except exception.ImageRefIsARedirect as e: - # At this point, we've got a redirect response from the - # webserver, and we're going to try to handle it as a single - # redirect action, as requests, by default, only lets a single - # redirect to occur. This is likely a URL pathing fix, like a - # trailing / on a path, - # or move to HTTPS from a user supplied HTTP url. - if e.redirect_url: - # Since we've got a redirect, we need to carry the rest of - # the request logic as well, which includes recording a - # disk format, if applicable. - instance_info['image_url'] = e.redirect_url - # We need to save the image_source back out so it caches - instance_info['image_source'] = e.redirect_url - task.node.instance_info = instance_info - if not isap: - # The redirect doesn't relate to a path being used, so - # the target is a filename, likely cause is webserver - # telling the client to use HTTPS. - validated_results = _validate_image_url( - node, e.redirect_url, - expected_format=instance_info.get( - 'image_disk_format', None)) - if 'disk_format' in validated_results: - instance_info['image_disk_format'] = \ - validated_results.get('disk_format') - else: - raise + redirect_target = validate_results.get('image_source') + if redirect_target: + instance_info['image_source'] = redirect_target + instance_info['image_url'] = redirect_target + if not instance_info.get('image_url'): + instance_info['image_url'] = image_source if not isap: if not iwdi: diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index 9340237407..e7dc3f09f1 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -44,7 +44,8 @@ class HttpImageServiceTestCase(base.TestCase): self.service.validate_href(self.href) path_mock.assert_not_called() head_mock.assert_called_once_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -62,7 +63,8 @@ class HttpImageServiceTestCase(base.TestCase): response.status_code = http_client.OK self.service.validate_href(self.href) head_mock.assert_called_once_with(self.href, verify=False, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -79,7 +81,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href, verify=False, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) head_mock.side_effect = requests.RequestException() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) @@ -92,7 +95,8 @@ class HttpImageServiceTestCase(base.TestCase): response.status_code = http_client.OK self.service.validate_href(self.href) head_mock.assert_called_once_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -110,7 +114,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) head_mock.side_effect = requests.RequestException() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) @@ -124,7 +129,8 @@ class HttpImageServiceTestCase(base.TestCase): self.service.validate_href(self.href) head_mock.assert_called_once_with(self.href, verify='/some/path', - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -150,7 +156,8 @@ class HttpImageServiceTestCase(base.TestCase): self.service.validate_href(self.href) head_mock.assert_called_once_with(self.href, verify='/some/path', - timeout=60, auth=auth_creds) + timeout=60, auth=auth_creds, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -179,7 +186,8 @@ class HttpImageServiceTestCase(base.TestCase): response.status_code = http_client.OK self.service.validate_href(self.href) head_mock.assert_called_once_with(self.href, verify=True, - timeout=15, auth=None) + timeout=15, auth=None, + allow_redirects=True) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -199,7 +207,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href, verify='/some/path', - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_verify_error(self, head_mock): @@ -208,7 +217,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href, verify='/some/path', - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_verify_os_error(self, head_mock): @@ -217,7 +227,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) head_mock.assert_called_once_with(self.href, verify='/some/path', - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_error_with_secret_parameter(self, head_mock): @@ -230,7 +241,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertIn('secreturl', str(e)) self.assertNotIn(self.href, str(e)) head_mock.assert_called_once_with(self.href, verify=False, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_path_forbidden(self, head_mock): @@ -241,73 +253,10 @@ class HttpImageServiceTestCase(base.TestCase): url = self.href + '/' resp = self.service.validate_href(url) head_mock.assert_called_once_with(url, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) self.assertEqual(http_client.FORBIDDEN, resp.status_code) - @mock.patch.object(requests, 'head', autospec=True) - def test_validate_href_path_moved_permanently(self, head_mock): - cfg.CONF.set_override('webserver_verify_ca', 'True') - - response = head_mock.return_value - response.status_code = http_client.MOVED_PERMANENTLY - url = self.href + '/' - new_url = 'http://new-url' - response.headers = {'location': new_url} - exc = self.assertRaises(exception.ImageRefIsARedirect, - self.service.validate_href, - url) - self.assertEqual(new_url, exc.redirect_url) - head_mock.assert_called_once_with(url, verify=True, - timeout=60, auth=None) - - @mock.patch.object(requests, 'head', autospec=True) - def test_validate_href_path_found(self, head_mock): - cfg.CONF.set_override('webserver_verify_ca', 'True') - - response = head_mock.return_value - response.status_code = http_client.FOUND - url = self.href + '/' - new_url = 'http://new-url' - response.headers = {'location': new_url} - exc = self.assertRaises(exception.ImageRefIsARedirect, - self.service.validate_href, - url) - self.assertEqual(new_url, exc.redirect_url) - head_mock.assert_called_once_with(url, verify=True, - timeout=60, auth=None) - - @mock.patch.object(requests, 'head', autospec=True) - def test_validate_href_path_temporary_redirect(self, head_mock): - cfg.CONF.set_override('webserver_verify_ca', 'True') - - response = head_mock.return_value - response.status_code = http_client.TEMPORARY_REDIRECT - url = self.href + '/' - new_url = 'http://new-url' - response.headers = {'location': new_url} - exc = self.assertRaises(exception.ImageRefIsARedirect, - self.service.validate_href, - url) - self.assertEqual(new_url, exc.redirect_url) - head_mock.assert_called_once_with(url, verify=True, - timeout=60, auth=None) - - @mock.patch.object(requests, 'head', autospec=True) - def test_validate_href_path_permanent_redirect(self, head_mock): - cfg.CONF.set_override('webserver_verify_ca', 'True') - - response = head_mock.return_value - response.status_code = http_client.PERMANENT_REDIRECT - url = self.href + '/' - new_url = 'http://new-url' - response.headers = {'location': new_url} - exc = self.assertRaises(exception.ImageRefIsARedirect, - self.service.validate_href, - url) - self.assertEqual(new_url, exc.redirect_url) - head_mock.assert_called_once_with(url, verify=True, - timeout=60, auth=None) - def test_verify_basic_auth_cred_format(self): self.assertIsNone(self .service @@ -373,7 +322,8 @@ class HttpImageServiceTestCase(base.TestCase): } result = self.service.show(self.href) head_mock.assert_called_once_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) self.assertEqual({'size': 100, 'updated_at': mtime_date, 'properties': {}, 'no_cache': False}, result) @@ -399,7 +349,8 @@ class HttpImageServiceTestCase(base.TestCase): } result = self.service.show(self.href) head_mock.assert_called_once_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) self.assertEqual({ 'size': 100, 'updated_at': datetime.datetime(2014, 11, 15, 8, 12, 31), @@ -423,7 +374,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.show, self.href) head_mock.assert_called_with(self.href, verify=True, - timeout=60, auth=None) + timeout=60, auth=None, + allow_redirects=True) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) diff --git a/ironic/tests/unit/common/test_images.py b/ironic/tests/unit/common/test_images.py index 076a5f4aa0..a469513a7d 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/ironic/tests/unit/common/test_images.py @@ -624,6 +624,7 @@ class IronicImagesTestCase(base.TestCase): mock_gip, mock_validate): mock_igi.return_value = False + mock_validate.return_value.url = 'http://whole-disk-image' instance_info = { 'image_source': 'http://whole-disk-image'} is_whole_disk_image = images.is_whole_disk_image('context', @@ -665,22 +666,6 @@ class IronicImagesTestCase(base.TestCase): self.assertTrue(images.is_source_a_path('context', 'http://foo/bar')) validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar') - @mock.patch.object(images, 'LOG', autospec=True) - @mock.patch.object(image_service.HttpImageService, 'validate_href', - autospec=True) - def test_is_source_a_path_redirect(self, validate_mock, log_mock): - url = 'http://foo/bar' - redirect_url = url + '/' - validate_mock.side_effect = exception.ImageRefIsARedirect( - url, redirect_url) - self.assertTrue(images.is_source_a_path('context', url)) - log_mock.debug.assert_called_once_with('Received a URL redirect when ' - 'attempting to evaluate image ' - 'reference http://foo/bar ' - 'pointing to http://foo/bar/. ' - 'This may, or may not be ' - 'recoverable.') - @mock.patch.object(image_service.HttpImageService, 'validate_href', autospec=True) def test_is_source_a_path_other_error(self, validate_mock): diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 1100e50786..f44458ef5e 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -2361,6 +2361,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): autospec=True) def test_build_instance_info_for_deploy_nonglance_image( self, validate_href_mock, mock_cache_image): + validate_href_mock.return_value.url = 'http://image-ref' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-ref' @@ -2392,6 +2393,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self, validate_href_mock, mock_cache_image): cfg.CONF.set_override('conductor_always_validates_images', True, group='conductor') + validate_href_mock.return_value.url = 'http://image-ref' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-ref' @@ -2426,6 +2428,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): group='conductor') cfg.CONF.set_override('disable_deep_image_inspection', True, group='conductor') + validate_href_mock.return_value.url = 'http://image-ref' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-ref' @@ -2473,11 +2476,14 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.driver_internal_info = driver_internal_info self.node.save() - validate_href_mock.side_effect = ['http://image-ref', - 'http://kernel-ref', - 'http://ramdisk-ref'] + validate_href_mock.side_effect = [ + mock.Mock(url='http://image-ref'), + mock.Mock(url='http://kernel-ref'), + mock.Mock(url='http://ramdisk-ref'), + ] parse_instance_info_mock.return_value = {'swap_mb': 5} - expected_i_info = {'image_source': 'http://image-ref', + expected_i_info = {'image_disk_format': 'qcow2', + 'image_source': 'http://image-ref', 'image_url': 'http://image-ref', 'image_type': 'partition', 'kernel': 'http://kernel-ref', @@ -2524,9 +2530,11 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.driver_internal_info = driver_internal_info self.node.save() - validate_href_mock.side_effect = ['http://image-ref', - 'http://kernel-ref', - 'http://ramdisk-ref'] + validate_href_mock.side_effect = [ + mock.Mock(url='http://image-ref'), + mock.Mock(url='http://kernel-ref'), + mock.Mock(url='http://ramdisk-ref'), + ] parse_instance_info_mock.return_value = {'swap_mb': 5} expected_i_info = {'image_source': 'http://image-ref', 'image_url': 'http://image-ref', @@ -2576,6 +2584,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): autospec=True) def test_build_instance_info_for_deploy_source_is_a_path( self, validate_href_mock): + validate_href_mock.return_value.url = 'http://image-url/folder/' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-url/folder/' @@ -2611,9 +2620,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.instance_info = i_info self.node.driver_internal_info = driver_internal_info self.node.save() - validate_href_mock.side_effect = exception.ImageRefIsARedirect( - image_ref=url, - redirect_url=r_url) + validate_href_mock.return_value = mock.Mock(url=r_url) with task_manager.acquire( self.context, self.node.uuid, shared=False) as task: @@ -2643,11 +2650,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.node.instance_info = i_info self.node.driver_internal_info = driver_internal_info self.node.save() - validate_href_mock.side_effect = iter([ - exception.ImageRefIsARedirect( - image_ref=url, - redirect_url=r_url), - None]) + validate_href_mock.return_value = mock.Mock(url=r_url) with task_manager.acquire( self.context, self.node.uuid, shared=False) as task: @@ -2657,10 +2660,8 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.assertNotEqual(self.node.instance_info['image_source'], info['image_url']) self.assertEqual(r_url, info['image_url']) - validate_href_mock.assert_has_calls([ - mock.call(mock.ANY, 'http://image-url/file', False), - mock.call(mock.ANY, 'https://image-url/file', False) - ]) + validate_href_mock.assert_called_once_with( + mock.ANY, 'http://image-url/file', False) mock_cache_image.assert_called_once_with(mock.ANY, task.node, force_raw=False, @@ -2849,6 +2850,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase): autospec=True) def test_build_instance_info_local_image(self, validate_href_mock): cfg.CONF.set_override('image_download_source', 'local', group='agent') + validate_href_mock.return_value.url = 'http://image-ref' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-ref' @@ -2926,6 +2928,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase): cfg.CONF.set_override('image_download_source', 'http', group='agent') cfg.CONF.set_override('conductor_always_validates_images', True, group='conductor') + validate_href_mock.return_value.url = 'http://image-ref' i_info = self.node.instance_info driver_internal_info = self.node.driver_internal_info i_info['image_source'] = 'http://image-ref' diff --git a/releasenotes/notes/redirect-f2f1bc4079763e7e.yaml b/releasenotes/notes/redirect-f2f1bc4079763e7e.yaml new file mode 100644 index 0000000000..f36ca470d9 --- /dev/null +++ b/releasenotes/notes/redirect-f2f1bc4079763e7e.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes validation of image source URLs that are redirects to another URL. + Previously, it would raise UnboundLocalError.