From fdc9127047b9993a1d4f64eda62f782968df9bc0 Mon Sep 17 00:00:00 2001 From: Sneha Maniraju Date: Tue, 14 Sep 2021 13:21:27 -0700 Subject: [PATCH] Validate network before creating or updating router Change-Id: I6d55cdf630b325995ea8ba4f9edea4d63907c84f (cherry picked from commit eab24a00227342d85206eba344a65acef59abcdf) --- .../neutron/services/apic_aim/l3_plugin.py | 136 +++++++++--------- .../unit/plugins/ml2plus/test_apic_aim.py | 53 ++++++- 2 files changed, 118 insertions(+), 71 deletions(-) diff --git a/gbpservice/neutron/services/apic_aim/l3_plugin.py b/gbpservice/neutron/services/apic_aim/l3_plugin.py index a1cb03b53..4f185a821 100644 --- a/gbpservice/neutron/services/apic_aim/l3_plugin.py +++ b/gbpservice/neutron/services/apic_aim/l3_plugin.py @@ -23,7 +23,6 @@ from neutron.db import dns_db from neutron.db import extraroute_db from neutron.db import l3_gwmode_db from neutron.db.models import l3 as l3_db -from neutron.db import models_v2 from neutron.quota import resource_registry from neutron_lib.api.definitions import l3 as l3_def from neutron_lib.api.definitions import portbindings @@ -140,6 +139,22 @@ class ApicL3Plugin(common_db_mixin.CommonDbMixin, results, None) return results + def validate_snat_extension(self, context, gw_info): + for fixed in gw_info['external_fixed_ips']: + sub = None + try: + sub = self._core_plugin.get_subnet(context, + fixed['subnet_id']) + except exceptions.SubnetNotFound: + pass + + if (sub and sub['network_id'] != gw_info['network_id']): + raise exceptions.SubnetNotFound(subnet_id=fixed['subnet_id']) + + # subnet should be used for snat ip only + if (sub and sub['apic:snat_subnet_only'] is True): + raise (aim_exceptions.SnatPoolCannotBeUsedForGatewayIp()) + # Overwrite the upstream implementation to take advantage # of the bulk extension support. @db_api.retry_if_session_inactive() @@ -158,42 +173,61 @@ class ApicL3Plugin(common_db_mixin.CommonDbMixin, page_reverse=page_reverse) return self._make_routers_dict(routers_db, fields) + def _check_snat_subnets(self, context, gw_info, router, method, *args): + result = None + self._core_plugin._get_network(context, gw_info['network_id']) + if ('external_fixed_ips' in gw_info): + # Check to make sure they haven't picked a subnet + # that has an extension set to disallow it. + self.validate_snat_extension(context, gw_info) + else: + # No subnet set, so check to see if should be allocating one + # from non-SNAT subnets. + subnets_in_nw = self._core_plugin.get_subnets_by_network(context, + gw_info['network_id']) + for ext_sn in subnets_in_nw: + if (ext_sn['apic:snat_subnet_only'] is False): + fixed_ips = [{'subnet_id': ext_sn['id']}] + gw_info.update({'external_fixed_ips': fixed_ips}) + router['router'].update({EXTERNAL_GW_INFO: gw_info}) + try: + result = method(*args) + break + except exceptions.IpAddressGenerationFailure: + LOG.info('No more IP addresses available ' + 'in subnet %s for gateway', ext_sn) + return result + # REVISIT: Eliminate this wrapper? @n_utils.transaction_guard def create_router(self, context, router): + method = super(ApicL3Plugin, self).create_router + args = (context, router) LOG.debug("APIC AIM L3 Plugin creating router: %s", router) # REVISIT: Do this in MD by registering for # ROUTER.BEFORE_CREATE event. self._md.ensure_tenant(context, router['router']['tenant_id']) + result = None current = router['router'] gw_info = (current[EXTERNAL_GW_INFO] if EXTERNAL_GW_INFO in current else lib_constants.ATTR_NOT_SPECIFIED) - if (gw_info and gw_info != lib_constants.ATTR_NOT_SPECIFIED): - if 'external_fixed_ips' in gw_info and 'network_id' in gw_info: - for fixed in gw_info['external_fixed_ips']: - # query for the subnet on the network - query = model_query.get_collection_query(context, - models_v2.Subnet) - query = query.filter(models_v2.Subnet.network_id == - gw_info['network_id']) - query = query.filter(models_v2.Subnet.id == - fixed['subnet_id']) - sub = query.one_or_none() - # subnet should be used for snat ip only - if (sub and - sub['aim_extension_mapping']['snat_subnet_only'] is - True): - raise (aim_exceptions. - SnatPoolCannotBeUsedForGatewayIp()) - return super(ApicL3Plugin, self).create_router(context, router) + if (gw_info and gw_info != lib_constants.ATTR_NOT_SPECIFIED and + 'network_id' in gw_info): + result = self._check_snat_subnets(context, gw_info, router, + method, *args) + if result: + return result + else: + return method(context, router) # REVISIT: Eliminate this wrapper? @n_utils.transaction_guard def update_router(self, context, id, router): + method = super(ApicL3Plugin, self).update_router + args = (context, id, router) original = self.get_router(context, id) current = router['router'] - original_gw_info = (original[EXTERNAL_GW_INFO] if EXTERNAL_GW_INFO in original else lib_constants.ATTR_NOT_SPECIFIED) @@ -206,62 +240,26 @@ class ApicL3Plugin(common_db_mixin.CommonDbMixin, # User is deleting the gateway info if (not current_gw_info and original_gw_info): - return super(ApicL3Plugin, self).update_router(context, id, router) + return method(context, id, router) # User is updating info other than gateway info if (current_gw_info == original_gw_info): - return super(ApicL3Plugin, self).update_router(context, id, router) + return method(context, id, router) # No gw info yet / gw info is being updated + result = None if ((current_gw_info and - current_gw_info != lib_constants.ATTR_NOT_SPECIFIED) and + current_gw_info != lib_constants.ATTR_NOT_SPECIFIED) and (not original_gw_info or - original_gw_info == lib_constants.ATTR_NOT_SPECIFIED)): - if 'network_id' in current_gw_info: - # query for all the subnets on the network - query = model_query.get_collection_query(context, - models_v2.Subnet) - query = query.filter(models_v2.Subnet.network_id == - current_gw_info['network_id']) - subnets_in_nw = query.all() - - if ('external_fixed_ips' in current_gw_info): - for fixed in current_gw_info['external_fixed_ips']: - query = query.filter(models_v2.Subnet.id == - fixed['subnet_id']) - sn = query.one_or_none() - if (sn and - sn['aim_extension_mapping']['snat_subnet_only'] - is True): - raise (aim_exceptions. - SnatPoolCannotBeUsedForGatewayIp()) - return (super(ApicL3Plugin, self).update_router( - context, id, router)) - else: - # find a subnet in the network and set as fixed_ip - result = None - for ext_sn in subnets_in_nw: - if (ext_sn['aim_extension_mapping']['snat_subnet_only'] - is False): - fixed_ips = [{'subnet_id': ext_sn['id']}] - current_gw_info.update( - {'external_fixed_ips': fixed_ips}) - router['router'].update( - {EXTERNAL_GW_INFO: current_gw_info}) - try: - result = (super( - ApicL3Plugin, self).update_router - (context, id, router)) - break - except exceptions.IpAddressGenerationFailure: - LOG.info('No more IP addresses available ' - 'in subnet %s for gateway', ext_sn) - if not result: - raise exceptions.IpAddressGenerationFailure( - net_id=current_gw_info['network_id']) - else: - return result - return super(ApicL3Plugin, self).update_router(context, id, router) + original_gw_info == lib_constants.ATTR_NOT_SPECIFIED or + original_gw_info != current_gw_info) and ('network_id' in + current_gw_info)): + result = self._check_snat_subnets(context, current_gw_info, + router, method, *args) + if result: + return result + else: + return method(context, id, router) # REVISIT: Eliminate this wrapper? @n_utils.transaction_guard diff --git a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py index 51915a820..7533ec727 100644 --- a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py +++ b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py @@ -47,6 +47,7 @@ from neutron.tests.unit import testlib_api from neutron_lib.api.definitions import portbindings from neutron_lib import constants as n_constants from neutron_lib import context as n_context +from neutron_lib import exceptions as n_exceptions from neutron_lib.plugins import constants as pconst from neutron_lib.plugins import directory from opflexagent import constants as ofcst @@ -12109,7 +12110,6 @@ class TestUpdateRouterSubnet(ApicAimTestCase): def test_update_gateway_no_extension(self): # create an external network ext_net = self._make_ext_network('ext-net1') - # create a snat subnet on the network without new extension snat_sub = self._make_subnet( self.fmt, {'network': ext_net}, '200.100.100.1', @@ -12121,7 +12121,6 @@ class TestUpdateRouterSubnet(ApicAimTestCase): # create a floating IP subnet on the same network too self._make_subnet(self.fmt, {'network': ext_net}, '250.100.100.1', '250.100.100.0/29')['subnet'] - # create a router and update the router gateway without fixed subnet router = self._make_router( self.fmt, self._tenant_id, 'router1')['router'] @@ -12129,6 +12128,7 @@ class TestUpdateRouterSubnet(ApicAimTestCase): data = {'router': {'external_gateway_info': {'network_id': ext_net['id']} }} + router = self._update('routers', router_id, data)['router'] self.assertIsNotNone(netaddr.IPAddress(router['external_gateway_info'] @@ -12271,3 +12271,52 @@ class TestUpdateRouterSubnet(ApicAimTestCase): data = {'router': {'external_gateway_info': {}}} router = self._update('routers', router_id, data)['router'] self.assertIsNone(router['external_gateway_info']) + + def test_router_add_gateway_invalid_network(self): + # Test adding gateway with invalid network for router + ext_net = self._make_ext_network('ext-net1') + + # create a snat subnet on the network + self._make_subnet(self.fmt, {'network': ext_net}, '200.100.100.1', + '200.100.100.0/29')['subnet'] + + router = self._make_router( + self.fmt, self._tenant_id, 'router1')['router'] + + data = {'router': {'external_gateway_info': + {'network_id': router['id'], + } + } + } + self.assertRaises(n_exceptions.NetworkNotFound, + self.l3_plugin.update_router, + n_context.get_admin_context(), + router['id'], + data) + + def test_create_router_netid_only(self): + ext_net = self._make_ext_network('ext-net1') + # create a snat subnet on the network + snat_sub = self._make_subnet( + self.fmt, {'network': ext_net}, '200.100.100.1', + '200.100.100.0/29')['subnet'] + + self._update('subnets', snat_sub['id'], + {'subnet': {SNAT_POOL: True}}) + + self._update('subnets', snat_sub['id'], + {'subnet': {SNAT_SUBNET_ONLY: True}}) + + # create a floating IP subnet on the same network too + fip_sub = self._make_subnet( + self.fmt, {'network': ext_net}, '250.100.100.1', + '250.100.100.0/29')['subnet'] + # create a router with fixed subnet + router = self._make_router( + self.fmt, self._tenant_id, 'router1', + external_gateway_info={'network_id': ext_net['id']} + )['router'] + + self._check_ip_in_cidr(router + ['external_gateway_info']['external_fixed_ips'][0]['ip_address'], + fip_sub['cidr'])