Match order of iptables arguments to iptables-save

The way we were forming our iptables rules was not matching
the output of iptables-save. This caused the logic that preserves
counters to miss many of the rules.

This patch corrects the order for the comments and the allowed address
pairs to match the output order of iptables-save.

Closes-Bug: #1456823
Change-Id: I34c2249d0865485578767865c82414e1d813d563

diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py
index 323d3501909c4a8e0200ffadddaa5e7e7fe358af..dc1d8901baeffc7ba815ca543a09e29100199ebb 100644 (file)
--- a/neutron/agent/linux/iptables_firewall.py
+++ b/neutron/agent/linux/iptables_firewall.py
@@ -303,11 +303,11 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
                     # of the list after the allowed_address_pair rules.
                     table.add_rule(chain_name,
                                    '-m mac --mac-source %s -j RETURN'
-                                   % mac, comment=ic.PAIR_ALLOW)
+                                   % mac.upper(), comment=ic.PAIR_ALLOW)
                 else:
                     table.add_rule(chain_name,
-                                   '-m mac --mac-source %s -s %s -j RETURN'
-                                   % (mac, ip), comment=ic.PAIR_ALLOW)
+                                   '-s %s -m mac --mac-source %s -j RETURN'
+                                   % (ip, mac.upper()), comment=ic.PAIR_ALLOW)
             table.add_rule(chain_name, '-j DROP', comment=ic.PAIR_DROP)
             rules.append('-j $%s' % chain_name)
 

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index f63098681f6fe7e4fb27a8df78a17affb6406228..63efcb3a1f7ecf84d023754a381948de37b30fac 100644 (file)
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -63,7 +63,15 @@ IPTABLES_ERROR_LINES_OF_CONTEXT = 5
 def comment_rule(rule, comment):
     if not cfg.CONF.AGENT.comment_iptables_rules or not comment:
         return rule
-    return '%s -m comment --comment "%s"' % (rule, comment)
+    # iptables-save outputs the comment before the jump so we need to match
+    # that order so _find_last_entry works
+    try:
+        start_of_jump = rule.index(' -j ')
+    except ValueError:
+        return '%s -m comment --comment "%s"' % (rule, comment)
+    return ' '.join([rule[0:start_of_jump],
+                     '-m comment --comment "%s"' % comment,
+                     rule[start_of_jump + 1:]])
 
 
 def get_chain_name(chain_name, wrap=True):

diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py
index 4fa622ccdbb34eb9a81d50bde952b63e953cf03b..77d98e8b185fd7ead5b1af2111d2fda879031933 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py
+++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py
@@ -122,8 +122,9 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                                     comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
-                     'sfake_dev', '-m mac --mac-source ff:ff:ff:ff:ff:ff '
-                     '-s 10.0.0.1 -j RETURN',
+                     'sfake_dev',
+                     '-s 10.0.0.1 -m mac --mac-source FF:FF:FF:FF:FF:FF '
+                     '-j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
                      'sfake_dev', '-j DROP',
