From e78f123ff8e3a4fcd5e3e596b526eb5eb39a34a9 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 22 Mar 2022 08:44:00 -0700 Subject: [PATCH] Make anaconda non-image deploys sane Ironic has a lot of logic built up around use of images for filesystems, however several recent additions, such as the ``ramdisk`` and ``anaconda`` deployment interfaces have started to break this mold. In working with some operators attempting to utilzie the anaconda deployment interface outside the context of full OpenStack, we discovered some issues which needed to be make simpler to help remove the need to route around data validation checks for things that are not required. Standalong users also have the ability to point to a URL with anaconda, where as Operators using OpenStack can only do so with customized kickstart files. While this is okay, the disparity in configuraiton checking was also creating additional issues. In this, we discovered we were not really graceful with redirects, so we're now a little more graceful with them. Story: 2009939 Story: 2009940 Task: 44834 Task: 44833 Change-Id: I8b0a50751014c6093faa26094d9f99e173dcdd38 --- .../admin/anaconda-deploy-interface.rst | 87 ++++++++++++++++ ironic/common/exception.py | 16 +++ ironic/common/image_service.py | 41 ++++++++ ironic/common/images.py | 75 +++++++++++++- ironic/conductor/manager.py | 1 + ironic/conductor/utils.py | 16 ++- ironic/drivers/modules/deploy_utils.py | 71 +++++++++++--- ironic/drivers/modules/ks.cfg.template | 3 + .../tests/unit/common/test_image_service.py | 28 ++++++ ironic/tests/unit/common/test_images.py | 70 ++++++++++++- .../tests/unit/conductor/test_deployments.py | 98 +++++++++++++++++++ ironic/tests/unit/conductor/test_manager.py | 16 ++- .../unit/drivers/modules/test_deploy_utils.py | 90 +++++++++++++++++ ...deploy-option-sanity-b98fa138747c16d2.yaml | 21 ++++ 14 files changed, 610 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml diff --git a/doc/source/admin/anaconda-deploy-interface.rst b/doc/source/admin/anaconda-deploy-interface.rst index 30848a1451..e2dad8febc 100644 --- a/doc/source/admin/anaconda-deploy-interface.rst +++ b/doc/source/admin/anaconda-deploy-interface.rst @@ -153,6 +153,93 @@ ironic node: openstack baremetal node set \ --instance_info ks_template=glance://uuid +.. warning:: + In the Ironic Project terminology, the word ``template`` often refers to + a file which is supplied to the deployment, which Ironic supplies + parameters to render a specific output. One critical example of this in + the Ironic workflow, specifically with this driver, is that the generated + ``agent token`` is conveyed to the booting ramdisk, facilitating it to call + back to Ironic and indicate the state. This token is randomly generated + for every deploy, and is required. Specifically this is leveraged in the + template's ``pre``, ``onerror``, and ``post`` steps. + For more infomation on Agent Token, please see :doc:`/admin/agent-token`. + +Standalone deployments +---------------------- + +While this deployment interface driver was developed around the use of other +OpenStack services, it is not explicitly required. For example HTTP(S) URLs +can be supplied by the API user to explictly set the expected baremetal node +``instance_info`` fields + +.. code-block:: shell + + baremetal node set \ + --instance_info image_source= \ + --instance_info kernel= \ + --instance_info ramdisk= \ + --instance_info stage2= + +When doing so, you may wish to also utilize a customized kickstart template, +which can also be a URL. Please reference the ironic community provided +template *ks.cfg.template* and use it as a basis of your own kickstart +as it accounts for the particular stages and appropriate callbacks to +Ironic. + +.. warning:: + The default template expects a ``instance_info\liveimg_url`` setting to + be provided by the user, which serves as a base operating system image. + In the context of the anaconda driver, it should be thought of almost + like "stage3". If you're using a custom template, it may not be required, + but proceed with caution. + See `pykickstart documentation `_ + for more information on liveimg file format, structure, and use. + +.. code-block:: shell + + baremetal node set \ + --instance_info ks_template= + +If you do choose to use a liveimg with a customized template, or if you wish +to use the stock template with a liveimg, you will need to provide parameter. + +.. code-block:: shell + + baremetal node set \ + --instance_info liveimg_url= + +.. warning:: + This is required if you do *not* utilize a customised template. As in use + Ironic's stock template. + +The pattern of deployment in this case is identical to a deployment case +where Ironic is integrated with OpenStack, however in this case Ironic +collects the files, and stages them appropriately. + +At this point, you should be able to request the baremetal node to deploy. + +Deployment Process +------------------ + +At a high level, the mechanics of the anaconda driver works in the following +flow, where we also note the stages and purpose of each part for informational +purposes. + +#. Network Boot Program (Such as iPXE) downloads the kernel, and initial + ramdisk. +#. Kernel launches, uncompresses initial ramdisk, and executes init inside + of the ramdisk. +#. The initial ramdisk boot scripts, such as Dracut, recognize the kernel + command line parameters Ironic supplied with the boot configuration, + and downloads the second stage artifacts, in this case called the + ``stage2`` image. This image contains Anaconda and base dependencies. +#. Anaconda downloads and parses the kickstart configuration which was + also supplied on the kernel command line, and executes the commands + as defined in the kickstart template. +#. The kickstart template, if specified in its contents, downloads a + ``liveimg`` which is used as the base operating system image to + start with. + Limitations ----------- diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 9d9420f9d2..9d607c2b80 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -835,3 +835,19 @@ class IncorrectConfiguration(IronicException): 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 ironic_lib's IronicException to convert + # the message. + super(ImageRefIsARedirect, self).__init__( + message=msg, + image_ref=image_ref, + redirect_url=redirect_url) diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index e878f8e63d..5280ee6bc3 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -86,6 +86,9 @@ 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 @@ -97,8 +100,46 @@ class HttpImageService(BaseImageService): verify = CONF.webserver_verify_ca try: + # 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) + + if response.status_code == http_client.MOVED_PERMANENTLY: + # 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) + + if (response.status_code == http_client.FORBIDDEN + and str(image_href).endswith('/')): + LOG.warning('Attempted to validate a URL %s, however we ' + 'received an HTTP Forbidden response and the ' + 'url ends with trailing slash (/), suggesting ' + 'non-image deploy may be in progress with ' + 'a webserver which is not permitting an index ' + 'to be generated. We will treat this as valid, ' + 'but return the response.', image_href) + return response + + # NOTE(TheJulia): Any file list reply will proceed past here just + # fine as they are conveyed as an HTTP 200 OK response with a + # server rendered HTML document payload. if response.status_code != http_client.OK: raise exception.ImageRefValidationFailed( image_href=output_url, diff --git a/ironic/common/images.py b/ironic/common/images.py index 839faa0880..6f78cff0a2 100644 --- a/ironic/common/images.py +++ b/ironic/common/images.py @@ -580,7 +580,8 @@ def is_whole_disk_image(ctx, instance_info): :param instance_info: a node's instance info dict :returns: True for whole disk images and False for partition images - and None on no image_source or Error. + and None on no image_source, the source being a path, or upon an + Error. """ image_source = instance_info.get('image_source') if not image_source: @@ -606,6 +607,11 @@ def is_whole_disk_image(ctx, instance_info): and not iproperties.get('ramdisk_id')) else: # Non glance image ref + if is_source_a_path(ctx, instance_info.get('image_source')): + # Nothing is returned if not valid or there was an error. + # A third possibility is it is not a disk image, which would + # still be None. + return if (not instance_info.get('kernel') and not instance_info.get('ramdisk')): is_whole_disk_image = True @@ -613,6 +619,73 @@ def is_whole_disk_image(ctx, instance_info): return is_whole_disk_image +def is_source_a_path(ctx, image_source): + """Determine if the image source is a path. + + This method determines if a supplied URL is a path. + + :param ctx: an admin/process context. + :param image_source: The supplied image source, expected to be a + URL, which can be used to attempt to determine + if the source is a path. + :returns: True if the image_source appears to be a path as opposed + to an image to be downloaded. If the image source is not + a path, False is returned. If any error is detected, + None is returned. + """ + if not image_source: + return + image_service = service.get_image_service(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 + except Exception: + # NOTE(TheJulia): I don't really like this pattern, *but* + # the wholedisk image support is similar. + return + # When issuing a head request, folders have no length + # A list can be generated by the server.. This is a solid + # hint. + if 'Content-Length' in headers: + LOG.debug('Evaluated %(url)s to determine if it is a URL to a path ' + 'or a file. A Content-Length header was returned ' + 'suggesting file.', + {'url': image_source}) + # NOTE(TheJulia): Files on a webserver have a length which is returned + # when headres are queried. + return False + if ('Content-Type' in headers + and str(headers['Content-Type']).startswith('text') + and 'Content-Length' not in headers): + LOG.debug('Evaluated %(url)s to determine if it is a URL to a path ' + 'or a file. A Content-Type header was returned with a text ' + 'content, which suggests a file list was returned.', + {'url': image_source}) + return True + # NOTE(TheJulia): Files should have been caught almost exclusively + # before with the Content-Length check. + if image_source.endswith('/'): + # If all else fails, looks like a URL, and the server didn't give + # us any hints. + return True + # We were unable to determine if this was a folder or a file. + return False + + def _extract_iso(extract_iso, extract_dir): # NOTE(rpittau): we could probably just extract the files we need # if we find them. Also we probably need to detect the correct iso diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 1b15f7856b..e1914221f0 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -1096,6 +1096,7 @@ class ConductorManager(base_manager.BaseConductorManager): node.del_driver_internal_info('instance') node.del_driver_internal_info('root_uuid_or_disk_id') node.del_driver_internal_info('is_whole_disk_image') + node.del_driver_internal_info('is_source_a_path') node.del_driver_internal_info('deploy_boot_mode') if node.driver_internal_info.get('automatic_lessee'): # Remove lessee, as it was automatically added diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index d3da64f0e9..1893cad6ce 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -1661,7 +1661,21 @@ def update_image_type(context, node): """ iwdi = images.is_whole_disk_image(context, node.instance_info) if iwdi is None: - return False + isap = images.is_source_a_path( + context, + node.instance_info.get('image_source') + ) + if isap is None: + return False + node.set_driver_internal_info('is_source_a_path', isap) + # TBD(TheJulia): should we need to set image_type back? + # rloo doesn't believe we should. I'm kind of on board with that + # idea since it is also user-settable, but laregely is just geared + # to take what is in glance. Line below should we wish to uncomment. + # node.set_instance_info('image_type', images.IMAGE_TYPE_DIRECTORY) + # An alternative is to explictly allow it to be configured by the + # caller/requester. + return True node.set_driver_internal_info('is_whole_disk_image', iwdi) # We need to gradually phase out is_whole_disk_image in favour of diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 89258b134d..2c0fef7330 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -561,8 +561,10 @@ def validate_image_properties(task, deploy_info): :raises: MissingParameterValue if the image doesn't contain the mentioned properties. """ + node = task.node image_href = deploy_info.get('image_source') boot_iso = deploy_info.get('boot_iso') + isap = task.node.driver_internal_info.get('is_source_a_path') if image_href and boot_iso: raise exception.InvalidParameterValue(_( "An 'image_source' and 'boot_iso' parameter may not be " @@ -573,15 +575,22 @@ def validate_image_properties(task, deploy_info): boot_option = get_boot_option(task.node) if (boot_iso - or task.node.driver_internal_info.get('is_whole_disk_image') - or boot_option == 'local'): + or node.driver_internal_info.get('is_whole_disk_image') + or boot_option == 'local' + or isap): # No image properties are required in this case, but validate that the # image at least looks reasonable. try: + # This doesn't actually test *getting* the defined url or file + # but actually validates we can parse the data *to* connect. image_service.get_image_service(image_href, context=task.context) except exception.ImageRefValidationFailed as e: raise exception.InvalidParameterValue(err=e) - return + if not isap: + # If the source is not a path, but otherwise matches, we need to + # exit validation here. Deployments, such as ramdisk or anaconda + # need further parameter validation and this supplies it. + return if service_utils.is_glance_image(image_href): properties = ['kernel_id', 'ramdisk_id'] @@ -589,6 +598,7 @@ def validate_image_properties(task, deploy_info): properties.append('stage2_id') image_props = get_image_properties(task.context, image_href) else: + # We are likely netbooting in this case... properties = ['kernel', 'ramdisk'] image_props = {} @@ -829,7 +839,7 @@ def get_image_instance_info(node): _ERR_MSG_INVALID_DEPLOY = _("Invalid parameter %(param)s: %(reason)s") -def parse_instance_info(node): +def parse_instance_info(node, image_deploy=True): """Gets the instance specific Node deployment info. This method validates whether the 'instance_info' property of the @@ -837,6 +847,9 @@ def parse_instance_info(node): deploy images to the node. :param node: a single Node. + :param image_deploy: If the deployment interface is aware this + is an image based deployment, default + True. :returns: A dict with the instance_info values. :raises: MissingParameterValue, if any of the required parameters are missing. @@ -856,7 +869,12 @@ def parse_instance_info(node): i_info['image_source'])): i_info['kernel'] = info.get('kernel') i_info['ramdisk'] = info.get('ramdisk') - i_info['root_gb'] = info.get('root_gb') + if image_deploy: + # root_gb is expected with partition images. + i_info['root_gb'] = info.get('root_gb') + + # NOTE(TheJulia): Kernel/ramdisk may be optional and originated + # with pure workload network booting. error_msg = _("Some parameters were missing in node's instance_info") check_for_missing_params(i_info, error_msg) @@ -879,7 +897,8 @@ def parse_instance_info(node): err_msg_invalid = _("Cannot deploy whole disk image with " "swap or ephemeral size set") raise exception.InvalidParameterValue(err_msg_invalid) - else: + elif image_deploy: + # NOTE(TheJulia): This only applies to partition image deploys. _validate_layout_properties(node, info, i_info) i_info['configdrive'] = info.get('configdrive') @@ -1066,6 +1085,10 @@ def _validate_image_url(node, url, secret=False): it will not be shown in logs. """ try: + # NOTE(TheJulia): This method only validates that an exception + # is NOT raised. In other words, that the endpoint does not + # return a 200. If we're fed a folder list, this will still + # work, which is a good and bad thing at the same time. :/ image_service.HttpImageService().validate_href(url, secret) except exception.ImageRefValidationFailed as e: with excutils.save_and_reraise_exception(): @@ -1178,6 +1201,10 @@ def build_instance_info_for_deploy(task): instance_info = node.instance_info iwdi = node.driver_internal_info.get('is_whole_disk_image') image_source = instance_info['image_source'] + isap = node.driver_internal_info.get('is_source_a_path') + # If our url ends with a /, i.e. we have been supplied with a path, + # we can only deploy this in limited cases for drivers and tools + # which are aware of such. i.e. anaconda. image_download_source = get_image_download_source(node) boot_option = get_boot_option(task.node) @@ -1207,17 +1234,35 @@ def build_instance_info_for_deploy(task): instance_info['ramdisk'] = image_info['properties']['ramdisk_id'] elif (image_source.startswith('file://') or image_download_source == 'local'): + # NOTE(TheJulia): Intentionally only supporting file:/// as image + # based deploy source since we don't want to, nor should we be in + # in the business of copying large numbers of files as it is a + # huge performance impact. _cache_and_convert_image(task, instance_info) else: - _validate_image_url(node, image_source) - instance_info['image_url'] = image_source + try: + _validate_image_url(node, image_source) + # image_url is internal, and used by IPA and some boot templates. + # in most cases, it needs to come from image_source explicitly. + instance_info['image_url'] = image_source + except exception.ImageRefIsARedirect as e: + if e.redirect_url: + instance_info['image_url'] = e.redirect_url + else: + raise - if not iwdi: - instance_info['image_type'] = images.IMAGE_TYPE_PARTITION - i_info = parse_instance_info(node) - instance_info.update(i_info) + if not isap: + if not iwdi: + instance_info['image_type'] = images.IMAGE_TYPE_PARTITION + i_info = parse_instance_info(node) + instance_info.update(i_info) + else: + instance_info['image_type'] = images.IMAGE_TYPE_WHOLE_DISK else: - instance_info['image_type'] = images.IMAGE_TYPE_WHOLE_DISK + # Call central parsing so we retain things like config drives. + i_info = parse_instance_info(node, image_deploy=False) + instance_info.update(i_info) + return instance_info diff --git a/ironic/drivers/modules/ks.cfg.template b/ironic/drivers/modules/ks.cfg.template index 941d3c37d1..825ea38c8c 100644 --- a/ironic/drivers/modules/ks.cfg.template +++ b/ironic/drivers/modules/ks.cfg.template @@ -16,6 +16,9 @@ clearpart --all --initlabel autopart # Downloading and installing OS image using liveimg section is mandatory +# in a *default* ironic configuration. Users (infrastructure operators) +# may choose to customize this pattern, or use release specific kickstart +# configurations which may already point to a mirror. liveimg --url {{ ks_options.liveimg_url }} # Following %pre and %onerror sections are mandatory diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index 30b6f4f37f..197518e1ad 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -194,6 +194,34 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.assert_called_once_with(self.href, verify=False, timeout=60) + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_path_forbidden(self, head_mock): + cfg.CONF.set_override('webserver_verify_ca', 'True') + + response = head_mock.return_value + response.status_code = http_client.FORBIDDEN + url = self.href + '/' + resp = self.service.validate_href(url) + head_mock.assert_called_once_with(url, verify=True, + timeout=60) + self.assertEqual(http_client.FORBIDDEN, resp.status_code) + + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_path_redirected(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) + @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/common/test_images.py b/ironic/tests/unit/common/test_images.py index 98cbedf275..dd782c687a 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/ironic/tests/unit/common/test_images.py @@ -274,26 +274,86 @@ class IronicImagesTestCase(base.TestCase): def test_is_whole_disk_image_partition_non_glance(self, mock_igi, mock_gip): mock_igi.return_value = False - instance_info = {'image_source': 'partition_image', - 'kernel': 'kernel', - 'ramdisk': 'ramdisk'} + instance_info = { + 'image_source': 'fcf5a777-d9d2-4b86-b3da-bb0b61d5a291', + 'kernel': 'kernel', + 'ramdisk': 'ramdisk'} is_whole_disk_image = images.is_whole_disk_image('context', instance_info) self.assertFalse(is_whole_disk_image) self.assertFalse(mock_gip.called) mock_igi.assert_called_once_with(instance_info['image_source']) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) @mock.patch.object(images, 'get_image_properties', autospec=True) @mock.patch.object(glance_utils, 'is_glance_image', autospec=True) def test_is_whole_disk_image_whole_disk_non_glance(self, mock_igi, - mock_gip): + mock_gip, + mock_validate): mock_igi.return_value = False - instance_info = {'image_source': 'whole_disk_image'} + instance_info = { + 'image_source': 'http://whole-disk-image'} is_whole_disk_image = images.is_whole_disk_image('context', instance_info) self.assertTrue(is_whole_disk_image) self.assertFalse(mock_gip.called) mock_igi.assert_called_once_with(instance_info['image_source']) + mock_validate.assert_called_once_with(mock.ANY, + 'http://whole-disk-image') + + def test_is_source_a_path_returns_none(self): + self.assertIsNone(images.is_source_a_path('context', {})) + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_simple(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {} + validate_mock.return_value = mock_response + 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(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_content_length(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {'Content-Length': 1} + validate_mock.return_value = mock_response + self.assertFalse(images.is_source_a_path('context', 'http://foo/bar/')) + validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar/') + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_content_type(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {'Content-Type': 'text/html'} + validate_mock.return_value = mock_response + 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): + url = 'http://foo/bar' + validate_mock.side_effect = OSError + self.assertIsNone(images.is_source_a_path('context', url)) class FsImageTestCase(base.TestCase): diff --git a/ironic/tests/unit/conductor/test_deployments.py b/ironic/tests/unit/conductor/test_deployments.py index ed722c1076..4834f2da4a 100644 --- a/ironic/tests/unit/conductor/test_deployments.py +++ b/ironic/tests/unit/conductor/test_deployments.py @@ -385,6 +385,104 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test_start_deploy_records_lessee(self): self._test_start_deploy(automatic_lessee=True) + @mock.patch.object(images, 'is_source_a_path', autospec=True) + @mock.patch.object(task_manager.TaskManager, 'process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.validate', + autospec=True) + @mock.patch.object(conductor_steps, + 'validate_user_deploy_steps_and_templates', + autospec=True) + @mock.patch.object(conductor_utils, 'validate_instance_info_traits', + autospec=True) + @mock.patch.object(images, 'is_whole_disk_image', autospec=True) + def test_start_deploy_source_path( + self, mock_iwdi, mock_validate_traits, + mock_validate_deploy_user_steps_and_templates, + mock_deploy_validate, mock_power_validate, + mock_process_event, mock_is_source_a_path): + self._start_service() + mock_iwdi.return_value = None + mock_is_source_a_path.return_value = True + deploy_steps = [{"interface": "bios", "step": "factory_reset", + "priority": 95}] + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.AVAILABLE, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + + deployments.start_deploy(task, self.service, configdrive=None, + event='deploy', deploy_steps=deploy_steps) + node.refresh() + mock_iwdi.assert_called_once_with(task.context, + task.node.instance_info) + mock_is_source_a_path.assert_called_once_with( + task.context, + task.node.instance_info.get('image_source') + ) + self.assertNotIn('is_whole_disk_iamge', node.driver_internal_info) + self.assertTrue(node.driver_internal_info['is_source_a_path']) + mock_power_validate.assert_called_once_with(task.driver.power, task) + mock_deploy_validate.assert_called_once_with(task.driver.deploy, task) + mock_validate_traits.assert_called_once_with(task.node) + mock_validate_deploy_user_steps_and_templates.assert_called_once_with( + task, deploy_steps, skip_missing=True) + mock_process_event.assert_called_with( + mock.ANY, 'deploy', call_args=( + deployments.do_node_deploy, task, 1, None, deploy_steps), + callback=mock.ANY, err_handler=mock.ANY) + + @mock.patch.object(images, 'is_source_a_path', autospec=True) + @mock.patch.object(task_manager.TaskManager, 'process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.validate', + autospec=True) + @mock.patch.object(conductor_steps, + 'validate_user_deploy_steps_and_templates', + autospec=True) + @mock.patch.object(conductor_utils, 'validate_instance_info_traits', + autospec=True) + @mock.patch.object(images, 'is_whole_disk_image', autospec=True) + def test_start_deploy_source_path_none( + self, mock_iwdi, mock_validate_traits, + mock_validate_deploy_user_steps_and_templates, + mock_deploy_validate, mock_power_validate, + mock_process_event, mock_is_source_a_path): + self._start_service() + mock_iwdi.return_value = None + mock_is_source_a_path.return_value = None + deploy_steps = [{"interface": "bios", "step": "factory_reset", + "priority": 95}] + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.AVAILABLE, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + + deployments.start_deploy(task, self.service, configdrive=None, + event='deploy', deploy_steps=deploy_steps) + node.refresh() + mock_iwdi.assert_called_once_with(task.context, + task.node.instance_info) + mock_is_source_a_path.assert_called_once_with( + task.context, + task.node.instance_info.get('image_source') + ) + self.assertNotIn('is_whole_disk_iamge', node.driver_internal_info) + self.assertNotIn('is_source_a_path', node.driver_internal_info) + mock_power_validate.assert_called_once_with(task.driver.power, task) + mock_deploy_validate.assert_called_once_with(task.driver.deploy, task) + mock_validate_traits.assert_called_once_with(task.node) + mock_validate_deploy_user_steps_and_templates.assert_called_once_with( + task, deploy_steps, skip_missing=True) + mock_process_event.assert_called_with( + mock.ANY, 'deploy', call_args=( + deployments.do_node_deploy, task, 1, None, deploy_steps), + callback=mock.ANY, err_handler=mock.ANY) + @mgr_utils.mock_record_keepalive class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 2c8ddcbd49..ddd9cbae47 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2410,7 +2410,8 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): provision_state=states.DELETING, target_provision_state=states.AVAILABLE, instance_info={'foo': 'bar'}, - driver_internal_info={'is_whole_disk_image': False}) + driver_internal_info={'is_whole_disk_image': False, + 'is_source_a_path': None}) task = task_manager.TaskManager(self.context, node.uuid) self._start_service() @@ -2463,7 +2464,8 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def _test__do_node_tear_down_ok(self, mock_tear_down, mock_clean, mock_unbind, mock_console, enabled_console=False, - with_allocation=False): + with_allocation=False, + source_a_path=False): # test when driver.deploy.tear_down succeeds node = obj_utils.create_test_node( self.context, driver='fake-hardware', @@ -2488,6 +2490,10 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): alloc.node_id = node.id alloc.save() node.refresh() + if source_a_path: + d_ii = node.driver_internal_info + d_ii['is_source_a_path'] = True + node.driver_internal_info = d_ii task = task_manager.TaskManager(self.context, node.uuid) self._start_service() @@ -2507,6 +2513,7 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertNotIn('root_uuid_or_disk_id', node.driver_internal_info) self.assertNotIn('is_whole_disk_image', node.driver_internal_info) self.assertNotIn('automatic_lessee', node.driver_internal_info) + self.assertNotIn('is_source_a_path', node.driver_internal_info) mock_tear_down.assert_called_once_with(task.driver.deploy, task) mock_clean.assert_called_once_with(task) self.assertEqual({}, port.internal_info) @@ -2529,6 +2536,9 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test__do_node_tear_down_with_allocation(self): self._test__do_node_tear_down_ok(with_allocation=True) + def test__do_node_tear_down_with_source_path(self): + self._test__do_node_tear_down_ok(source_a_path=True) + @mock.patch('ironic.drivers.modules.fake.FakeRescue.clean_up', autospec=True) @mock.patch('ironic.conductor.cleaning.do_node_clean', autospec=True) @@ -7333,7 +7343,7 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): provision_state=states.ADOPTING, instance_info={ 'capabilities': {'boot_option': 'netboot'}, - 'image_source': 'image', + 'image_source': 'http://127.0.0.1/image', }) task = task_manager.TaskManager(self.context, node.uuid) diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index e85ab631d6..230eec8f16 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -1487,6 +1487,24 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase): utils.validate_image_properties(self.task, inst_info) image_service_show_mock.assert_not_called() + @mock.patch.object(utils, 'get_boot_option', autospec=True, + return_value='kickstart') + @mock.patch.object(image_service.HttpImageService, 'show', autospec=True) + def test_validate_image_properties_anaconda_deploy_image_source( + self, image_service_show_mock, boot_options_mock): + d_ii = self.node.driver_internal_info + d_ii.pop('is_whole_disk_image') + d_ii['is_source_a_path'] = True + instance_info = { + 'kernel': 'file://kernel', + 'ramdisk': 'file://initrd', + 'image_source': 'http://foo/bar/' + } + self.node.instance_info = instance_info + inst_info = utils.get_image_instance_info(self.node) + utils.validate_image_properties(self.task, inst_info) + image_service_show_mock.assert_not_called() + class ValidateParametersTestCase(db_base.DbTestCase): @@ -1869,6 +1887,25 @@ class InstanceInfoTestCase(db_base.DbTestCase): self.assertNotIn('ephemeral_mb', instance_info) self.assertNotIn('swap_mb', instance_info) + def test_parse_instance_info_non_image_deploy(self): + driver_internal_info = dict(DRV_INTERNAL_INFO_DICT) + driver_internal_info['is_whole_disk_image'] = None + instance_info = {'image_source': 'http://cat/meow/', + 'kernel': 'corgi', + 'ramdisk': 'mushroom'} + node = obj_utils.create_test_node( + self.context, instance_info=instance_info, + driver_internal_info=driver_internal_info, + deploy_interface='anaconda' + ) + instance_info = utils.parse_instance_info(node, image_deploy=False) + self.assertIsNotNone(instance_info['image_source']) + self.assertNotIn('root_mb', instance_info) + self.assertNotIn('ephemeral_mb', instance_info) + self.assertNotIn('swap_mb', instance_info) + self.assertIn('kernel', instance_info) + self.assertIn('ramdisk', instance_info) + class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): def setUp(self): @@ -2133,6 +2170,59 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.assertRaises(exception.ImageRefValidationFailed, utils.build_instance_info_for_deploy, task) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_build_instance_info_for_deploy_source_is_a_path( + self, validate_href_mock): + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + i_info['image_source'] = 'http://image-url/folder/' + driver_internal_info.pop('is_whole_disk_image', None) + driver_internal_info['is_source_a_path'] = True + self.node.instance_info = i_info + self.node.driver_internal_info = driver_internal_info + self.node.save() + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + + info = utils.build_instance_info_for_deploy(task) + + self.assertEqual(self.node.instance_info['image_source'], + info['image_url']) + self.assertNotIn('root_gb', info) + validate_href_mock.assert_called_once_with( + mock.ANY, 'http://image-url/folder/', False) + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_build_instance_info_for_deploy_source_redirect( + self, validate_href_mock): + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + url = 'http://image-url/folder' + r_url = url + '/' + i_info['image_source'] = 'http://image-url/folder' + driver_internal_info.pop('is_whole_disk_image', None) + driver_internal_info['is_source_a_path'] = True + 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) + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + + info = utils.build_instance_info_for_deploy(task) + + self.assertNotEqual(self.node.instance_info['image_source'], + info['image_url']) + self.assertEqual(r_url, info['image_url']) + validate_href_mock.assert_called_once_with( + mock.ANY, 'http://image-url/folder', False) + class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase): def setUp(self): diff --git a/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml b/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml new file mode 100644 index 0000000000..455d553eec --- /dev/null +++ b/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml @@ -0,0 +1,21 @@ +--- +fixes: + - | + Fixes an issue where ``root_gb`` became a required field when using the + ``anaconda`` deployment interface, with a recent bug fix as the code path + largely expected all deployment operations to utilize images, which is + not the case. The case handling for non-image based deployments is now + explicitly in internal parameter validation code. + - | + Fixes handling of ``image_source`` parameters where internal validations + would not gracefully handle received redirects and treat it as a failure. + We now no longer explicitly fail when a redirect is received. + - | + Fixes an issue where an ``image_source`` could not be set to a mirror URL + to facilitate deployments using a mirror with the ``anaconda`` deployment + interface. Ironic still presently has an explicit requirement on a + ``stage2`` parameter to be explicitly defined. +other: + - | + Adds documentation of standalone deployment use case with the ``anaconda`` + deployment interface.