From 4341a4faeed937d014e95a94b77844d5a835acbe Mon Sep 17 00:00:00 2001 From: Jenkins Date: Fri, 16 Oct 2015 02:26:57 +0000 Subject: [PATCH] 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 --- neutron/agent/l3/router_info.py | 26 ++++++++++++-------- neutron/tests/unit/agent/l3/test_agent.py | 29 ++++++++++++++++------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index 038b3d715..7806eaa49 100644 --- 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 c205e5d3e..42d539cb2 100644 --- 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): -- 2.45.2