Predictable iptables chains output order

This fixes the iptables unit tests that break with a randomized PYTHONHASHSEED
(see the bug report).

The chains for iptables are stored as sets to avoid duplicates. When they are
output by iptables_manager their order can therefore be unpredictable. This was
found hash seed 1016732220.

To fix this we:
 - Sort the chains output by iptables_manager
 - Update the unit tests to check for sorted chains

When multiple tables are processed, they can be processed in any order or
dumped in any order. Found with hash seed 3728666619.

To fix this we:
 - Traverse the tables in sorted order for dumping
 - Fix tests to allow for tables to be processed in any order

Note: There are several other unrelated unit tests that also break with a
randomized PYTHONHASHSEED, but they are not addressed here. They will be
addressed in separate patches.

Partial-bug: #1348818

Change-Id: Ic3f4cd85316c9fc2e78bc7f5e900cfac87baf39d

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 2506d745fffeab49119426c0aa674fb563fe2903..6bb97711dbe3845cb2dc650bddf5ef3b0c2135c9 100644 (file)
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -392,7 +392,9 @@ class IptablesManager(object):
                 args = ['ip', 'netns', 'exec', self.namespace] + args
             all_tables = self.execute(args, root_helper=self.root_helper)
             all_lines = all_tables.split('\n')
-            for table_name, table in tables.iteritems():
+            # Traverse tables in sorted order for predictable dump output
+            for table_name in sorted(tables):
+                table = tables[table_name]
                 start, end = self._find_table(all_lines, table_name)
                 all_lines[start:end] = self._modify_rules(
                     all_lines[start:end], table, table_name)
@@ -463,8 +465,10 @@ class IptablesManager(object):
                 return s
 
     def _modify_rules(self, current_lines, table, table_name):
-        unwrapped_chains = table.unwrapped_chains
-        chains = table.chains
+        # Chains are stored as sets to avoid duplicates.
+        # Sort the output chains here to make their order predictable.
+        unwrapped_chains = sorted(table.unwrapped_chains)
+        chains = sorted(table.chains)
         remove_chains = table.remove_chains
         rules = table.rules
         remove_rules = table.remove_rules

diff --git a/neutron/tests/tools.py b/neutron/tests/tools.py
index f80b305089279457a18ad56c2c43f5fe08684f3f..2f80e42412d00369b542b840bf74ea5ed73a1e2a 100644 (file)
--- a/neutron/tests/tools.py
+++ b/neutron/tests/tools.py
@@ -42,6 +42,7 @@ def setup_mock_calls(mocked_call, expected_calls_and_values):
     mocked_call.side_effect = return_values
 
 
-def verify_mock_calls(mocked_call, expected_calls_and_values):
+def verify_mock_calls(mocked_call, expected_calls_and_values,
+                      any_order=False):
     expected_calls = [call[0] for call in expected_calls_and_values]
-    mocked_call.assert_has_calls(expected_calls)
+    mocked_call.assert_has_calls(expected_calls, any_order=any_order)

