Merge "[apic-mapping] Explicit end of the chain for ptgs"

gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/apic_mapping.py

@@ -51,7 +51,6 @@ from oslo_serialization import jsonutils
 import sqlalchemy as sa
 
 from gbpservice.neutron.db.grouppolicy import group_policy_mapping_db as gpdb
-from gbpservice.neutron.extensions import driver_proxy_group as proxy_group
 from gbpservice.neutron.extensions import group_policy as gpolicy
 from gbpservice.neutron.services.grouppolicy.common import constants as g_const
 from gbpservice.neutron.services.grouppolicy.common import exceptions as gpexc
@@ -199,6 +198,10 @@ class ExplicitPortOverlap(gpexc.GroupPolicyBadRequest):
                 'in network %(net)s')
 
 
+class AdminOnlyOperation(gpexc.GroupPolicyBadRequest):
+    message = _("This operation is reserved to admins")
+
+
 class TenantSpecificNatEpg(model_base.BASEV2):
     """Tenants that use a specific NAT EPG for an external segment."""
     __tablename__ = 'gp_apic_tenant_specific_nat_epg'
@@ -208,6 +211,7 @@ class TenantSpecificNatEpg(model_base.BASEV2):
         primary_key=True)
     tenant_id = sa.Column(sa.String(36), primary_key=True)
 
+
 REVERSE_PREFIX = 'reverse-'
 SHADOW_PREFIX = 'Shd-'
 AUTO_PREFIX = 'Auto-'
@@ -224,6 +228,7 @@ REVERTIBLE_PROTOCOLS = [n_constants.PROTO_NAME_TCP.lower(),
                         n_constants.PROTO_NAME_UDP.lower(),
                         n_constants.PROTO_NAME_ICMP.lower()]
 PROXY_PORT_PREFIX = "opflex_proxy:"
+EOC_PREFIX = "opflex_eoc:"
 ICMP_REPLY_TYPES = ['echo-rep', 'dst-unreach', 'src-quench', 'time-exceeded']
 
 
@@ -430,22 +435,36 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                                   owned=own_addr, inject_default_route=
                                   l2p['inject_default_route'])
         self._add_vrf_details(context, details)
-        if self._is_pt_chain_head(context, pt, ptg, owned_ips=own_addr):
+        if self._is_pt_chain_head(context, pt, ptg, owned_ips=own_addr,
+                                  port_id=port_id):
             # is a relevant proxy_gateway, push all the addresses from this
             # chain to this PT
             extra_map = details
-            master_mac = self._is_master_owner(context, pt,
+            master_port = self._is_master_owner(context, pt,
-                                               owned_ips=own_addr)
+                                                owned_ips=own_addr)
-            if master_mac:
+            if master_port:
                 extra_map = details['extra_details'].setdefault(
-                    master_mac, {'extra_ips': [], 'floating_ip': [],
+                    master_port['mac_address'],
-                        'ip_mapping': [], 'host_snat_ips': []})
+                    {'extra_ips': [], 'floating_ip': [],
-            if bool(master_mac) == bool(pt['cluster_id']):
+                     'ip_mapping': [], 'host_snat_ips': []})
+            if bool(master_port) == bool(pt['cluster_id']):
                 l3_policy = context._plugin.get_l3_policy(
                     context, l2p['l3_policy_id'])
-                while ptg['proxied_group_id']:
+                proxied_ptgs = []
+                while ptg.get('proxied_group_id'):
                     proxied = self.gbp_plugin.get_policy_target_group(
                         context, ptg['proxied_group_id'])
+                    proxied_ptgs.append(proxied)
+                    ptg = proxied
+                # Retrieve PTGs explicitly proxied
+                descriptions = [EOC_PREFIX + port_id]
+                if master_port:
+                    # Also retrieve groups proxied by master of the cluster
+                    descriptions.append(EOC_PREFIX + master_port['id'])
+                proxied_ptgs.extend(
+                    self._get_policy_target_groups(
+                        context, filters={'description': descriptions}))
+                for proxied in proxied_ptgs:
                     for port in self._get_ptg_ports(proxied):
                         extra_map['extra_ips'].extend(
                             [x['ip_address'] for x in port['fixed_ips'] +
@@ -460,7 +479,6 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                         if not extra_map['host_snat_ips']:
                             extra_map['host_snat_ips'].extend(
                                 host_snat_ips)
-                    ptg = proxied
             else:
                 LOG.info(_LI("Active master has changed for PT %s"),
                          pt['id'])
@@ -858,6 +876,10 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                     context._plugin_context, context.current['id'])
                 db_group.l2_policy_id = proxied['l2_policy_id']
                 context.current['l2_policy_id'] = proxied['l2_policy_id']
+            if context.current['description']:
+                if (EOC_PREFIX in context.current['description']
+                        and not context._plugin_context.is_admin):
+                    raise AdminOnlyOperation()
             else:
                 self.name_mapper.has_valid_name(context.current)
 
@@ -1192,6 +1214,11 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                  context.current['subnets']):
                 raise ExplicitSubnetAssociationNotSupported()
             self._reject_shared_update(context, 'policy_target_group')
