Merge "Fix mismatch in expected loop functionality"

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)
 
 

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()
         }

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:

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))

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)

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.