]> review.fuel-infra Code Review - openstack-build/neutron-build.git/commitdiff
Make floatingip reachable from the same network
authorItsuro Oda <oda@valinux.co.jp>
Wed, 25 Feb 2015 04:34:04 +0000 (13:34 +0900)
committerItsuro Oda <oda@valinux.co.jp>
Tue, 31 Mar 2015 01:05:03 +0000 (10:05 +0900)
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
neutron/agent/l3/config.py
neutron/agent/l3/router_info.py
neutron/agent/linux/iptables_manager.py
neutron/tests/functional/agent/test_l3_agent.py
neutron/tests/unit/test_iptables_manager.py
neutron/tests/unit/test_l3_agent.py
neutron/tests/unit/test_security_groups_rpc.py

index af6740d7930655a4ea7e80538863cfe191f43d9f..801d76c1e57706d3a7f672766b13225600f9a2f4 100644 (file)
@@ -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
index 49e5dd51614b5858a10fede54e53e5a62673ed78..220eec011621120c033135927efeddc15deb9175 100644 (file)
@@ -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')),
 ]
index 841f1caef6d8481774a96e91d6fa4aaf64d428a5..c84fe9f49c89c1faefabb93ca09c4385461c8b9c 100644 (file)
@@ -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,
index a1299b2eed5b3d1a68af27f7d1ee38b4b2720d02..f7c0be4b821a9555d4c0a6f33c8784b76e5dc1e2 100644 (file)
@@ -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]
index 3f990563bee6d94188b739403e08ff6a9173e2f3..d933a1ed5fadc8b06a810100a531d3c4de98306e 100644 (file)
@@ -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):
 
index 564e46186c1470fef524327abee69cd549abe070..ddd55b258c6defdd721811997d97fe7b007bc4b7 100644 (file)
@@ -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'
index ae71eb3550b5f29f4234c0bf36a998a0cae7dc5f..dcb7d574a47f0e00697c2862e93ffa3e4a5e89b5 100644 (file)
@@ -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)
index 138e742e55adfb7abd1878ebdc9ecd25430cfd70..d15c883171ade738b9542fcaf84990ef648bc2c8 100644 (file)
@@ -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