From 6b127ab6859588d97833b9caea92661b2ae4591a Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 15 May 2015 19:44:16 -0700 Subject: [PATCH] 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 (cherry picked from commit 12889f70e1ae547598f4c663e9da5b9bb03e347e) --- neutron/agent/linux/iptables_firewall.py | 6 ++-- neutron/agent/linux/iptables_manager.py | 10 ++++++- .../agent/linux/test_iptables_firewall.py | 17 +++++------ .../unit/agent/linux/test_iptables_manager.py | 4 +-- .../unit/agent/test_securitygroups_rpc.py | 28 +++++++++---------- 5 files changed, 37 insertions(+), 28 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 613c49fd6..54f8b5dcb 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -295,11 +295,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 f7c0be4b8..d7362d5ba 100644 --- a/neutron/agent/linux/iptables_manager.py +++ b/neutron/agent/linux/iptables_manager.py @@ -61,7 +61,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 7de641359..d2e9743b4 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -118,8 +118,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', @@ -959,7 +960,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( @@ -1058,7 +1059,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( @@ -1130,7 +1131,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( @@ -1299,12 +1300,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( @@ -1378,7 +1379,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 ddd55b258..d6609760e 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_manager.py +++ b/neutron/tests/unit/agent/linux/test_iptables_manager.py @@ -102,8 +102,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 6a4e2990f..f5b6a126a 100644 --- a/neutron/tests/unit/agent/test_securitygroups_rpc.py +++ b/neutron/tests/unit/agent/test_securitygroups_rpc.py @@ -1700,7 +1700,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] @@ -1785,7 +1785,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 @@ -1835,7 +1835,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 @@ -1887,7 +1887,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 @@ -1944,7 +1944,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 @@ -1972,7 +1972,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 @@ -2028,7 +2028,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 @@ -2057,7 +2057,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 @@ -2111,7 +2111,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 @@ -2138,7 +2138,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 @@ -2201,7 +2201,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 @@ -2232,7 +2232,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 @@ -2287,7 +2287,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 @@ -2315,7 +2315,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 -- 2.45.2