From a786706373edce5a29b26b2ffcabf471f18000a5 Mon Sep 17 00:00:00 2001 From: Thomas Bachman Date: Thu, 24 Jan 2019 02:07:08 +0000 Subject: [PATCH] Add support for CIDRs in allowed-adddress-pairs The current support for allowed-address-pairs only covers host addresses. This patch adds support for using CIDRs in allowed address pairs. Since CIDRs are already supported in upstream neutron, this patch only affects the backend implementation, which is the get_gbp_details RPC and the port notifications. The patch also addresses cleanup of the IP => Port ID mapping table. Entries in the table weren't affected if an existing port's allowed-address-pairs property was changed. This patch checks to see if the changes address any addresses in the table, and if needed, removes them. Change-Id: I7bcfee46b0239577c500f2fa970e4c8400c968d0 --- .../drivers/apic_aim/mechanism_driver.py | 44 +++++- .../drivers/cisco/apic/aim_mapping.py | 22 ++- .../drivers/cisco/apic/aim_mapping_rpc.py | 4 +- .../cisco/apic/port_ha_ipaddress_binding.py | 4 +- .../grouppolicy/test_aim_mapping_driver.py | 131 ++++++++++++++++-- 5 files changed, 179 insertions(+), 26 deletions(-) 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 0e3741fcf..d9a4ddb60 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py @@ -2074,6 +2074,32 @@ class ApicMechanismDriver(api_plus.MechanismDriver, context._plugin_context, port['id'], resources.PORT, provisioning_blocks.L2_AGENT_ENTITY) + def _check_allowed_address_pairs(self, context, port): + if not self.gbp_driver: + return + aap_current = context.current.get('allowed_address_pairs', []) + aap_original = context.original.get('allowed_address_pairs', []) + # If there was a change in configured AAPs, then we may need + # to clean up the owned IPs table + p_context = context._plugin_context + if aap_current != aap_original: + curr_ips = [aap['ip_address'] for aap in aap_current] + orig_ips = [aap['ip_address'] for aap in aap_original] + removed = list(set(orig_ips) - set(curr_ips)) + for aap in removed: + cidr = netaddr.IPNetwork(aap) + with db_api.context_manager.writer.using(p_context) as session: + # Get all the owned IP addresses for the port, and if + # they match a removed AAP entry, delete that entry + # from the DB + ha_handler = self.gbp_driver.ha_ip_handler + ha_ips = ha_handler.get_ha_ipaddresses_for_port(port['id'], + session=session) + for ip in ha_ips: + if ip in cidr: + ha_handler.delete_port_id_for_ha_ipaddress( + port['id'], ip, session=session) + def update_port_precommit(self, context): port = context.current if context.original_host and context.original_host != context.host: @@ -2092,6 +2118,7 @@ class ApicMechanismDriver(api_plus.MechanismDriver, context.bottom_bound_segment[api.NETWORK_TYPE])): self._associate_domain(context, is_vmm=True) self._update_sg_rule_with_remote_group_set(context, port) + self._check_allowed_address_pairs(context, port) self._insert_provisioning_block(context) registry.notify(aim_cst.GBP_PORT, events.PRECOMMIT_UPDATE, self, driver_context=context) @@ -3459,14 +3486,19 @@ class ApicMechanismDriver(api_plus.MechanismDriver, models_v2.Port.id == n_addr_pair_db.AllowedAddressPair.port_id) query += lambda q: q.filter( models_v2.Port.network_id == sa.bindparam('network_id')) - query += lambda q: q.filter( - n_addr_pair_db.AllowedAddressPair.ip_address.in_( - sa.bindparam('fixed_ips', expanding=True))) addr_pair = query(plugin_context.session).params( - network_id=port['network_id'], - fixed_ips=fixed_ips).all() + network_id=port['network_id']).all() + notify_pairs = [] + # In order to support use of CIDRs in allowed-address-pairs, + # we can't include the fxied IPs in the DB query, and instead + # have to qualify that with post-DB processing + for a_pair in addr_pair: + cidr = netaddr.IPNetwork(a_pair['ip_address']) + for addr in fixed_ips: + if addr in cidr: + notify_pairs.append(a_pair) - ports_to_notify.extend([x['port_id'] for x in addr_pair]) + ports_to_notify.extend([x['port_id'] for x in set(notify_pairs)]) for p in sorted(ports_to_notify): self._notify_port_update(plugin_context, p) diff --git a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping.py b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping.py index 2c5f63b83..c3a6d391e 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping.py @@ -11,6 +11,7 @@ # under the License. import hashlib +import netaddr import re import six import sqlalchemy as sa @@ -1942,11 +1943,27 @@ class AIMMappingDriver(nrd.CommonNeutronBase, aim_rpc.AIMMappingRPCMixin): else: owned_addresses = self._get_owned_addresses( plugin_context, port['id']) + extra_aaps = [] for allowed in aaps: - if allowed['ip_address'] in owned_addresses: + cidr = netaddr.IPNetwork(allowed['ip_address']) + if ((cidr.version == 4 and cidr.prefixlen != 32) or + (cidr.version == 6 and cidr.prefixlen != 128)): + # Never mark CIDRs as "active", but + # look for owned addresses in this CIDR, and + # if present, add them to the allowed-address-pairs + # list, and mark those as "active" + for addr in owned_addresses: + entry = {'ip_address': addr, + 'mac_address': allowed['mac_address'], + 'active': True} + if addr in cidr and entry not in extra_aaps: + extra_aaps.append(entry) + elif allowed['ip_address'] in owned_addresses: # Signal the agent that this particular address is active # on its port allowed['active'] = True + if extra_aaps: + aaps.extend(extra_aaps) return aaps def _get_port_vrf(self, plugin_context, port, details): @@ -2080,7 +2097,8 @@ class AIMMappingDriver(nrd.CommonNeutronBase, aim_rpc.AIMMappingRPCMixin): plugin_context, filters={'network_id': [port['network_id']], 'fixed_ips': {'ip_address': active_addrs}}) - fips_filter.extend([p['id'] for p in others]) + fips_filter.extend([p['id'] for p in others + if p['id'] != port['id']]) fips = self._get_fips(plugin_context, filters={'port_id': fips_filter}) diff --git a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping_rpc.py b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping_rpc.py index 20fb2b0e7..89c2d9975 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping_rpc.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping_rpc.py @@ -627,9 +627,7 @@ class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin): # network whose address is owned by this port. # If those ports have FIPs, then steal them. fips_filter = [str(port_id)] - active_addrs = [str(a['ip_address']) - for a in details['allowed_address_pairs'] - if a.get('active')] + active_addrs = details['_cache']['owned_addresses'] if active_addrs: in_str = self._compose_in_filter_str(active_addrs) ports_query = ( diff --git a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/port_ha_ipaddress_binding.py b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/port_ha_ipaddress_binding.py index 3bd3074bd..e92b48ca4 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/port_ha_ipaddress_binding.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/port_ha_ipaddress_binding.py @@ -81,9 +81,9 @@ class PortForHAIPAddress(object): ipaddress=ipaddress, network_id=network_id).first() return port_ha_ip - def get_ha_ipaddresses_for_port(self, port_id): + def get_ha_ipaddresses_for_port(self, port_id, session=None): """Returns the HA IP Addressses associated with a Port.""" - session = db_api.get_reader_session() + session = session or db_api.get_reader_session() query = BAKERY(lambda s: s.query( HAIPAddressToPortAssocation)) diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_mapping_driver.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_mapping_driver.py index 2ac5e3767..71df38dbe 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_mapping_driver.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_mapping_driver.py @@ -5704,7 +5704,15 @@ class TestNeutronPortOperation(AIMBaseTestCase): host='h1') self.assertTrue(details['promiscuous_mode']) - def test_gbp_details_for_allowed_address_pair(self): + def _aap_is_cidr(self, aap): + cidr = netaddr.IPNetwork(aap['ip_address']) + if cidr.prefixlen != 32: + return True + else: + return False + + def _test_gbp_details_for_allowed_address_pair(self, allow_addr, + owned_addr, update_addr, update_owned_addr): self._register_agent('h1', test_aim_md.AGENT_CONF_OPFLEX) self._register_agent('h2', test_aim_md.AGENT_CONF_OPFLEX) net = self._make_network(self.fmt, 'net1', True) @@ -5712,12 +5720,8 @@ class TestNeutronPortOperation(AIMBaseTestCase): 'subnet'] sub2 = self._make_subnet(self.fmt, net, '1.2.3.1', '1.2.3.0/24')[ 'subnet'] - allow_addr = [{'ip_address': '1.2.3.250', - 'mac_address': '00:00:00:AA:AA:AA'}, - {'ip_address': '1.2.3.251', - 'mac_address': '00:00:00:BB:BB:BB'}] - # create 2 ports with same allowed-addresses + # create 2 ports configured with the same allowed-addresses p1 = self._make_port(self.fmt, net['network']['id'], arg_list=('allowed_address_pairs',), device_owner='compute:', @@ -5730,22 +5734,63 @@ class TestNeutronPortOperation(AIMBaseTestCase): allowed_address_pairs=allow_addr)['port'] self._bind_port_to_host(p1['id'], 'h1') self._bind_port_to_host(p2['id'], 'h2') - self.driver.ha_ip_handler.set_port_id_for_ha_ipaddress( - p1['id'], '1.2.3.250') - self.driver.ha_ip_handler.set_port_id_for_ha_ipaddress( - p2['id'], '1.2.3.251') - allow_addr[0]['active'] = True + # Call agent => plugin RPC to get the details for each port. The + # results should only have the configured AAPs, with none of them + # active. details = self.driver.get_gbp_details( self._neutron_admin_context, device='tap%s' % p1['id'], host='h1') self.assertEqual(allow_addr, details['allowed_address_pairs']) - del allow_addr[0]['active'] - allow_addr[1]['active'] = True details = self.driver.get_gbp_details( self._neutron_admin_context, device='tap%s' % p2['id'], host='h2') self.assertEqual(allow_addr, details['allowed_address_pairs']) + # Call agent => plugin RPC, requesting ownership of a /32 IP + ip_owner_info = {'port': p1['id'], + 'ip_address_v4': owned_addr[0], + 'network_id': p1['network_id']} + self.driver.update_ip_owner(ip_owner_info) + # Call RPC sent by the agent to get the details for p1 + details = self.driver.get_gbp_details( + self._neutron_admin_context, device='tap%s' % p1['id'], + host='h1') + + # response should be a list containing both configured and owned AAPs. + # Only the owned entries will have the 'active' property (set to True). + def _get_expected_aaps(allow_addr, owned_addr): + extra_aaps = [] + expected_aaps = copy.deepcopy(allow_addr) + for aap in expected_aaps: + if aap['ip_address'] == owned_addr: + aap['active'] = True + elif self._aap_is_cidr(aap): + # If the configured AAP is a CIDR, the response should + # include both the CIDR (not active) and the owned IP + # address (i.e. /32) from that CIDR (active) + extra_aaps.append({'ip_address': owned_addr, + 'mac_address': aap['mac_address'], + 'active': True}) + if extra_aaps: + expected_aaps.extend(extra_aaps) + return expected_aaps + + expected_aaps1 = _get_expected_aaps(allow_addr, owned_addr[0]) + self.assertEqual(expected_aaps1, details['allowed_address_pairs']) + + # Call RPC sent by the agent, requesting ownership of a /32 IP + ip_owner_info = {'port': p2['id'], + 'ip_address_v4': owned_addr[1], + 'network_id': p2['network_id']} + self.driver.update_ip_owner(ip_owner_info) + # Call RPC sent by the agent to get the details for p2 + details = self.driver.get_gbp_details( + self._neutron_admin_context, device='tap%s' % p2['id'], + host='h2') + expected_aaps2 = _get_expected_aaps(allow_addr, owned_addr[1]) + + self.assertEqual(expected_aaps2, details['allowed_address_pairs']) + # set allowed-address as fixed-IP of ports p3 and p4, which also have # floating-IPs. Verify that FIP is "stolen" by p1 and p2 net_ext, rtr, _ = self._setup_external_network( @@ -5802,6 +5847,66 @@ class TestNeutronPortOperation(AIMBaseTestCase): expected_calls, self.driver.aim_mech_driver._notify_port_update.call_args_list) + # Change the allowed address pair, and verify that the IP(s) + # from the old pair are removed from the mapping table + p1 = self._update('ports', p1['id'], + {'port': {'allowed_address_pairs': update_addr}}, + neutron_context=self._neutron_admin_context)['port'] + ips = self.driver.ha_ip_handler.get_ha_ipaddresses_for_port(p1['id']) + self.assertEqual(ips, []) + # Request ownership of the new AAP + ip_owner_info = {'port': p1['id'], + 'ip_address_v4': update_owned_addr[0], + 'network_id': p1['network_id']} + self.driver.update_ip_owner(ip_owner_info) + details = self.driver.get_gbp_details( + self._neutron_admin_context, device='tap%s' % p1['id'], + host='h1') + expected_aaps3 = _get_expected_aaps(update_addr, update_owned_addr[0]) + self.assertEqual(expected_aaps3, details['allowed_address_pairs']) + + p2 = self._update('ports', p2['id'], + {'port': {'allowed_address_pairs': update_addr}}, + neutron_context=self._neutron_admin_context)['port'] + ips = self.driver.ha_ip_handler.get_ha_ipaddresses_for_port(p2['id']) + self.assertEqual(ips, []) + # Request ownership of the new AAP + ip_owner_info = {'port': p2['id'], + 'ip_address_v4': update_owned_addr[1], + 'network_id': p2['network_id']} + self.driver.update_ip_owner(ip_owner_info) + details = self.driver.get_gbp_details( + self._neutron_admin_context, device='tap%s' % p2['id'], + host='h2') + expected_aaps4 = _get_expected_aaps(update_addr, update_owned_addr[1]) + self.assertEqual(expected_aaps4, details['allowed_address_pairs']) + + def test_gbp_details_for_allowed_address_pair(self): + # 'aap' is configured, 'owned' is IP requested from agent + allow_addr = [{'ip_address': '1.2.3.250', + 'mac_address': '00:00:00:AA:AA:AA'}, + {'ip_address': '1.2.3.251', + 'mac_address': '00:00:00:BB:BB:BB'}] + owned_addr = ['1.2.3.250', '1.2.3.251'] + update_addr = [{'ip_address': '2.3.4.250', + 'mac_address': '00:00:00:CC:CC:CC'}, + {'ip_address': '2.3.4.251', + 'mac_address': '00:00:00:DD:DD:DD'}] + update_owned_addr = ['2.3.4.250', '2.3.4.251'] + self._test_gbp_details_for_allowed_address_pair(allow_addr, + owned_addr, update_addr, update_owned_addr) + + def test_gbp_details_for_allowed_address_pair_cidr(self): + # 'aap' is configured, 'owned' is IP requested from agent + allow_addr = [{'ip_address': '1.2.3.0/24', + 'mac_address': '00:00:00:AA:AA:AA'}] + owned_addr = ['1.2.3.250', '1.2.3.251'] + update_addr = [{'ip_address': '2.3.4.0/24', + 'mac_address': '00:00:00:BB:BB:BB'}] + update_owned_addr = ['2.3.4.250', '2.3.4.251'] + self._test_gbp_details_for_allowed_address_pair(allow_addr, + owned_addr, update_addr, update_owned_addr) + def test_port_bound_other_agent(self): self._register_agent('h1', test_aim_md.AGENT_CONF_OPFLEX) self._register_agent('h2', test_aim_md.AGENT_CONF_OPFLEX)