From 32182010c2c2050f37221824788a91d762081a0f Mon Sep 17 00:00:00 2001 From: Ryan Tidwell Date: Wed, 26 Jun 2019 00:29:00 -0500 Subject: [PATCH] Relax subnet pool network affinity constraints This change relaxes the constraint that all subnets of the same address family on a network must be allocated from the same subnet pool. It allows subnets that share the same address scope to co-exist on a network. If there is no address scope involved the subnet pool / network affinity constraints continue to enforced as done previously. Change-Id: I33bd17c723b3e8d409415bda008440f8ed9cfa68 Closes: 1830240 Implements: subnets-different-pools-same-net --- neutron/db/db_base_plugin_v2.py | 44 ++++- neutron/db/ipam_backend_mixin.py | 54 +++++- neutron/objects/address_scope.py | 18 ++ .../tests/unit/db/test_db_base_plugin_v2.py | 5 +- .../tests/unit/db/test_ipam_backend_mixin.py | 29 +++ .../unit/extensions/test_address_scope.py | 173 ++++++++++++++++++ ...ool-network-affinity-837c1fc28f835de5.yaml | 12 ++ 7 files changed, 325 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/relax-subnetpool-network-affinity-837c1fc28f835de5.yaml diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 515a9f2e7da..b93c3b08e7c 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -32,6 +32,7 @@ from neutron_lib.db import model_query from neutron_lib.db import resource_extend from neutron_lib.db import utils as ndb_utils from neutron_lib import exceptions as exc +from neutron_lib.exceptions import address_scope as addr_scope_exc from neutron_lib.exceptions import l3 as l3_exc from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory @@ -1131,6 +1132,9 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, subnetpool_id=subnetpool_id, address_scope_id=address_scope_id, ip_version=as_ip_version) + self._check_subnetpool_address_scope_network_affinity( + context, subnetpool_id, ip_version) + subnetpools = subnetpool_obj.SubnetPool.get_objects( context, address_scope_id=address_scope_id) @@ -1142,6 +1146,44 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, if sp_set.intersection(new_set): raise exc.AddressScopePrefixConflict() + def _check_subnetpool_address_scope_network_affinity(self, context, + subnetpool_id, + ip_version): + """Check whether updating a subnet pool's address scope is allowed. + + - Identify the subnets that would be re-scoped + - Identify the networks that would be affected by re-scoping + - Find all subnets associated with the affected networks + - Perform set difference (all - to_be_rescoped) + - If the set difference yields non-zero result size, re-scoping the + subnet pool will leave subnets in different address scopes and result + in address scope / network affinity violations so raise an exception to + block the operation. + """ + + # TODO(tidwellr) potentially lots of subnets here, optimize this code + subnets_to_rescope = self._get_subnets_by_subnetpool(context, + subnetpool_id) + rescoped_subnet_ids = set() + affected_source_network_ids = set() + for subnet in subnets_to_rescope: + rescoped_subnet_ids.add(subnet.id) + affected_source_network_ids.add(subnet.network_id) + + all_network_subnets = subnet_obj.Subnet.get_objects( + context, + network_id=affected_source_network_ids, + ip_version=ip_version) + all_affected_subnet_ids = set( + [subnet.id for subnet in all_network_subnets]) + + # Use set difference to identify the subnets that would be + # violating address scope affinity constraints if the subnet + # pool's address scope was changed. + violations = all_affected_subnet_ids.difference(rescoped_subnet_ids) + if violations: + raise addr_scope_exc.NetworkAddressScopeAffinityError() + def _check_subnetpool_update_allowed(self, context, subnetpool_id, address_scope_id): """Check if the subnetpool can be updated or not. @@ -1178,7 +1220,7 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, self._check_default_subnetpool_exists(context, sp_reader.ip_version) self._validate_address_scope_id(context, sp_reader.address_scope_id, - id, sp_reader.prefixes, + sp_reader.id, sp_reader.prefixes, sp_reader.ip_version) pool_args = {'project_id': sp['tenant_id'], 'id': sp_reader.id, diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py index 2ac050ea97d..15656d1880c 100644 --- a/neutron/db/ipam_backend_mixin.py +++ b/neutron/db/ipam_backend_mixin.py @@ -25,6 +25,7 @@ from neutron_lib import constants as const from neutron_lib.db import api as db_api from neutron_lib.db import utils as db_utils from neutron_lib import exceptions as exc +from neutron_lib.exceptions import address_scope as addr_scope_exc from neutron_lib.utils import net as net_utils from oslo_config import cfg from oslo_log import log as logging @@ -37,6 +38,7 @@ from neutron.db import models_v2 from neutron.extensions import segment from neutron.ipam import exceptions as ipam_exceptions from neutron.ipam import utils as ipam_utils +from neutron.objects import address_scope as addr_scope_obj from neutron.objects import network as network_obj from neutron.objects import subnet as subnet_obj from neutron.services.segments import exceptions as segment_exc @@ -252,15 +254,43 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): 'cidr': subnet.cidr}) raise exc.InvalidInput(error_message=err_msg) - def _validate_network_subnetpools(self, network, - new_subnetpool_id, ip_version): + def _validate_network_subnetpools(self, network, subnet_ip_version, + new_subnetpool, network_scope): """Validate all subnets on the given network have been allocated from - the same subnet pool as new_subnetpool_id + the same subnet pool as new_subnetpool if no address scope is + used. If address scopes are used, validate that all subnets on the + given network participate in the same address scope. """ + # 'new_subnetpool' might just be the Prefix Delegation ID + ipv6_pd_subnetpool = new_subnetpool == const.IPV6_PD_POOL_ID + + # Check address scope affinities + if network_scope: + if (ipv6_pd_subnetpool or + new_subnetpool and + new_subnetpool.address_scope_id != network_scope.id): + raise addr_scope_exc.NetworkAddressScopeAffinityError() + + # Checks for situations where address scopes aren't involved for subnet in network.subnets: - if (subnet.ip_version == ip_version and - new_subnetpool_id != subnet.subnetpool_id): - raise exc.NetworkSubnetPoolAffinityError() + if ipv6_pd_subnetpool: + # Check the prefix delegation case. Since there is no + # subnetpool object, we just check against the PD ID. + if (subnet.ip_version == const.IP_VERSION_6 and + subnet.subnetpool_id != const.IPV6_PD_POOL_ID): + raise exc.NetworkSubnetPoolAffinityError() + else: + if new_subnetpool: + # In this case we have the new subnetpool object, so + # we can check the ID and IP version. + if (subnet.subnetpool_id != new_subnetpool.id and + subnet.ip_version == new_subnetpool.ip_version and + not network_scope): + raise exc.NetworkSubnetPoolAffinityError() + else: + if (subnet.subnetpool_id and + subnet.ip_version == subnet_ip_version): + raise exc.NetworkSubnetPoolAffinityError() def validate_allocation_pools(self, ip_pools, subnet_cidr): """Validate IP allocation pools. @@ -517,10 +547,18 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): dns_nameservers, host_routes, subnet_request): + network_scope = addr_scope_obj.AddressScope.get_network_address_scope( + context, network.id, subnet_args['ip_version']) + # 'subnetpool' is not necessarily an object + subnetpool = subnet_args.get('subnetpool_id') + if subnetpool and subnetpool != const.IPV6_PD_POOL_ID: + subnetpool = self._get_subnetpool(context, subnetpool) + self._validate_subnet_cidr(context, network, subnet_args['cidr']) self._validate_network_subnetpools(network, - subnet_args['subnetpool_id'], - subnet_args['ip_version']) + subnet_args['ip_version'], + subnetpool, + network_scope) service_types = subnet_args.pop('service_types', []) diff --git a/neutron/objects/address_scope.py b/neutron/objects/address_scope.py index a7a8ad6ece1..0ab12b05ef3 100644 --- a/neutron/objects/address_scope.py +++ b/neutron/objects/address_scope.py @@ -15,6 +15,7 @@ from oslo_versionedobjects import fields as obj_fields from neutron.db.models import address_scope as models +from neutron.db import models_v2 from neutron.objects import base from neutron.objects import common_types @@ -33,3 +34,20 @@ class AddressScope(base.NeutronDbObject): 'shared': obj_fields.BooleanField(), 'ip_version': common_types.IPVersionEnumField(), } + + @classmethod + def get_network_address_scope(cls, context, network_id, ip_version): + query = context.session.query(cls.db_model) + query = query.join( + models_v2.SubnetPool, + models_v2.SubnetPool.address_scope_id == cls.db_model.id) + query = query.filter( + cls.db_model.ip_version == ip_version, + models_v2.Subnet.subnetpool_id == models_v2.SubnetPool.id, + models_v2.Subnet.network_id == network_id) + scope_model_obj = query.one_or_none() + + if scope_model_obj: + return cls._load_object(context, scope_model_obj) + + return None diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index e4b81b82d6a..47ff036a4e4 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -6868,7 +6868,10 @@ class NeutronDbPluginV2AsMixinTestCase(NeutronDbPluginV2TestCase, new_subnetpool_id = None self.assertRaises(lib_exc.NetworkSubnetPoolAffinityError, self.plugin.ipam._validate_network_subnetpools, - network, new_subnetpool_id, 4) + network, + constants.IP_VERSION_4, + new_subnetpool_id, + None) class TestNetworks(testlib_api.SqlTestCase): diff --git a/neutron/tests/unit/db/test_ipam_backend_mixin.py b/neutron/tests/unit/db/test_ipam_backend_mixin.py index e0d24ae469c..804f6657219 100644 --- a/neutron/tests/unit/db/test_ipam_backend_mixin.py +++ b/neutron/tests/unit/db/test_ipam_backend_mixin.py @@ -17,6 +17,8 @@ import mock import netaddr from neutron_lib.api.definitions import portbindings from neutron_lib import constants +from neutron_lib import exceptions as exc +from neutron_lib.exceptions import address_scope as addr_scope_exc from oslo_utils import uuidutils import webob.exc @@ -299,6 +301,33 @@ class TestIpamBackendMixin(base.BaseTestCase): self.assertFalse(result) self.assertTrue(self.mixin._get_subnet_object.called) + def test__validate_network_subnetpools_mismatch_address_scopes(self): + address_scope_id = "dummy-scope" + subnetpool = mock.MagicMock() + address_scope = mock.MagicMock() + subnetpool.address_scope.return_value = address_scope_id + address_scope.id.return_value = address_scope_id + self.assertRaises(addr_scope_exc.NetworkAddressScopeAffinityError, + self.mixin._validate_network_subnetpools, + mock.MagicMock(), + constants.IP_VERSION_4, + subnetpool, + address_scope) + + def test__validate_network_subnetpools_subnetpool_mismatch(self): + subnet = mock.MagicMock(ip_version=constants.IP_VERSION_4) + subnet.subnetpool_id = 'fake-subnetpool' + network = mock.MagicMock(subnets=[subnet]) + subnetpool = mock.MagicMock(id=uuidutils.generate_uuid()) + subnetpool.ip_version = constants.IP_VERSION_4 + + self.assertRaises(exc.NetworkSubnetPoolAffinityError, + self.mixin._validate_network_subnetpools, + network, + constants.IP_VERSION_4, + subnetpool, + None) + class TestPlugin(db_base_plugin_v2.NeutronDbPluginV2, portbindings_db.PortBindingMixin): diff --git a/neutron/tests/unit/extensions/test_address_scope.py b/neutron/tests/unit/extensions/test_address_scope.py index 3c9152cf9f1..d98a26ecc31 100644 --- a/neutron/tests/unit/extensions/test_address_scope.py +++ b/neutron/tests/unit/extensions/test_address_scope.py @@ -22,6 +22,7 @@ from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources from neutron_lib import constants from neutron_lib import context +from neutron_lib.plugins import directory import webob.exc from neutron.db import address_scope_db @@ -516,3 +517,175 @@ class TestSubnetPoolsWithAddressScopes(AddressScopeTestCase): res = req.get_response(api) self.assertEqual(webob.exc.HTTPBadRequest.code, res.status_int) + + def test_create_two_subnets_different_subnetpools_same_network(self): + with self.address_scope(constants.IP_VERSION_4, + name='foo-address-scope') as addr_scope: + addr_scope = addr_scope['address_scope'] + with self.subnetpool( + ['10.10.0.0/16'], + name='subnetpool_a', + tenant_id=addr_scope['tenant_id'], + default_prefixlen=24, + address_scope_id=addr_scope['id']) as subnetpool_a,\ + self.subnetpool( + ['10.20.0.0/16'], + name='subnetpool_b', + tenant_id=addr_scope['tenant_id'], + default_prefixlen=24, + address_scope_id=addr_scope['id']) as subnetpool_b: + subnetpool_a = subnetpool_a['subnetpool'] + subnetpool_b = subnetpool_b['subnetpool'] + + with self.network( + tenant_id=addr_scope['tenant_id']) as network: + subnet_a = self._make_subnet( + self.fmt, + network, + constants.ATTR_NOT_SPECIFIED, + None, + subnetpool_id=subnetpool_a['id'], + ip_version=constants.IP_VERSION_4, + tenant_id=addr_scope['tenant_id']) + subnet_b = self._make_subnet( + self.fmt, + network, + constants.ATTR_NOT_SPECIFIED, + None, + subnetpool_id=subnetpool_b['id'], + ip_version=constants.IP_VERSION_4, + tenant_id=addr_scope['tenant_id']) + + # Look up subnet counts and perform assertions + ctx = context.Context('', addr_scope['tenant_id']) + pl = directory.get_plugin() + total_count = pl.get_subnets_count( + ctx, + filters={'network_id': + [network['network']['id']]}) + subnets_pool_a_count = pl.get_subnets_count( + ctx, + filters={'id': [subnet_a['subnet']['id']], + 'subnetpool_id': [subnetpool_a['id']], + 'network_id': [network['network']['id']]}) + subnets_pool_b_count = pl.get_subnets_count( + ctx, + filters={'id': [subnet_b['subnet']['id']], + 'subnetpool_id': [subnetpool_b['id']], + 'network_id': [network['network']['id']]}) + self.assertEqual(2, total_count) + self.assertEqual(1, subnets_pool_a_count) + self.assertEqual(1, subnets_pool_b_count) + + def test_block_update_subnetpool_network_affinity(self): + with self.address_scope(constants.IP_VERSION_4, + name='scope-a') as scope_a,\ + self.address_scope(constants.IP_VERSION_4, + name='scope-b') as scope_b: + scope_a = scope_a['address_scope'] + scope_b = scope_b['address_scope'] + + with self.subnetpool( + ['10.10.0.0/16'], + name='subnetpool_a', + tenant_id=scope_a['tenant_id'], + default_prefixlen=24, + address_scope_id=scope_a['id']) as subnetpool_a,\ + self.subnetpool( + ['10.20.0.0/16'], + name='subnetpool_b', + tenant_id=scope_a['tenant_id'], + default_prefixlen=24, + address_scope_id=scope_a['id']) as subnetpool_b: + subnetpool_a = subnetpool_a['subnetpool'] + subnetpool_b = subnetpool_b['subnetpool'] + + with self.network( + tenant_id=scope_a['tenant_id']) as network: + self._make_subnet( + self.fmt, + network, + constants.ATTR_NOT_SPECIFIED, + None, + subnetpool_id=subnetpool_a['id'], + ip_version=constants.IP_VERSION_4, + tenant_id=scope_a['tenant_id']) + self._make_subnet( + self.fmt, + network, + constants.ATTR_NOT_SPECIFIED, + None, + subnetpool_id=subnetpool_b['id'], + ip_version=constants.IP_VERSION_4, + tenant_id=scope_a['tenant_id']) + + # Attempt to update subnetpool_b's address scope and + # assert failure. + data = {'subnetpool': {'address_scope_id': + scope_b['id']}} + req = self.new_update_request('subnetpools', data, + subnetpool_b['id']) + api = self._api_for_resource('subnetpools') + res = req.get_response(api) + self.assertEqual(webob.exc.HTTPBadRequest.code, + res.status_int) + + def test_ipv6_pd_add_non_pd_subnet_to_same_network(self): + with self.address_scope(constants.IP_VERSION_6, + name='foo-address-scope') as addr_scope: + addr_scope = addr_scope['address_scope'] + with self.subnetpool( + ['2001:db8:1234::/48'], + name='non_pd_pool', + tenant_id=addr_scope['tenant_id'], + default_prefixlen=64, + address_scope_id=addr_scope['id']) as non_pd_pool: + non_pd_pool = non_pd_pool['subnetpool'] + + with self.network( + tenant_id=addr_scope['tenant_id']) as network: + with self.subnet(cidr=None, + network=network, + ip_version=constants.IP_VERSION_6, + subnetpool_id=constants.IPV6_PD_POOL_ID, + ipv6_ra_mode=constants.IPV6_SLAAC, + ipv6_address_mode=constants.IPV6_SLAAC): + res = self._create_subnet( + self.fmt, + cidr=None, + net_id=network['network']['id'], + subnetpool_id=non_pd_pool['id'], + tenant_id=addr_scope['tenant_id'], + ip_version=constants.IP_VERSION_6) + self.assertEqual(webob.exc.HTTPBadRequest.code, + res.status_int) + + def test_ipv6_non_pd_add_pd_subnet_to_same_network(self): + with self.address_scope(constants.IP_VERSION_6, + name='foo-address-scope') as addr_scope: + addr_scope = addr_scope['address_scope'] + with self.subnetpool( + ['2001:db8:1234::/48'], + name='non_pd_pool', + tenant_id=addr_scope['tenant_id'], + default_prefixlen=64, + address_scope_id=addr_scope['id']) as non_pd_pool: + non_pd_pool = non_pd_pool['subnetpool'] + + with self.network( + tenant_id=addr_scope['tenant_id']) as network: + with self.subnet(cidr=None, + network=network, + ip_version=constants.IP_VERSION_6, + subnetpool_id=non_pd_pool['id']): + res = self._create_subnet( + self.fmt, + cidr=None, + net_id=network['network']['id'], + tenant_id=addr_scope['tenant_id'], + subnetpool_id=constants.IPV6_PD_POOL_ID, + ip_version=constants.IP_VERSION_6, + ipv6_ra_mode=constants.IPV6_SLAAC, + ipv6_address_mode=constants.IPV6_SLAAC) + self.assertEqual(webob.exc.HTTPBadRequest.code, + res.status_int) diff --git a/releasenotes/notes/relax-subnetpool-network-affinity-837c1fc28f835de5.yaml b/releasenotes/notes/relax-subnetpool-network-affinity-837c1fc28f835de5.yaml new file mode 100644 index 00000000000..ca8c4c22481 --- /dev/null +++ b/releasenotes/notes/relax-subnetpool-network-affinity-837c1fc28f835de5.yaml @@ -0,0 +1,12 @@ +--- +features: + - | + When different subnet pools participate in the same address scope, the + constraints disallowing subnets to be allocated from different pools on + the same network have been relaxed. As long as subnet pools participate + in the same address scope, subnets can now be created from different + subnet pools when multiple subnets are created on a network. When + address scopes are not used, subnets with the same ``ip_version`` on the + same network must still be allocated from the same subnet pool. + For more information, see bug + `1830240 `_.