From e4fb858b3cdd04e5eaf00ed1f8597204094ead63 Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <divius.inside@gmail.com>
Date: Mon, 15 May 2017 16:59:28 +0200
Subject: [PATCH] Do not fail rules rollback on bad formatting key

The reason we're running rollback may be because some keys are not present.
We don't want to fail due to that, see bug 1686942 for an example.

Change-Id: Iac242df9987f1ce0c7a6db4f70440fa8f2aabc46
Closes-Bug: #1686942
(cherry picked from commit ce4fb101152db7c78ef5b1324192e069ba245cd1)
---
 ironic_inspector/rules.py                     | 26 ++++++++++++-------
 ironic_inspector/test/unit/test_rules.py      | 17 ++++++++++++
 .../rollback-formatting-7d61c9af2600d42f.yaml |  7 +++++
 3 files changed, 41 insertions(+), 9 deletions(-)
 create mode 100644 releasenotes/notes/rollback-formatting-7d61c9af2600d42f.yaml

diff --git a/ironic_inspector/rules.py b/ironic_inspector/rules.py
index 60b752a16..6ee97cc24 100644
--- a/ironic_inspector/rules.py
+++ b/ironic_inspector/rules.py
@@ -201,6 +201,7 @@ class IntrospectionRule(object):
         ext_mgr = plugins_base.rule_actions_manager()
         for act in self._actions:
             ext = ext_mgr[act.action].obj
+
             for formatted_param in ext.FORMATTED_PARAMS:
                 value = act.params.get(formatted_param)
                 if not value or not isinstance(value, six.string_types):
@@ -212,15 +213,22 @@ class IntrospectionRule(object):
                 try:
                     act.params[formatted_param] = value.format(data=data)
                 except KeyError as e:
-                    raise utils.Error(_('Invalid formatting variable key '
-                                        'provided: %s') % e,
-                                      node_info=node_info, data=data)
-
-            LOG.debug('Running %(what)s action `%(action)s %(params)s`',
-                      {'action': act.action, 'params': act.params,
-                       'what': method},
-                      node_info=node_info, data=data)
-            getattr(ext, method)(node_info, act.params)
+                    if rollback:
+                        LOG.warning('Invalid formatting variable key provided:'
+                                    ' %(key)s, skipping rollback for action '
+                                    '%(action)s',
+                                    {'key': e, 'action': act.action})
+                        break
+                    else:
+                        raise utils.Error(_('Invalid formatting variable key '
+                                            'provided: %s') % e,
+                                          node_info=node_info, data=data)
+            else:
+                LOG.debug('Running %(what)s action `%(action)s %(params)s`',
+                          {'action': act.action, 'params': act.params,
+                           'what': method},
+                          node_info=node_info, data=data)
+                getattr(ext, method)(node_info, act.params)
 
         LOG.debug('Successfully applied %s',
                   'rollback actions' if rollback else 'actions',
diff --git a/ironic_inspector/test/unit/test_rules.py b/ironic_inspector/test/unit/test_rules.py
index 4164a6c6d..80c2865a6 100644
--- a/ironic_inspector/test/unit/test_rules.py
+++ b/ironic_inspector/test/unit/test_rules.py
@@ -445,6 +445,23 @@ class TestApplyActions(BaseTest):
                          self.act_mock.rollback.call_count)
         self.assertFalse(self.act_mock.apply.called)
 
+    @mock.patch.object(rules.LOG, 'warning', autospec=True)
+    def test_rollback_ignores_formatting_failures(self, mock_log,
+                                                  mock_ext_mgr):
+        self.rule = rules.create(
+            actions_json=[
+                {'action': 'set-attribute',
+                 'path': '/driver_info/ipmi_address',
+                 'value': '{data[foo][bar]}'}],
+            conditions_json=self.conditions_json
+        )
+        mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
+
+        self.rule.apply_actions(self.node_info, rollback=True, data=self.data)
+
+        self.assertTrue(mock_log.called)
+        self.assertFalse(self.act_mock.rollback.called)
+
 
 @mock.patch.object(rules, 'get_all', autospec=True)
 class TestApply(BaseTest):
diff --git a/releasenotes/notes/rollback-formatting-7d61c9af2600d42f.yaml b/releasenotes/notes/rollback-formatting-7d61c9af2600d42f.yaml
new file mode 100644
index 000000000..23189527d
--- /dev/null
+++ b/releasenotes/notes/rollback-formatting-7d61c9af2600d42f.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Do not fail the whole introspection due to a value formatting error during
+    introspection rules rollback. See `bug 1686942
+    <https://bugs.launchpad.net/ironic-inspector/+bug/1686942>`_ for an example
+    and detailed investigation.