From daa20e21fbe769fdba0242a3f1264d5970c11e35 Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@redhat.com>
Date: Tue, 9 Feb 2016 11:45:26 +0100
Subject: [PATCH] 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                  | 16 +++++-
 ironic_inspector/test/test_firewall.py        | 52 +++++++++++++++++++
 .../less-iptables-calls-759e89d103df504c.yaml |  3 ++
 3 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/less-iptables-calls-759e89d103df504c.yaml

diff --git a/ironic_inspector/firewall.py b/ironic_inspector/firewall.py
index 2f160a9fd..da51bb541 100644
--- a/ironic_inspector/firewall.py
+++ b/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
diff --git a/ironic_inspector/test/test_firewall.py b/ironic_inspector/test/test_firewall.py
index 6fba22100..2f7aaf6e9 100644
--- a/ironic_inspector/test/test_firewall.py
+++ b/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])
diff --git a/releasenotes/notes/less-iptables-calls-759e89d103df504c.yaml b/releasenotes/notes/less-iptables-calls-759e89d103df504c.yaml
new file mode 100644
index 000000000..3cda8aaea
--- /dev/null
+++ b/releasenotes/notes/less-iptables-calls-759e89d103df504c.yaml
@@ -0,0 +1,3 @@
+---
+fixes:
+  - Only issue iptables calls when list of active MAC's changes.