From 794d9588ce5f1ce341218268dcfd1f04004eefac Mon Sep 17 00:00:00 2001 From: Amit Bose Date: Wed, 15 Jun 2016 18:22:26 -0700 Subject: [PATCH] [APIC mapping] Associate BDs to L3Outs even when NAT is disabled Right now we only associate the NAT BD with L3Out (or the tenant's private BD with shadow L3Out for edge-NAT). However, even with NAT disabled, it is useful to connected the private BD to the L3Out so that private subnets are advertised through dynamic routing protocols. Closes-Bug: 1593025 Change-Id: I5ef7b2f6ae34605a27b19cb18c665d5e21c3bb42 Signed-off-by: Amit Bose --- .../drivers/cisco/apic/apic_mapping.py | 84 ++++++++---- .../services/grouppolicy/test_apic_mapping.py | 123 ++++++++++++------ 2 files changed, 137 insertions(+), 70 deletions(-) diff --git a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/apic_mapping.py b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/apic_mapping.py index 5eafd5079..42e0c8133 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/apic_mapping.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/apic_mapping.py @@ -894,7 +894,8 @@ class ApicMappingDriver(api.ResourceMappingDriver, context._plugin_context, context.current['id'], subnets, [], transaction=trs) - # query all the ESs under this vrf + # query all the ESs for this VRF, connect BD to corresponding + # (shadow) L3Outs for no-NAT and edge-NAT cases. if l3_policy_object['external_segments']: ess = context._plugin.get_external_segments( context._plugin_context, @@ -903,12 +904,19 @@ class ApicMappingDriver(api.ResourceMappingDriver, for es in ess: ext_info = self.apic_manager.ext_net_dict.get( es['name']) - if (ext_info and self._is_edge_nat(ext_info) and - self._is_nat_enabled_on_es(es)): + nat_enabled = self._is_nat_enabled_on_es(es) + is_edge_nat = (nat_enabled and ext_info and + self._is_edge_nat(ext_info)) + if not nat_enabled or is_edge_nat: es_name = self.name_mapper.external_segment( context, es, prefix=self._get_shadow_prefix(context, - True, l3_policy_object, is_edge_nat=True)) + is_edge_nat, l3_policy_object, + is_edge_nat=is_edge_nat)) + if not is_edge_nat and self._is_pre_existing(es): + nm = self.name_mapper.name_mapper + es_name = nm.pre_existing( + context._plugin_context, es['name']) self.apic_manager.set_l3out_for_bd(tenant, l2_policy, es_name, transaction=trs) @@ -1855,6 +1863,8 @@ class ApicMappingDriver(api.ResourceMappingDriver, es_name = self.name_mapper.external_segment(context, es, prefix=self._get_shadow_prefix(context, is_shadow, context.current, self._is_edge_nat(ext_info))) + es_name_pre = self.name_mapper.name_mapper.pre_existing( + context._plugin_context, es['name']) es_tenant = self._get_tenant_for_shadow(is_shadow, context.current, es) nat_enabled = self._is_nat_enabled_on_es(es) pre_existing = False if is_shadow else self._is_pre_existing(es) @@ -1877,14 +1887,12 @@ class ApicMappingDriver(api.ResourceMappingDriver, transaction=trs) # Associate pre-existing, no-NAT L3-out with L3policy if pre_existing and not nat_enabled: - mapped_es = self.name_mapper.name_mapper.pre_existing( - context._plugin_context, es['name']) - l3out_info = self._query_l3out_info(mapped_es, + l3out_info = self._query_l3out_info(es_name_pre, self.name_mapper.tenant(es)) if l3out_info: mapped_tenant = l3out_info['l3out_tenant'] self.apic_manager.set_context_for_external_routed_network( - mapped_tenant, mapped_es, l3_policy, transaction=trs) + mapped_tenant, es_name_pre, l3_policy, transaction=trs) is_details_needed = False if not is_shadow and not pre_existing: @@ -1936,19 +1944,29 @@ class ApicMappingDriver(api.ResourceMappingDriver, (self.name_mapper.name_mapper.pre_existing( context, es['name']) if pre_existing else es_name), transaction=trs) - if nat_enabled and not is_shadow: - # create shadow external-networks - self._plug_l3p_to_es(context, es, True) - # create shadow external EPGs indirectly by re-plugging - # external policies to external segment - eps = context._plugin.get_external_policies( - context._plugin_context, - filters={'id': es['external_policies'], - 'tenant_id': [context.current['tenant_id']]}) - for ep in eps: - self._plug_external_policy_to_segment(context, ep, - [es['id']], ep['provided_policy_rule_sets'], - ep['consumed_policy_rule_sets']) + if not is_shadow: + if nat_enabled: + # create shadow external-networks + self._plug_l3p_to_es(context, es, True) + # create shadow external EPGs indirectly by re-plugging + # external policies to external segment + eps = context._plugin.get_external_policies( + context._plugin_context, + filters={'id': es['external_policies'], + 'tenant_id': [context.current['tenant_id']]}) + for ep in eps: + self._plug_external_policy_to_segment(context, ep, + [es['id']], ep['provided_policy_rule_sets'], + ep['consumed_policy_rule_sets']) + else: + # Associate BDs of the VRF to L3-out + l2ps = self._get_l2_policies(context._plugin_context, + {'id': context.current['l2_policies']}) + for l2p in l2ps: + self.apic_manager.set_l3out_for_bd( + self._tenant_by_sharing_policy(l2p), + self.name_mapper.l2_policy(context, l2p), + es_name_pre if pre_existing else es_name) def _unplug_l3p_from_es(self, context, es, is_shadow=False): is_edge_nat = False @@ -1958,12 +1976,24 @@ class ApicMappingDriver(api.ResourceMappingDriver, es_name = self.name_mapper.external_segment(context, es, prefix=self._get_shadow_prefix(context, is_shadow, context.current, is_edge_nat)) + es_name_pre = self.name_mapper.name_mapper.pre_existing( + context._plugin_context, es['name']) es_tenant = self._get_tenant_for_shadow(is_shadow, context.current, es) nat_enabled = self._is_nat_enabled_on_es(es) pre_existing = False if is_shadow else self._is_pre_existing(es) - # remove shadow external-networks - if nat_enabled and not is_shadow: - self._unplug_l3p_from_es(context, es, True) + if not is_shadow: + if nat_enabled: + # remove shadow external-networks + self._unplug_l3p_from_es(context, es, True) + else: + # Dissociate BDs of the VRF from L3-out + l2ps = self._get_l2_policies(context._plugin_context, + {'id': context.current['l2_policies']}) + for l2p in l2ps: + self.apic_manager.unset_l3out_for_bd( + self._tenant_by_sharing_policy(l2p), + self.name_mapper.l2_policy(context, l2p), + es_name_pre if pre_existing else es_name) set_ctx = self.apic_manager.set_context_for_external_routed_network if (is_shadow or not [x for x in es['l3_policies'] if x != context.current['id']]): @@ -1974,13 +2004,11 @@ class ApicMappingDriver(api.ResourceMappingDriver, # Dissociate L3policy from pre-existing, no-NAT L3-out if pre_existing and not nat_enabled: - mapped_es = self.name_mapper.name_mapper.pre_existing( - context._plugin_context, es['name']) - l3out_info = self._query_l3out_info(mapped_es, + l3out_info = self._query_l3out_info(es_name_pre, self.name_mapper.tenant(es)) if l3out_info: mapped_tenant = l3out_info['l3out_tenant'] - set_ctx(mapped_tenant, mapped_es, None, + set_ctx(mapped_tenant, es_name_pre, None, transaction=trs) if nat_enabled and not is_shadow: diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py index 4eaa7f6a1..108f2d890 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py @@ -2104,9 +2104,11 @@ class TestL3Policy(ApicMappingTestCase): self.assertEqual(1, len(l3p['external_segments'][es['id']])) self.assertEqual('169.254.0.2', l3p['external_segments'][es['id']][0]) - if is_edge_nat: - l2p = self.create_l2_policy(name='myl2p', - l3_policy_id=l3p['id'])['l2_policy'] + l2ps = [self.create_l2_policy(name='myl2p-%s' % x, + tenant_id=l3p['tenant_id'], + shared=shared_l3p, + l3_policy_id=l3p['id'])['l2_policy'] + for x in range(0, 3)] owner = self.common_tenant if shared_es else es['tenant_id'] l3p_owner = self.common_tenant if shared_l3p else l3p['tenant_id'] @@ -2198,18 +2200,24 @@ class TestL3Policy(ApicMappingTestCase): self.assertFalse(mgr.ensure_logical_node_profile_created.called) self.assertFalse(mgr.ensure_static_route_created.called) + expected_set_l3out_for_bd_calls = [] if self.nat_enabled: - expected_set_l3out_for_bd_calls = [ + expected_set_l3out_for_bd_calls.append( mock.call(owner, "NAT-bd-%s" % es['id'], es['name' if self.pre_l3out else 'id'], - transaction=mock.ANY)] + transaction=mock.ANY)) if is_edge_nat: - expected_set_l3out_for_bd_calls.append( + expected_set_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es['id']), - transaction=mock.ANY)) - self._check_call_list(expected_set_l3out_for_bd_calls, - mgr.set_l3out_for_bd.call_args_list) + transaction=mock.ANY) for l2p in l2ps]) + else: + expected_set_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es['name' if self.pre_l3out else 'id'], + transaction=mock.ANY) for l2p in l2ps]) + self._check_call_list(expected_set_l3out_for_bd_calls, + mgr.set_l3out_for_bd.call_args_list) # Although the naming convention used here has been chosen poorly, # I'm separating the tests in order to get the mock re-set. @@ -2243,20 +2251,24 @@ class TestL3Policy(ApicMappingTestCase): {'destination': '128.0.0.0/16', 'nexthop': None}])['external_segment'] - # Create with explicit address l3p = self.create_l3_policy( name='myl3p', expected_res_status=201, tenant_id=es['tenant_id'] if not shared_es else 'another_tenant', - shared=shared_l3p, - external_segments={es['id']: []})['l3_policy'] + shared=shared_l3p)['l3_policy'] + l2ps = [self.create_l2_policy(name='myl2p-%s' % x, + tenant_id=l3p['tenant_id'], + shared=shared_l3p, + l3_policy_id=l3p['id'])['l2_policy'] + for x in range(0, 3)] + + # update L3P with ES + l3p = self.update_l3_policy(l3p['id'], tenant_id=l3p['tenant_id'], + external_segments={es['id']: []}, + expected_res_status=200)['l3_policy'] self.assertEqual(1, len(l3p['external_segments'][es['id']])) self.assertEqual('169.254.0.2', l3p['external_segments'][es['id']][0]) - if is_edge_nat: - l2p = self.create_l2_policy(name='myl2p', - l3_policy_id=l3p['id'])['l2_policy'] - mgr = self.driver.apic_manager owner = self.common_tenant if shared_es else es['tenant_id'] l3p_owner = self.common_tenant if shared_l3p else l3p['tenant_id'] @@ -2348,18 +2360,24 @@ class TestL3Policy(ApicMappingTestCase): self.assertFalse(mgr.ensure_logical_node_profile_created.called) self.assertFalse(mgr.ensure_static_route_created.called) + expected_set_l3out_for_bd_calls = [] if self.nat_enabled: - expected_set_l3out_for_bd_calls = [ + expected_set_l3out_for_bd_calls.append( mock.call(owner, "NAT-bd-%s" % es['id'], es['name' if self.pre_l3out else 'id'], - transaction=mock.ANY)] + transaction=mock.ANY)) if is_edge_nat: - expected_set_l3out_for_bd_calls.append( + expected_set_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es['id']), - transaction=mock.ANY)) - self._check_call_list(expected_set_l3out_for_bd_calls, - mgr.set_l3out_for_bd.call_args_list) + transaction=mock.ANY) for l2p in l2ps]) + else: + expected_set_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es['name' if self.pre_l3out else 'id']) + for l2p in l2ps]) + self._check_call_list(expected_set_l3out_for_bd_calls, + mgr.set_l3out_for_bd.call_args_list) # Although the naming convention used here has been chosen poorly, # I'm separating the tests in order to get the mock re-set. @@ -2536,9 +2554,11 @@ class TestL3Policy(ApicMappingTestCase): external_segments={es1['id']: ['169.254.0.3']}, expected_res_status=201)['l3_policy'] - if is_edge_nat: - l2p = self.create_l2_policy(name='myl2p', - l3_policy_id=l3p['id'])['l2_policy'] + l2ps = [self.create_l2_policy(name='myl2p-%s' % x, + tenant_id=l3p['tenant_id'], + shared=shared_l3p, + l3_policy_id=l3p['id'])['l2_policy'] + for x in range(0, 3)] mgr = self.driver.apic_manager owner = self.common_tenant if shared_es else es1['tenant_id'] @@ -2579,19 +2599,24 @@ class TestL3Policy(ApicMappingTestCase): expected_calls, mgr.set_context_for_external_routed_network.call_args_list) + expected_unset_l3out_for_bd_calls = [] if self.nat_enabled: - expected_unset_l3out_for_bd_calls = [] if is_edge_nat: - expected_unset_l3out_for_bd_calls.append( + expected_unset_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es1['id']), - transaction=mock.ANY)) + transaction=mock.ANY) for l2p in l2ps]) expected_unset_l3out_for_bd_calls.append( mock.call(owner, "NAT-bd-%s" % es1['id'], es1['name' if self.pre_l3out else 'id'], transaction=mock.ANY)) - self._check_call_list(expected_unset_l3out_for_bd_calls, - mgr.unset_l3out_for_bd.call_args_list) + else: + expected_unset_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es1['name' if self.pre_l3out else 'id']) + for l2p in l2ps]) + self._check_call_list(expected_unset_l3out_for_bd_calls, + mgr.unset_l3out_for_bd.call_args_list) if is_edge_nat and self.nat_enabled: self.driver.l3out_vlan_alloc.release_vlan.assert_called_once_with( @@ -2657,19 +2682,24 @@ class TestL3Policy(ApicMappingTestCase): self.assertFalse(mgr.ensure_static_route_created.called) + expected_set_l3out_for_bd_calls = [] if self.nat_enabled: - expected_set_l3out_for_bd_calls = [] expected_set_l3out_for_bd_calls.append( mock.call(owner, "NAT-bd-%s" % es2['id'], es2['name' if self.pre_l3out else 'id'], transaction=mock.ANY)) if is_edge_nat: - expected_set_l3out_for_bd_calls.append( + expected_set_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es2['id']), - transaction=mock.ANY)) - self._check_call_list(expected_set_l3out_for_bd_calls, - mgr.set_l3out_for_bd.call_args_list) + transaction=mock.ANY) for l2p in l2ps]) + else: + expected_set_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es2['name' if self.pre_l3out else 'id']) + for l2p in l2ps]) + self._check_call_list(expected_set_l3out_for_bd_calls, + mgr.set_l3out_for_bd.call_args_list) self.driver.l3out_vlan_alloc.release_vlan.reset_mock() mgr.delete_external_routed_network.reset_mock() @@ -2696,17 +2726,17 @@ class TestL3Policy(ApicMappingTestCase): self._check_call_list( expected_delete_calls, mgr.delete_external_routed_network.call_args_list) + expected_unset_l3out_for_bd_calls = [] if self.nat_enabled: - expected_unset_l3out_for_bd_calls = [] if is_edge_nat: - expected_unset_l3out_for_bd_calls.append( + expected_unset_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es1['id']), - transaction=mock.ANY)) - expected_unset_l3out_for_bd_calls.append( + transaction=mock.ANY) for l2p in l2ps]) + expected_unset_l3out_for_bd_calls.extend([ mock.call(l3p_owner, l2p['id'], l3out_str % (l3p['id'], es2['id']), - transaction=mock.ANY)) + transaction=mock.ANY) for l2p in l2ps]) expected_unset_l3out_for_bd_calls.append( mock.call(owner, "NAT-bd-%s" % es1['id'], es1['name' if self.pre_l3out else 'id'], @@ -2715,8 +2745,17 @@ class TestL3Policy(ApicMappingTestCase): mock.call(owner, "NAT-bd-%s" % es2['id'], es2['name' if self.pre_l3out else 'id'], transaction=mock.ANY)) - self._check_call_list(expected_unset_l3out_for_bd_calls, - mgr.unset_l3out_for_bd.call_args_list) + else: + expected_unset_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es1['name' if self.pre_l3out else 'id']) + for l2p in l2ps]) + expected_unset_l3out_for_bd_calls.extend([ + mock.call(l3p_owner, l2p['id'], + es2['name' if self.pre_l3out else 'id']) + for l2p in l2ps]) + self._check_call_list(expected_unset_l3out_for_bd_calls, + mgr.unset_l3out_for_bd.call_args_list) if self.pre_l3out and not self.nat_enabled: expected_calls = [