From 1abb77d7a63cde2aa9640351f663870c14430919 Mon Sep 17 00:00:00 2001 From: Fernando Royo Date: Fri, 23 Sep 2022 20:53:09 +0200 Subject: [PATCH] Check subnet overlapping after add router interface When simultaneous attempts are made to add an interface to the same router including overlapping networks in cidrs, both attempts are successful. There is a check to avoid this overlap but is performed when creating the network interface and it is done over the ports already attached to the router, so at this moment the check is not able to detect the overlapping. Furthermore, the create_port operation over the ML2 plugin must be executed in isolated transactions, so trying to control the execution context or adding additional steps to the transaction is not feasible. This patch checks once the RouterPort is created on the neutron database if there is more than one overlapping port, triggering in that case the exception that will remove the the culprit of overlapping. Closes-Bug: #1987666 Change-Id: I7cec8b53e72e7abf34012906e6adfecf079525af --- neutron/db/l3_db.py | 108 +++++++++++++----- neutron/tests/fullstack/test_l3_agent.py | 31 +++++ neutron/tests/unit/db/test_l3_db.py | 137 +++++++++++++++++++++++ 3 files changed, 246 insertions(+), 30 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 618ddb11fb0..886af4af115 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -13,6 +13,7 @@ # under the License. import functools +import itertools import random import netaddr @@ -564,6 +565,23 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, ip_address_change = not ip_addresses == new_ip_addresses return ip_address_change + def _raise_on_subnets_overlap(self, subnet_1, subnet_2): + cidr = subnet_1['cidr'] + ipnet = netaddr.IPNetwork(cidr) + new_cidr = subnet_2['cidr'] + new_ipnet = netaddr.IPNetwork(new_cidr) + match1 = netaddr.all_matching_cidrs(new_ipnet, [cidr]) + match2 = netaddr.all_matching_cidrs(ipnet, [new_cidr]) + if match1 or match2: + data = {'subnet_cidr': new_cidr, + 'subnet_id': subnet_2['id'], + 'cidr': cidr, + 'sub_id': subnet_1['id']} + msg = (_("Cidr %(subnet_cidr)s of subnet " + "%(subnet_id)s overlaps with cidr %(cidr)s " + "of subnet %(sub_id)s") % data) + raise n_exc.BadRequest(resource='router', msg=msg) + def _ensure_router_not_in_use(self, context, router_id): """Ensure that no internal network interface is attached to the router. @@ -672,22 +690,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, subnets = self._core_plugin.get_subnets(context.elevated(), filters=id_filter) for sub in subnets: - cidr = sub['cidr'] - ipnet = netaddr.IPNetwork(cidr) for s in new_subnets: - new_cidr = s['cidr'] - new_ipnet = netaddr.IPNetwork(new_cidr) - match1 = netaddr.all_matching_cidrs(new_ipnet, [cidr]) - match2 = netaddr.all_matching_cidrs(ipnet, [new_cidr]) - if match1 or match2: - data = {'subnet_cidr': new_cidr, - 'subnet_id': s['id'], - 'cidr': cidr, - 'sub_id': sub['id']} - msg = (_("Cidr %(subnet_cidr)s of subnet " - "%(subnet_id)s overlaps with cidr %(cidr)s " - "of subnet %(sub_id)s") % data) - raise n_exc.BadRequest(resource='router', msg=msg) + self._raise_on_subnets_overlap(sub, s) def _get_device_owner(self, context, router=None): """Get device_owner for the specified router.""" @@ -768,17 +772,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, port = self._check_router_port(context, port_id, router.id) # Only allow one router port with IPv6 subnets per network id - if self._port_has_ipv6_address(port): - for existing_port in (rp.port for rp in router.attached_ports): - if (existing_port['network_id'] == port['network_id'] and - self._port_has_ipv6_address(existing_port)): - msg = _("Cannot have multiple router ports with the " - "same network id if both contain IPv6 " - "subnets. Existing port %(p)s has IPv6 " - "subnet(s) and network id %(nid)s") - raise n_exc.BadRequest(resource='router', msg=msg % { - 'p': existing_port['id'], - 'nid': existing_port['network_id']}) + self._validate_one_router_ipv6_port_per_network(router, port) fixed_ips = list(port['fixed_ips']) subnets = [] @@ -844,6 +838,20 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, if netaddr.IPNetwork(fixed_ip['ip_address']).version == 6: return True + def _validate_one_router_ipv6_port_per_network(self, router, port): + if self._port_has_ipv6_address(port): + for existing_port in (rp.port for rp in router.attached_ports): + if (existing_port["id"] != port["id"] and + existing_port["network_id"] == port["network_id"] and + self._port_has_ipv6_address(existing_port)): + msg = _("Router already contains IPv6 port %(p)s " + "belonging to network id %(nid)s. Only one IPv6 port " + "from the same network subnet can be connected to a " + "router.") + raise n_exc.BadRequest(resource='router', msg=msg % { + 'p': existing_port['id'], + 'nid': existing_port['network_id']}) + def _find_ipv6_router_port_by_network(self, context, router, net_id): router_dev_owner = self._get_device_owner(context, router) for port in router.attached_ports: @@ -962,7 +970,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, port=port, interface_info=interface_info) self._add_router_port( - context, port['id'], router.id, device_owner) + context, port['id'], router, device_owner) gw_ips = [] gw_network_id = None @@ -990,18 +998,58 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, subnets[-1]['id'], [subnet['id'] for subnet in subnets]) @db_api.retry_if_session_inactive() - def _add_router_port(self, context, port_id, router_id, device_owner): + def _add_router_port(self, context, port_id, router, device_owner): l3_obj.RouterPort( context, port_id=port_id, - router_id=router_id, + router_id=router.id, port_type=device_owner ).create() + + # NOTE(froyo): Check after create the RouterPort if we have generated + # an overlapping. Like creation of port is an ML2 plugin command it + # runs in an isolated transaction so we could not control there the + # addition of ports to different subnets that collides in cidrs. So + # make the check here an trigger the overlaping that will remove all + # created items. + router_ports = l3_obj.RouterPort.get_objects( + context, router_id=router.id) + + if len(router_ports) > 1: + subnets_id = [] + for rp in router_ports: + port = port_obj.Port.get_object(context.elevated(), + id=rp.port_id) + if port: + # Only allow one router port with IPv6 subnets per network + # id + self._validate_one_router_ipv6_port_per_network( + router, port) + subnets_id.extend([fixed_ip['subnet_id'] + for fixed_ip in port['fixed_ips']]) + else: + raise l3_exc.RouterInterfaceNotFound( + router_id=router.id, port_id=rp.port_id) + + if subnets_id: + id_filter = {'id': subnets_id} + subnets = self._core_plugin.get_subnets(context.elevated(), + filters=id_filter) + + # Ignore temporary Prefix Delegation CIDRs + subnets = [ + s + for s in subnets + if s["cidr"] != constants.PROVISIONAL_IPV6_PD_PREFIX + ] + for sub1, sub2 in itertools.combinations(subnets, 2): + self._raise_on_subnets_overlap(sub1, sub2) + # Update owner after actual process again in order to # make sure the records in routerports table and ports # table are consistent. self._core_plugin.update_port( - context, port_id, {'port': {'device_id': router_id, + context, port_id, {'port': {'device_id': router.id, 'device_owner': device_owner}}) def _check_router_interface_not_in_use(self, router_id, subnet): diff --git a/neutron/tests/fullstack/test_l3_agent.py b/neutron/tests/fullstack/test_l3_agent.py index 4fe977fb299..84603266dc5 100644 --- a/neutron/tests/fullstack/test_l3_agent.py +++ b/neutron/tests/fullstack/test_l3_agent.py @@ -18,6 +18,7 @@ import time import netaddr from neutron_lib import constants +from neutronclient.common import exceptions from oslo_utils import uuidutils from neutron.agent.l3 import ha_router @@ -266,6 +267,29 @@ class TestL3Agent(base.BaseFullStackTestCase): return (_is_filter_set(constants.INGRESS_DIRECTION) and _is_filter_set(constants.EGRESS_DIRECTION)) + def _test_concurrent_router_subnet_attachment_overlapping_cidr(self, + ha=False): + tenant_id = uuidutils.generate_uuid() + subnet_cidr = '10.100.0.0/24' + network1 = self.safe_client.create_network( + tenant_id, name='foo-network1') + subnet1 = self.safe_client.create_subnet( + tenant_id, network1['id'], subnet_cidr) + network2 = self.safe_client.create_network( + tenant_id, name='foo-network2') + subnet2 = self.safe_client.create_subnet( + tenant_id, network2['id'], subnet_cidr) + router = self.safe_client.create_router(tenant_id, ha=ha) + + funcs = [self.safe_client.add_router_interface, + self.safe_client.add_router_interface] + args = [(router['id'], subnet1['id']), (router['id'], subnet2['id'])] + self.assertRaises( + exceptions.BadRequest, + self._simulate_concurrent_requests_process_and_raise, + funcs, + args) + class TestLegacyL3Agent(TestL3Agent): @@ -417,6 +441,9 @@ class TestLegacyL3Agent(TestL3Agent): def test_router_fip_qos_after_admin_state_down_up(self): self._router_fip_qos_after_admin_state_down_up() + def test_concurrent_router_subnet_attachment_overlapping_cidr_(self): + self._test_concurrent_router_subnet_attachment_overlapping_cidr() + class TestHAL3Agent(TestL3Agent): @@ -573,3 +600,7 @@ class TestHAL3Agent(TestL3Agent): def test_router_fip_qos_after_admin_state_down_up(self): self._router_fip_qos_after_admin_state_down_up(ha=True) + + def test_concurrent_router_subnet_attachment_overlapping_cidr_(self): + self._test_concurrent_router_subnet_attachment_overlapping_cidr( + ha=True) diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 3201a06a2c4..d50179237df 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -514,6 +514,93 @@ class TestL3_NAT_dbonly_mixin( self.assertIsNone( self.db._validate_gw_info(mock.ANY, info, [], router)) + def test__raise_on_subnets_overlap_does_not_raise(self): + subnets = [ + {'id': uuidutils.generate_uuid(), + 'cidr': '10.1.0.0/24'}, + {'id': uuidutils.generate_uuid(), + 'cidr': '10.2.0.0/24'}] + self.db._raise_on_subnets_overlap(subnets[0], subnets[1]) + + def test__raise_on_subnets_overlap_raises(self): + subnets = [ + {'id': uuidutils.generate_uuid(), + 'cidr': '10.1.0.0/20'}, + {'id': uuidutils.generate_uuid(), + 'cidr': '10.1.10.0/24'}] + self.assertRaises( + n_exc.BadRequest, self.db._raise_on_subnets_overlap, subnets[0], + subnets[1]) + + def test__validate_one_router_ipv6_port_per_network(self): + port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '2001:db8::/32').ip + 1), + subnet_id='foo_subnet')]) + rports = [l3_models.RouterPort(router_id='foo_router', port=port)] + router = l3_models.Router( + id='foo_router', attached_ports=rports, route_list=[], + gw_port_id=None) + new_port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network2', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '2001:db8::/32').ip + 2), + subnet_id='foo_subnet')]) + self.db._validate_one_router_ipv6_port_per_network( + router, new_port) + + def test__validate_one_router_ipv6_port_per_network_mix_ipv4_ipv6(self): + port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '10.1.10.0/24').ip + 1), + subnet_id='foo_subnet')]) + rports = [l3_models.RouterPort(router_id='foo_router', port=port)] + router = l3_models.Router( + id='foo_router', attached_ports=rports, route_list=[], + gw_port_id=None) + new_port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '2001:db8::/32').ip + 2), + subnet_id='foo_subnet')]) + self.db._validate_one_router_ipv6_port_per_network( + router, new_port) + + def test__validate_one_router_ipv6_port_per_network_failed(self): + port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '2001:db8::/32').ip + 1), + subnet_id='foo_subnet')]) + rports = [l3_models.RouterPort(router_id='foo_router', port=port)] + router = l3_models.Router( + id='foo_router', attached_ports=rports, route_list=[], + gw_port_id=None) + new_port = models_v2.Port( + id=uuidutils.generate_uuid(), + network_id='foo_network', + fixed_ips=[models_v2.IPAllocation( + ip_address=str(netaddr.IPNetwork( + '2001:db8::/32').ip + 2), + subnet_id='foo_subnet')]) + self.assertRaises( + n_exc.BadRequest, + self.db._validate_one_router_ipv6_port_per_network, + router, + new_port) + class L3_NAT_db_mixin(base.BaseTestCase): def setUp(self): @@ -697,6 +784,56 @@ class L3TestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): mock_log.warning.not_called_once() self._check_routerports((True, False)) + @mock.patch.object(l3_db.L3_NAT_dbonly_mixin, + '_check_for_dup_router_subnets') + @mock.patch.object(l3_db.L3_NAT_dbonly_mixin, + '_raise_on_subnets_overlap') + def test_add_router_interface_by_port_overlap_detected( + self, mock_raise_on_subnets_overlap, mock_check_dup): + # NOTE(froyo): On a normal behaviour this overlapping would be detected + # by _check_for_dup_router_subnets, in order to evalue the code + # implemented to cover the race condition when two ports are added + # simultaneously using colliding cidrs we need to "fake" this method + # to overpass it and check we achieve the code part that cover the case + mock_check_dup.return_value = True + network2 = self.create_network('network2') + subnet = self.create_subnet(network2, '1.1.1.1', '1.1.1.0/24') + ipa = str(netaddr.IPNetwork(subnet['subnet']['cidr']).ip + 10) + fixed_ips = [{'subnet_id': subnet['subnet']['id'], 'ip_address': ipa}] + port = self.create_port( + network2['network']['id'], {'fixed_ips': fixed_ips}) + self.mixin.add_router_interface( + self.ctx, self.router['id'], + interface_info={'port_id': port['port']['id']}) + mock_raise_on_subnets_overlap.assert_not_called() + self.mixin.add_router_interface( + self.ctx, self.router['id'], + interface_info={'port_id': self.ports[0]['port']['id']}) + mock_raise_on_subnets_overlap.assert_called_once() + + @mock.patch.object(l3_db.L3_NAT_dbonly_mixin, + '_check_for_dup_router_subnets') + @mock.patch.object(l3_db.L3_NAT_dbonly_mixin, + '_raise_on_subnets_overlap') + def test_add_router_interface_by_subnet_overlap_detected( + self, mock_raise_on_subnets_overlap, mock_check_dup): + # NOTE(froyo): On a normal behaviour this overlapping would be detected + # by _check_for_dup_router_subnets, in order to evalue the code + # implemented to cover the race condition when two ports are added + # simultaneously using colliding cidrs we need to "fake" this method + # to overpass it and check we achieve the code part that cover the case + mock_check_dup.return_value = True + network2 = self.create_network('network2') + subnet = self.create_subnet(network2, '1.1.1.1', '1.1.1.0/24') + self.mixin.add_router_interface( + self.ctx, self.router['id'], + interface_info={'subnet_id': subnet['subnet']['id']}) + mock_raise_on_subnets_overlap.assert_not_called() + self.mixin.add_router_interface( + self.ctx, self.router['id'], + interface_info={'subnet_id': self.subnets[0]['subnet']['id']}) + mock_raise_on_subnets_overlap.assert_called_once() + @mock.patch.object(port_obj, 'LOG') def test_remove_router_interface_by_subnet_removed_rport(self, mock_log): self._add_router_interfaces()