@@ -967,7 +968,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                   mock.call.add_chain('sfake_dev'),
                   mock.call.add_rule(
                       'sfake_dev',
-                      '-m mac --mac-source ff:ff:ff:ff:ff:ff -s %s -j RETURN'
+                      '-s %s -m mac --mac-source FF:FF:FF:FF:FF:FF -j RETURN'
                       % prefix,
                       comment=ic.PAIR_ALLOW),
                   mock.call.add_rule(
@@ -1066,7 +1067,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
+                     '-s 10.0.0.1 -m mac --mac-source FF:FF:FF:FF:FF:FF '
                      '-j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
@@ -1138,7 +1139,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
+                     '-s 10.0.0.1 -m mac --mac-source FF:FF:FF:FF:FF:FF '
                      '-j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
@@ -1308,12 +1309,12 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
+                     '-s 10.0.0.1 -m mac --mac-source FF:FF:FF:FF:FF:FF '
                      '-j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.2 '
+                     '-s 10.0.0.2 -m mac --mac-source FF:FF:FF:FF:FF:FF '
                      '-j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
@@ -1388,7 +1389,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -j RETURN',
+                     '-m mac --mac-source FF:FF:FF:FF:FF:FF -j RETURN',
                      comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
                      'sfake_dev', '-j DROP',

diff --git a/neutron/tests/unit/agent/linux/test_iptables_manager.py b/neutron/tests/unit/agent/linux/test_iptables_manager.py
index 5b8dcb1df2a618ea7e3aae459b377808ab0d6269..8637c35801a408b45315283a8a7851c9ae0d0aa8 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_iptables_manager.py
+++ b/neutron/tests/unit/agent/linux/test_iptables_manager.py
@@ -101,8 +101,8 @@ COMMENTED_NAT_DUMP = (
     '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
     '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n'
     '[0:0] -A POSTROUTING -j neutron-postrouting-bottom\n'
-    '[0:0] -A neutron-postrouting-bottom -j %(bn)s-snat '
-    '-m comment --comment "%(snat_out_comment)s"\n'
+    '[0:0] -A neutron-postrouting-bottom '
+    '-m comment --comment "%(snat_out_comment)s" -j %(bn)s-snat\n'
     '[0:0] -A %(bn)s-snat -j '
     '%(bn)s-float-snat\n'
     'COMMIT\n'

diff --git a/neutron/tests/unit/agent/test_securitygroups_rpc.py b/neutron/tests/unit/agent/test_securitygroups_rpc.py
index 31dd3993e89ce6d7060b5b4c04e18a65dece9aa5..d11c3158f458d1063c5154d3019cc2bd781de66d 100644 (file)
--- a/neutron/tests/unit/agent/test_securitygroups_rpc.py
+++ b/neutron/tests/unit/agent/test_securitygroups_rpc.py
@@ -1631,7 +1631,7 @@ CHAINS_NAT = 'OUTPUT|POSTROUTING|PREROUTING|float-snat|snat'
 # TestSecurityGroupAgentWithIptables() to ensure that the ordering
 # is consistent regardless of hashseed value
 PORTS = {'tap_port1': 'port1', 'tap_port2': 'port2'}
-MACS = {'tap_port1': '12:34:56:78:9a:bc', 'tap_port2': '12:34:56:78:9a:bd'}
+MACS = {'tap_port1': '12:34:56:78:9A:BC', 'tap_port2': '12:34:56:78:9A:BD'}
 IPS = {'tap_port1': '10.0.0.3/32', 'tap_port2': '10.0.0.4/32'}
 
 IPTABLES_ARG['port1'] = PORTS.values()[0]
@@ -1752,7 +1752,7 @@ RETURN
 %(physdev_is_bridged)s -j %(bn)s-o_port1
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_port1 \
 %(physdev_is_bridged)s -j %(bn)s-o_port1
-[0:0] -A %(bn)s-s_port1 -m mac --mac-source 12:34:56:78:9a:bc -s 10.0.0.3/32 \
+[0:0] -A %(bn)s-s_port1 -s 10.0.0.3/32 -m mac --mac-source 12:34:56:78:9A:BC \
 -j RETURN
 [0:0] -A %(bn)s-s_port1 -j DROP
 [0:0] -A %(bn)s-o_port1 -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -1802,7 +1802,7 @@ IPTABLES_FILTER_1 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_port1
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_port1 \
 %(physdev_is_bridged)s -j %(bn)s-o_port1
-[0:0] -A %(bn)s-s_port1 -m mac --mac-source 12:34:56:78:9a:bc -s 10.0.0.3/32 \
+[0:0] -A %(bn)s-s_port1 -s 10.0.0.3/32 -m mac --mac-source 12:34:56:78:9A:BC \
 -j RETURN
 [0:0] -A %(bn)s-s_port1 -j DROP
 [0:0] -A %(bn)s-o_port1 -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -1854,7 +1854,7 @@ IPTABLES_FILTER_1_2 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_port1
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_port1 \
 %(physdev_is_bridged)s -j %(bn)s-o_port1
-[0:0] -A %(bn)s-s_port1 -m mac --mac-source 12:34:56:78:9a:bc -s 10.0.0.3/32 \
+[0:0] -A %(bn)s-s_port1 -s 10.0.0.3/32 -m mac --mac-source 12:34:56:78:9A:BC \
 -j RETURN
 [0:0] -A %(bn)s-s_port1 -j DROP
 [0:0] -A %(bn)s-o_port1 -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -1911,7 +1911,7 @@ RETURN
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port1)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
-[0:0] -A %(bn)s-s_%(port1)s -m mac --mac-source %(mac1)s -s %(ip1)s \
+[0:0] -A %(bn)s-s_%(port1)s -s %(ip1)s -m mac --mac-source %(mac1)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port1)s -j DROP
 [0:0] -A %(bn)s-o_%(port1)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -1939,7 +1939,7 @@ RETURN
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port2)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
-[0:0] -A %(bn)s-s_%(port2)s -m mac --mac-source %(mac2)s -s %(ip2)s \
+[0:0] -A %(bn)s-s_%(port2)s -s %(ip2)s -m mac --mac-source %(mac2)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port2)s -j DROP
 [0:0] -A %(bn)s-o_%(port2)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -1995,7 +1995,7 @@ RETURN
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port1)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
-[0:0] -A %(bn)s-s_%(port1)s -m mac --mac-source %(mac1)s -s %(ip1)s \
+[0:0] -A %(bn)s-s_%(port1)s -s %(ip1)s -m mac --mac-source %(mac1)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port1)s -j DROP
 [0:0] -A %(bn)s-o_%(port1)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2024,7 +2024,7 @@ RETURN
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port2)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
-[0:0] -A %(bn)s-s_%(port2)s -m mac --mac-source %(mac2)s -s %(ip2)s \
+[0:0] -A %(bn)s-s_%(port2)s -s %(ip2)s -m mac --mac-source %(mac2)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port2)s -j DROP
 [0:0] -A %(bn)s-o_%(port2)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2078,7 +2078,7 @@ IPTABLES_FILTER_2 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port1)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
