From fcb12b5c2744a08ce99286141a632c8b50e4e385 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 (cherry picked from commit a388f78c8cb4b1c860bfc11029b5210955f1932d) --- 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 28ca1c7402b..756de6dc2eb 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -289,11 +289,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 8a2ad2cb025..15a44e7f173 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -55,6 +55,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'] @@ -132,16 +159,12 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): self._add_floating_ip_rule(floating_ip, fip['fixed_ip_address']) 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 move_floating_ip(self, fip): @@ -529,6 +552,28 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): # configured self.agent.process_router_add(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 1d138898541..0f9bcd01363 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -548,6 +548,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() @@ -578,6 +587,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_dvr_router_floating_ip_moved(self): self.agent.conf.agent_mode = 'dvr' router_info = self.generate_dvr_router_info() @@ -603,19 +619,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) @@ -1045,21 +1057,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 c4cddab6468..8a056be63c5 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -284,24 +284,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 22d750a0938..98e2c17c604 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -304,14 +304,13 @@ class TestDvrRouterOperations(base.BaseTestCase): priority=FIP_PRI) 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): @@ -322,13 +321,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()) @@ -336,8 +328,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) @@ -651,9 +642,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(