From cea149212e6387932eaac8448c951d2ceb7ae23d Mon Sep 17 00:00:00 2001 From: Hong Hui Xiao Date: Tue, 1 Mar 2016 05:42:42 +0000 Subject: [PATCH] Add fip nat rules even if router disables shared snat For legacy router, there are some iptables rules added for external gateway port. Some of these rules are for shared snat, some are for floating ip. When user disables shared snat of router gateway, some of the iptables rules that floating ip needs will not be added to router. This will cause the reported bug, ping floating ip but reply with fixed ip. The fix will add the iptables rules that floating ip needs, no matter if router enables shared snat. A functional test is also added for the issue. Change-Id: I3cf4dff90f47a720a2e6a92c9ede2bc067ebd6e7 Closes-Bug: #1551530 --- neutron/agent/l3/router_info.py | 34 +++++++++---------- .../tests/functional/agent/l3/framework.py | 11 ++++++ .../functional/agent/l3/test_legacy_router.py | 33 ++++++++++++++---- neutron/tests/unit/agent/l3/test_agent.py | 18 +++++----- 4 files changed, 63 insertions(+), 33 deletions(-) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index bcf74f1d15f..bfeddbd1f0d 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -694,19 +694,12 @@ class RouterInfo(object): gw_port = self._router.get('gw_port') self._handle_router_snat_rules(gw_port, interface_name) - def external_gateway_nat_postroute_rules(self, interface_name): + def external_gateway_nat_fip_rules(self, ex_gw_ip, 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)) - # Makes replies come back through the router to reverse DNAT ext_in_mark = self.agent_conf.external_ingress_mark snat_internal_traffic_to_floating_ip = ( @@ -714,9 +707,15 @@ class RouterInfo(object): '-m conntrack --ctstate DNAT ' '-j SNAT --to-source %s' % (ext_in_mark, l3_constants.ROUTER_MARK_MASK, ex_gw_ip)) - return [snat_normal_external_traffic, + return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip, snat_internal_traffic_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)) + return [snat_normal_external_traffic] + def external_gateway_mangle_rules(self, interface_name): mark = self.agent_conf.external_ingress_mark mark_packets_entering_external_gateway_port = ( @@ -740,19 +739,20 @@ class RouterInfo(object): 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_postroute_rules( - interface_name) - for rule in rules: - iptables_manager.ipv4['nat'].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) + + rules = self.external_gateway_nat_fip_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/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index 395f40a1913..d3b2e434f9a 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -498,3 +498,14 @@ class L3AgentTestFramework(base.BaseSudoTestCase): namespace, interface, ip_address): self.assertIn( ip_address, self._get_addresses_on_device(namespace, interface)) + + def _assert_ping_reply_from_expected_address( + self, ping_result, expected_address): + ping_results = ping_result.split('\n') + self.assertGreater( + len(ping_results), 1, + "The result from ping should be multiple lines") + self.assertIn( + expected_address, ping_results[1], + ("Expect to see %s in the reply of ping, but failed" % + expected_address)) diff --git a/neutron/tests/functional/agent/l3/test_legacy_router.py b/neutron/tests/functional/agent/l3/test_legacy_router.py index f8ba0b95721..35e6272b45a 100644 --- a/neutron/tests/functional/agent/l3/test_legacy_router.py +++ b/neutron/tests/functional/agent/l3/test_legacy_router.py @@ -193,13 +193,12 @@ class L3AgentTestCase(framework.L3AgentTestFramework): routers_deleted=[], routers_deleted_during_resync=routers_deleted_during_resync) - 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) + def _setup_fip_with_fixed_ip_from_same_subnet(self, enable_snat): + """Setup 2 FakeMachines from same subnet, one with floatingip + associated. + """ + router_info = self.generate_router_info(enable_ha=False, + enable_snat=enable_snat) 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] @@ -218,6 +217,16 @@ class L3AgentTestCase(framework.L3AgentTestFramework): self._add_fip(router, dst_fip, fixed_address=dst_machine.ip) router.process(self.agent) + return src_machine, dst_machine, dst_fip + + 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. + ''' + src_machine, dst_machine, dst_fip = ( + self._setup_fip_with_fixed_ip_from_same_subnet(enable_snat=True)) protocol_port = net_helpers.get_free_namespace_port( l3_constants.PROTO_NAME_TCP, dst_machine.namespace) # client sends to fip @@ -228,6 +237,16 @@ class L3AgentTestCase(framework.L3AgentTestFramework): self.addCleanup(netcat.stop_processes) self.assertTrue(netcat.test_connectivity()) + def test_ping_floatingip_reply_with_floatingip(self): + src_machine, _, dst_fip = ( + self._setup_fip_with_fixed_ip_from_same_subnet(enable_snat=False)) + + # Verify that the ping replys with fip + ns_ip_wrapper = ip_lib.IPWrapper(src_machine.namespace) + result = ns_ip_wrapper.netns.execute( + ['ping', '-c', 1, '-W', 5, dst_fip]) + self._assert_ping_reply_from_expected_address(result, dst_fip) + def _setup_address_scope(self, internal_address_scope1, internal_address_scope2, gw_address_scope=None): router_info = self.generate_router_info(enable_ha=False, diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index ced85c9ea03..3021d17f4dc 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -1148,11 +1148,11 @@ 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(2, len(nat_rules_delta)) + self.assertEqual(1, len(nat_rules_delta)) mangle_rules_delta = [ r for r in orig_mangle_rules if r not in ri.iptables_manager.ipv4['mangle'].rules] - self.assertEqual(2, len(mangle_rules_delta)) + self.assertEqual(1, len(mangle_rules_delta)) self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta, router) self.assertEqual(1, self.send_adv_notif.call_count) @@ -1175,11 +1175,11 @@ 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(2, len(nat_rules_delta)) + self.assertEqual(1, len(nat_rules_delta)) mangle_rules_delta = [ r for r in ri.iptables_manager.ipv4['mangle'].rules if r not in orig_mangle_rules] - self.assertEqual(2, len(mangle_rules_delta)) + self.assertEqual(1, len(mangle_rules_delta)) self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta, router) self.assertEqual(1, self.send_adv_notif.call_count) @@ -1245,15 +1245,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # Get NAT rules with the gw_port router['gw_port'] = gw_port ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs) - p = ri.external_gateway_nat_postroute_rules + p = ri.external_gateway_nat_fip_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_fip_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_fip_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[:] @@ -1261,13 +1261,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # NAT rules should only change for dual_stack operation if dual_stack: self.assertTrue( - mocks['external_gateway_nat_postroute_rules'].called) + mocks['external_gateway_nat_fip_rules'].called) self.assertTrue( mocks['external_gateway_nat_snat_rules'].called) self.assertNotEqual(orig_nat_rules, new_nat_rules) else: self.assertFalse( - mocks['external_gateway_nat_postroute_rules'].called) + mocks['external_gateway_nat_fip_rules'].called) self.assertFalse( mocks['external_gateway_nat_snat_rules'].called) self.assertEqual(orig_nat_rules, new_nat_rules)