diff --git a/ironic/common/inspection_rules/actions.py b/ironic/common/inspection_rules/actions.py index 2e4e11327d..50a8c6acab 100644 --- a/ironic/common/inspection_rules/actions.py +++ b/ironic/common/inspection_rules/actions.py @@ -68,20 +68,29 @@ class ActionBase(base.Base, metaclass=abc.ABCMeta): """Run action on successful rule match.""" def execute_with_loop(self, task, action, inventory, plugin_data): - loop_items = action.get('loop', []) - results = [] + loop_items = None + if action.get('loop', []): + loop_items = action['loop'] if isinstance(loop_items, (list, dict)): - for item in loop_items: + if isinstance(loop_items, dict): + loop_context = {'item': loop_items} action_copy = action.copy() - action_copy['args'] = item - results.append(self.execute_action(task, action_copy, - inventory, plugin_data)) - return results + self.execute_action(task, action_copy, inventory, plugin_data, + loop_context) + return - def execute_action(self, task, action, inventory, plugin_data): + for item in loop_items: + loop_context = {'item': item} + action_copy = action.copy() + self.execute_action(task, action_copy, inventory, plugin_data, + loop_context) + + def execute_action(self, task, action, inventory, plugin_data, + loop_context=None): processed_args = self._process_args(task, action, inventory, - plugin_data) + plugin_data, loop_context) + return self(task, **processed_args) diff --git a/ironic/common/inspection_rules/base.py b/ironic/common/inspection_rules/base.py index f60ea129d8..29d4d07dc4 100644 --- a/ironic/common/inspection_rules/base.py +++ b/ironic/common/inspection_rules/base.py @@ -30,7 +30,7 @@ class Base(object): REQUIRES_PLUGIN_DATA = False """Flag to indicate if this action needs plugin_data as an arg.""" - def _get_validation_signature(self): + def get_validation_signature(self): """Get the signature to validate against.""" signature = inspect.signature(self.__call__) @@ -79,7 +79,7 @@ class Base(object): :param op_args: Operator args as a dictionary :raises: InspectionRuleValidationFailure on validation failure """ - required_args, optional_args = self._get_validation_signature() + required_args, optional_args = self.get_validation_signature() normalized_args = self._normalize_list_args( required_args=required_args, optional_args=optional_args, op_args=op_args) @@ -105,30 +105,70 @@ class Base(object): raise exception.InspectionRuleValidationFailure( _("args must be either a list or dictionary")) - def interpolate_variables(value, node, inventory, plugin_data): + def interpolate_variables(value, node, inventory, plugin_data, + loop_context=None): + loop_context = loop_context or {} + format_context = { + 'node': node, + 'inventory': inventory, + 'plugin_data': plugin_data, + **loop_context + } + + def safe_format(val, context): + if isinstance(val, str): + try: + return val.format(**context) + except (AttributeError, KeyError, ValueError, IndexError, + TypeError) as e: + LOG.warning( + "Interpolation failed: %(value)s: %(error_class)s, " + "%(error)s", {'value': val, + 'error_class': e.__class__.__name__, + 'error': e}) + return val + return val + + if loop_context: + # Format possible dictionary loop item containing replacement + # fields. + # + # E.g: + # 'args': {'path': '{item[path]}', 'value': '{item[value]}'}, + # 'loop': [ + # { + # 'path': 'driver_info/ipmi_address', + # 'value': '{inventory[bmc_address]}' + # } + # ] + # or a dict loop (which seems to defeat the purpose of a loop + # field, but is supported all the same): + # 'args': {'path': '{item[path]}', 'value': '{item[value]}'}, + # 'loop': { + # 'path': 'driver_info/ipmi_address', + # 'value': '{inventory[bmc_address]}' + # } + value = safe_format(value, format_context) + if isinstance(value, str): - try: - return value.format(node=node, inventory=inventory, - plugin_data=plugin_data) - except (AttributeError, KeyError, ValueError, IndexError, - TypeError) as e: - LOG.warning( - "Interpolation failed: %(value)s: %(error_class)s, " - "%(error)s", {'value': value, - 'error_class': e.__class__.__name__, - 'error': e}) - return value + return safe_format(value, format_context) elif isinstance(value, dict): return { - Base.interpolate_variables(k, node, inventory, plugin_data): - Base.interpolate_variables(v, node, inventory, plugin_data) - for k, v in value.items()} + safe_format(k, format_context): Base.interpolate_variables( + v, node, inventory, plugin_data, loop_context) + for k, v in value.items() + } elif isinstance(value, list): - return [Base.interpolate_variables( - v, node, inventory, plugin_data) for v in value] + return [ + safe_format(v, format_context) if isinstance(v, str) + else Base.interpolate_variables( + v, node, inventory, plugin_data, loop_context) + for v in value + ] return value - def _process_args(self, task, operation, inventory, plugin_data): + def _process_args(self, task, operation, inventory, plugin_data, + loop_context=None): "Normalize and process args based on the operator." op = operation.get('op') @@ -136,7 +176,7 @@ class Base(object): raise exception.InspectionRuleExecutionFailure( _("Operation must contain 'op' key")) - required_args, optional_args = self._get_validation_signature() + required_args, optional_args = self.get_validation_signature() op, invtd = utils.parse_inverted_operator(op) dict_args = self._normalize_list_args( @@ -151,7 +191,8 @@ class Base(object): node = task.node formatted_args = getattr(self, 'FORMATTED_ARGS', []) return { - k: (Base.interpolate_variables(v, node, inventory, plugin_data) - if k in formatted_args else v) + k: (Base.interpolate_variables( + v, node, inventory, plugin_data, loop_context) + if (k in formatted_args or loop_context) else v) for k, v in dict_args.items() } diff --git a/ironic/common/inspection_rules/engine.py b/ironic/common/inspection_rules/engine.py index d30ea0f0c4..c4ca4864e1 100644 --- a/ironic/common/inspection_rules/engine.py +++ b/ironic/common/inspection_rules/engine.py @@ -90,7 +90,7 @@ def check_conditions(task, rule, inventory, plugin_data): raise ValueError(msg) plugin = operators.get_operator(op) - if 'loop' in condition: + if condition.get('loop', []): result = plugin().check_with_loop(task, condition, inventory, plugin_data) else: @@ -123,7 +123,7 @@ def apply_actions(task, rule, inventory, plugin_data): raise ValueError(msg) plugin = actions.get_action(op) - if 'loop' in action: + if action.get('loop', []): plugin().execute_with_loop(task, action, inventory, plugin_data) else: diff --git a/ironic/common/inspection_rules/operators.py b/ironic/common/inspection_rules/operators.py index 673416a092..02d8166567 100644 --- a/ironic/common/inspection_rules/operators.py +++ b/ironic/common/inspection_rules/operators.py @@ -45,15 +45,6 @@ def get_operator(op_name): return globals()[class_name] -def coerce(value, expected): - if isinstance(expected, float): - return float(value) - elif isinstance(expected, int): - return int(value) - else: - return value - - class OperatorBase(base.Base, metaclass=abc.ABCMeta): """Abstract base class for rule condition plugins.""" @@ -62,22 +53,36 @@ class OperatorBase(base.Base, metaclass=abc.ABCMeta): """Checks if condition holds for a given field.""" def check_with_loop(self, task, condition, inventory, plugin_data): - loop_items = condition.get('loop', []) - multiple = condition.get('multiple', 'any') + loop_items = None + if condition.get('loop', []): + loop_items = condition['loop'] + multiple = condition.get('multiple', 'any') + results = [] - if isinstance(loop_items, (list, dict)): - for item in loop_items: + if isinstance(loop_items, dict): + loop_context = {'item': loop_items} condition_copy = condition.copy() - condition_copy['args'] = item result = self.check_condition(task, condition_copy, - inventory, plugin_data) + inventory, plugin_data, + loop_context) results.append(result) + if multiple in ('first', 'last'): + return result - if multiple == 'first' and result: - return True - elif multiple == 'last': - results = [result] + elif isinstance(loop_items, list): + for item in loop_items: + loop_context = {'item': item} + condition_copy = condition.copy() + result = self.check_condition(task, condition_copy, + inventory, plugin_data, + loop_context) + results.append(result) + + if multiple == 'first' and result: + return True + elif multiple == 'last': + results = [result] if multiple == 'any': return any(results) @@ -86,14 +91,15 @@ class OperatorBase(base.Base, metaclass=abc.ABCMeta): return results[0] if results else False return self.check_condition(task, condition, inventory, plugin_data) - def check_condition(self, task, condition, inventory, plugin_data): + def check_condition(self, task, condition, inventory, plugin_data, + loop_context=None): """Process condition arguments and apply the check logic. :param task: TaskManger instance :param condition: condition to check - :param args: parameters as a dictionary, changing it here will change - what will be stored in database - :param kwargs: used for extensibility without breaking existing plugins + :param inventory: Node inventory data with hardware information + :param plugin_data: Data from inspection plugins + :param loop_context: Current loop item when called from check_with_loop :raises InspectionRuleExecutionFailure: on unacceptable field value :returns: True if check succeeded, otherwise False """ @@ -101,7 +107,15 @@ class OperatorBase(base.Base, metaclass=abc.ABCMeta): condition['op']) processed_args = self._process_args(task, condition, inventory, - plugin_data) + plugin_data, loop_context) + + required_args, optional_args = self.get_validation_signature() + if loop_context and 'force_strings' in optional_args: + # When in a loop context, variable interpolation might convert + # numbers to strings. Setting force_strings ensures consistent + # comparison by converting all values to strings before + # comparison. + processed_args['force_strings'] = True result = self(task, **processed_args) return not result if is_inverted else result @@ -125,7 +139,7 @@ class SimpleOperator(OperatorBase): return True if force_strings: - values = [coerce(value, str) for value in values] + values = [str(value) for value in values] return all(self.op(values[i], values[i + 1]) for i in range(len(values) - 1)) diff --git a/ironic/tests/unit/common/test_inspection_rule.py b/ironic/tests/unit/common/test_inspection_rule.py index d7cbe42991..e19be23471 100644 --- a/ironic/tests/unit/common/test_inspection_rule.py +++ b/ironic/tests/unit/common/test_inspection_rule.py @@ -16,6 +16,7 @@ from oslo_utils import uuidutils from ironic.common import exception from ironic.common import inspection_rules +from ironic.common.inspection_rules import base from ironic.common.inspection_rules import engine from ironic.common.inspection_rules import utils from ironic.conductor import task_manager @@ -27,7 +28,10 @@ class TestInspectionRules(db_base.DbTestCase): def setUp(self): super(TestInspectionRules, self).setUp() self.node = obj_utils.create_test_node(self.context, - driver='fake-hardware') + driver='fake-hardware', + driver_info={}, + extra={}) + self.sensitive_fields = ['password', 'auth_token', 'bmc_password'] self.test_data = { 'username': 'testuser', @@ -378,40 +382,78 @@ class TestOperators(TestInspectionRules): def test_operator_with_loop(self): """Test operator check_with_loop method.""" - condition = { + eq_condition = { 'op': 'eq', - 'loop': [ - {'values': [1, 1]}, - {'values': [2, 2]}, - {'values': [3, 4]} - ], + 'args': {'values': [1, '{item}']}, + 'loop': [1, 2, 3, 4], 'multiple': 'any' } - inventory = {'data': 'test'} - plugin_data = {'plugin': 'data'} + contains_condition = { + 'op': 'contains', + 'args': {'value': '{item}', 'regex': '4'}, + 'loop': ['test4', 'value5', 'string6'], + 'multiple': 'any' + } + + oneof_condition = { + 'op': 'one-of', + 'args': {'value': '{inventory[cpu][architecture]}', + 'values': ['{item}']}, + 'loop': ['x86_64', 'aarch64', 'ppc64le'], + 'multiple': 'any' + } with task_manager.acquire(self.context, self.node.uuid) as task: - op = inspection_rules.operators.EqOperator() + eq_op = inspection_rules.operators.EqOperator() + contains_op = inspection_rules.operators.ContainsOperator() + oneof_op = inspection_rules.operators.OneOfOperator() # 'any' multiple (should return True) - self.assertTrue(op.check_with_loop(task, condition, inventory, - plugin_data)) + self.assertTrue(eq_op.check_with_loop( + task, eq_condition, self.inventory, self.plugin_data)) + self.assertTrue(contains_op.check_with_loop( + task, contains_condition, self.inventory, self.plugin_data)) + self.assertTrue(oneof_op.check_with_loop( + task, oneof_condition, self.inventory, self.plugin_data)) # 'all' multiple (should return False) - condition['multiple'] = 'all' - self.assertFalse(op.check_with_loop(task, condition, inventory, - plugin_data)) + eq_condition['multiple'] = 'all' + contains_condition['multiple'] = 'all' + oneof_condition['multiple'] = 'all' + + self.assertFalse(eq_op.check_with_loop( + task, eq_condition, self.inventory, self.plugin_data)) + self.assertFalse(contains_op.check_with_loop( + task, contains_condition, self.inventory, self.plugin_data)) + self.assertFalse(oneof_op.check_with_loop( + task, oneof_condition, self.inventory, self.plugin_data)) # 'first' multiple (should return True) - condition['multiple'] = 'first' - self.assertTrue(op.check_with_loop(task, condition, inventory, - plugin_data)) + eq_condition['multiple'] = 'first' + contains_condition['multiple'] = 'first' + oneof_condition['multiple'] = 'first' - # 'last' multiple (should return False) - condition['multiple'] = 'last' - self.assertFalse(op.check_with_loop(task, condition, inventory, - plugin_data)) + self.assertTrue(eq_op.check_with_loop( + task, eq_condition, self.inventory, self.plugin_data)) + self.assertTrue(contains_op.check_with_loop( + task, contains_condition, self.inventory, self.plugin_data)) + self.assertTrue(oneof_op.check_with_loop( + task, oneof_condition, self.inventory, self.plugin_data)) + + # 'last' multiple (should return False for eq, True for others) + eq_condition['multiple'] = 'last' + contains_condition['multiple'] = 'last' + oneof_condition['multiple'] = 'last' + + self.assertFalse(eq_op.check_with_loop(task, eq_condition, + self.inventory, + self.plugin_data)) + self.assertFalse(contains_op.check_with_loop( + task, contains_condition, self.inventory, self.plugin_data)) + # This should be False since 'ppc64le' doesn't match 'x86_64' + self.assertFalse(oneof_op.check_with_loop( + task, oneof_condition, self.inventory, self.plugin_data)) def test_rule_operators(self): """Test all inspection_rules.operators with True and False cases.""" @@ -492,12 +534,34 @@ class TestActions(TestInspectionRules): self.assertRaises(exception.HardwareInspectionFailure, action, task, msg=error_msg) + def test_action_path_dot_slash_notation(self): + with task_manager.acquire(self.context, self.node.uuid) as task: + action = inspection_rules.actions.SetAttributeAction() + + # slash notation + action( + task, path='driver_info/new', value={'new_key': 'test_value'}) + + # dot notation + action(task, path='driver_info.next.nested.deeper', + value={'next_key': 'test_value'}) + + self.assertEqual( + {'new_key': 'test_value'}, task.node.driver_info['new']) + self.assertEqual( + {'nested': {'deeper': {'next_key': 'test_value'}}}, + task.node.driver_info['next']) + def test_set_attribute_action(self): """Test SetAttributeAction sets node attribute.""" with task_manager.acquire(self.context, self.node.uuid) as task: action = inspection_rules.actions.SetAttributeAction() - action(task, path='extra', value={'test_key': 'test_value'}) - self.assertEqual({'test_key': 'test_value'}, task.node.extra) + + action( + task, path='driver_info/new', value={'new_key': 'test_value'}) + + self.assertEqual( + {'new_key': 'test_value'}, task.node.driver_info['new']) def test_extend_attribute_action(self): """Test ExtendAttributeAction extends a list attribute.""" @@ -667,31 +731,50 @@ class TestActions(TestInspectionRules): log_action, task, msg='test message', level='invalid_level' ) - def test_action_with_loop(self): + def test_action_with_list_loop(self): """Test action execute_with_loop method.""" - action_data = { + list_loop_data = { 'op': 'set-attribute', + 'args': {'path': '{item[path]}', 'value': '{item[value]}'}, 'loop': [ - {'path': 'extra/test1', 'value': 'value1'}, - {'path': 'extra/test2', 'value': 'value2'} + {'path': 'driver_info/ipmi_username', 'value': 'cidadmin'}, + {'path': 'driver_info/ipmi_password', 'value': 'cidpassword'}, + { + 'path': 'driver_info/ipmi_address', + 'value': '{inventory[bmc_address]}' + } ] } - inventory = {'data': 'test'} - plugin_data = {'plugin': 'data'} + with task_manager.acquire(self.context, self.node.uuid) as task: + action = inspection_rules.actions.SetAttributeAction() + action.execute_with_loop(task, list_loop_data, self.inventory, + self.plugin_data) + self.assertEqual('cidadmin', + task.node.driver_info['ipmi_username']) + self.assertEqual('cidpassword', + task.node.driver_info['ipmi_password']) + self.assertEqual('192.168.1.100', + task.node.driver_info['ipmi_address']) + + def test_action_with_dict_loop(self): + """Test action execute_with_loop method.""" + dict_loop_data = { + 'op': 'set-attribute', + 'args': {'path': '{item[path]}', 'value': '{item[value]}'}, + 'loop': { + 'path': 'driver_info/ipmi_address', + 'value': '{inventory[bmc_address]}' + } + } with task_manager.acquire(self.context, self.node.uuid) as task: - task.node.extra = {} - - # execute_with_loop action = inspection_rules.actions.SetAttributeAction() - results = action.execute_with_loop(task, action_data, inventory, - plugin_data) + action.execute_with_loop(task, dict_loop_data, self.inventory, + self.plugin_data) - # verify both loop items were processed - self.assertEqual(2, len(results)) - self.assertEqual('value1', task.node.extra['test1']) - self.assertEqual('value2', task.node.extra['test2']) + self.assertEqual('192.168.1.100', + task.node.driver_info['ipmi_address']) class TestShallowMask(TestInspectionRules): @@ -806,3 +889,106 @@ class TestShallowMask(TestInspectionRules): self.assertEqual('new_value', original_data['new_key']) self.assertEqual([1, 2, 3, 4], original_data['data']) + + +class TestInterpolation(TestInspectionRules): + def setUp(self): + super(TestInterpolation, self).setUp() + + def test_variable_interpolation(self): + """Test variable interpolation.""" + loop_context = { + 'item': { + 'key': 'value', + 'nested': {'deep': 'nested_value'} + } + } + + with task_manager.acquire(self.context, self.node.uuid) as task: + value = "{inventory[cpu][architecture]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, self.plugin_data) + self.assertEqual("x86_64", result) + + value = "{inventory[interfaces][0][mac_address]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, self.plugin_data) + self.assertEqual("2a:03:9c:53:4e:46", result) + + value = "{plugin_data[plugin]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, self.plugin_data) + self.assertEqual("data", result) + + value = "{node.driver}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, self.plugin_data) + self.assertEqual("fake-hardware", result) + + value = "{item}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, + self.plugin_data, loop_context) + + self.assertEqual( + "{'key': 'value', 'nested': {'deep': 'nested_value'}}", + result) + + value = "{item[key]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, + self.plugin_data, loop_context) + self.assertEqual("value", result) + + value = "{item[nested][deep]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, + self.plugin_data, loop_context) + self.assertEqual("nested_value", result) + + value = "CPU: {inventory[cpu][count]}, Item: {item[key]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, + self.plugin_data, loop_context) + self.assertEqual("CPU: 4, Item: value", result) + + dict_value = { + "normal_key": "normal_value", + "interpolated_key": "{inventory[cpu][architecture]}", + "nested": { + "item_key": "{item[key]}", + "inventory_key": "{inventory[bmc_address]}" + } + } + result = base.Base.interpolate_variables( + dict_value, task.node, self.inventory, + self.plugin_data, loop_context) + self.assertEqual({ + "normal_key": "normal_value", + "interpolated_key": "x86_64", + "nested": { + "item_key": "value", + "inventory_key": "192.168.1.100" + } + }, result) + + list_value = [ + "normal_value", + "{inventory[cpu][architecture]}", + "{item[key]}", + ["{inventory[bmc_address]}", "{item[nested][deep]}"] + ] + result = base.Base.interpolate_variables( + list_value, task.node, self.inventory, + self.plugin_data, loop_context) + self.assertEqual([ + "normal_value", + "x86_64", + "value", + ["192.168.1.100", "nested_value"] + ], result) + + value = "{inventory[missing][key]}" + result = base.Base.interpolate_variables( + value, task.node, self.inventory, self.plugin_data) + self.assertEqual(value, result) diff --git a/releasenotes/notes/fix-loop-functionality-in-inspection-rules-9bf61e8355297804.yaml b/releasenotes/notes/fix-loop-functionality-in-inspection-rules-9bf61e8355297804.yaml new file mode 100644 index 0000000000..2c31da5f38 --- /dev/null +++ b/releasenotes/notes/fix-loop-functionality-in-inspection-rules-9bf61e8355297804.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes loop functionality to align more closely with the spec where, + with `loop` present, `args` reference loop items using '{item}' + placeholder to support direct array iteration; plus, + separately handle list and dict loop item types.