From a388f78c8cb4b1c860bfc11029b5210955f1932d Mon Sep 17 00:00:00 2001 From: Hong Hui Xiao Date: Thu, 12 May 2016 05:48:15 +0000 Subject: [PATCH] DVR: Pings to floatingip returns with fixed-ip on same network Pinging a floatingip of VM1 from a second VM(VM2) which has SNAT enabled connected to a DVR router on the same network returns with fixed-ip address rather than the floatingip address. The NAT forwarding rules for floatingip in the router namespace does not check for the in coming port and tries to add the rule for all in coming ports. This causes the packets that are originating from the router namespace to be modified and forwarded directly to the VM2 fixed ip instead of forwarding the traffic to the SNAT namespace. The fix in here will make sure that for all routers, the floatingip forwarding rules will be applied only to the 'rfp-' internal ports and not to all ports. Change-Id: I9453beffd94bf685afd74b0820506fb6b7c996c4 Closes-Bug: #1462154 Co-Authored-By: Hong Hui Xiao --- neutron/agent/l3/dvr_fip_ns.py | 9 +--- neutron/agent/l3/dvr_local_router.py | 53 +++++++++++++++++-- .../functional/agent/l3/test_dvr_router.py | 46 +++++++--------- .../tests/unit/agent/l3/test_dvr_fip_ns.py | 13 ++--- .../unit/agent/l3/test_dvr_local_router.py | 20 ++----- 5 files changed, 80 insertions(+), 61 deletions(-) diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 776ca9e8d74..26e5c3c0743 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -276,11 +276,4 @@ class FipNamespace(namespaces.Namespace): rtr_2_fip_interface = self.get_rtr_ext_device_name(ri.router_id) device = ip_lib.IPDevice(rtr_2_fip_interface, namespace=ri.ns_name) if device.exists(): - existing_cidrs = [addr['cidr'] for addr in device.addr.list()] - fip_cidrs = [c for c in existing_cidrs if - common_utils.is_cidr_host(c)] - for fip_cidr in fip_cidrs: - fip_ip = fip_cidr.split('/')[0] - rule_pr = self._rule_priorities.allocate(fip_ip) - ri.floating_ips_dict[fip_ip] = rule_pr - ri.dist_fip_count = len(fip_cidrs) + ri.dist_fip_count = len(ri.get_router_cidrs(device)) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 4d8b56cbbe2..2d1a29592ba 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -56,6 +56,33 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): (i['host'] == self.host) or (i.get('dest_host') == self.host))] + def floating_forward_rules(self, floating_ip, fixed_ip): + """Override this function defined in router_info for dvr routers.""" + if not self.fip_ns: + return [] + + rtr_2_fip_name = self.fip_ns.get_rtr_ext_device_name(self.router_id) + dnat_from_floatingip_to_fixedip = ( + 'PREROUTING', '-d %s/32 -i %s -j DNAT --to-destination %s' % ( + floating_ip, rtr_2_fip_name, fixed_ip)) + snat_from_fixedip_to_floatingip = ( + 'float-snat', '-s %s/32 -j SNAT --to-source %s' % ( + fixed_ip, floating_ip)) + return [dnat_from_floatingip_to_fixedip, + snat_from_fixedip_to_floatingip] + + def floating_mangle_rules(self, floating_ip, fixed_ip, internal_mark): + if not self.fip_ns: + return [] + + rtr_2_fip_name = self.fip_ns.get_rtr_ext_device_name(self.router_id) + mark_traffic_to_floating_ip = ( + 'floatingip', '-d %s -i %s -j MARK --set-xmark %s' % ( + floating_ip, rtr_2_fip_name, internal_mark)) + mark_traffic_from_fixed_ip = ( + 'FORWARD', '-s %s -j $float-snat' % fixed_ip) + return [mark_traffic_to_floating_ip, mark_traffic_from_fixed_ip] + def floating_ip_added_dist(self, fip, fip_cidr): """Add floating IP to FIP namespace.""" floating_ip = fip['floating_ip_address'] @@ -121,16 +148,12 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): ns_ip.del_veth(fip_2_rtr_name) def add_floating_ip(self, fip, interface_name, device): - if not self._add_fip_addr_to_device(fip, device): - return l3_constants.FLOATINGIP_STATUS_ERROR - # Special Handling for DVR - update FIP namespace ip_cidr = common_utils.ip_to_cidr(fip['floating_ip_address']) self.floating_ip_added_dist(fip, ip_cidr) return l3_constants.FLOATINGIP_STATUS_ACTIVE def remove_floating_ip(self, device, ip_cidr): - super(DvrLocalRouter, self).remove_floating_ip(device, ip_cidr) self.floating_ip_removed_dist(ip_cidr) def _get_internal_port(self, subnet_id): @@ -477,6 +500,28 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): (self.dist_fip_count == 0)): self.fip_ns.create_rtr_2_fip_link(self) + def get_router_cidrs(self, device): + """As no floatingip will be set on the rfp device. Get floatingip from + the route of fip namespace. + """ + if not self.fip_ns: + return set() + + fip_ns_name = self.fip_ns.get_name() + fip_2_rtr_name = self.fip_ns.get_int_device_name(self.router_id) + device = ip_lib.IPDevice(fip_2_rtr_name, namespace=fip_ns_name) + if not device.exists(): + return set() + + if self.rtr_fip_subnet is None: + self.rtr_fip_subnet = self.fip_ns.local_subnets.allocate( + self.router_id) + rtr_2_fip, _fip_2_rtr = self.rtr_fip_subnet.get_pair() + exist_routes = device.route.list_routes( + l3_constants.IP_VERSION_4, via=str(rtr_2_fip.ip)) + return {common_utils.ip_to_cidr(route['cidr']) + for route in exist_routes} + def process(self, agent): ex_gw_port = self.get_ex_gw_port() if ex_gw_port: diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index 538de572a27..8290a98627b 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -413,6 +413,15 @@ class TestDvrRouter(framework.L3AgentTestFramework): self.assertTrue(ip_lib.device_exists( device_name, namespace=router.ns_name)) + # In the router namespace, check the iptables rules are set correctly + for fip in floating_ips: + floatingip = fip['floating_ip_address'] + fixedip = fip['fixed_ip_address'] + expected_rules = router.floating_forward_rules(floatingip, + fixedip) + self._assert_iptables_rules_exist( + router.iptables_manager, 'nat', expected_rules) + def test_dvr_router_rem_fips_on_restarted_agent(self): self.agent.conf.agent_mode = 'dvr_snat' router_info = self.generate_dvr_router_info() @@ -443,6 +452,13 @@ class TestDvrRouter(framework.L3AgentTestFramework): router_ns, floating_ips[0]['fixed_ip_address']) self.assertNotEqual(fip_rule_prio_1, fip_rule_prio_2) + def _assert_iptables_rules_exist( + self, router_iptables_manager, table_name, expected_rules): + rules = router_iptables_manager.get_rules_for_table(table_name) + for rule in expected_rules: + self.assertIn( + str(iptables_manager.IptablesRule(rule[0], rule[1])), rules) + def test_prevent_snat_rule_exist_on_restarted_agent(self): self.agent.conf.agent_mode = 'dvr_snat' router_info = self.generate_dvr_router_info() @@ -452,19 +468,15 @@ class TestDvrRouter(framework.L3AgentTestFramework): prevent_snat_rule = router._prevent_snat_for_internal_traffic_rule( rfp_devicename) - def is_prevent_snat_rule_exist(router_iptables_manager): - rules = router_iptables_manager.get_rules_for_table('nat') - return (str(iptables_manager.IptablesRule( - prevent_snat_rule[0], prevent_snat_rule[1])) in rules) - - self.assertTrue(is_prevent_snat_rule_exist(router.iptables_manager)) + self._assert_iptables_rules_exist( + router.iptables_manager, 'nat', [prevent_snat_rule]) restarted_agent = neutron_l3_agent.L3NATAgentWithStateReport( self.agent.host, self.agent.conf) restarted_router = self.manage_router(restarted_agent, router_info) - self.assertTrue(is_prevent_snat_rule_exist( - restarted_router.iptables_manager)) + self._assert_iptables_rules_exist( + restarted_router.iptables_manager, 'nat', [prevent_snat_rule]) def _get_fixed_ip_rule_priority(self, namespace, fip): iprule = ip_lib.IPRule(namespace) @@ -883,21 +895,3 @@ class TestDvrRouter(framework.L3AgentTestFramework): # external networks. SNAT will be used. Direct route will not work # here. src_machine.assert_no_ping(machine_diff_scope.ip) - - def test_connection_from_diff_address_scope_with_fip(self): - (machine_same_scope, machine_diff_scope, - router) = self._setup_address_scope('scope1', 'scope2', 'scope1') - fip = '19.4.4.11' - self._add_fip(router, fip, - fixed_address=machine_diff_scope.ip, - host=self.agent.conf.host, - fixed_ip_address_scope='scope2') - router.process(self.agent) - - # For the internal networks that are in the same address scope as - # external network, they should be able to reach the floating ip - net_helpers.assert_ping(machine_same_scope.namespace, fip, 5) - # For the port with fip, it should be able to reach the internal - # networks that are in the same address scope as external network - net_helpers.assert_ping(machine_diff_scope.namespace, - machine_same_scope.ip, 5) diff --git a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py index e7d160e1016..089b3c93e6f 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -272,24 +272,21 @@ class TestDvrFipNs(base.BaseTestCase): @mock.patch.object(ip_lib, 'IPDevice') def _test_scan_fip_ports(self, ri, ip_list, IPDevice): IPDevice.return_value = device = mock.Mock() - device.addr.list.return_value = ip_list + device.exists.return_value = True + ri.get_router_cidrs.return_value = ip_list self.fip_ns.get_rtr_ext_device_name = mock.Mock( return_value=mock.sentinel.rtr_ext_device_name) self.fip_ns.scan_fip_ports(ri) - @mock.patch.object(ip_lib, 'device_exists') - def test_scan_fip_ports_restart_fips(self, device_exists): - device_exists.return_value = True + def test_scan_fip_ports_restart_fips(self): ri = mock.Mock() ri.dist_fip_count = None ri.floating_ips_dict = {} - ip_list = [{'cidr': '111.2.3.4/32'}, {'cidr': '111.2.3.5/32'}] + ip_list = [{'cidr': '111.2.3.4'}, {'cidr': '111.2.3.5'}] self._test_scan_fip_ports(ri, ip_list) self.assertEqual(2, ri.dist_fip_count) - @mock.patch.object(ip_lib, 'device_exists') - def test_scan_fip_ports_restart_none(self, device_exists): - device_exists.return_value = True + def test_scan_fip_ports_restart_none(self): ri = mock.Mock() ri.dist_fip_count = None ri.floating_ips_dict = {} diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index a75f814b514..195e4cc9e7a 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -283,14 +283,13 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.fip_ns.local_subnets.allocate.assert_called_once_with(ri.router_id) def _test_add_floating_ip(self, ri, fip, is_failure): - ri._add_fip_addr_to_device = mock.Mock(return_value=is_failure) ri.floating_ip_added_dist = mock.Mock() result = ri.add_floating_ip(fip, mock.sentinel.interface_name, mock.sentinel.device) - ri._add_fip_addr_to_device.assert_called_once_with( - fip, mock.sentinel.device) + ri.floating_ip_added_dist.assert_called_once_with( + fip, mock.ANY) return result def test_add_floating_ip(self): @@ -301,13 +300,6 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.floating_ip_added_dist.assert_called_once_with(fip, ip + '/32') self.assertEqual(l3_constants.FLOATINGIP_STATUS_ACTIVE, result) - def test_add_floating_ip_error(self): - ri = self._create_router(mock.MagicMock()) - result = self._test_add_floating_ip( - ri, {'floating_ip_address': '15.1.2.3'}, False) - self.assertFalse(ri.floating_ip_added_dist.called) - self.assertEqual(l3_constants.FLOATINGIP_STATUS_ERROR, result) - @mock.patch.object(router_info.RouterInfo, 'remove_floating_ip') def test_remove_floating_ip(self, super_remove_floating_ip): ri = self._create_router(mock.MagicMock()) @@ -315,8 +307,7 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.remove_floating_ip(mock.sentinel.device, mock.sentinel.ip_cidr) - super_remove_floating_ip.assert_called_once_with( - mock.sentinel.device, mock.sentinel.ip_cidr) + self.assertFalse(super_remove_floating_ip.called) ri.floating_ip_removed_dist.assert_called_once_with( mock.sentinel.ip_cidr) @@ -630,9 +621,8 @@ class TestDvrRouterOperations(base.BaseTestCase): _, fip_to_rtr = ri.rtr_fip_subnet.get_pair() self.mock_ip.get_devices.return_value = [ l3_test_common.FakeDev(ri.fip_ns.get_ext_device_name(_uuid()))] - self.mock_ip_dev.addr.list.return_value = [ - {'cidr': vm_floating_ip + '/32'}, - {'cidr': '19.4.4.1/24'}] + ri.get_router_cidrs = mock.Mock( + return_value={vm_floating_ip + '/32', '19.4.4.1/24'}) self.device_exists.return_value = True ri.external_gateway_removed(