From c5f2ced1201f3add50e491872da49383f8ca0fe0 Mon Sep 17 00:00:00 2001 From: Himanshu Roy Date: Mon, 18 May 2026 23:53:02 +0530 Subject: [PATCH] Add operator-configurable step disallow lists Add [api]disallow_deploy_steps, disallow_clean_steps, and disallow_service_steps config options. Steps are blocked at the API layer (with error) and filtered at the conductor layer (silently) to cover all paths: manual, automated, templates, and runbooks. note(JayF) This backport contained some tests which depended on the changes to allow network_interfaces to have service steps. Those tests have been removed. note(JayF) When backporting 2025.2->2025.1, we also had to remove an additional test which does not apply to this branch. Closes-Bug: #2150456 Change-Id: I54d854f81cd6e8ee2d6738c37df3f61ea9341b34 Assisted-By: Claude Opus 4.6 Signed-off-by: Himanshu Roy Signed-off-by: Jay Faulkner (cherry picked from commit ae71bf1b23807149d0792e15d163cd140e334071) (cherry picked from commit be74d8a227e7e8e0328b0c7f8278e18d3bd4be27) (cherry picked from commit 904c5e595edd41acfae161841571d4ea80c939fc) (cherry picked from commit b62a57448c147e28bedfe7204194e16ba11889c3) (cherry picked from commit 449ff671b367658cdb5fc771e390ee8eb819f9a6) --- ironic/api/controllers/v1/node.py | 24 ++ ironic/common/exception.py | 7 + ironic/conductor/manager.py | 28 +- ironic/conductor/steps.py | 58 +++- ironic/conf/api.py | 71 +++++ .../unit/api/controllers/v1/test_node.py | 108 +++++++ ironic/tests/unit/conductor/test_manager.py | 148 +++++++++ ironic/tests/unit/conductor/test_steps.py | 284 ++++++++++++++++++ 8 files changed, 723 insertions(+), 5 deletions(-) diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 65224abf25..9ea4b7a38b 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1327,6 +1327,14 @@ def _check_clean_steps(clean_steps): """ _check_steps(clean_steps, 'clean', _STEPS_SCHEMA) + disallowed_steps = CONF.api.disallow_clean_steps + if disallowed_steps: + for step in clean_steps: + step_id = '%s.%s' % (step['interface'], step['step']) + if step_id in disallowed_steps: + raise exception.StepNotAllowed(step=step_id, + step_type='clean') + def _check_deploy_steps(deploy_steps): """Ensure all necessary keys are present and correct in steps for deploy @@ -1337,6 +1345,14 @@ def _check_deploy_steps(deploy_steps): """ _check_steps(deploy_steps, 'deploy', _DEPLOY_STEPS_SCHEMA) + disallowed_steps = CONF.api.disallow_deploy_steps + if disallowed_steps: + for step in deploy_steps: + step_id = '%s.%s' % (step['interface'], step['step']) + if step_id in disallowed_steps: + raise exception.StepNotAllowed(step=step_id, + step_type='deploy') + def _check_service_steps(service_steps): """Ensure all necessary keys are present and correct in steps for service @@ -1347,6 +1363,14 @@ def _check_service_steps(service_steps): """ _check_steps(service_steps, 'service', _STEPS_SCHEMA) + disallowed_steps = CONF.api.disallow_service_steps + if disallowed_steps: + for step in service_steps: + step_id = '%s.%s' % (step['interface'], step['step']) + if step_id in disallowed_steps: + raise exception.StepNotAllowed(step=step_id, + step_type='service') + def _check_steps(steps, step_type, schema): """Ensure all necessary keys are present and correct in steps. diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 23b3a23a0d..05d24bdd3c 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -1006,6 +1006,13 @@ class BootModeNotAllowed(Invalid): _msg_fmt = _("'%(mode)s' boot mode is not allowed for %(op)s operation.") +class StepNotAllowed(Invalid): + _msg_fmt = _("%(step_type)s step '%(step)s' is not allowed. Disallowed " + "by operator configuration " + "[api]disallow_%(step_type)s_steps.") + code = http_client.BAD_REQUEST + + class InvalidImage(ImageUnacceptable): _msg_fmt = _("The requested image is not valid for use.") diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 1d6b4dbe7a..20861fe9de 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -895,7 +895,8 @@ def _do_node_rescue_abort(self, task): exception.InstanceDeployFailure, exception.InvalidStateRequested, exception.NodeProtected, - exception.ConcurrentActionLimit) + exception.ConcurrentActionLimit, + exception.StepNotAllowed) def do_node_deploy(self, context, node_id, rebuild=False, configdrive=None, deploy_steps=None): """RPC method to initiate deployment to a node. @@ -933,6 +934,12 @@ def do_node_deploy(self, context, node_id, rebuild=False, with task_manager.acquire(context, node_id, shared=False, purpose='node deployment') as task: deployments.validate_node(task, event=event) + # Check user-provided deploy steps against the disallow + # list before transitioning state so the node stays in + # its current state on rejection. + if deploy_steps: + conductor_steps.check_disallowed_steps( + deploy_steps, 'deploy', raise_on_disallowed=True) deployments.start_deploy(task, self, configdrive, event=event, deploy_steps=deploy_steps) @@ -1178,7 +1185,8 @@ def _do_node_tear_down(self, task, initial_state): exception.NodeInMaintenance, exception.NodeLocked, exception.NoFreeConductorWorker, - exception.ConcurrentActionLimit) + exception.ConcurrentActionLimit, + exception.StepNotAllowed) def do_node_clean(self, context, node_id, clean_steps, disable_ramdisk=False): """RPC method to initiate manual cleaning. @@ -1232,6 +1240,12 @@ def do_node_clean(self, context, node_id, clean_steps, {'node': node.uuid, 'msg': e}) raise exception.InvalidParameterValue(msg) + # Check user-provided clean steps against the disallow + # list before transitioning state so the node stays in + # its current state on rejection. + conductor_steps.check_disallowed_steps( + clean_steps, 'clean', raise_on_disallowed=True) + try: task.process_event( 'clean', @@ -3806,7 +3820,8 @@ def continue_inspection(self, context, node_id, inventory, exception.NodeInMaintenance, exception.NodeLocked, exception.NoFreeConductorWorker, - exception.ConcurrentActionLimit) + exception.ConcurrentActionLimit, + exception.StepNotAllowed) def do_node_service(self, context, node_id, service_steps, disable_ramdisk=False): """RPC method to initiate node service. @@ -3858,6 +3873,13 @@ def do_node_service(self, context, node_id, service_steps, 'failed: %(msg)s') % {'node': node.uuid, 'msg': e}) raise exception.InvalidParameterValue(msg) + + # Check user-provided service steps against the disallow + # list before transitioning state so the node stays in + # its current state on rejection. + conductor_steps.check_disallowed_steps( + service_steps, 'service', raise_on_disallowed=True) + try: task.process_event( 'service', diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py index 79a23edeff..6552b31af1 100644 --- a/ironic/conductor/steps.py +++ b/ironic/conductor/steps.py @@ -325,6 +325,12 @@ def set_node_cleaning_steps(task, disable_ramdisk=False): task, node.driver_internal_info['clean_steps'], disable_ramdisk=disable_ramdisk) + # Filter out operator-disallowed clean steps + disallowed = CONF.api.disallow_clean_steps + if disallowed: + steps = [s for s in steps + if step_id(s) not in disallowed] + LOG.debug('List of the steps for %(type)s cleaning of node %(node)s: ' '%(steps)s', {'type': 'manual' if manual_clean else 'automated', 'node': node.uuid, @@ -464,8 +470,15 @@ def set_node_deployment_steps(task, reset_current=True, skip_missing=False): deployment steps. """ node = task.node - node.set_driver_internal_info('deploy_steps', _get_all_deployment_steps( - task, skip_missing=skip_missing)) + steps = _get_all_deployment_steps(task, skip_missing=skip_missing) + + # Filter out operator-disallowed deploy steps + disallowed = CONF.api.disallow_deploy_steps + if disallowed: + steps = [s for s in steps + if step_id(s) not in disallowed] + + node.set_driver_internal_info('deploy_steps', steps) LOG.debug('List of the deploy steps for node %(node)s: %(steps)s', { 'node': node.uuid, @@ -495,6 +508,13 @@ def set_node_service_steps(task, disable_ramdisk=False): steps = _validate_user_service_steps( task, node.driver_internal_info.get('service_steps', []), disable_ramdisk=disable_ramdisk) + + # Filter out operator-disallowed service steps + disallowed = CONF.api.disallow_service_steps + if disallowed: + steps = [s for s in steps + if step_id(s) not in disallowed] + LOG.debug('List of the steps for service of node %(node)s: ' '%(steps)s', {'node': node.uuid, 'steps': steps}) @@ -516,6 +536,40 @@ def step_id(step): return '.'.join([step['interface'], step['step']]) +def check_disallowed_steps(steps, step_type, raise_on_disallowed=True): + """Check and optionally filter steps against the operator disallow list. + + When raise_on_disallowed is True (user-provided steps), raises + StepNotAllowed for the first disallowed step found. When False + (automated / runbook steps), silently filters them out. + + :param steps: list of step dicts, or None. + :param step_type: one of 'clean', 'deploy', 'service'. + :param raise_on_disallowed: if True, raise on first disallowed step; + if False, silently remove disallowed steps. + :returns: the (possibly filtered) list of steps, or None. + :raises: StepNotAllowed if raise_on_disallowed and a step is disallowed. + """ + if steps is None: + return None + config_map = { + 'clean': CONF.api.disallow_clean_steps, + 'deploy': CONF.api.disallow_deploy_steps, + 'service': CONF.api.disallow_service_steps, + } + disallowed = config_map[step_type] + if not disallowed: + return steps + if raise_on_disallowed: + for step in steps: + sid = step_id(step) + if sid in disallowed: + raise exception.StepNotAllowed(step=sid, step_type=step_type) + return steps + else: + return [s for s in steps if step_id(s) not in disallowed] + + def _validate_deploy_steps_unique(user_steps): """Validate that deploy steps from deploy templates are unique. diff --git a/ironic/conf/api.py b/ironic/conf/api.py index b04048d1d8..450695a901 100644 --- a/ironic/conf/api.py +++ b/ironic/conf/api.py @@ -103,6 +103,77 @@ def __call__(self, value): mutable=True, help=_("Specifies a list of boot modes that are not allowed " "during enrollment. Eg: ['bios']")), + cfg.ListOpt('disallow_deploy_steps', + default=[], + mutable=True, + help=_("List of steps not allowed across the deploy " + "workflow. Each entry should be in 'interface.step' " + "format, e.g. ['raid.apply_configuration']. " + "Applies to user-requested steps, deploy template " + "steps, and driver steps alike.")), + cfg.ListOpt('disallow_service_steps', + default=[], + mutable=True, + help=_("List of steps not allowed across the service " + "workflow. Each entry should be in 'interface.step' " + "format, e.g. " + "['bios.factory_reset','bios.apply_configuration']. " + "Applies to user-requested steps and driver steps " + "alike.")), + cfg.ListOpt('disallow_clean_steps', + default=[], + mutable=True, + help=_("List of steps not allowed across the clean " + "workflow. Each entry should be in 'interface.step' " + "format, e.g. " + "['bios.factory_reset','bios.apply_configuration']. " + "Applies to user-requested (manual) steps, " + "automated cleaning steps, and runbook steps " + "alike.")), + cfg.IntOpt('max_json_body_depth', + default=25, + min=8, + mutable=True, + help=_('Maximum JSON nesting depth allowed in API ' + 'request bodies. Requests exceeding this ' + 'depth are rejected with HTTP 400 to prevent ' + 'recursion-based crashes in the JSON parser. ' + 'The deepest known legitimate structure in ' + 'the Ironic API is approximately 7 levels ' + '(configdrive network_data).')), + cfg.IntOpt('max_json_body_size', + default=1024, + min=4, + mutable=True, + help=_('Maximum size of a JSON request body, ' + 'in KiB. Requests with a Content-Length ' + 'exceeding this value are rejected with ' + 'HTTP 413 before the body is read into ' + 'memory. The node provision and inspection ' + 'endpoints use the separate ' + '[api]max_json_body_size_provision and ' + '[api]max_json_body_size_inspection limits ' + 'respectively. Defaults to 1 MiB.')), + cfg.IntOpt('max_json_body_size_provision', + default=65536, + min=4, + mutable=True, + help=_('Maximum size of a JSON request body ' + 'for the node provision state endpoint, ' + 'in KiB. This endpoint may carry ' + 'configdrive data and deploy steps. ' + 'Defaults to 64 MiB.')), + cfg.IntOpt('max_json_body_size_inspection', + default=16384, + min=4, + mutable=True, + help=_('Maximum size of a JSON request body ' + 'for the continue_inspection endpoint, ' + 'in KiB. Inspection data from the ' + 'ramdisk may include system logs such ' + 'as the journal, making the payload ' + 'significantly larger than normal API ' + 'requests. Defaults to 16 MiB.')), ] opt_group = cfg.OptGroup(name='api', diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 9aeec840e9..d7f0fe0050 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -7521,6 +7521,114 @@ def test_check__check_steps_wrappers(self, check_mock): self.assertEqual(3, check_mock.call_count) +class TestCheckDisallowedSteps(db_base.DbTestCase): + + def test__check_clean_steps_disallowed(self): + self.config(disallow_clean_steps=['raid.create_configuration'], + group='api') + clean_steps = [{"interface": "raid", "step": "create_configuration"}] + self.assertRaisesRegex(exception.StepNotAllowed, + "clean step 'raid.create_configuration' " + "is not allowed", + api_node._check_clean_steps, clean_steps) + + def test__check_clean_steps_disallowed_not_in_list(self): + self.config(disallow_clean_steps=['raid.create_configuration'], + group='api') + clean_steps = [{"interface": "raid", "step": "delete_configuration"}] + api_node._check_clean_steps(clean_steps) + + def test__check_clean_steps_disallowed_empty(self): + self.config(disallow_clean_steps=[], group='api') + clean_steps = [{"interface": "raid", "step": "create_configuration"}] + api_node._check_clean_steps(clean_steps) + + def test__check_clean_steps_disallowed_multiple(self): + self.config(disallow_clean_steps=['raid.create_configuration', + 'bios.factory_reset'], + group='api') + clean_steps = [{"interface": "bios", "step": "factory_reset"}] + self.assertRaisesRegex(exception.StepNotAllowed, + "clean step 'bios.factory_reset' " + "is not allowed", + api_node._check_clean_steps, clean_steps) + + def test__check_deploy_steps_disallowed(self): + self.config(disallow_deploy_steps=['raid.create_configuration'], + group='api') + deploy_steps = [{"interface": "raid", "step": "create_configuration", + "args": {}, "priority": 100}] + self.assertRaisesRegex(exception.StepNotAllowed, + "deploy step 'raid.create_configuration' " + "is not allowed", + api_node._check_deploy_steps, deploy_steps) + + def test__check_deploy_steps_disallowed_not_in_list(self): + self.config(disallow_deploy_steps=['raid.create_configuration'], + group='api') + deploy_steps = [{"interface": "raid", "step": "delete_configuration", + "args": {}, "priority": 100}] + api_node._check_deploy_steps(deploy_steps) + + def test__check_deploy_steps_disallowed_empty(self): + self.config(disallow_deploy_steps=[], group='api') + deploy_steps = [{"interface": "raid", "step": "create_configuration", + "args": {}, "priority": 100}] + api_node._check_deploy_steps(deploy_steps) + + def test__check_service_steps_disallowed(self): + self.config(disallow_service_steps=['bios.apply_configuration'], + group='api') + service_steps = [{"interface": "bios", + "step": "apply_configuration"}] + self.assertRaisesRegex(exception.StepNotAllowed, + "service step 'bios.apply_configuration' " + "is not allowed", + api_node._check_service_steps, service_steps) + + def test__check_service_steps_disallowed_not_in_list(self): + self.config(disallow_service_steps=['bios.apply_configuration'], + group='api') + service_steps = [{"interface": "bios", "step": "factory_reset"}] + api_node._check_service_steps(service_steps) + + def test__check_service_steps_disallowed_empty(self): + self.config(disallow_service_steps=[], group='api') + service_steps = [{"interface": "bios", + "step": "apply_configuration"}] + api_node._check_service_steps(service_steps) + + def test__check_disallowed_steps_independent_per_type(self): + """Disallow lists are independent per step type.""" + self.config(disallow_clean_steps=['bios.factory_reset'], + disallow_deploy_steps=[], + disallow_service_steps=[], + group='api') + # Disallowed for clean + clean_steps = [{"interface": "bios", "step": "factory_reset"}] + self.assertRaisesRegex(exception.StepNotAllowed, + "clean step", + api_node._check_clean_steps, clean_steps) + # Same step is allowed for deploy + deploy_steps = [{"interface": "bios", "step": "factory_reset", + "args": {}, "priority": 100}] + api_node._check_deploy_steps(deploy_steps) + # Same step is allowed for service + service_steps = [{"interface": "bios", "step": "factory_reset"}] + api_node._check_service_steps(service_steps) + + def test__check_clean_steps_disallowed_schema_validated_first(self): + """Schema validation runs before the disallow check.""" + self.config(disallow_clean_steps=['raid.create_configuration'], + group='api') + # Bad schema: missing 'step' key. Should raise InvalidParameterValue + # from schema validation, not StepNotAllowed. + clean_steps = [{"interface": "raid"}] + self.assertRaisesRegex(exception.InvalidParameterValue, + 'step', + api_node._check_clean_steps, clean_steps) + + class TestAttachDetachVif(test_api_base.BaseApiTest): def setUp(self): diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 20a660b24b..d4b28f4778 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2151,6 +2151,49 @@ def test_do_node_deploy_worker_pool_full(self, mock_iwdi): mock_iwdi.assert_called_once_with(self.context, node.instance_info) self.assertFalse(node.driver_internal_info['is_whole_disk_image']) + @mock.patch.object(deployments, 'start_deploy', autospec=True) + def test_do_node_deploy_disallowed_step_raises(self, mock_start, + mock_iwdi): + """Disallowed deploy step raises before state transition.""" + self.config(disallow_deploy_steps=['bios.factory_reset'], group='api') + mock_iwdi.return_value = False + self._start_service() + 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.NOSTATE) + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_deploy, + self.context, node.uuid, + deploy_steps=deploy_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + # start_deploy must NOT have been called + self.assertFalse(mock_start.called) + node.refresh() + self.assertEqual(states.AVAILABLE, node.provision_state) + + @mock.patch.object(deployments, 'start_deploy', autospec=True) + def test_do_node_deploy_allowed_step_proceeds(self, mock_start, + mock_iwdi): + """Allowed deploy step proceeds normally.""" + self.config(disallow_deploy_steps=['raid.create_configuration'], + group='api') + mock_iwdi.return_value = False + self._start_service() + 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.NOSTATE) + self.service.do_node_deploy(self.context, node.uuid, + deploy_steps=deploy_steps) + mock_start.assert_called_once_with( + mock.ANY, mock.ANY, None, event='deploy', + deploy_steps=deploy_steps) + @mgr_utils.mock_record_keepalive class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, @@ -3220,6 +3263,60 @@ def test_do_provision_action_unlocks_deploying(self, mock_spawn): self.assertEqual(states.DEPLOYING, node.provision_state) self._stop_service() + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_clean_disallowed_step_raises(self, mock_power_valid, + mock_network_valid, + mock_process): + """Disallowed manual clean step raises before state transition.""" + self.config(disallow_clean_steps=['deploy.build_raid'], group='api') + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.MANAGEABLE, + target_provision_state=states.NOSTATE) + self._start_service() + clean_steps = [self.deploy_raid] + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_clean, + self.context, node.uuid, clean_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + # process_event must NOT have been called + self.assertFalse(mock_process.called) + node.refresh() + # Node stays in original state + self.assertEqual(states.MANAGEABLE, node.provision_state) + + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_clean_allowed_step_proceeds(self, mock_power_valid, + mock_network_valid, + mock_process): + """Allowed clean step proceeds normally.""" + self.config(disallow_clean_steps=['raid.create_configuration'], + group='api') + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.MANAGEABLE, + target_provision_state=states.NOSTATE) + self._start_service() + clean_steps = [self.deploy_raid] + self.service.do_node_clean(self.context, node.uuid, clean_steps) + mock_process.assert_called_once_with( + mock.ANY, + 'clean', + callback=mock.ANY, + call_args=(cleaning.do_node_clean, mock.ANY, + clean_steps, False), + err_handler=mock.ANY, target_state='manageable') + @mgr_utils.mock_record_keepalive class DoNodeServiceTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): @@ -3443,6 +3540,57 @@ def test_do_node_provision_action_unhold_service(self, mock_spawn): self.service, servicing.continue_node_service, mock.ANY) self._stop_service() + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_service_disallowed_step_raises(self, mock_pv, mock_nv, + mock_event): + """Disallowed service step raises before state transition.""" + self.config(disallow_service_steps=['deploy.update_firmware'], + group='api') + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.ACTIVE, + target_provision_state=states.NOSTATE) + self._start_service() + service_steps = [self.deploy_update] + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_service, + self.context, node.uuid, service_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + self.assertFalse(mock_event.called) + node.refresh() + self.assertEqual(states.ACTIVE, node.provision_state) + + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_service_allowed_step_proceeds(self, mock_pv, mock_nv, + mock_event): + """Allowed service step proceeds normally.""" + self.config(disallow_service_steps=['raid.create_configuration'], + group='api') + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.ACTIVE, + target_provision_state=states.NOSTATE) + self._start_service() + service_steps = [self.deploy_update] + self.service.do_node_service(self.context, node.uuid, service_steps) + mock_event.assert_called_once_with( + mock.ANY, + 'service', + callback=mock.ANY, + call_args=(servicing.do_node_service, mock.ANY, + service_steps, False), + err_handler=mock.ANY, target_state='active') + class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): diff --git a/ironic/tests/unit/conductor/test_steps.py b/ironic/tests/unit/conductor/test_steps.py index 8782c4a62c..3280c2710d 100644 --- a/ironic/tests/unit/conductor/test_steps.py +++ b/ironic/tests/unit/conductor/test_steps.py @@ -1494,3 +1494,287 @@ def test_set_node_service_steps(self, mock_steps): self.assertIsNone( self.node.driver_internal_info['service_step_index']) mock_steps.assert_called_once_with(task, [], disable_ramdisk=False) + + +class DisallowStepFilteringTestCase(db_base.DbTestCase): + """Tests for conductor-level disallow step filtering.""" + + def setUp(self): + super(DisallowStepFilteringTestCase, self).setUp() + self.node = obj_utils.create_test_node( + self.context, driver='fake-hardware') + + # Clean steps + self.clean_erase = { + 'step': 'erase_disks', 'priority': 20, 'interface': 'deploy', + 'abortable': True} + self.clean_update = { + 'step': 'update_firmware', 'priority': 10, 'interface': 'power'} + self.clean_steps = [self.clean_erase, self.clean_update] + + # Deploy steps + self.deploy_start = { + 'step': 'deploy_start', 'priority': 50, 'interface': 'deploy'} + self.deploy_raid = { + 'step': 'create_configuration', 'priority': 40, + 'interface': 'raid'} + self.deploy_steps = [self.deploy_start, self.deploy_raid] + + @mock.patch.object(conductor_steps, '_validate_user_clean_steps', + autospec=True) + @mock.patch.object(conductor_steps, '_get_cleaning_steps', autospec=True) + def test_set_node_cleaning_steps_automated_filters_disallowed( + self, mock_steps, mock_validate): + """Automated clean filters out disallowed steps.""" + self.config(disallow_clean_steps=['deploy.erase_disks'], group='api') + mock_steps.return_value = self.clean_steps + + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + uuid=uuidutils.generate_uuid(), + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE) + + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + conductor_steps.set_node_cleaning_steps(task) + node.refresh() + stored = node.driver_internal_info['clean_steps'] + self.assertEqual([self.clean_update], stored) + mock_steps.assert_called_once_with(task, enabled=True) + + @mock.patch.object(conductor_steps, '_validate_user_clean_steps', + autospec=True) + @mock.patch.object(conductor_steps, '_get_cleaning_steps', autospec=True) + def test_set_node_cleaning_steps_manual_filters_disallowed( + self, mock_steps, mock_validate): + """Manual clean also filters disallowed steps (defense-in-depth).""" + self.config(disallow_clean_steps=['power.update_firmware'], + group='api') + clean_steps = [self.clean_update] + mock_validate.return_value = clean_steps + + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + uuid=uuidutils.generate_uuid(), + provision_state=states.CLEANING, + target_provision_state=states.MANAGEABLE, + driver_internal_info={'clean_steps': clean_steps}) + + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + conductor_steps.set_node_cleaning_steps(task) + node.refresh() + stored = node.driver_internal_info['clean_steps'] + self.assertEqual([], stored) + + @mock.patch.object(conductor_steps, '_validate_user_clean_steps', + autospec=True) + @mock.patch.object(conductor_steps, '_get_cleaning_steps', autospec=True) + def test_set_node_cleaning_steps_empty_disallow_passes_all( + self, mock_steps, mock_validate): + """Empty disallow list passes all steps through.""" + self.config(disallow_clean_steps=[], group='api') + mock_steps.return_value = self.clean_steps + + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + uuid=uuidutils.generate_uuid(), + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE) + + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + conductor_steps.set_node_cleaning_steps(task) + node.refresh() + stored = node.driver_internal_info['clean_steps'] + self.assertEqual(self.clean_steps, stored) + + @mock.patch.object(conductor_steps, '_validate_user_clean_steps', + autospec=True) + @mock.patch.object(conductor_steps, '_get_cleaning_steps', autospec=True) + def test_set_node_cleaning_steps_disallow_multiple( + self, mock_steps, mock_validate): + """Multiple disallowed steps are all filtered.""" + self.config(disallow_clean_steps=['deploy.erase_disks', + 'power.update_firmware'], + group='api') + mock_steps.return_value = self.clean_steps + + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + uuid=uuidutils.generate_uuid(), + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE) + + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + conductor_steps.set_node_cleaning_steps(task) + node.refresh() + stored = node.driver_internal_info['clean_steps'] + self.assertEqual([], stored) + + @mock.patch.object(conductor_steps, '_get_all_deployment_steps', + autospec=True) + def test_set_node_deployment_steps_filters_disallowed(self, mock_steps): + """Disallowed deploy steps are filtered before storing.""" + self.config(disallow_deploy_steps=['raid.create_configuration'], + group='api') + mock_steps.return_value = self.deploy_steps + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + conductor_steps.set_node_deployment_steps(task) + self.node.refresh() + stored = self.node.driver_internal_info['deploy_steps'] + self.assertEqual([self.deploy_start], stored) + + @mock.patch.object(conductor_steps, '_get_all_deployment_steps', + autospec=True) + def test_set_node_deployment_steps_empty_disallow_passes_all( + self, mock_steps): + """Empty disallow list passes all steps through.""" + self.config(disallow_deploy_steps=[], group='api') + mock_steps.return_value = self.deploy_steps + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + conductor_steps.set_node_deployment_steps(task) + self.node.refresh() + stored = self.node.driver_internal_info['deploy_steps'] + self.assertEqual(self.deploy_steps, stored) + + @mock.patch.object(conductor_steps, '_get_all_deployment_steps', + autospec=True) + def test_set_node_deployment_steps_nonmatching_disallow(self, mock_steps): + """Non-matching disallow list leaves steps unchanged.""" + self.config(disallow_deploy_steps=['bios.factory_reset'], group='api') + mock_steps.return_value = self.deploy_steps + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + conductor_steps.set_node_deployment_steps(task) + self.node.refresh() + stored = self.node.driver_internal_info['deploy_steps'] + self.assertEqual(self.deploy_steps, stored) + + @mock.patch.object(conductor_steps, '_validate_user_service_steps', + autospec=True) + def test_set_node_service_steps_filters_disallowed(self, mock_validate): + """Disallowed service steps are filtered before storing.""" + self.config(disallow_service_steps=['bios.apply_configuration'], + group='api') + service_steps = [ + {'step': 'apply_configuration', 'priority': 50, + 'interface': 'bios'}, + {'step': 'factory_reset', 'priority': 40, 'interface': 'bios'}, + ] + mock_validate.return_value = service_steps + + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + uuid=uuidutils.generate_uuid(), + driver_internal_info={'service_steps': service_steps}) + + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + conductor_steps.set_node_service_steps(task) + node.refresh() + stored = node.driver_internal_info['service_steps'] + self.assertEqual([service_steps[1]], stored) + + +class CheckDisallowedStepsTestCase(db_base.DbTestCase): + """Tests for the check_disallowed_steps() helper.""" + + def setUp(self): + super(CheckDisallowedStepsTestCase, self).setUp() + self.steps = [ + {'interface': 'deploy', 'step': 'erase_disks'}, + {'interface': 'power', 'step': 'update_firmware'}, + {'interface': 'bios', 'step': 'factory_reset'}, + ] + + def test_none_steps_returns_none(self): + result = conductor_steps.check_disallowed_steps( + None, 'clean', raise_on_disallowed=True) + self.assertIsNone(result) + + def test_empty_disallow_list_passes_all(self): + self.config(disallow_clean_steps=[], group='api') + result = conductor_steps.check_disallowed_steps( + self.steps, 'clean', raise_on_disallowed=True) + self.assertEqual(self.steps, result) + + def test_raise_on_disallowed_clean(self): + self.config(disallow_clean_steps=['deploy.erase_disks'], group='api') + self.assertRaises( + exception.StepNotAllowed, + conductor_steps.check_disallowed_steps, + self.steps, 'clean', raise_on_disallowed=True) + + def test_raise_on_disallowed_deploy(self): + self.config(disallow_deploy_steps=['power.update_firmware'], + group='api') + self.assertRaises( + exception.StepNotAllowed, + conductor_steps.check_disallowed_steps, + self.steps, 'deploy', raise_on_disallowed=True) + + def test_raise_on_disallowed_service(self): + self.config(disallow_service_steps=['bios.factory_reset'], + group='api') + self.assertRaises( + exception.StepNotAllowed, + conductor_steps.check_disallowed_steps, + self.steps, 'service', raise_on_disallowed=True) + + def test_filter_silently(self): + self.config(disallow_clean_steps=['deploy.erase_disks'], group='api') + result = conductor_steps.check_disallowed_steps( + self.steps, 'clean', raise_on_disallowed=False) + self.assertEqual( + [self.steps[1], self.steps[2]], result) + + def test_filter_silently_multiple(self): + self.config(disallow_clean_steps=['deploy.erase_disks', + 'bios.factory_reset'], + group='api') + result = conductor_steps.check_disallowed_steps( + self.steps, 'clean', raise_on_disallowed=False) + self.assertEqual([self.steps[1]], result) + + def test_filter_silently_all_disallowed(self): + self.config(disallow_clean_steps=['deploy.erase_disks', + 'power.update_firmware', + 'bios.factory_reset'], + group='api') + result = conductor_steps.check_disallowed_steps( + self.steps, 'clean', raise_on_disallowed=False) + self.assertEqual([], result) + + def test_nonmatching_disallow_passes_all(self): + self.config(disallow_clean_steps=['raid.create_configuration'], + group='api') + result = conductor_steps.check_disallowed_steps( + self.steps, 'clean', raise_on_disallowed=True) + self.assertEqual(self.steps, result) + + def test_raise_reports_first_disallowed(self): + self.config(disallow_clean_steps=['deploy.erase_disks', + 'power.update_firmware'], + group='api') + exc = self.assertRaises( + exception.StepNotAllowed, + conductor_steps.check_disallowed_steps, + self.steps, 'clean', raise_on_disallowed=True) + self.assertIn('deploy.erase_disks', str(exc)) + + def test_disallow_lists_independent_per_type(self): + """Disallow list for one type does not affect another.""" + self.config(disallow_clean_steps=['deploy.erase_disks'], group='api') + self.config(disallow_deploy_steps=[], group='api') + # Should pass for deploy type even though clean disallows it + result = conductor_steps.check_disallowed_steps( + self.steps, 'deploy', raise_on_disallowed=True) + self.assertEqual(self.steps, result)