-[0:0] -A %(bn)s-s_%(port1)s -m mac --mac-source %(mac1)s -s %(ip1)s \
+[0:0] -A %(bn)s-s_%(port1)s -s %(ip1)s -m mac --mac-source %(mac1)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port1)s -j DROP
 [0:0] -A %(bn)s-o_%(port1)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2105,7 +2105,7 @@ IPTABLES_FILTER_2 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port2)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
-[0:0] -A %(bn)s-s_%(port2)s -m mac --mac-source %(mac2)s -s %(ip2)s \
+[0:0] -A %(bn)s-s_%(port2)s -s %(ip2)s -m mac --mac-source %(mac2)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port2)s -j DROP
 [0:0] -A %(bn)s-o_%(port2)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2168,7 +2168,7 @@ IPTABLES_FILTER_2_2 += """[0:0] -A %(bn)s-i_%(port1)s -j %(bn)s-sg-fallback
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port1)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
-[0:0] -A %(bn)s-s_%(port1)s -m mac --mac-source %(mac1)s -s %(ip1)s \
+[0:0] -A %(bn)s-s_%(port1)s -s %(ip1)s -m mac --mac-source %(mac1)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port1)s -j DROP
 [0:0] -A %(bn)s-o_%(port1)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2199,7 +2199,7 @@ IPTABLES_FILTER_2_2 += """[0:0] -A %(bn)s-i_%(port2)s -j %(bn)s-sg-fallback
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port2)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
-[0:0] -A %(bn)s-s_%(port2)s -m mac --mac-source %(mac2)s -s %(ip2)s \
+[0:0] -A %(bn)s-s_%(port2)s -s %(ip2)s -m mac --mac-source %(mac2)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port2)s -j DROP
 [0:0] -A %(bn)s-o_%(port2)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2254,7 +2254,7 @@ IPTABLES_FILTER_2_3 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port1)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port1)s
-[0:0] -A %(bn)s-s_%(port1)s -m mac --mac-source %(mac1)s -s %(ip1)s \
+[0:0] -A %(bn)s-s_%(port1)s -s %(ip1)s -m mac --mac-source %(mac1)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port1)s -j DROP
 [0:0] -A %(bn)s-o_%(port1)s -p udp -m udp --sport 68 --dport 67 -j RETURN
@@ -2282,7 +2282,7 @@ IPTABLES_FILTER_2_3 = """# Generated by iptables_manager
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
 [0:0] -A %(bn)s-INPUT %(physdev_mod)s --physdev-EGRESS tap_%(port2)s \
 %(physdev_is_bridged)s -j %(bn)s-o_%(port2)s
-[0:0] -A %(bn)s-s_%(port2)s -m mac --mac-source %(mac2)s -s %(ip2)s \
+[0:0] -A %(bn)s-s_%(port2)s -s %(ip2)s -m mac --mac-source %(mac2)s \
 -j RETURN
 [0:0] -A %(bn)s-s_%(port2)s -j DROP
 [0:0] -A %(bn)s-o_%(port2)s -p udp -m udp --sport 68 --dport 67 -j RETURN