From 0eda3d65ea8b05e250e61e9957c50a226a3baf74 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Wed, 24 Jul 2024 15:43:14 -0700 Subject: [PATCH] Support Automatic Lessee from instance metadata Ironic already has support for automatically setting a lessee on deployment, but it is only supported for direct deployments with Ironic, as it uses request context which is not preserved in the Nova driver. Now, when combined with the related Nova change, Ironic can support this behavior for fully integrated installations. On deploy time, Nova will set several fields -- including project_id -- in instance info. If enabled, Ironic will then use that project_id as the automatic lessee. The previous behavior of using the project_id from the request context is still supported as a fallback. This is being tracked in nova as blueprint ironic-guest-metadata. Closes-Bug: #2063352 Change-Id: Id381a3d201c2f1b137279decc0e32096d4d95012 --- doc/source/admin/secure-rbac.rst | 19 ++++ ironic/common/lessee_sources.py | 28 ++++++ ironic/conductor/deployments.py | 95 ++++++++++++++---- ironic/conf/conductor.py | 38 +++++-- .../tests/unit/conductor/test_deployments.py | 98 +++++++++++++++++-- ...omatic-lessee-source-37abe917b8cb5c36.yaml | 24 +++++ 6 files changed, 264 insertions(+), 38 deletions(-) create mode 100644 ironic/common/lessee_sources.py create mode 100644 releasenotes/notes/automatic-lessee-source-37abe917b8cb5c36.yaml diff --git a/doc/source/admin/secure-rbac.rst b/doc/source/admin/secure-rbac.rst index 3e431e4eb5..ae67724826 100644 --- a/doc/source/admin/secure-rbac.rst +++ b/doc/source/admin/secure-rbac.rst @@ -257,6 +257,25 @@ How do I assign a lessee? # baremetal node set --lessee +Ironic will, by default, automatically manage lessee at deployment time, +setting the lessee field on deploy of a node and unset it before the node +begins cleaning. + +Operators can customize or disable this behavior via +:oslo.config:option:`conductor.automatic_lessee_source` configuration. + +If :oslo.config:option:`conductor.automatic_lessee_source` is set to +``instance`` (the default), this uses ``node.instance_info['project_id']``, +which is set when OpenStack Nova deploys an instance. + +If :oslo.config:option:`conductor.automatic_lessee_source` is set to +``request``, the lessee is set to the project_id in the request context -- +ideal for standalone Ironic deployments still utilizing OpenStack Keystone. + +If :oslo.config:option:`conductor.automatic_lessee_source` is set to to +``none``, Ironic not will set a lessee on deploy. + + What is the difference between an owner and lessee? --------------------------------------------------- diff --git a/ironic/common/lessee_sources.py b/ironic/common/lessee_sources.py new file mode 100644 index 0000000000..785200adba --- /dev/null +++ b/ironic/common/lessee_sources.py @@ -0,0 +1,28 @@ +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +""" +Mapping of values for CONF.conductor.automatic_lessee_source, representing +different possible sources for lessee data. +""" + +NONE = 'none' +"Do not set lessee" + +REQUEST = 'request' +"Use metadata in request context" + +INSTANCE = 'instance' +"Use instance_info[\'project_id\']" diff --git a/ironic/conductor/deployments.py b/ironic/conductor/deployments.py index a14898ede4..6ac51c7b37 100644 --- a/ironic/conductor/deployments.py +++ b/ironic/conductor/deployments.py @@ -23,6 +23,7 @@ from ironic.common import async_steps from ironic.common import exception from ironic.common.glance_service import service_utils as glance_utils from ironic.common.i18n import _ +from ironic.common import lessee_sources from ironic.common import states from ironic.common import swift from ironic.conductor import notification_utils as notify_utils @@ -64,6 +65,75 @@ def validate_node(task, event='deploy'): op=_('provisioning')) +def apply_automatic_lessee(task): + """Apply a automatic lessee to the node, if applicable + + First of all, until removed next cycle, we check to see if + CONF.automatic_lessee was explicitly set "False" by an operator -- if so, + we do not apply a lessee. + + When CONF.conductor.automatic_lessee_source is instance: + - Take the lessee from instance_info[project_id] (e.g. as set by nova) + + When CONF.conductor.automatic_lessee_source is request: + - Take the lessee from request context (e.g. from keystone) + + When CONF.conductor.automatic_lessee_source is none: + OR the legacy CONF.automatic_lessee is explicitly set by an operator to + False (regardless of lessee_source) + - Don't apply a lessee to the node + + :param task: a TaskManager instance. + :returns: True if node had a lessee applied + """ + node = task.node + applied = False + # TODO(JayF): During 2025.1 cycle, remove automatic_lessee boolean config. + if CONF.conductor.automatic_lessee: + project = None + if CONF.conductor.automatic_lessee_source == lessee_sources.REQUEST: + project = utils.get_token_project_from_request(task.context) + if project is None: + LOG.debug('Could not automatically save lessee: No project ' + 'found in request context for node %(uuid)s.', + {'uuid': node.uuid}) + + elif CONF.conductor.automatic_lessee_source == lessee_sources.INSTANCE: + # NOTE(JayF): If we have a project_id explicitly set (typical nova + # case), use it. Otherwise, try to derive it from the context of + # the request (typical standalone+keystone) case. + project = node.instance_info.get('project_id') + if project is None: + LOG.debug('Could not automatically save lessee: node[' + '\'instance_info\'][\'project_id\'] is unset for ' + 'node %(uuid)s.', + {'uuid': node.uuid}) + + # NOTE(JayF): the CONF.conductor.automatic_lessee_source == 'none' + # falls through since project will never be set. + if project: + if node.lessee is None: + LOG.debug('Adding lessee %(project)s to node %(uuid)s.', + {'project': project, + 'uuid': node.uuid}) + node.set_driver_internal_info('automatic_lessee', True) + node.lessee = project + applied = True + else: + # Since the model is a bit of a matrix and we're largely + # just empowering operators, lets at least log a warning + # since they may need to remedy something here. Or maybe + # not. + LOG.warning('Could not automatically save lessee ' + '%(project)s to node %(uuid)s. Node already ' + 'has a defined lessee of %(lessee)s.', + {'project': project, + 'uuid': node.uuid, + 'lessee': node.lessee}) + + return applied + + @METRICS.timer('start_deploy') @task_manager.require_exclusive_lock def start_deploy(task, manager, configdrive=None, event='deploy', @@ -92,31 +162,14 @@ def start_deploy(task, manager, configdrive=None, event='deploy', instance_info.pop('kernel', None) instance_info.pop('ramdisk', None) node.instance_info = instance_info - elif CONF.conductor.automatic_lessee: - # This should only be on deploy... - project = utils.get_token_project_from_request(task.context) - if (project and node.lessee is None): - LOG.debug('Adding lessee $(project)s to node %(uuid)s.', - {'project': project, - 'uuid': node.uuid}) - node.set_driver_internal_info('automatic_lessee', True) - node.lessee = project - elif project and node.lessee is not None: - # Since the model is a bit of a matrix and we're largely - # just empowering operators, lets at least log a warning - # since they may need to remedy something here. Or maybe - # not. - LOG.warning('Could not automatically save lessee ' - '$(project)s to node %(uuid)s. Node already ' - 'has a defined lessee of %(lessee)s.', - {'project': project, - 'uuid': node.uuid, - 'lessee': node.lessee}) + else: + # NOTE(JayF): Don't apply lessee when rebuilding + auto_lessee = apply_automatic_lessee(task) # Infer the image type to make sure the deploy driver # validates only the necessary variables for different # image types. - if utils.update_image_type(task.context, task.node): + if utils.update_image_type(task.context, task.node) or auto_lessee: node.save() try: diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index cabd796b29..9b6b32bb21 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -20,6 +20,7 @@ from oslo_config import types from ironic.common import boot_modes from ironic.common.i18n import _ +from ironic.common import lessee_sources opts = [ @@ -345,15 +346,36 @@ opts = [ 'for multiple steps. If set to 0, this specific step ' 'will not run during verification. ')), cfg.BoolOpt('automatic_lessee', - default=False, + default=True, mutable=True, - help=_('If the conductor should record the Project ID ' - 'indicated by Keystone for a requested deployment. ' - 'Allows rights to be granted to directly access the ' - 'deployed node as a lessee within the RBAC security ' - 'model. The conductor does *not* record this value ' - 'otherwise, and this information is not backfilled ' - 'for prior instances which have been deployed.')), + help=_('Deprecated. If Ironic should set the node.lessee ' + 'field at deployment. Use ' + '[\'conductor\']/automatic_lessee_source instead.'), + deprecated_for_removal=True), + cfg.StrOpt('automatic_lessee_source', + help=_('Source for Project ID the Ironic should ' + 'record at deployment time in node.lessee field. If set ' + 'to none, Ironic will not set a lessee field. ' + 'If set to instance (default), uses Project ID ' + 'indicated in instance metadata set by Nova or ' + 'another external deployment service. ' + 'If set to keystone, Ironic uses Project ID indicated ' + 'by Keystone context. '), + choices=[ + (lessee_sources.INSTANCE, _( # 'instance' + 'Populates node.lessee field using metadata from ' + 'node.instance_info[\'project_id\'] at deployment ' + 'time. Useful for Nova-fronted deployments.')), + (lessee_sources.REQUEST, _( # 'request' + 'Populates node.lessee field using metadata ' + 'from request context. Only useful for direct ' + 'deployment requests to Ironic; not those proxied ' + 'via an external service like Nova.')), + (lessee_sources.NONE, _( # 'none' + 'Ironic will not populate the node.lessee field.')) + ], + default='instance', + mutable=True), cfg.IntOpt('max_concurrent_deploy', default=250, min=1, diff --git a/ironic/tests/unit/conductor/test_deployments.py b/ironic/tests/unit/conductor/test_deployments.py index f073460853..c96a6fae1a 100644 --- a/ironic/tests/unit/conductor/test_deployments.py +++ b/ironic/tests/unit/conductor/test_deployments.py @@ -21,6 +21,7 @@ from oslo_utils import uuidutils from ironic.common import exception from ironic.common import images +from ironic.common import lessee_sources from ironic.common import states from ironic.common import swift from ironic.conductor import deployments @@ -391,19 +392,29 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): mock_validate_deploy_user_steps_and_templates, mock_deploy_validate, mock_power_validate, mock_process_event, - automatic_lessee=False): + automatic_lessee=None, iinfo=None, + automatic_lessee_source=None): self.context.auth_token_info = { 'token': {'project': {'id': 'user-project'}} } - if automatic_lessee: - self.config(automatic_lessee=True, group='conductor') + if iinfo is None: + iinfo = {} + + if automatic_lessee is not None: + self.config(automatic_lessee=automatic_lessee, group='conductor') + + if automatic_lessee_source is not None: + self.config(automatic_lessee_source=automatic_lessee_source, + group='conductor') + self._start_service() mock_iwdi.return_value = False 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) + target_provision_state=states.ACTIVE, + instance_info=iinfo) task = task_manager.TaskManager(self.context, node.uuid) deployments.start_deploy(task, self.service, configdrive=None, @@ -422,18 +433,87 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): mock.ANY, 'deploy', call_args=( deployments.do_node_deploy, task, 1, None, deploy_steps), callback=mock.ANY, err_handler=mock.ANY) - if automatic_lessee: - self.assertEqual('user-project', node.lessee) + expected_project = iinfo.get('project_id', 'user-project') + if (automatic_lessee_source not in [None, lessee_sources.NONE] + and automatic_lessee in [None, True]): + self.assertEqual(expected_project, node.lessee) self.assertIn('automatic_lessee', node.driver_internal_info) else: self.assertIsNone(node.lessee) self.assertNotIn('automatic_lessee', node.driver_internal_info) - def test_start_deploy(self): + def test_start_deploy_lessee_legacy_false(self): self._test_start_deploy(automatic_lessee=False) - def test_start_deploy_records_lessee(self): - self._test_start_deploy(automatic_lessee=True) + def test_start_deploy_lessee_source_none(self): + self._test_start_deploy(automatic_lessee_source=lessee_sources.NONE) + + def test_start_deploy_lessee_source_request(self): + """Validates that project_id from request context is the lessee.""" + self._test_start_deploy(automatic_lessee_source=lessee_sources.REQUEST) + + def test_start_deploy_lessee_source_instance(self): + """Validates that project_id from instance info is the lessee.""" + self._test_start_deploy( + automatic_lessee_source=lessee_sources.INSTANCE, + iinfo={'project_id': 'user-project-iinfo'}) + + @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_lessee_legacy_false_even_if_src_set( + self, mock_iwdi, mock_validate_traits, + mock_validate_deploy_user_steps_and_templates, + mock_deploy_validate, mock_power_validate, mock_process_event): + self.context.auth_token_info = { + 'token': {'project': {'id': 'user-project'}} + } + iinfo = {'project_id': 'user-project-iinfo'} + + # Legacy lessee is disabled + self.config(automatic_lessee=False, group='conductor') + # but source is also set -- we should respect the disablement above all + self.config(automatic_lessee_source=lessee_sources.INSTANCE, + group='conductor') + + self._start_service() + mock_iwdi.return_value = False + 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, + instance_info=iinfo) + 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) + self.assertFalse(node.driver_internal_info['is_whole_disk_image']) + self.assertEqual('partition', node.instance_info['image_type']) + 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) + + self.assertIsNone(node.lessee) + self.assertNotIn('automatic_lessee', node.driver_internal_info) @mock.patch.object(images, 'is_source_a_path', autospec=True) @mock.patch.object(task_manager.TaskManager, 'process_event', diff --git a/releasenotes/notes/automatic-lessee-source-37abe917b8cb5c36.yaml b/releasenotes/notes/automatic-lessee-source-37abe917b8cb5c36.yaml new file mode 100644 index 0000000000..69b9937c11 --- /dev/null +++ b/releasenotes/notes/automatic-lessee-source-37abe917b8cb5c36.yaml @@ -0,0 +1,24 @@ +features: + - | + Ironic now supports automatically setting node.lessee at deployment time + using metadata provided at deploy time, typically by OpenStack Nova. When + ``[conductor]/automatic_lessee_source`` is set to ``instance``, + Ironic will set the lessee field on the node and remove it before cleaning. +upgrade: + - | + ``[conductor]/automatic_lessee`` has been deprecated in favor of + ``[conductor]/automatic_lessee_source``. + + Standalone Ironic deployments previously setting ``automatic_lessee`` to + ``True`` now may want to set ``automatic_lessee_source`` to ``request`` to + retain existing behavior. + + Deployers explicitly setting ``automatic_lessee`` to false may want to set + ``automatic_lessee_source`` to ``none`` to retain existing behavior. The + old configuration option, when explicitly set, will be honored until + fully removed. + - | + Ironic will now automatically set the node.lessee field for all + deployments by default when provided in node instance_info at deployment + time. Deployers are encouraged to review their security settings and + Ironic Secure RBAC documentation to ensure no unexpected access is granted. \ No newline at end of file