From cff0663e197bd3b6f245d56b6bcdd127c124ba92 Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Mon, 3 May 2021 07:31:18 -0700 Subject: [PATCH] Relax IPAM validation for port IPs Allow for setting multiple IPs from the same subnet, only if the subnet is v4 and DHCP is disabled for the subnet. Still make sure at most 1 IPv4 and 1 IPv6 subnet can be configured. Change-Id: I1113a24fa8cc09892bc89917d50c64f6a72c0dab --- vmware_nsx/plugins/common_v3/plugin.py | 106 ++++++++++++++---- .../tests/unit/common_plugin/common_v3.py | 72 +++++++++++- vmware_nsx/tests/unit/nsx_v3/test_plugin.py | 31 +---- 3 files changed, 156 insertions(+), 53 deletions(-) diff --git a/vmware_nsx/plugins/common_v3/plugin.py b/vmware_nsx/plugins/common_v3/plugin.py index 0a47d547b6..97adaca135 100644 --- a/vmware_nsx/plugins/common_v3/plugin.py +++ b/vmware_nsx/plugins/common_v3/plugin.py @@ -627,30 +627,88 @@ class NsxPluginV3Base(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, if not validators.is_attr_set(fixed_ip_list): return - msg = _('Exceeded maximum amount of fixed ips per port and ip version') - if len(fixed_ip_list) > 2: - raise n_exc.InvalidInput(error_message=msg) - - if len(fixed_ip_list) < 2: + # Do not perform additional checks in the trivial case where there is a + # single fixed IP + if len(fixed_ip_list) <= 1: return - def get_fixed_ip_version(i): - if 'ip_address' in fixed_ip_list[i]: - return netaddr.IPAddress( - fixed_ip_list[i]['ip_address']).version - if 'subnet_id' in fixed_ip_list[i]: - subnet = self.get_subnet(context.elevated(), - fixed_ip_list[i]['subnet_id']) - return subnet['ip_version'] - return None + # Validation needed + err_msg = _("Exceeded maximum amount of fixed IPs " + "per port and IP version") + ip1_err_msg = ("Subnet %s is IPv6 or has DHCP enabled. IP %s " + "already allocated for this port. Cannot allocate %s") + ip2_err_msg = ("Subnet %s is IPv6 or has DHCP enabled. Can only " + "allocate a single IP per subnet. Requested %d") - ipver1 = get_fixed_ip_version(0) - ipver2 = get_fixed_ip_version(1) - if ipver1 and ipver2 and ipver1 != ipver2: - # One fixed IP is allowed for each IP version - return + # Build a map of fixed IPs per subnet id + subnet_fixed_ip_dict = {} + fixed_ips_no_subnet = [] + for item in fixed_ip_list: + if item.get('subnet_id'): + subnet_id = item['subnet_id'] + subnet_fixed_ips = subnet_fixed_ip_dict.setdefault( + subnet_id, []) + subnet_fixed_ips.append(item.get('ip_address', 'UNSPECIFIED')) + else: + # Log something so we know we know this entry will be + # processed differently + LOG.debug("Fixed IP entry %s has no subnet ID", item) + if item.get('ip_address'): + fixed_ips_no_subnet.append(item['ip_address']) + if len(subnet_fixed_ip_dict) > 2: + raise n_exc.InvalidInput(error_message=err_msg) - raise n_exc.InvalidInput(error_message=msg) + # This works as a cache, fetch from DB only the subnets we need + # and reuse them -> optimize DB access + subnet_map = {} + + def infer_fixed_ip_version(subnet_id, fixed_ips): + # Infer from IPs + for ip_address in fixed_ips: + if ip_address != 'UNSPECIFIED': + return (subnet_id, + netaddr.IPAddress(ip_address).version) + # Infer from subnet + subnet = subnet_map.setdefault( + subnet_id, self.get_subnet(context.elevated(), subnet_id)) + return (subnet_id, subnet['ip_version']) + + # If we are here we have at most 2 subnets, and must have different IP + # version + ip_versions = [infer_fixed_ip_version(subnet_id, fixed_ips) for + (subnet_id, fixed_ips) in subnet_fixed_ip_dict.items()] + + for item in zip(ip_versions, ip_versions): + if item[0][0] != item[1][0] and item[0][1] == item[1][1]: + # Not good! Different subnet but same IP version + # One fixed IP is allowed for each IP version + raise n_exc.InvalidInput(error_message=err_msg) + + def validate_subnet_dhcp_and_ipv6_state(subnet_id, fixed_ips): + subnet = subnet_map.setdefault( + subnet_id, self.get_subnet(context.elevated(), subnet_id)) + if ((subnet['enable_dhcp'] or subnet.get('ip_version') == 6) and + len(fixed_ips) > 1): + raise n_exc.InvalidInput( + error_message=ip2_err_msg % + (subnet_id, len(fixed_ips))) + if ((subnet['enable_dhcp'] or subnet.get('ip_version') == 6) and + fixed_ips): + cidr = netaddr.IPNetwork(subnet['cidr']) + for ip in fixed_ips_no_subnet: + # netaddr converts string IP into IPAddress + if ip in cidr: + raise n_exc.InvalidInput( + error_message=ip1_err_msg % + (subnet_id, ",".join(fixed_ips), ip)) + + # Validate if any "unbound" IP would fall on a subnet which has DHCP + # enabled and other IPs. Also ensure no more than one IP for any + # DHCP subnet is requested. fixed_ip attribute does not have patch + # behaviour, so all requested IP allocations must be included in it + + for subnet_id, fixed_ips in subnet_fixed_ip_dict.items(): + validate_subnet_dhcp_and_ipv6_state(subnet_id, fixed_ips) def _get_subnets_for_fixed_ips_on_port(self, context, port_data): # get the subnet id from the fixed ips of the port @@ -1696,10 +1754,11 @@ class NsxPluginV3Base(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, LOG.error("Unable to delete DHCP server mapping for " "network %s", network_id) - def _filter_ipv4_dhcp_fixed_ips(self, context, fixed_ips): + def _filter_dhcp_fixed_ips(self, context, fixed_ips, version=None): ips = [] for fixed_ip in fixed_ips: - if netaddr.IPNetwork(fixed_ip['ip_address']).version != 4: + if (version and + netaddr.IPNetwork(fixed_ip['ip_address']).version != version): continue with db_api.CONTEXT_READER.using(context): subnet = self.get_subnet(context, fixed_ip['subnet_id']) @@ -1707,6 +1766,9 @@ class NsxPluginV3Base(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, ips.append(fixed_ip) return ips + def _filter_ipv4_dhcp_fixed_ips(self, context, fixed_ips): + return self._filter_dhcp_fixed_ips(context, fixed_ips, 4) + def _add_port_mp_dhcp_binding(self, context, port): if not utils.is_port_dhcp_configurable(port): return diff --git a/vmware_nsx/tests/unit/common_plugin/common_v3.py b/vmware_nsx/tests/unit/common_plugin/common_v3.py index a83e008bbf..c3a25b5a37 100644 --- a/vmware_nsx/tests/unit/common_plugin/common_v3.py +++ b/vmware_nsx/tests/unit/common_plugin/common_v3.py @@ -17,9 +17,12 @@ import contextlib import decorator + from neutron.tests.unit.db import test_db_base_plugin_v2 as test_plugin +import netaddr from neutron_lib import constants +from webob import exc class FixExternalNetBaseTest(object): @@ -400,8 +403,75 @@ class NsxV3TestPorts(test_plugin.TestPortsV2): def test_create_port_anticipating_allocation(self): self.skipTest('Multiple fixed ips on a port are not supported') + def test_create_port_additional_ip(self): + """Test that creation of port with additional IP fails.""" + with self.subnet() as subnet: + data = {'port': {'network_id': subnet['subnet']['network_id'], + 'tenant_id': subnet['subnet']['tenant_id'], + 'fixed_ips': [{'subnet_id': + subnet['subnet']['id']}, + {'subnet_id': + subnet['subnet']['id']}]}} + port_req = self.new_create_request('ports', data) + res = port_req.get_response(self.api) + self.assertEqual(exc.HTTPBadRequest.code, + res.status_int) + + def test_create_port_additional_ip_no_dhcp(self): + """Creation of port with additional IP succeeds with DHCP off.""" + with self.subnet(enable_dhcp=False) as subnet: + data = {'port': {'network_id': subnet['subnet']['network_id'], + 'tenant_id': subnet['subnet']['tenant_id'], + 'fixed_ips': [{'subnet_id': + subnet['subnet']['id']}, + {'subnet_id': + subnet['subnet']['id']}]}} + port_req = self.new_create_request('ports', data) + res = port_req.get_response(self.api) + port = self.deserialize(self.fmt, res) + ips = port['port']['fixed_ips'] + self.assertEqual(2, len(ips)) + self.assertNotEqual(ips[0]['ip_address'], + ips[1]['ip_address']) + network_ip_net = netaddr.IPNetwork(subnet['subnet']['cidr']) + self.assertIn(ips[0]['ip_address'], network_ip_net) + self.assertIn(ips[1]['ip_address'], network_ip_net) + def test_update_port_add_additional_ip(self): - self.skipTest('Multiple fixed ips on a port are not supported') + """Test update of port with additional IP fails.""" + with self.subnet() as subnet: + with self.port(subnet=subnet) as port: + data = {'port': {'admin_state_up': False, + 'fixed_ips': [{'subnet_id': + subnet['subnet']['id']}, + {'subnet_id': + subnet['subnet']['id']}]}} + req = self.new_update_request('ports', data, + port['port']['id']) + res = req.get_response(self.api) + self.assertEqual(exc.HTTPBadRequest.code, + res.status_int) + + def test_update_port_add_additional_ip_no_dhcp(self): + """Test update of port with additional IP succeeds if DHCP is off.""" + with self.subnet(enable_dhcp=False) as subnet: + with self.port(subnet=subnet) as port: + data = {'port': {'admin_state_up': False, + 'fixed_ips': [{'subnet_id': + subnet['subnet']['id']}, + {'subnet_id': + subnet['subnet']['id']}]}} + req = self.new_update_request('ports', data, + port['port']['id']) + res = req.get_response(self.api) + port = self.deserialize(self.fmt, res) + ips = port['port']['fixed_ips'] + self.assertEqual(2, len(ips)) + self.assertNotEqual(ips[0]['ip_address'], + ips[1]['ip_address']) + network_ip_net = netaddr.IPNetwork(subnet['subnet']['cidr']) + self.assertIn(ips[0]['ip_address'], network_ip_net) + self.assertIn(ips[1]['ip_address'], network_ip_net) def test_delete_network_port_exists_owned_by_network_race(self): self.skipTest('Skip need to address in future') diff --git a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py index 0780b6b08d..0e8f4a2ffe 100644 --- a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py +++ b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py @@ -1141,7 +1141,7 @@ class TestPortsV2(common_v3.NsxV3SubnetMixin, 'fixed_ips': []} } port = self.plugin.create_port(self.ctx, data) - data['port']['fixed_ips'] = '10.0.0.1' + data['port']['fixed_ips'] = [{'ip_address': '10.0.0.1'}] self.assertRaises( n_exc.InvalidInput, self.plugin.update_port, self.ctx, port['id'], data) @@ -1589,35 +1589,6 @@ class TestPortsV2(common_v3.NsxV3SubnetMixin, created_port = self.plugin.create_port(self.ctx, data) self.assertEqual('fake_device', created_port['device_id']) - def test_update_port_add_additional_ip(self): - """Test update of port with additional IP fails.""" - with self.subnet() as subnet: - with self.port(subnet=subnet) as port: - data = {'port': {'admin_state_up': False, - 'fixed_ips': [{'subnet_id': - subnet['subnet']['id']}, - {'subnet_id': - subnet['subnet']['id']}]}} - req = self.new_update_request('ports', data, - port['port']['id']) - res = req.get_response(self.api) - self.assertEqual(exc.HTTPBadRequest.code, - res.status_int) - - def test_create_port_additional_ip(self): - """Test that creation of port with additional IP fails.""" - with self.subnet() as subnet: - data = {'port': {'network_id': subnet['subnet']['network_id'], - 'tenant_id': subnet['subnet']['tenant_id'], - 'fixed_ips': [{'subnet_id': - subnet['subnet']['id']}, - {'subnet_id': - subnet['subnet']['id']}]}} - port_req = self.new_create_request('ports', data) - res = port_req.get_response(self.api) - self.assertEqual(exc.HTTPBadRequest.code, - res.status_int) - def test_create_port_with_switching_profiles(self): """Tests that nsx ports get the configures switching profiles""" self.plugin = directory.get_plugin()