+            if (context.current['description'] !=
+                    context.original['description']):
+                if (EOC_PREFIX in context.current['description']
+                        and not context._plugin_context.is_admin):
+                    raise AdminOnlyOperation()
 
     def update_policy_target_group_postcommit(self, context):
         if not self.name_mapper._is_apic_reference(context.current):
@@ -2420,13 +2447,15 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                 object):
             return apic_manager.TENANT_COMMON
         else:
-            if object.get('proxied_group_id'):  # Then it's a proxy PTG
+            if object.get('proxied_group_id') or object.get(
-                # Even though they may belong to a different tenant,
+                    'enforce_service_chains') is False:
+                # Then it's a proxy
+                # PTG Even though they may belong to a different tenant,
                 # the proxy PTGs will be created on the L2P's tenant to
                 # make APIC happy
                 l2p = self.gbp_plugin.get_l2_policy(
                     nctx.get_admin_context(), object['l2_policy_id'])
-                return self.name_mapper.tenant(l2p)
+                return self._tenant_by_sharing_policy(l2p)
             else:
                 return self.name_mapper.tenant(object)
 
@@ -2545,12 +2574,11 @@ class ApicMappingDriver(api.ResourceMappingDriver,
         bd_name = self.name_mapper.policy_target_group(
                 context, proxied, prefix=SHADOW_PREFIX)
         ptg_name = self.name_mapper.policy_target_group(context, proxied)
-        is_l2 = context.current['proxy_type'] == proxy_group.PROXY_TYPE_L2
         with self.apic_manager.apic.transaction(None) as trs:
             # Create shadow BD to host the proxied EPG
             self.apic_manager.ensure_bd_created_on_apic(
                 tenant, bd_name, ctx_owner=ctx_owner, ctx_name=l3_policy_name,
-                allow_broadcast=is_l2, unicast_route=False, transaction=trs,
+                allow_broadcast=True, unicast_route=False, transaction=trs,
                 enforce_subnet_check=False)
             # Move current PTG to different BD
             self.apic_manager.ensure_epg_created(
@@ -3175,6 +3203,17 @@ class ApicMappingDriver(api.ResourceMappingDriver,
     def _notify_head_chain_ports(self, ptg_id):
         context = nctx.get_admin_context()
         ptg = self.gbp_plugin.get_policy_target_group(context, ptg_id)
+        explicit_eoc_id = self._extract_ptg_explicit_eoc(context, ptg)
+        if explicit_eoc_id:
+            # Could be a cluster master
+            updates = [explicit_eoc_id]
+            eoc_pt = self._port_id_to_pt(context, explicit_eoc_id)
+            if eoc_pt:
+                updates.extend(
+                    x['port_id'] for x in self.gbp_plugin.get_policy_targets(
+                        context, {'cluster_id': [eoc_pt['id']]}))
+            for update in updates:
+                self._notify_port_update(context, update)
         # to avoid useless double notification exit now if no proxy
         if not ptg.get('proxy_group_id'):
             return
@@ -3281,33 +3320,48 @@ class ApicMappingDriver(api.ResourceMappingDriver,
             return opt.lower() in ['true', 'yes', '1']
         return False
 
-    def _is_pt_chain_head(self, plugin_context, pt, ptg=None, owned_ips=None):
+    def _is_pt_chain_head(self, plugin_context, pt, ptg=None, owned_ips=None,
-        if pt:
+                          port_id=None):
-            ptg = ptg or self._get_policy_target_group(
+        if not pt:
-                plugin_context, pt['policy_target_group_id'])
+            return False
-            # Check whenther PTG is the end of a chain
-            chain_end = bool(ptg.get('proxied_group_id') and
-                             not ptg.get('proxy_group'))
-            if chain_end:
-                cluster_id = pt['cluster_id']
-                if cluster_id:
-                    # The master PT must be a proxy gateway for this to be
-                    # eligible as the chain head
-                    master_pt = self._get_pt_cluster_master(plugin_context, pt)
 
-                    # Verify whether this is the active PT
+        master_pt = None
-                    return bool(master_pt['proxy_gateway'] and
+        if port_id or pt.get('cluster_id'):
-                                self._is_master_owner(plugin_context, pt,
+            descr = []
-                                                      master_pt=master_pt,
+            if port_id:
-                                                      owned_ips=owned_ips))
+                descr.append(EOC_PREFIX + port_id)
-                # regular PT not part of a cluster, return if proxy gateway
+            if pt.get('cluster_id'):
-                return bool(pt['proxy_gateway'])
+                master_pt = self._get_pt_cluster_master(plugin_context, pt)
+                descr.append(EOC_PREFIX + master_pt['port_id'])
+            if bool(self._get_policy_target_groups(
+                    plugin_context, filters={'description': descr})):
+                return True
+
+        ptg = ptg or self._get_policy_target_group(
+            plugin_context, pt['policy_target_group_id'])
+        # Check whether PTG is the end of a chain
+        chain_end = bool(ptg.get('proxied_group_id') and
+                         not ptg.get('proxy_group'))
+        if chain_end:
+            cluster_id = pt['cluster_id']
+            if cluster_id:
+                # The master PT must be a proxy gateway for this to be
+                # eligible as the chain head
+                master_pt = master_pt or self._get_pt_cluster_master(
+                    plugin_context, pt)
+                # Verify whether this is the active PT
+                return bool(master_pt['proxy_gateway'] and
+                            self._is_master_owner(plugin_context, pt,
+                                                  master_pt=master_pt,
+                                                  owned_ips=owned_ips))
+            # regular PT not part of a cluster, return if proxy gateway
+            return bool(pt['proxy_gateway'])
 
     def _is_master_owner(self, plugin_context, pt, master_pt=None,
                          owned_ips=None):
         """Verifies if the port owns the master address.
 
-        Returns the master MAC address or False
+        Returns the master port address or False
         """
         if pt['cluster_id']:
             master_pt = master_pt or self._get_pt_cluster_master(
@@ -3319,10 +3373,9 @@ class ApicMappingDriver(api.ResourceMappingDriver,
             master_port = self._get_port(plugin_context, master_pt['port_id'])
             master_addresses = set([x['ip_address'] for x in
                                     master_port['fixed_ips']])
-            master_mac = master_port['mac_address']
             if bool(owned_addresses & master_addresses):
-                return master_mac
+                return master_port
-        return False
+        return None
 
     def _get_owned_addresses(self, plugin_context, port_id):
         return set(self.ha_ip_handler.get_ha_ipaddresses_for_port(port_id))
@@ -3807,3 +3860,13 @@ class ApicMappingDriver(api.ResourceMappingDriver,
                       'for external segment %(es)s',
                       {'tenant': nat_epg_tenant, 'epg': nat_epg_name,
                        'es': es['id']})
+
+    def _extract_ptg_explicit_eoc(self, plugin_context, ptg):
+        """Extract PTG End of Chain
+
+        Given a PTG, retrieves the explicit End of the Chain from its
+        description and returns a Neutron port
+        :return: Neutron port or None
+        """
+        if EOC_PREFIX in ptg.get('description', ''):
+            return ptg['description'][len(EOC_PREFIX):]

gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py

@@ -244,6 +244,13 @@ class ApicMappingTestCase(
         sys.modules["apicapi"] = self.saved_apicapi
         super(ApicMappingTestCase, self).tearDown()
 
+    def _get_pts_addresses(self, pts):
+        addresses = []
+        for pt in pts:
+            port = self._get_object('ports', pt['port_id'], self.api)['port']
+            addresses.extend([x['ip_address'] for x in port['fixed_ips']])
+        return addresses
+
     def _build_external_dict(self, name, cidr_exposed, is_edge_nat=False):
         ext_info = {
             'enable_nat': 'True' if self.nat_enabled else 'False'
@@ -553,6 +560,24 @@ class TestPolicyTarget(ApicMappingTestCase):
             details['host_snat_ip'])
         self.assertEqual(24, details['prefixlen'])
 
+    def test_get_gbp_details_extra_ips_explicit_eoc(self):
+        ptg = self.create_policy_target_group(
+            name="ptg1")['policy_target_group']
+        pt1 = self.create_policy_target(
+            policy_target_group_id=ptg['id'])['policy_target']
+        ptg2 = self.create_policy_target_group(
+            name="ptg2",
+            description='opflex_eoc:' + pt1['port_id'],
+            is_admin_context=True)['policy_target_group']
+        pt2 = self.create_policy_target(
+            policy_target_group_id=ptg2['id'])['policy_target']
+        mapping = self.driver.get_gbp_details(context.get_admin_context(),
+            device='tap%s' % pt1['port_id'], host='h2')
+
+        # Extra ip set for pt2
+        ips = self._get_pts_addresses([pt2])
+        self.assertEqual(set(ips), set(mapping['extra_ips']))
+
     def test_snat_pool_subnet_deletion(self):
         self._mock_external_dict([('supported', '192.168.0.2/24')])
         self.driver.apic_manager.ext_net_dict[
@@ -1100,6 +1125,55 @@ class TestPolicyTarget(ApicMappingTestCase):
         self.assertEqual('1.1.1.1', entries[0].ha_ip_address)
         self.assertEqual('1.1.1.1', entries[1].ha_ip_address)
 
+    def test_explicit_end_of_chain(self):
+        self.driver._notify_port_update = mock.Mock()
+        ptg = self.create_policy_target_group(
+            name="ptg1")['policy_target_group']
+        pt1 = self.create_policy_target(
+            policy_target_group_id=ptg['id'])['policy_target']
+        ptg2 = self.create_policy_target_group(
+            name="ptg2",
+            description='opflex_eoc:' + pt1['port_id'],
+            is_admin_context=True)['policy_target_group']
+        self.create_policy_target(policy_target_group_id=ptg2['id'])
+        # pt1 notified
+        self.driver._notify_port_update.assert_called_once_with(
+            mock.ANY, pt1['port_id'])
+
+    def test_explicit_end_of_chain_cluster(self):
+        self.driver._notify_port_update = mock.Mock()
+        ptg = self.create_policy_target_group(
+            name="ptg1")['policy_target_group']
+        pt1 = self.create_policy_target(
+            policy_target_group_id=ptg['id'])['policy_target']
+        # Same cluster
+        pt2 = self.create_policy_target(
+            policy_target_group_id=ptg['id'],
+            cluster_id=pt1['id'])['policy_target']
+        pt3 = self.create_policy_target(
+            policy_target_group_id=ptg['id'],
+            cluster_id=pt1['id'])['policy_target']
+        ptg2 = self.create_policy_target_group(
+            name="ptg2",
+            description='opflex_eoc:' + pt1['port_id'],
+            is_admin_context=True)['policy_target_group']
+        self.create_policy_target(policy_target_group_id=ptg2['id'])
+        # pt1, 2 and 3 notified
+        expected_calls = [
+            mock.call(mock.ANY, pt1['port_id']),
+            mock.call(mock.ANY, pt2['port_id']),
+            mock.call(mock.ANY, pt3['port_id'])]
+        self._check_call_list(
+            expected_calls, self.driver._notify_port_update.call_args_list)
+
+    def test_explicit_eoc_raises(self):
+        ptg = self.create_policy_target_group(
+            name="ptg1")['policy_target_group']
+        self.create_policy_target_group(
+            name="ptg2", description='opflex_eoc:', expected_res_status=400)
+        self.update_policy_target_group(
+            ptg['id'], description='opflex_eoc:', expected_res_status=400)
+
 
 class TestPolicyTargetVlanNetwork(ApicMappingVlanTestCase,
                                   TestPolicyTarget):

gbpservice/neutron/tests/unit/services/servicechain/ncp/test_tscp_apic_mapping.py

@@ -809,13 +809,6 @@ class TestApicChains(ApicMappingStitchingPlumberGBPTestCase,
 
 class TestProxyGroup(ApicMappingStitchingPlumberGBPTestCase):
 
-    def _get_pts_addresses(self, pts):
-        addresses = []
-        for pt in pts:
-            port = self._get_object('ports', pt['port_id'], self.api)['port']
-            addresses.extend([x['ip_address'] for x in port['fixed_ips']])
-        return addresses
-
     def _proxy_tenant(self, ptg, admin_proxy):
         return 'admin' if admin_proxy else ptg['tenant_id']
 
@@ -926,7 +919,7 @@ class TestProxyGroup(ApicMappingStitchingPlumberGBPTestCase):
         # Shadow BD created
         self.mgr.ensure_bd_created_on_apic.assert_called_once_with(
             ptg['tenant_id'], 'Shd-' + ptg['id'], ctx_owner=l3p['tenant_id'],
-            ctx_name=l3p['id'], allow_broadcast=False, unicast_route=False,
+            ctx_name=l3p['id'], allow_broadcast=True, unicast_route=False,
             transaction=mock.ANY, enforce_subnet_check=False)
         # Proxied PTG moved
         expected_calls = [
@@ -1217,7 +1210,7 @@ class TestProxyGroup(ApicMappingStitchingPlumberGBPTestCase):
         # Shadow BD created in non-admin tenant
         self.mgr.ensure_bd_created_on_apic.assert_called_once_with(
             'non-admin', 'Shd-' + ptg['id'], ctx_owner=l3p['tenant_id'],
-            ctx_name=l3p['id'], allow_broadcast=False, unicast_route=False,
+            ctx_name=l3p['id'], allow_broadcast=True, unicast_route=False,
             transaction=mock.ANY, enforce_subnet_check=False)
         # Proxied PTG moved
         expected_calls = [