From b79000c34c1e9b2d7446ae41d775fad7459fc0cf 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 f32aab414f0..40401a54bc5 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -488,8 +488,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) @@ -613,8 +612,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} @@ -625,8 +623,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 10f9190090c..11e096c3a28 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -1753,18 +1753,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: @@ -1776,11 +1819,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: