Merge "Check subnet overlapping after add router interface" into stable/yoga

Author: Zuul

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()