[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

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,

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 = {}

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

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']