Don't snat traffic between fixed IPs behind same router

This fixes a bug where an iptables rule to not snat traffic between
fixed IPs is only being added if enable_snat=true. We should add
this rule no matter what the value is for enable_snat.

Without this patch, current code will break such use case:
2 fixed IPs behind same router both have floatingip associated. And
the router has enable_snat=false. When fixed IP A want to ping
fixed IP B, fixed IP A will get the reply from fixed IP B's floating
IP.

More details could be found at bug description.

Change-Id: I322e8d454ef1d529ceda541fb5fe577cd70b412f
Closes-bug: #1505781

diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index 038b3d715057717458c32da5685a5dca5e445ddd..7806eaa49335acda10fd44f74a788c4e5b476d3b 100644 (file)
--- a/neutron/agent/l3/router_info.py
+++ b/neutron/agent/l3/router_info.py
@@ -590,13 +590,15 @@ class RouterInfo(object):
         gw_port = self._router.get('gw_port')
         self._handle_router_snat_rules(gw_port, interface_name)
 
-    def external_gateway_nat_rules(self, ex_gw_ip, interface_name):
+    def external_gateway_nat_postroute_rules(self, interface_name):
         dont_snat_traffic_to_internal_ports_if_not_to_floating_ip = (
             'POSTROUTING', '! -i %(interface_name)s '
                            '! -o %(interface_name)s -m conntrack ! '
                            '--ctstate DNAT -j ACCEPT' %
                            {'interface_name': interface_name})
+        return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip]
 
+    def external_gateway_nat_snat_rules(self, ex_gw_ip, interface_name):
         snat_normal_external_traffic = (
             'snat', '-o %s -j SNAT --to-source %s' %
                     (interface_name, ex_gw_ip))
@@ -608,9 +610,7 @@ class RouterInfo(object):
                     '-m conntrack --ctstate DNAT '
                     '-j SNAT --to-source %s'
                     % (ext_in_mark, l3_constants.ROUTER_MARK_MASK, ex_gw_ip))
-
-        return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip,
-                snat_normal_external_traffic,
+        return [snat_normal_external_traffic,
                 snat_internal_traffic_to_floating_ip]
 
     def external_gateway_mangle_rules(self, interface_name):
@@ -627,19 +627,25 @@ class RouterInfo(object):
 
     def _add_snat_rules(self, ex_gw_port, iptables_manager,
                         interface_name):
-        if self._snat_enabled and ex_gw_port:
+        if ex_gw_port:
             # ex_gw_port should not be None in this case
             # NAT rules are added only if ex_gw_port has an IPv4 address
             for ip_addr in ex_gw_port['fixed_ips']:
                 ex_gw_ip = ip_addr['ip_address']
                 if netaddr.IPAddress(ex_gw_ip).version == 4:
-                    rules = self.external_gateway_nat_rules(ex_gw_ip,
-                                                            interface_name)
+                    rules = self.external_gateway_nat_postroute_rules(
+                        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)
+                    if self._snat_enabled:
+                        rules = self.external_gateway_nat_snat_rules(
+                            ex_gw_ip, 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, interface_name):

diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index c205e5d3e19ad2509c3e41a7a0c4099148de3674..42d539cb254f65c1caab5a49a4e96afefeb449cf 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -1046,7 +1046,7 @@ 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(3, len(nat_rules_delta))
+        self.assertEqual(2, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in orig_mangle_rules
             if r not in ri.iptables_manager.ipv4['mangle'].rules]
@@ -1073,7 +1073,7 @@ 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(3, len(nat_rules_delta))
+        self.assertEqual(2, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in ri.iptables_manager.ipv4['mangle'].rules
             if r not in orig_mangle_rules]
@@ -1134,20 +1134,31 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # Get NAT rules with the gw_port
         router['gw_port'] = gw_port
         ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs)
-        orig_ext_gw_nat_rules = ri.external_gateway_nat_rules
-        with mock.patch.object(
-                ri,
-                'external_gateway_nat_rules') as external_gateway_nat_rules:
-            external_gateway_nat_rules.side_effect = orig_ext_gw_nat_rules
+        p = ri.external_gateway_nat_postroute_rules
+        s = ri.external_gateway_nat_snat_rules
+        attrs_to_mock = dict(
+            [(a, mock.DEFAULT) for a in
+                ['external_gateway_nat_postroute_rules',
+                 'external_gateway_nat_snat_rules']]
+        )
+        with mock.patch.multiple(ri, **attrs_to_mock) as mocks:
+            mocks['external_gateway_nat_postroute_rules'].side_effect = p
+            mocks['external_gateway_nat_snat_rules'].side_effect = s
             self._process_router_instance_for_agent(agent, ri, router)
             new_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
 
             # NAT rules should only change for dual_stack operation
             if dual_stack:
-                self.assertTrue(external_gateway_nat_rules.called)
+                self.assertTrue(
+                    mocks['external_gateway_nat_postroute_rules'].called)
+                self.assertTrue(
+                    mocks['external_gateway_nat_snat_rules'].called)
                 self.assertNotEqual(orig_nat_rules, new_nat_rules)
             else:
-                self.assertFalse(external_gateway_nat_rules.called)
+                self.assertFalse(
+                    mocks['external_gateway_nat_postroute_rules'].called)
+                self.assertFalse(
+                    mocks['external_gateway_nat_snat_rules'].called)
                 self.assertEqual(orig_nat_rules, new_nat_rules)
 
     def test_process_ipv6_only_gw(self):