diff --git a/neutron/tests/unit/test_iptables_manager.py b/neutron/tests/unit/test_iptables_manager.py
index 74f0bb7ca587fa935abd5f15b5c2796967ee66e6..48173262f21bb1588988caf5cb50f23cd1fd8ee1 100644 (file)
--- a/neutron/tests/unit/test_iptables_manager.py
+++ b/neutron/tests/unit/test_iptables_manager.py
@@ -31,10 +31,10 @@ NAT_DUMP = ('# Generated by iptables_manager\n'
             '*nat\n'
             ':neutron-postrouting-bottom - [0:0]\n'
             ':%(bn)s-OUTPUT - [0:0]\n'
-            ':%(bn)s-snat - [0:0]\n'
+            ':%(bn)s-POSTROUTING - [0:0]\n'
             ':%(bn)s-PREROUTING - [0:0]\n'
             ':%(bn)s-float-snat - [0:0]\n'
-            ':%(bn)s-POSTROUTING - [0:0]\n'
+            ':%(bn)s-snat - [0:0]\n'
             '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
             '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
             '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n'
@@ -50,8 +50,8 @@ FILTER_DUMP = ('# Generated by iptables_manager\n'
                ':neutron-filter-top - [0:0]\n'
                ':%(bn)s-FORWARD - [0:0]\n'
                ':%(bn)s-INPUT - [0:0]\n'
-               ':%(bn)s-local - [0:0]\n'
                ':%(bn)s-OUTPUT - [0:0]\n'
+               ':%(bn)s-local - [0:0]\n'
                '[0:0] -A FORWARD -j neutron-filter-top\n'
                '[0:0] -A OUTPUT -j neutron-filter-top\n'
                '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -120,9 +120,9 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                        ':neutron-filter-top - [0:0]\n'
                        ':%(bn)s-FORWARD - [0:0]\n'
                        ':%(bn)s-INPUT - [0:0]\n'
-                       ':%(bn)s-local - [0:0]\n'
-                       ':%(bn)s-filter - [0:0]\n'
                        ':%(bn)s-OUTPUT - [0:0]\n'
+                       ':%(bn)s-filter - [0:0]\n'
+                       ':%(bn)s-local - [0:0]\n'
                        '[0:0] -A FORWARD -j neutron-filter-top\n'
                        '[0:0] -A OUTPUT -j neutron-filter-top\n'
                        '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -137,8 +137,8 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                             ':neutron-filter-top - [0:0]\n'
                             ':%(bn)s-FORWARD - [0:0]\n'
                             ':%(bn)s-INPUT - [0:0]\n'
-                            ':%(bn)s-local - [0:0]\n'
                             ':%(bn)s-OUTPUT - [0:0]\n'
+                            ':%(bn)s-local - [0:0]\n'
                             '[0:0] -A FORWARD -j neutron-filter-top\n'
                             '[0:0] -A OUTPUT -j neutron-filter-top\n'
                             '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -154,9 +154,9 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                            ':neutron-filter-top - [0:0]\n'
                            ':%(bn)s-FORWARD - [0:0]\n'
                            ':%(bn)s-INPUT - [0:0]\n'
-                           ':%(bn)s-local - [0:0]\n'
-                           ':%(bn)s-filter - [0:0]\n'
                            ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-filter - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
                            '[0:0] -A FORWARD -j neutron-filter-top\n'
                            '[0:0] -A OUTPUT -j neutron-filter-top\n'
                            '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -171,10 +171,10 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                     '*nat\n'
                     ':neutron-postrouting-bottom - [0:0]\n'
                     ':%(bn)s-OUTPUT - [0:0]\n'
-                    ':%(bn)s-snat - [0:0]\n'
+                    ':%(bn)s-POSTROUTING - [0:0]\n'
                     ':%(bn)s-PREROUTING - [0:0]\n'
                     ':%(bn)s-float-snat - [0:0]\n'
-                    ':%(bn)s-POSTROUTING - [0:0]\n'
+                    ':%(bn)s-snat - [0:0]\n'
                     '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
                     '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
                     '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n'
@@ -237,8 +237,8 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                        ':neutron-filter-top - [0:0]\n'
                        ':%(bn)s-FORWARD - [0:0]\n'
                        ':%(bn)s-INPUT - [0:0]\n'
-                       ':%(bn)s-local - [0:0]\n'
                        ':%(bn)s-OUTPUT - [0:0]\n'
+                       ':%(bn)s-local - [0:0]\n'
                        '[0:0] -A FORWARD -j neutron-filter-top\n'
                        '[0:0] -A OUTPUT -j neutron-filter-top\n'
                        '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -253,9 +253,9 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                            ':neutron-filter-top - [0:0]\n'
                            ':%(bn)s-FORWARD - [0:0]\n'
                            ':%(bn)s-INPUT - [0:0]\n'
-                           ':%(bn)s-local - [0:0]\n'
-                           ':%(bn)s-filter - [0:0]\n'
                            ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-filter - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
                            '[0:0] -A FORWARD -j neutron-filter-top\n'
                            '[0:0] -A OUTPUT -j neutron-filter-top\n'
                            '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -271,10 +271,10 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                     '*nat\n'
                     ':neutron-postrouting-bottom - [0:0]\n'
                     ':%(bn)s-OUTPUT - [0:0]\n'
-                    ':%(bn)s-snat - [0:0]\n'
+                    ':%(bn)s-POSTROUTING - [0:0]\n'
                     ':%(bn)s-PREROUTING - [0:0]\n'
                     ':%(bn)s-float-snat - [0:0]\n'
-                    ':%(bn)s-POSTROUTING - [0:0]\n'
+                    ':%(bn)s-snat - [0:0]\n'
                     '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
                     '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
                     '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n'
@@ -334,9 +334,9 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                            ':neutron-filter-top - [0:0]\n'
                            ':%(bn)s-FORWARD - [0:0]\n'
                            ':%(bn)s-INPUT - [0:0]\n'
-                           ':%(bn)s-local - [0:0]\n'
-                           ':%(bn)s-filter - [0:0]\n'
                            ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-filter - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
                            '[0:0] -A FORWARD -j neutron-filter-top\n'
                            '[0:0] -A OUTPUT -j neutron-filter-top\n'
                            '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -394,9 +394,9 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
                            ':neutron-filter-top - [0:0]\n'
                            ':%(bn)s-FORWARD - [0:0]\n'
                            ':%(bn)s-INPUT - [0:0]\n'
-                           ':%(bn)s-local - [0:0]\n'
-                           ':%(bn)s-filter - [0:0]\n'
                            ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-filter - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
                            '[0:0] -A FORWARD -j neutron-filter-top\n'
                            '[0:0] -A OUTPUT -j neutron-filter-top\n'
                            '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -473,11 +473,11 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         filter_dump_mod = ('# Generated by iptables_manager\n'
                            '*filter\n'
                            ':neutron-filter-top - [0:0]\n'
+                           ':%(wrap)s - [0:0]\n'
                            ':%(bn)s-FORWARD - [0:0]\n'
                            ':%(bn)s-INPUT - [0:0]\n'
-                           ':%(bn)s-local - [0:0]\n'
-                           ':%(wrap)s - [0:0]\n'
                            ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
                            '[0:0] -A FORWARD -j neutron-filter-top\n'
                            '[0:0] -A OUTPUT -j neutron-filter-top\n'
                            '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
@@ -542,10 +542,10 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         nat_dump = ('# Generated by iptables_manager\n'
                     '*nat\n'
                     ':neutron-postrouting-bottom - [0:0]\n'
-                    ':%(bn)s-float-snat - [0:0]\n'
+                    ':%(bn)s-OUTPUT - [0:0]\n'
                     ':%(bn)s-POSTROUTING - [0:0]\n'
                     ':%(bn)s-PREROUTING - [0:0]\n'
-                    ':%(bn)s-OUTPUT - [0:0]\n'
+                    ':%(bn)s-float-snat - [0:0]\n'
                     ':%(bn)s-snat - [0:0]\n'
                     '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
                     '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
@@ -560,11 +560,11 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         nat_dump_mod = ('# Generated by iptables_manager\n'
                         '*nat\n'
                         ':neutron-postrouting-bottom - [0:0]\n'
-                        ':%(bn)s-float-snat - [0:0]\n'
+                        ':%(bn)s-OUTPUT - [0:0]\n'
                         ':%(bn)s-POSTROUTING - [0:0]\n'
                         ':%(bn)s-PREROUTING - [0:0]\n'
+                        ':%(bn)s-float-snat - [0:0]\n'
                         ':%(bn)s-nat - [0:0]\n'
-                        ':%(bn)s-OUTPUT - [0:0]\n'
                         ':%(bn)s-snat - [0:0]\n'
                         '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
                         '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
@@ -764,7 +764,8 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         self.assertEqual(acc['pkts'], exp_packets)
         self.assertEqual(acc['bytes'], exp_bytes)
 
-        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+        tools.verify_mock_calls(self.execute, expected_calls_and_values,
+                                any_order=True)
 
     def test_get_traffic_counters(self):
         self._test_get_traffic_counters_helper(False)
@@ -814,7 +815,8 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         self.assertEqual(acc['pkts'], exp_packets)
         self.assertEqual(acc['bytes'], exp_bytes)
 
-        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+        tools.verify_mock_calls(self.execute, expected_calls_and_values,
+                                any_order=True)
 
     def test_get_traffic_counters_with_zero(self):
         self._test_get_traffic_counters_with_zero_helper(False)