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.

Conflicts:
      neutron/db/l3_db.py
(manually cherry picked from commit 1abb77d)

Closes-Bug: #1987666
Change-Id: I7cec8b53e72e7abf34012906e6adfecf079525af
(cherry picked from commit 1abb77d)

Author: fernandoroyosanchez

neutron/db/l3_db.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 import functools
+import itertools
 import random
 
 import netaddr
@@ -563,6 +564,23 @@ def _check_for_external_ip_change(self, context, gw_port, ext_ips):
         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.
@@ -669,22 +687,8 @@ def _check_for_dup_router_subnets(self, context, router,
         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."""
@@ -765,17 +769,7 @@ def _validate_router_port_info(self, context, router, port_id):
             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 = []
@@ -841,6 +835,20 @@ def _port_has_ipv6_address(self, port):
             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:
@@ -959,7 +967,7 @@ def add_router_interface(self, context, router_id, interface_info=None):
                                                  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
@@ -987,18 +995,58 @@ def add_router_interface(self, context, router_id, interface_info=None):
             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):

neutron/tests/fullstack/test_l3_agent.py

@@ -18,6 +18,7 @@
 
 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 @@ def _is_filter_set(direction):
         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 @@ def test_external_subnet_changed(self):
     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 @@ def test_external_subnet_changed(self):
 
     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)

neutron/tests/unit/db/test_l3_db.py

@@ -514,6 +514,93 @@ def test__validate_gw_info_delete_gateway_no_route(self):
             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 @@ def test_remove_router_interface_by_subnet(self, mock_log):
         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()