From bcadb2d3f1df80ef45d00e0779a9ac5b1745da91 Mon Sep 17 00:00:00 2001 From: Robert Kukura Date: Sun, 9 Sep 2018 21:24:11 -0400 Subject: [PATCH] [AIM] Fix isomorphic address scope issues When an isomorphic pair of address scopes reference the same non-pre-existing VRF, set the VRF's display name deterministically. Previously, the VRF's display name would depend on the order in which the scopes were created or updated, which could potentially fail validation. If both scopes have different names that are not empty, the VRF's display name is a combination of the v4 scope name and the v6 scope name, seperated by a '-'. If the scopes have identical names, that name is used as-is. If one scope's name is empty and the other's is not, the non-empty name is used. Another issue with validation of isomorphic address scopes is also fixed. If the existing AddressScopeMapping record indicated that the VRF was owned (i.e. not pre-existing), the contents of that mapping record was ignored and was "repaired" using the normal mapping rules. This would result in each scope in a previously isomorphic pair having its own unique VRF rather than sharing the same VRF is intended. Existing AddressScopeMapping records are now correctly handled during validation. Finally, the _check_dn() method used in many apic_aim UTs (including, but not limited to those for address scopes) now makes sure that the AIM resource referenced by the DN actually exists and that it has the expected display name. This UT enhancement did not turn up any issues unrelated to isomorphic address scopes, but should prevent such issues from being introduced in future changes. Change-Id: Ia3e0cec16c0181fe100d999a3cc741d272d2008d --- .../plugins/ml2plus/drivers/apic_aim/db.py | 11 +++ .../drivers/apic_aim/mechanism_driver.py | 42 +++++++--- .../unit/plugins/ml2plus/test_apic_aim.py | 77 ++++++++++++++----- .../grouppolicy/test_aim_validation.py | 59 ++++++++++++-- 4 files changed, 155 insertions(+), 34 deletions(-) diff --git a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py index 41076d285..1f9a89481 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py @@ -94,6 +94,17 @@ class DbMixin(object): vrf_name=vrf.name). all()) + def _get_address_scopes_owning_vrf(self, session, vrf): + return (session.query(as_db.AddressScope). + join(AddressScopeMapping, + AddressScopeMapping.scope_id == as_db.AddressScope.id). + filter(AddressScopeMapping.vrf_tenant_name == + vrf.tenant_name, + AddressScopeMapping.vrf_name == vrf.name, + AddressScopeMapping.vrf_owned). + order_by(as_db.AddressScope.ip_version). + all()) + def _get_address_scope_vrf(self, mapping): return aim_resource.VRF( tenant_name=mapping.vrf_tenant_name, diff --git a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py index 4517f51c5..50e79f69d 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py @@ -1091,6 +1091,8 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mapping = self._get_address_scope_mapping(session, id) if mapping: vrf = self._get_address_scope_vrf(mapping) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) else: dname = aim_utils.sanitize_display_name(current['name']) vrf = self._map_address_scope(session, current) @@ -1114,9 +1116,9 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mapping = self._get_address_scope_mapping(session, current['id']) if current['name'] != original['name'] and mapping.vrf_owned: - dname = aim_utils.sanitize_display_name(current['name']) vrf = self._get_address_scope_vrf(mapping) - self.aim.update(aim_ctx, vrf, display_name=dname) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) def delete_address_scope_precommit(self, context): current = context.current @@ -1128,10 +1130,11 @@ class ApicMechanismDriver(api_plus.MechanismDriver, if mapping.vrf_owned: vrf = self._get_address_scope_vrf(mapping) - mappings = self._get_address_scope_mappings_for_vrf(session, vrf) - if len(mappings) == 1: + session.delete(mapping) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) + if not scopes: self.aim.delete(aim_ctx, vrf) - session.delete(mapping) def extend_address_scope_dict(self, session, scope, result): if result.get(api_plus.BULK_EXTENDED): @@ -1153,6 +1156,20 @@ class ApicMechanismDriver(api_plus.MechanismDriver, result[cisco_apic.DIST_NAMES] = dist_names result[cisco_apic.SYNC_STATE] = sync_state + def _update_vrf_display_name(self, aim_ctx, vrf, scopes): + # Assumes scopes is sorted by ip_version. + if not scopes: + return + elif (len(scopes) == 1 or not scopes[1].name or + scopes[0].name == scopes[1].name): + dname = scopes[0].name + elif not scopes[0].name: + dname = scopes[1].name + else: + dname = scopes[0].name + '-' + scopes[1].name + dname = aim_utils.sanitize_display_name(dname) + self.aim.update(aim_ctx, vrf, display_name=dname) + def create_router(self, context, current): LOG.debug("APIC AIM MD creating router: %s", current) @@ -4110,10 +4127,11 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mgr.expected_aim_ctx) def _validate_address_scopes(self, mgr): + owned_scopes_by_vrf = defaultdict(list) for scope_db in mgr.actual_session.query(as_db.AddressScope): self._expect_project(mgr, scope_db.project_id) mapping = scope_db.aim_mapping - if mapping and not mapping.vrf_owned: + if mapping: mgr.expect_db_instance(mapping) else: vrf = self._map_address_scope(mgr.expected_session, scope_db) @@ -4121,13 +4139,19 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mgr.expected_session, scope_db.id, vrf) vrf = self._get_address_scope_vrf(mapping) vrf.monitored = not mapping.vrf_owned - # REVISIT: Need deterministic display_name for isomorphic - # address scopes that own VRF. vrf.display_name = ( aim_utils.sanitize_display_name(scope_db.name) if mapping.vrf_owned else "") vrf.policy_enforcement_pref = 'enforced' - mgr.expect_aim_resource(vrf) + mgr.expect_aim_resource(vrf, replace=True) + if mapping.vrf_owned: + scopes = owned_scopes_by_vrf[tuple(vrf.identity)] + scopes.append(scope_db) + # REVISIT: Fail if multiple scopes for same address family? + if len(scopes) > 1: + scopes = sorted(scopes, key=lambda scope: scope.ip_version) + self._update_vrf_display_name( + mgr.expected_aim_ctx, vrf, scopes) def _validate_routers(self, mgr): router_dbs = {} 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 d13eaa681..71fae29b9 100644 --- a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py +++ b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py @@ -336,12 +336,19 @@ class ApicAimTestCase(test_address_scope.AddressScopeTestCase, resource = self._find_by_dn(dn, cls) self.assertIsNotNone(resource) - def _check_dn(self, resource, aim_resource, key): + def _check_dn(self, resource, aim_resource, key, dname=None): dist_names = resource.get('apic:distinguished_names') self.assertIsInstance(dist_names, dict) dn = dist_names.get(key) self.assertIsInstance(dn, six.string_types) self.assertEqual(aim_resource.dn, dn) + aim_resource = self._find_by_dn(dn, aim_resource.__class__) + self.assertIsNotNone(aim_resource) + if dname is None: + name = resource.get('name') + self.assertIsInstance(name, six.string_types) + dname = aim_utils.sanitize_display_name(name) + self.assertEqual(dname, aim_resource.display_name) def _check_no_dn(self, resource, key): dist_names = resource.get('apic:distinguished_names') @@ -463,6 +470,7 @@ class TestAimMapping(ApicAimTestCase): self.mock_ns = self.call_wrapper.setUp( nat_strategy.DistributedNatStrategy) self._actual_scopes = {} + self._scope_vrf_dnames = {} super(TestAimMapping, self).setUp() def tearDown(self): @@ -647,7 +655,8 @@ class TestAimMapping(ApicAimTestCase): elif scope: scope = self._actual_scopes.get(scope['id'], scope) vrf_aname = self.name_mapper.address_scope(None, scope['id']) - vrf_dname = scope['name'] + vrf_dname = self._scope_vrf_dnames.get( + scope['id'], scope['name']) vrf_project = scope['tenant_id'] vrf_tenant_aname = self.name_mapper.project(None, vrf_project) tenant_aname = vrf_tenant_aname @@ -777,6 +786,8 @@ class TestAimMapping(ApicAimTestCase): dns = copy.copy(scope.get(DN)) actual_scope = self._actual_scopes.get(scope['id'], scope) + dname = self._scope_vrf_dnames.get( + actual_scope['id'], actual_scope['name']) tenant_aname = self.name_mapper.project( None, actual_scope['tenant_id']) @@ -787,7 +798,7 @@ class TestAimMapping(ApicAimTestCase): aim_vrf = self._get_vrf(aname, tenant_aname) self.assertEqual(tenant_aname, aim_vrf.tenant_name) self.assertEqual(aname, aim_vrf.name) - self.assertEqual(actual_scope['name'], aim_vrf.display_name) + self.assertEqual(dname, aim_vrf.display_name) self.assertEqual('enforced', aim_vrf.policy_enforcement_pref) self._check_dn_is_resource(dns, 'VRF', aim_vrf) @@ -869,10 +880,12 @@ class TestAimMapping(ApicAimTestCase): for scope in scopes or []: actual_scope = self._actual_scopes.get(scope['id'], scope) + dname = self._scope_vrf_dnames.get( + actual_scope['id'], actual_scope['name']) self._check_router_vrf( self.name_mapper.address_scope(None, actual_scope['id']), - actual_scope['name'], actual_scope['tenant_id'], - dns, 'as_%s-VRF' % scope['id']) + dname, actual_scope['tenant_id'], dns, + 'as_%s-VRF' % scope['id']) # The AIM Subnets are validated in _check_subnet, so just # check that their DNs are present and valid. @@ -1755,6 +1768,8 @@ class TestAimMapping(ApicAimTestCase): scope46i_vrf, 4, name='as4i')['address_scope'] scope4i_id = scope4i['id'] self._actual_scopes[scope4i_id] = scope6 + self._scope_vrf_dnames[scope4i_id] = 'as4i-as6' + self._scope_vrf_dnames[scope6_id] = 'as4i-as6' self._check_address_scope(scope4i) pool4i = self._make_subnetpool( self.fmt, ['10.1.0.0/16'], name='sp4i', tenant_id=self._tenant_id, @@ -4849,19 +4864,36 @@ class TestExtensionAttributes(ApicAimTestCase): def test_isomorphic_address_scopes_lifecycle(self): # Create initial v4 scope. scope4 = self._make_address_scope( - self.fmt, 4, name='as')['address_scope'] + self.fmt, 4, name='as4')['address_scope'] dn = scope4['apic:distinguished_names']['VRF'] + vrf = self._find_by_dn(dn, aim_resource.VRF) + self._check_dn(scope4, vrf, 'VRF', 'as4') # Create isomorphic v6 scope, using v4 scope's pre-existing # DN. scope6 = self._make_address_scope_for_vrf( - dn, n_constants.IP_VERSION_6)['address_scope'] + dn, n_constants.IP_VERSION_6, name='as6')['address_scope'] self.assertEqual(dn, scope6['apic:distinguished_names']['VRF']) + self._check_dn(scope6, vrf, 'VRF', 'as4-as6') + + # Update v4 scope name. + data = {'address_scope': {'name': ''}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'as6') + + # Update v4 scope name again. + data = {'address_scope': {'name': 'x4'}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'x4-as6') + + # Update v6 scope name. + data = {'address_scope': {'name': 'x6'}} + self._update('address-scopes', scope6['id'], data) + self._check_dn(scope6, vrf, 'VRF', 'x4-x6') # Delete v4 scope. self._delete('address-scopes', scope4['id']) - vrf = self._find_by_dn(dn, aim_resource.VRF) - self.assertIsNotNone(vrf) + self._check_dn(scope6, vrf, 'VRF', 'x6') # Delete v6 scope. self._delete('address-scopes', scope6['id']) @@ -4872,17 +4904,24 @@ class TestExtensionAttributes(ApicAimTestCase): scope4 = self._make_address_scope( self.fmt, 4, name='as')['address_scope'] dn = scope4['apic:distinguished_names']['VRF'] + vrf = self._find_by_dn(dn, aim_resource.VRF) + self._check_dn(scope4, vrf, 'VRF', 'as') # Create isomorphic v6 scope, using v4 scope's pre-existing - # DN. + # DN, with same name. scope6 = self._make_address_scope_for_vrf( - dn, n_constants.IP_VERSION_6)['address_scope'] + dn, n_constants.IP_VERSION_6, name='as')['address_scope'] self.assertEqual(dn, scope6['apic:distinguished_names']['VRF']) + self._check_dn(scope6, vrf, 'VRF', 'as') + + # Update v4 scope name to not match. + data = {'address_scope': {'name': 'as4'}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'as4-as') # Delete v6 scope. self._delete('address-scopes', scope6['id']) - vrf = self._find_by_dn(dn, aim_resource.VRF) - self.assertIsNotNone(vrf) + self._check_dn(scope4, vrf, 'VRF', 'as4') # Delete v4 scope. self._delete('address-scopes', scope4['id']) @@ -4999,14 +5038,14 @@ class TestExternalConnectivityBase(object): ext_bd = aim_resource.BridgeDomain( tenant_name=self.t1_aname, name='EXT-l1') ext_vrf = aim_resource.VRF(tenant_name=self.t1_aname, name='EXT-l1') - self._check_dn(net1, ext_epg, 'EndpointGroup') - self._check_dn(net1, ext_bd, 'BridgeDomain') - self._check_dn(net1, ext_vrf, 'VRF') + self._check_dn(net1, ext_epg, 'EndpointGroup', 'EXT-l1') + self._check_dn(net1, ext_bd, 'BridgeDomain', 'EXT-l1') + self._check_dn(net1, ext_vrf, 'VRF', 'EXT-l1') net1 = self._show('networks', net1['id'])['network'] - self._check_dn(net1, ext_epg, 'EndpointGroup') - self._check_dn(net1, ext_bd, 'BridgeDomain') - self._check_dn(net1, ext_vrf, 'VRF') + self._check_dn(net1, ext_epg, 'EndpointGroup', 'EXT-l1') + self._check_dn(net1, ext_bd, 'BridgeDomain', 'EXT-l1') + self._check_dn(net1, ext_vrf, 'VRF', 'EXT-l1') self._validate() # test no-op CIDR update diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py index 7af8b48ec..db906e255 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py @@ -22,6 +22,7 @@ from aim import context as aim_context from neutron.db.models import segment from neutron.plugins.ml2 import models as ml2_models from neutron.tests.unit.extensions import test_securitygroup +from neutron_lib import constants as n_constants from neutron_lib import context as n_context from oslo_config import cfg @@ -76,11 +77,17 @@ class AimValidationTestMixin(object): # Delete the AIM resource and test. self.aim_mgr.delete(self.aim_ctx, resource) self._validate_repair_validate() + self.assertTrue( + actual_resource.user_equal( + self.aim_mgr.get(self.aim_ctx, resource))) # Modify the AIM resource and test. self.aim_mgr.update( self.aim_ctx, resource, display_name='not what it was') self._validate_repair_validate() + self.assertTrue( + actual_resource.user_equal( + self.aim_mgr.get(self.aim_ctx, resource))) # Add unexpected AIM resource and test. setattr(resource, unexpected_attr_name, unexpected_attr_value) @@ -270,15 +277,15 @@ class TestNeutronMapping(AimValidationTestCase): def test_address_scope(self): # Create address scope. - scope = self._make_address_scope( - self.fmt, 4, name='as1')['address_scope'] - scope_id = scope['id'] - vrf_dn = scope['apic:distinguished_names']['VRF'] + scope4 = self._make_address_scope( + self.fmt, 4, name='as4')['address_scope'] + scope4_id = scope4['id'] + vrf_dn = scope4['apic:distinguished_names']['VRF'] self._validate() # Delete the address scope's mapping record and test. (self.db_session.query(db.AddressScopeMapping). - filter_by(scope_id=scope_id). + filter_by(scope_id=scope4_id). delete()) self._validate_repair_validate() @@ -286,7 +293,47 @@ class TestNeutronMapping(AimValidationTestCase): vrf = aim_resource.VRF.from_dn(vrf_dn) self._test_aim_resource(vrf) - # REVISIT: Test isomorphic address scopes. + # Create isomorphic v6 address scope. + scope6 = self._make_address_scope_for_vrf( + vrf_dn, n_constants.IP_VERSION_6, name='as6')['address_scope'] + scope6_id = scope6['id'] + self.assertEqual(vrf_dn, scope6['apic:distinguished_names']['VRF']) + self._validate() + + # Test AIM VRF. + self._test_aim_resource(vrf) + + # Delete the initial address scope's mapping record and test. + (self.db_session.query(db.AddressScopeMapping). + filter_by(scope_id=scope4_id). + delete()) + self._validate_repair_validate() + scope4 = self._show('address-scopes', scope4_id)['address_scope'] + self.assertEqual(vrf_dn, scope4['apic:distinguished_names']['VRF']) + scope6 = self._show('address-scopes', scope6_id)['address_scope'] + self.assertEqual(vrf_dn, scope6['apic:distinguished_names']['VRF']) + + # Test AIM VRF. + self._test_aim_resource(vrf) + + # Delete the 2nd address scope's mapping record and + # test. Without this record, there is no way to know that the + # scopes were previously isomorphic, so they no longer will + # be isomorphic after repair. + (self.db_session.query(db.AddressScopeMapping). + filter_by(scope_id=scope6_id). + delete()) + self._validate_repair_validate() + scope4 = self._show('address-scopes', scope4_id)['address_scope'] + self.assertEqual(vrf_dn, scope4['apic:distinguished_names']['VRF']) + scope6 = self._show('address-scopes', scope6_id)['address_scope'] + scope6_vrf_dn = scope6['apic:distinguished_names']['VRF'] + self.assertNotEqual(vrf_dn, scope6_vrf_dn) + + # Test both AIM VRFs. + self._test_aim_resource(vrf) + scope6_vrf = aim_resource.VRF.from_dn(scope6_vrf_dn) + self._test_aim_resource(scope6_vrf) def _test_network_attrs(self, original): current = self._show('networks', original['id'])['network']