Do not update firewall rules if list of MAC's did not change

Currently we issue a lot of iptables requests in vain. Change firewall
handling logic to cache last known set of MAC's to black list.
If the current list matches the previous one, do not update rules.

Change-Id: I135d7f3b18dd6ea17058fe23a7fa7e58a45c2270
Closes-Bug: #1543516

ironic_inspector/firewall.py

@@ -30,6 +30,7 @@ CHAIN = None
 INTERFACE = None
 LOCK = semaphore.BoundedSemaphore()
 BASE_COMMAND = None
+BLACKLIST_CACHE = None
 
 
 def _iptables(*args, **kwargs):
@@ -58,7 +59,8 @@ def init():
     if not CONF.firewall.manage_firewall:
         return
 
-    global INTERFACE, CHAIN, NEW_CHAIN, BASE_COMMAND
+    global INTERFACE, CHAIN, NEW_CHAIN, BASE_COMMAND, BLACKLIST_CACHE
+    BLACKLIST_CACHE = None
     INTERFACE = CONF.firewall.dnsmasq_interface
     CHAIN = CONF.firewall.firewall_chain
     NEW_CHAIN = CHAIN + '_temp'
@@ -117,6 +119,8 @@ def update_filters(ironic=None):
 
     :param ironic: Ironic client instance, optional.
     """
+    global BLACKLIST_CACHE
+
     if not CONF.firewall.manage_firewall:
         return
 
@@ -126,7 +130,14 @@ def update_filters(ironic=None):
     with LOCK:
         macs_active = set(p.address for p in ironic.port.list(limit=0))
         to_blacklist = macs_active - node_cache.active_macs()
+        if BLACKLIST_CACHE is not None and to_blacklist == BLACKLIST_CACHE:
+            LOG.debug('Not updating iptables - no changes in MAC list %s',
+                      to_blacklist)
+            return
+
         LOG.debug('Blacklisting active MAC\'s %s', to_blacklist)
+        # Force update on the next iteration if this attempt fails
+        BLACKLIST_CACHE = None
 
         # Clean up a bit to account for possible troubles on previous run
         _clean_up(NEW_CHAIN)
@@ -148,3 +159,6 @@ def update_filters(ironic=None):
         _iptables('-F', CHAIN, ignore=True)
         _iptables('-X', CHAIN, ignore=True)
         _iptables('-E', NEW_CHAIN, CHAIN)
+
+        # Cache result of successful iptables update
+        BLACKLIST_CACHE = to_blacklist

ironic_inspector/test/test_firewall.py

@@ -192,3 +192,55 @@ class TestFirewall(test_base.NodeTest):
         for (args, call) in zip(update_filters_expected_args,
                                 call_args_list):
             self.assertEqual(args, call[0])
+
+        # check caching
+
+        mock_iptables.reset_mock()
+        firewall.update_filters(mock_get_client)
+        self.assertFalse(mock_iptables.called)
+
+    def test_update_filters_clean_cache_on_error(self, mock_call,
+                                                 mock_get_client,
+                                                 mock_iptables):
+        active_macs = ['11:22:33:44:55:66', '66:55:44:33:22:11']
+        inactive_mac = ['AA:BB:CC:DD:EE:FF']
+        self.macs = active_macs + inactive_mac
+        self.ports = [mock.Mock(address=m) for m in self.macs]
+        mock_get_client.port.list.return_value = self.ports
+        node_cache.add_node(self.node.uuid, mac=active_macs,
+                            bmc_address='1.2.3.4', foo=None)
+        firewall.init()
+
+        update_filters_expected_args = [
+            ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport',
+             '67', '-j', firewall.NEW_CHAIN),
+            ('-F', firewall.NEW_CHAIN),
+            ('-X', firewall.NEW_CHAIN),
+            ('-N', firewall.NEW_CHAIN),
+            # Blacklist
+            ('-A', firewall.NEW_CHAIN, '-m', 'mac', '--mac-source',
+             inactive_mac[0], '-j', 'DROP'),
+            ('-A', firewall.NEW_CHAIN, '-j', 'ACCEPT'),
+            ('-I', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport',
+             '67', '-j', firewall.NEW_CHAIN),
+            ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport',
+             '67', '-j', CONF.firewall.firewall_chain),
+            ('-F', CONF.firewall.firewall_chain),
+            ('-X', CONF.firewall.firewall_chain),
+            ('-E', firewall.NEW_CHAIN, CONF.firewall.firewall_chain)
+        ]
+
+        mock_iptables.side_effect = [None, None, RuntimeError()]
+        self.assertRaises(RuntimeError, firewall.update_filters,
+                          mock_get_client)
+
+        # check caching
+
+        mock_iptables.reset_mock()
+        mock_iptables.side_effect = None
+        firewall.update_filters(mock_get_client)
+        call_args_list = mock_iptables.call_args_list
+
+        for (args, call) in zip(update_filters_expected_args,
+                                call_args_list):
+            self.assertEqual(args, call[0])

releasenotes/notes/less-iptables-calls-759e89d103df504c.yaml

@@ -0,0 +1,3 @@
+---
+fixes:
+  - Only issue iptables calls when list of active MAC's changes.