Switch to dictionary for iptables find

The code to find the matching entry was scanning through a
list of all rules for every rule. This became extremely slow
as the number of rules became large, leading to long delays
waiting for firewall rules to be applied.

This patch switches to the use of a dictionary so the cost
becomes a hash lookup instead of a list scan.

Closes-Bug: #1453264
Closes-Bug: #1455675
Change-Id: I1e6fe5e50b9c13066c966c252cadc8ed1d08f686

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 63efcb3a1f7ecf84d023754a381948de37b30fac..38e40f0c7357046c3189dc04e9d97c8b80f97ec2 100644 (file)
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -18,6 +18,7 @@
 
 """Implements iptables rules using linux utilities."""
 
+import collections
 import contextlib
 import os
 import re
@@ -511,11 +512,12 @@ class IptablesManager(object):
 
         return rules_index
 
-    def _find_last_entry(self, filter_list, match_str):
-        # find a matching entry, starting from the bottom
-        for s in reversed(filter_list):
-            if match_str in s:
-                return s.strip()
+    def _find_last_entry(self, filter_map, match_str):
+        # find last matching entry
+        try:
+            return filter_map[match_str][-1]
+        except KeyError:
+            pass
 
     def _modify_rules(self, current_lines, table, table_name):
         # Chains are stored as sets to avoid duplicates.
@@ -540,6 +542,9 @@ class IptablesManager(object):
             (old_filter if self.wrap_name in line else
              new_filter).append(line.strip())
 
+        old_filter_map = make_filter_map(old_filter)
+        new_filter_map = make_filter_map(new_filter)
+
         rules_index = self._find_rules_index(new_filter)
 
         all_chains = [':%s' % name for name in unwrapped_chains]
@@ -551,9 +556,9 @@ class IptablesManager(object):
         for chain in all_chains:
             chain_str = str(chain).strip()
 
-            old = self._find_last_entry(old_filter, chain_str)
+            old = self._find_last_entry(old_filter_map, chain_str)
             if not old:
-                dup = self._find_last_entry(new_filter, chain_str)
+                dup = self._find_last_entry(new_filter_map, chain_str)
             new_filter = [s for s in new_filter if chain_str not in s.strip()]
 
             # if no old or duplicates, use original chain
@@ -574,9 +579,9 @@ class IptablesManager(object):
             # Further down, we weed out duplicates from the bottom of the
             # list, so here we remove the dupes ahead of time.
 
-            old = self._find_last_entry(old_filter, rule_str)
+            old = self._find_last_entry(old_filter_map, rule_str)
             if not old:
-                dup = self._find_last_entry(new_filter, rule_str)
+                dup = self._find_last_entry(new_filter_map, rule_str)
             new_filter = [s for s in new_filter if rule_str not in s.strip()]
 
             # if no old or duplicates, use original rule
@@ -722,3 +727,29 @@ class IptablesManager(object):
                 acc['bytes'] += int(data[1])
 
         return acc
+
+
+def make_filter_map(filter_list):
+    filter_map = collections.defaultdict(list)
+    for data in filter_list:
+        # strip any [packet:byte] counts at start or end of lines,
+        # for example, chains look like ":neutron-foo - [0:0]"
+        # and rules look like "[0:0] -A neutron-foo..."
+        if data.startswith('['):
+            key = data.rpartition('] ')[2]
+        elif data.endswith(']'):
+            key = data.rsplit(' [', 1)[0]
+            if key.endswith(' -'):
+                key = key[:-2]
+        else:
+            # things like COMMIT, *filter, and *nat land here
+            continue
+        filter_map[key].append(data)
+        # regular IP(v6) entries are translated into /32s or /128s so we
+        # include a lookup without the CIDR here to match as well
+        for cidr in ('/32', '/128'):
+            if cidr in key:
+                alt_key = key.replace(cidr, '')
+                filter_map[alt_key].append(data)
+    # return a regular dict so readers don't accidentally add entries
+    return dict(filter_map)

diff --git a/neutron/tests/unit/agent/linux/test_iptables_manager.py b/neutron/tests/unit/agent/linux/test_iptables_manager.py
index 8637c35801a408b45315283a8a7851c9ae0d0aa8..674b1a872f78ce397641ae21d16f200da56a9de4 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_iptables_manager.py
+++ b/neutron/tests/unit/agent/linux/test_iptables_manager.py
@@ -1007,11 +1007,11 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                        '[0:0] -A FORWARD -j neutron-filter-top',
                        '[0:0] -A OUTPUT -j neutron-filter-top'
                        % IPTABLES_ARG]
-
-        return self.iptables._find_last_entry(filter_list, find_str)
+        filter_map = iptables_manager.make_filter_map(filter_list)
+        return self.iptables._find_last_entry(filter_map, find_str)
 
     def test_find_last_entry_old_dup(self):
-        find_str = 'neutron-filter-top'
+        find_str = '-A OUTPUT -j neutron-filter-top'
         match_str = '[0:0] -A OUTPUT -j neutron-filter-top'
         ret_str = self._test_find_last_entry(find_str)
         self.assertEqual(ret_str, match_str)
@@ -1021,6 +1021,19 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         ret_str = self._test_find_last_entry(find_str)
         self.assertIsNone(ret_str)
 
+    def test_make_filter_map_cidr_stripping(self):
+        filter_rules = ('[0:0] -A OUTPUT -j DROP',
+                        '[0:0] -A INPUT -d 192.168.0.2/32 -j DROP',
+                        '[0:0] -A INPUT -d 1234:31::001F/128 -j DROP',
+                        'OUTPUT - [0:0]')
+        filter_map = iptables_manager.make_filter_map(filter_rules)
+        # make sure /128 works without CIDR
+        self.assertEqual(filter_rules[2],
+                         filter_map['-A INPUT -d 1234:31::001F -j DROP'][0])
+        # make sure /32 works without CIDR
+        self.assertEqual(filter_rules[1],
+                         filter_map['-A INPUT -d 192.168.0.2 -j DROP'][0])
+
 
 class IptablesManagerStateLessTestCase(base.BaseTestCase):