From 4a90dcd85f65fde415829fd5dac9bf4b645a2121 Mon Sep 17 00:00:00 2001 From: Henry Gessau Date: Sat, 26 Jul 2014 22:05:02 -0400 Subject: [PATCH] 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 --- neutron/agent/linux/iptables_manager.py | 10 ++-- neutron/tests/tools.py | 5 +- neutron/tests/unit/test_iptables_manager.py | 56 +++++++++++---------- 3 files changed, 39 insertions(+), 32 deletions(-) diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py index 2506d745ff..6bb97711db 100644 --- 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 f80b305089..2f80e42412 100644 --- 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 74f0bb7ca5..48173262f2 100644 --- 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)