From a1a03385c5f6b87e66b8f5df2711fd0976545bca Mon Sep 17 00:00:00 2001 From: Yang JianFeng Date: Sun, 8 Nov 2020 07:06:10 +0000 Subject: [PATCH] Improve the CIDRs overlap check method for router add interface If an external network have multiple subnets, the router whose external gateway at the network will have all routes about those subnets. So, when a internal subnet was added to the router, all subnets of the the network which the router's external gateway belong to need to be joined to check whether the CIDRs is overlapped. Also, this patch revert the patch [1] [1] https://review.opendev.org/#/c/473356/ Change-Id: Id5d8ac09a38c656619f88a6f87b8f384fe4c55a8 Closes-Bug: #1903433 Depends-On: https://review.opendev.org/763621 Depends-On: https://review.opendev.org/763626 (cherry picked from commit 8ebd54d1bcb6d4437cf9501f1c5993177747fac2) --- neutron/db/l3_db.py | 15 +++-- neutron/tests/unit/extensions/test_l3.py | 79 +++++++++++++++++++----- 2 files changed, 73 insertions(+), 21 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 351d3081950..86839fb499c 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -508,8 +508,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, self._check_for_dup_router_subnets(context, router, new_network_id, - subnets, - include_gateway=True) + subnets) self._create_router_gw_port(context, router, new_network_id, ext_ips) registry.notify(resources.ROUTER_GATEWAY, @@ -630,8 +629,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, filters=filters) def _check_for_dup_router_subnets(self, context, router, - network_id, new_subnets, - include_gateway=False): + network_id, new_subnets): # It's possible these ports are on the same network, but # different subnets. new_subnet_ids = {s['id'] for s in new_subnets} @@ -642,8 +640,13 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, msg = (_("Router already has a port on subnet %s") % ip['subnet_id']) raise n_exc.BadRequest(resource='router', msg=msg) - gw_owner = (p.get('device_owner') == DEVICE_OWNER_ROUTER_GW) - if include_gateway == gw_owner: + if p.get('device_owner') == DEVICE_OWNER_ROUTER_GW: + ext_subts = self._core_plugin.get_subnets( + context.elevated(), + filters={'network_id': p['network_id']}) + for sub in ext_subts: + router_subnets.append(sub['id']) + else: router_subnets.append(ip['subnet_id']) # Ignore temporary Prefix Delegation CIDRs diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 80706037cd7..5e429c3e0c9 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -1681,18 +1681,61 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): HTTPBadRequest.code) def test_router_add_interface_cidr_overlapped_with_gateway(self): - with self.router() as r: + with self.router() as r, self.network() as ext_net: with self.subnet(cidr='10.0.1.0/24') as s1, self.subnet( - cidr='10.0.0.0/16') as s2: - self._set_net_external(s2['subnet']['network_id']) + network=ext_net, cidr='10.0.0.0/16') as s2: + ext_net_id = ext_net['network']['id'] + self._set_net_external(ext_net_id) self._add_external_gateway_to_router( - r['router']['id'], - s2['subnet']['network_id']) - res = self._router_interface_action('add', - r['router']['id'], - s1['subnet']['id'], - None) - self.assertIn('port_id', res) + r['router']['id'], ext_net_id) + res = self._router_interface_action( + 'add', r['router']['id'], s1['subnet']['id'], None, + expected_code=exc.HTTPBadRequest.code) + expected_msg = ("Bad router request: Cidr 10.0.1.0/24 of " + "subnet %(internal_subnet_id)s overlaps with " + "cidr 10.0.0.0/16 of subnet " + "%(external_subnet_id)s.") % { + "external_subnet_id": s2['subnet']['id'], + "internal_subnet_id": s1['subnet']['id']} + self.assertEqual(expected_msg, res['NeutronError']['message']) + + # External network have multiple subnets. + with self.subnet(network=ext_net, + cidr='192.168.1.0/24') as s3, \ + self.subnet(cidr='192.168.1.0/24') as s4: + res = self._router_interface_action( + 'add', r['router']['id'], s4['subnet']['id'], None, + expected_code=exc.HTTPBadRequest.code) + expected_msg = ( + "Bad router request: Cidr 192.168.1.0/24 of subnet " + "%(internal_subnet_id)s overlaps with cidr " + "192.168.1.0/24 of subnet %(external_subnet_id)s.") % { + "external_subnet_id": s3['subnet']['id'], + "internal_subnet_id": s4['subnet']['id']} + self.assertEqual(expected_msg, + res['NeutronError']['message']) + + def test_router_set_gateway_cidr_overlapped_with_subnets(self): + with self.router() as r, self.network() as ext_net: + with self.subnet(network=ext_net, cidr='10.0.1.0/24') as s1, \ + self.subnet(network=ext_net, cidr='10.0.2.0/24') as s2, \ + self.subnet(cidr='10.0.2.0/24') as s3: + ext_net_id = ext_net['network']['id'] + self._set_net_external(ext_net_id) + self._router_interface_action( + 'add', r['router']['id'], + s3['subnet']['id'], None) + res = self._add_external_gateway_to_router( + r['router']['id'], ext_net_id, + ext_ips=[{'subnet_id': s1['subnet']['id']}], + expected_code=exc.HTTPBadRequest.code) + expected_msg = ( + "Bad router request: Cidr 10.0.2.0/24 of subnet " + "%(external_subnet_id)s overlaps with cidr 10.0.2.0/24 of " + "subnet %(internal_subnet_id)s.") % { + "external_subnet_id": s2["subnet"]["id"], + "internal_subnet_id": s3["subnet"]["id"]} + self.assertEqual(expected_msg, res['NeutronError']['message']) def test_router_add_interface_by_port_cidr_overlapped_with_gateway(self): with self.router() as r: @@ -1704,11 +1747,17 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): r['router']['id'], s2['subnet']['network_id']) - res = self._router_interface_action('add', - r['router']['id'], - None, - p['port']['id']) - self.assertIn('port_id', res) + res = self._router_interface_action( + 'add', r['router']['id'], None, p['port']['id'], + expected_code=exc.HTTPBadRequest.code) + expected_msg = ( + "Bad router request: Cidr 10.0.1.0/24 of subnet " + "%(internal_subnet_id)s overlaps with cidr " + "10.0.0.0/16 of subnet %(external_subnet_id)s.") % { + "external_subnet_id": s2['subnet']['id'], + "internal_subnet_id": s1['subnet']['id']} + self.assertEqual(expected_msg, + res['NeutronError']['message']) def test_router_add_gateway_dup_subnet1_returns_400(self): with self.router() as r: