From: Kevin Benton <blak111@gmail.com>
Date: Sat, 16 May 2015 02:44:16 +0000 (-0700)
Subject: Match order of iptables arguments to iptables-save
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=12889f70e1ae547598f4c663e9da5b9bb03e347e;p=openstack-build%2Fneutron-build.git

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 323d35019..dc1d8901b 100644
--- 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 f63098681..63efcb3a1 100644
--- 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 4fa622ccd..77d98e8b1 100644
--- 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 5b8dcb1df..8637c3580 100644
--- 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 31dd3993e..d11c3158f 100644
--- 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