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

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.