From 5154d974fdce4625710d3b4f360d45568678eb2f Mon Sep 17 00:00:00 2001 From: Itsuro Oda Date: Wed, 25 Feb 2015 13:34:04 +0900 Subject: [PATCH] Make floatingip reachable from the same network The problem is that if one tries to communicate from a tenant network to floatingip which attached to a port on the same network, the communication fails. This problem is a regression cased by [1]. [1] https://review.openstack.org/131905/ Before [1] SNAT rule is as follows: -s %(internal_cidr)s -j SNAT --to-source ... (for each internal interface) After [1] SNAT rule is as follows: -o %(interface_name)s -j SNAT --to-source ... (for an external interface) The new rule was considered a super-set of the packets going out to the external interface compared to the old rules. This is true but there is a lack of consideration. Note that the packet is 'going out to external interface' OR 'DNATed' at this point since the rule: ! -o %(interdace_name)s -m conntrack ! --ctstate DNAT -j ACCEPT was applied already. So we should consider the following three cases. 1) going out to external interface should be SNATed. It is OK under the new rule but there was a lack of rules for packets from indirectly connected to the router under the old rules. ([1] fixed this.) 2) DNATed (and going out to internal interface) 2-1) came in from internal interface should be SNATed because the return traffic needs to go through the router to complete the conntrack association and to reverse the effect of DNAT on the return packets. If a packet is not SNATed, the return packet may be sent directly to the private IP of the initiator. The old rules done SNAT in this case but the new rule doesn't. 2-2) came in from external interface nothing to do. This patch adds a rule for the case 2-1). This patch also adds mangle rules to examine whether a packet came from external interface. Change-Id: Ifa695ac5428fb0edba60129a4d61ec0e127a5818 Closes-Bug: #1428887 --- etc/l3_agent.ini | 3 ++ neutron/agent/l3/config.py | 6 ++- neutron/agent/l3/router_info.py | 18 ++++++- neutron/agent/linux/iptables_manager.py | 5 ++ .../tests/functional/agent/test_l3_agent.py | 37 +++++++++++++ neutron/tests/unit/test_iptables_manager.py | 4 ++ neutron/tests/unit/test_l3_agent.py | 52 +++++++++++++++---- .../tests/unit/test_security_groups_rpc.py | 4 +- 8 files changed, 116 insertions(+), 13 deletions(-) diff --git a/etc/l3_agent.ini b/etc/l3_agent.ini index af6740d7930..801d76c1e57 100644 --- a/etc/l3_agent.ini +++ b/etc/l3_agent.ini @@ -68,6 +68,9 @@ # Iptables mangle mark used to mark metadata valid requests # metadata_access_mark = 0x1 +# Iptables mangle mark used to mark ingress from external network +# external_ingress_mark = 0x2 + # router_delete_namespaces, which is false by default, can be set to True if # namespaces can be deleted cleanly on the host running the L3 agent. # Do not enable this until you understand the problem with the Linux iproute diff --git a/neutron/agent/l3/config.py b/neutron/agent/l3/config.py index 49e5dd51614..220eec01162 100644 --- a/neutron/agent/l3/config.py +++ b/neutron/agent/l3/config.py @@ -64,5 +64,9 @@ OPTS = [ cfg.StrOpt('metadata_access_mark', default='0x1', help=_('Iptables mangle mark used to mark metadata valid ' - 'requests')) + 'requests')), + cfg.StrOpt('external_ingress_mark', + default='0x2', + help=_('Iptables mangle mark used to mark ingress from ' + 'external network')), ] diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index 841f1caef6d..c84fe9f49c8 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -29,6 +29,8 @@ LOG = logging.getLogger(__name__) INTERNAL_DEV_PREFIX = namespaces.INTERNAL_DEV_PREFIX EXTERNAL_DEV_PREFIX = namespaces.EXTERNAL_DEV_PREFIX +EXTERNAL_INGRESS_MARK_MASK = '0xffffffff' + class RouterInfo(object): @@ -468,17 +470,28 @@ class RouterInfo(object): interface_name) def external_gateway_nat_rules(self, ex_gw_ip, interface_name): + mark = self.agent_conf.external_ingress_mark rules = [('POSTROUTING', '! -i %(interface_name)s ' '! -o %(interface_name)s -m conntrack ! ' '--ctstate DNAT -j ACCEPT' % {'interface_name': interface_name}), ('snat', '-o %s -j SNAT --to-source %s' % - (interface_name, ex_gw_ip))] + (interface_name, ex_gw_ip)), + ('snat', '-m mark ! --mark %s ' + '-m conntrack --ctstate DNAT ' + '-j SNAT --to-source %s' % (mark, ex_gw_ip))] + return rules + + def external_gateway_mangle_rules(self, interface_name): + mark = self.agent_conf.external_ingress_mark + rules = [('mark', '-i %s -j MARK --set-xmark %s/%s' % + (interface_name, mark, EXTERNAL_INGRESS_MARK_MASK))] return rules def _empty_snat_chains(self, iptables_manager): iptables_manager.ipv4['nat'].empty_chain('POSTROUTING') iptables_manager.ipv4['nat'].empty_chain('snat') + iptables_manager.ipv4['mangle'].empty_chain('mark') def _add_snat_rules(self, ex_gw_port, iptables_manager, interface_name, action): @@ -492,6 +505,9 @@ class RouterInfo(object): interface_name) for rule in rules: iptables_manager.ipv4['nat'].add_rule(*rule) + rules = self.external_gateway_mangle_rules(interface_name) + for rule in rules: + iptables_manager.ipv4['mangle'].add_rule(*rule) break def _handle_router_snat_rules(self, ex_gw_port, diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py index a1299b2eed5..f7c0be4b821 100644 --- a/neutron/agent/linux/iptables_manager.py +++ b/neutron/agent/linux/iptables_manager.py @@ -364,6 +364,11 @@ class IptablesManager(object): self.ipv4['nat'].add_chain('float-snat') self.ipv4['nat'].add_rule('snat', '-j $float-snat') + # Add a mark chain to mangle PREROUTING chain. It is used to + # identify ingress packets from a certain interface. + self.ipv4['mangle'].add_chain('mark') + self.ipv4['mangle'].add_rule('PREROUTING', '-j $mark') + def get_chain(self, table, chain, ip_version=4, wrap=True): try: requested_table = {4: self.ipv4, 6: self.ipv6}[ip_version][table] diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py index 3f990563bee..d933a1ed5fa 100644 --- a/neutron/tests/functional/agent/test_l3_agent.py +++ b/neutron/tests/functional/agent/test_l3_agent.py @@ -619,6 +619,43 @@ class L3AgentTestCase(L3AgentTestFramework): router1.ns_name, router1.get_ha_device_name())) + def test_fip_connection_from_same_subnet(self): + '''Test connection to floatingip which is associated with + fixed_ip on the same subnet of the source fixed_ip. + In other words it confirms that return packets surely + go through the router. + ''' + router_info = self.generate_router_info(enable_ha=False) + router = self.manage_router(self.agent, router_info) + router_ip_cidr = self._port_first_ip_cidr(router.internal_ports[0]) + router_ip = router_ip_cidr.partition('/')[0] + + src_ip_cidr = net_helpers.increment_ip_cidr(router_ip_cidr) + dst_ip_cidr = net_helpers.increment_ip_cidr(src_ip_cidr) + dst_ip = dst_ip_cidr.partition('/')[0] + dst_fip = '19.4.4.10' + router.router[l3_constants.FLOATINGIP_KEY] = [] + self._add_fip(router, dst_fip, fixed_address=dst_ip) + router.process(self.agent) + + src_ns = self._create_namespace(prefix='test-src-') + dst_ns = self._create_namespace(prefix='test-dst-') + br_int = get_ovs_bridge(self.agent.conf.ovs_integration_bridge) + src_port = self.bind_namespace_to_cidr(src_ns, br_int, src_ip_cidr) + net_helpers.set_namespace_gateway(src_port, router_ip) + dst_port = self.bind_namespace_to_cidr(dst_ns, br_int, dst_ip_cidr) + net_helpers.set_namespace_gateway(dst_port, router_ip) + + protocol_port = helpers.get_free_namespace_port(dst_ns) + # client sends to fip + netcat = helpers.NetcatTester(src_ns, dst_ns, dst_ip, + protocol_port, + client_address=dst_fip, + run_as_root=True, + udp=False) + self.addCleanup(netcat.stop_processes) + self.assertTrue(netcat.test_connectivity()) + class L3HATestFramework(L3AgentTestFramework): diff --git a/neutron/tests/unit/test_iptables_manager.py b/neutron/tests/unit/test_iptables_manager.py index 564e46186c1..ddd55b258c6 100644 --- a/neutron/tests/unit/test_iptables_manager.py +++ b/neutron/tests/unit/test_iptables_manager.py @@ -203,11 +203,13 @@ def _generate_mangle_dump(iptables_args): ':%(bn)s-OUTPUT - [0:0]\n' ':%(bn)s-POSTROUTING - [0:0]\n' ':%(bn)s-PREROUTING - [0:0]\n' + ':%(bn)s-mark - [0:0]\n' '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n' '[0:0] -A INPUT -j %(bn)s-INPUT\n' '[0:0] -A FORWARD -j %(bn)s-FORWARD\n' '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n' '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n' + '[0:0] -A %(bn)s-PREROUTING -j %(bn)s-mark\n' 'COMMIT\n' '# Completed by iptables_manager\n' % iptables_args) @@ -593,11 +595,13 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase): ':%(bn)s-POSTROUTING - [0:0]\n' ':%(bn)s-PREROUTING - [0:0]\n' ':%(bn)s-mangle - [0:0]\n' + ':%(bn)s-mark - [0:0]\n' '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n' '[0:0] -A INPUT -j %(bn)s-INPUT\n' '[0:0] -A FORWARD -j %(bn)s-FORWARD\n' '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n' '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n' + '[0:0] -A %(bn)s-PREROUTING -j %(bn)s-mark\n' '[0:0] -A %(bn)s-PREROUTING -j MARK --set-xmark 0x1/0xffffffff\n' 'COMMIT\n' '# Completed by iptables_manager\n' diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index ae71eb3550b..dcb7d574a47 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -727,7 +727,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router['gw_port_host'] = HOSTNAME self._test_external_gateway_action('remove', router, dual_stack=True) - def _verify_snat_rules(self, rules, router, negate=False): + def _verify_snat_mangle_rules(self, nat_rules, mangle_rules, router, + negate=False): interfaces = router[l3_constants.INTERFACE_KEY] source_cidrs = [] for iface in interfaces: @@ -741,8 +742,17 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): expected_rules = [ '! -i %s ! -o %s -m conntrack ! --ctstate DNAT -j ACCEPT' % (interface_name, interface_name), - '-o %s -j SNAT --to-source %s' % (interface_name, source_nat_ip)] - for r in rules: + '-o %s -j SNAT --to-source %s' % (interface_name, source_nat_ip), + '-m mark ! --mark 0x2 -m conntrack --ctstate DNAT ' + '-j SNAT --to-source %s' % source_nat_ip] + for r in nat_rules: + if negate: + self.assertNotIn(r.rule, expected_rules) + else: + self.assertIn(r.rule, expected_rules) + expected_rules = [ + '-i %s -j MARK --set-xmark 0x2/0xffffffff' % interface_name] + for r in mangle_rules: if negate: self.assertNotIn(r.rule, expected_rules) else: @@ -1112,6 +1122,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # Process with NAT ri.process(agent) orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:] + orig_mangle_rules = ri.iptables_manager.ipv4['mangle'].rules[:] # Reprocess without NAT router['enable_snat'] = False # Reassign the router object to RouterInfo @@ -1121,8 +1132,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # IpTablesRule instances nat_rules_delta = [r for r in orig_nat_rules if r not in ri.iptables_manager.ipv4['nat'].rules] - self.assertEqual(len(nat_rules_delta), 2) - self._verify_snat_rules(nat_rules_delta, router) + self.assertEqual(len(nat_rules_delta), 3) + mangle_rules_delta = [ + r for r in orig_mangle_rules + if r not in ri.iptables_manager.ipv4['mangle'].rules] + self.assertEqual(len(mangle_rules_delta), 1) + self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta, + router) self.assertEqual(self.send_arp.call_count, 1) def test_process_router_snat_enabled(self): @@ -1133,6 +1149,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # Process without NAT ri.process(agent) orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:] + orig_mangle_rules = ri.iptables_manager.ipv4['mangle'].rules[:] # Reprocess with NAT router['enable_snat'] = True # Reassign the router object to RouterInfo @@ -1142,8 +1159,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # IpTablesRule instances nat_rules_delta = [r for r in ri.iptables_manager.ipv4['nat'].rules if r not in orig_nat_rules] - self.assertEqual(len(nat_rules_delta), 2) - self._verify_snat_rules(nat_rules_delta, router) + self.assertEqual(len(nat_rules_delta), 3) + mangle_rules_delta = [ + r for r in ri.iptables_manager.ipv4['mangle'].rules + if r not in orig_mangle_rules] + self.assertEqual(len(mangle_rules_delta), 1) + self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta, + router) self.assertEqual(self.send_arp.call_count, 1) def test_process_router_interface_added(self): @@ -1481,14 +1503,24 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): jump_float_rule = "-A %s-snat -j %s-float-snat" % (wrap_name, wrap_name) - snat_rule = ("-A %s-snat -o iface -j SNAT --to-source %s") % ( + snat_rule1 = ("-A %s-snat -o iface -j SNAT --to-source %s") % ( + wrap_name, ex_gw_port['fixed_ips'][0]['ip_address']) + snat_rule2 = ("-A %s-snat -m mark ! --mark 0x2 " + "-m conntrack --ctstate DNAT " + "-j SNAT --to-source %s") % ( wrap_name, ex_gw_port['fixed_ips'][0]['ip_address']) self.assertIn(jump_float_rule, nat_rules) - self.assertIn(snat_rule, nat_rules) + self.assertIn(snat_rule1, nat_rules) + self.assertIn(snat_rule2, nat_rules) self.assertThat(nat_rules.index(jump_float_rule), - matchers.LessThan(nat_rules.index(snat_rule))) + matchers.LessThan(nat_rules.index(snat_rule1))) + + mangle_rules = map(str, ri.iptables_manager.ipv4['mangle'].rules) + mangle_rule = ("-A %s-mark -i iface " + "-j MARK --set-xmark 0x2/0xffffffff") % wrap_name + self.assertIn(mangle_rule, mangle_rules) def test_process_router_delete_stale_internal_devices(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) diff --git a/neutron/tests/unit/test_security_groups_rpc.py b/neutron/tests/unit/test_security_groups_rpc.py index 138e742e55a..d15c883171a 100644 --- a/neutron/tests/unit/test_security_groups_rpc.py +++ b/neutron/tests/unit/test_security_groups_rpc.py @@ -1673,7 +1673,7 @@ IPTABLES_ARG = {'bn': iptables_manager.binary_name, 'physdev_mod': PHYSDEV_MOD, 'physdev_is_bridged': PHYSDEV_IS_BRIDGED} -CHAINS_MANGLE = 'FORWARD|INPUT|OUTPUT|POSTROUTING|PREROUTING' +CHAINS_MANGLE = 'FORWARD|INPUT|OUTPUT|POSTROUTING|PREROUTING|mark' IPTABLES_ARG['chains'] = CHAINS_MANGLE IPTABLES_MANGLE = """# Generated by iptables_manager @@ -1683,11 +1683,13 @@ IPTABLES_MANGLE = """# Generated by iptables_manager :%(bn)s-(%(chains)s) - [0:0] :%(bn)s-(%(chains)s) - [0:0] :%(bn)s-(%(chains)s) - [0:0] +:%(bn)s-(%(chains)s) - [0:0] [0:0] -A PREROUTING -j %(bn)s-PREROUTING [0:0] -A INPUT -j %(bn)s-INPUT [0:0] -A FORWARD -j %(bn)s-FORWARD [0:0] -A OUTPUT -j %(bn)s-OUTPUT [0:0] -A POSTROUTING -j %(bn)s-POSTROUTING +[0:0] -A %(bn)s-PREROUTING -j %(bn)s-mark COMMIT # Completed by iptables_manager """ % IPTABLES_ARG