From 8a9aac391773a318f669aa971af920eaaf3c2b49 Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Mon, 18 Sep 2017 14:10:19 +0300 Subject: [PATCH] NSX|V3 Add validations to DHCP relay 1. add relay service only if the subnet is with dhcp 2. do not add native dhcp if dhcp relay is configured for this subnet 3. do not allow router creation with dhcp relay and without IPAM 4. do not allow creation of VM port from a network with dhcp relay and without a router. Change-Id: I05fa71f69ded69ea58a4e4df0a1f20c963cb3fc5 --- .../plugins/nsx_v3/availability_zones.py | 6 + vmware_nsx/plugins/nsx_v3/plugin.py | 79 ++++++++++++- vmware_nsx/tests/unit/nsx_v3/test_plugin.py | 110 ++++++++++++++++-- 3 files changed, 177 insertions(+), 18 deletions(-) diff --git a/vmware_nsx/plugins/nsx_v3/availability_zones.py b/vmware_nsx/plugins/nsx_v3/availability_zones.py index 9ab854a472..021e3e2dfc 100644 --- a/vmware_nsx/plugins/nsx_v3/availability_zones.py +++ b/vmware_nsx/plugins/nsx_v3/availability_zones.py @@ -201,3 +201,9 @@ class NsxV3AvailabilityZones(common_az.ConfiguredAvailabilityZones): super(NsxV3AvailabilityZones, self).__init__( cfg.CONF.nsx_v3.availability_zones, NsxV3AvailabilityZone) + + def dhcp_relay_configured(self): + for az in self.availability_zones.values(): + if az.dhcp_relay_service: + return True + return False diff --git a/vmware_nsx/plugins/nsx_v3/plugin.py b/vmware_nsx/plugins/nsx_v3/plugin.py index 8a2a9e98fe..6ff764fddb 100644 --- a/vmware_nsx/plugins/nsx_v3/plugin.py +++ b/vmware_nsx/plugins/nsx_v3/plugin.py @@ -1382,7 +1382,6 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, def create_subnet(self, context, subnet): self._validate_address_space(subnet['subnet']) - # TODO(berlin): public external subnet announcement if (cfg.CONF.nsx_v3.native_dhcp_metadata and subnet['subnet'].get('enable_dhcp', False)): @@ -1399,8 +1398,12 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, NsxV3Plugin, self).create_subnet(context, subnet) self._extension_manager.process_create_subnet(context, subnet['subnet'], created_subnet) - self._enable_native_dhcp(context, network, - created_subnet) + dhcp_relay = self.get_network_az_by_net_id( + context, + subnet['subnet']['network_id']).dhcp_relay_service + if not dhcp_relay: + self._enable_native_dhcp(context, network, + created_subnet) msg = None else: msg = (_("Can not create more than one DHCP-enabled " @@ -1849,6 +1852,52 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, LOG.warning(err_msg) raise n_exc.InvalidInput(error_message=err_msg) + def _assert_on_dhcp_relay_without_router(self, context, port_data, + original_port=None): + # Prevent creating/updating port with device owner prefix 'compute' + # on a subnet with dhcp relay but no router. + if not original_port: + original_port = port_data + device_owner = port_data.get('device_owner') + if (device_owner is None or + not device_owner.startswith(const.DEVICE_OWNER_COMPUTE_PREFIX)): + # not a compute port + return + + if not self.get_network_az_by_net_id( + context, + original_port['network_id']).dhcp_relay_service: + # No dhcp relay for the net of this port + return + + # get the subnet id from the fixed ips of the port + if 'fixed_ips' in port_data and port_data['fixed_ips']: + subnet_id = port_data['fixed_ips'][0]['subnet_id'] + elif 'fixed_ips' in original_port and original_port['fixed_ips']: + subnet_id = original_port['fixed_ips'][0]['subnet_id'] + else: + return + + # check only dhcp enabled subnets + subnet = self.get_subnet(context.elevated(), subnet_id) + if not subnet['enable_dhcp']: + return + + # check if the subnet is attached to a router + port_filters = {'device_owner': [l3_db.DEVICE_OWNER_ROUTER_INTF], + 'network_id': [original_port['network_id']]} + interfaces = self.get_ports(context.elevated(), filters=port_filters) + router_found = False + for interface in interfaces: + if interface['fixed_ips'][0]['subnet_id'] == subnet_id: + router_found = True + break + if not router_found: + err_msg = _("Neutron is configured with DHCP_Relay but no router " + "connected to the subnet") + LOG.warning(err_msg) + raise n_exc.InvalidInput(error_message=err_msg) + def _cleanup_port(self, context, port_id, lport_id): super(NsxV3Plugin, self).delete_port(context, port_id) if lport_id: @@ -2220,6 +2269,7 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, self._validate_extra_dhcp_options(dhcp_opts) self._validate_max_ips_per_port(port_data.get('fixed_ips', []), port_data.get('device_owner')) + self._assert_on_dhcp_relay_without_router(context, port_data) # TODO(salv-orlando): Undo logical switch creation on failure with db_api.context_manager.writer.using(context): @@ -2662,6 +2712,8 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, if is_external_net: self._assert_on_external_net_with_compute(port_data) self._assert_on_external_net_port_with_qos(port_data) + self._assert_on_dhcp_relay_without_router(context, port_data, + original_port) dhcp_opts = port_data.get(ext_edo.EXTRADHCPOPTS) self._validate_extra_dhcp_options(dhcp_opts) @@ -3001,9 +3053,18 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, LOG.warning(err_msg) raise n_exc.InvalidInput(error_message=err_msg) + def validate_router_dhcp_relay(self, context): + """Fail router creation dhcp relay is configured without IPAM""" + if (self._availability_zones_data.dhcp_relay_configured() and + cfg.CONF.ipam_driver == 'internal'): + err_msg = _("Neutron is configured with DHCP_Relay but no IPAM " + "plugin configured.") + LOG.warning(err_msg) + raise n_exc.InvalidInput(error_message=err_msg) + def create_router(self, context, router): r = router['router'] - + self.validate_router_dhcp_relay(context) gw_info = self._extract_external_gw(context, router, is_extract=True) r['id'] = (r.get('id') or uuidutils.generate_uuid()) tags = self.nsxlib.build_v3_tags_payload( @@ -3322,7 +3383,13 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, port, resource_type='os-neutron-rport-id', project_name=context.tenant_name) tags.append({'scope': 'os-subnet-id', 'tag': subnet['id']}) - net_az = self.get_network_az_by_net_id(context, network_id) + + # Add the dhcp relay service to the NSX interface + relay_service = None + if subnet['enable_dhcp']: + net_az = self.get_network_az_by_net_id(context, network_id) + relay_service = net_az.dhcp_relay_service + self._routerlib.create_logical_router_intf_port_by_ls_id( logical_router_id=nsx_router_id, display_name=display_name, @@ -3330,7 +3397,7 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, ls_id=nsx_net_id, logical_switch_port_id=nsx_port_id, address_groups=address_groups, - relay_service_uuid=net_az.dhcp_relay_service) + relay_service_uuid=relay_service) if router_db.gw_port and not router_db.enable_snat: # TODO(berlin): Announce the subnet on tier0 if enable_snat diff --git a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py index 974af99844..341c0472cd 100644 --- a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py +++ b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py @@ -257,6 +257,18 @@ class NsxV3PluginTestCaseMixin(test_plugin.NeutronDbPluginV2TestCase, with ctx.session.begin(subtransactions=True): ctx.session.add(models_v2.Network(id=network_id)) + def _enable_dhcp_relay(self): + # Add the relay service to the config and availability zones + cfg.CONF.set_override('native_dhcp_metadata', True, 'nsx_v3') + cfg.CONF.set_override('dhcp_relay_service', NSX_DHCP_RELAY_SRV, + 'nsx_v3') + mock_nsx_version = mock.patch.object( + self.plugin.nsxlib, 'feature_supported', return_value=True) + mock_nsx_version.start() + self.plugin.init_availability_zones() + for az in self.plugin.get_azs_list(): + az.translate_configured_names_to_uuids(self.plugin.nsxlib) + class TestNetworksV2(test_plugin.TestNetworksV2, NsxV3PluginTestCaseMixin): @@ -458,6 +470,34 @@ class TestSubnetsV2(test_plugin.TestSubnetsV2, NsxV3PluginTestCaseMixin): def test_subnet_update_ipv4_and_ipv6_pd_slaac_subnets(self): self.skipTest('Multiple fixed ips on a port are not supported') + def test_subnet_native_dhcp_subnet_enabled(self): + cfg.CONF.set_override('native_dhcp_metadata', True, 'nsx_v3') + with self.network() as network: + with mock.patch.object(self.plugin, + '_enable_native_dhcp') as enable_dhcp,\ + self.subnet(network=network, enable_dhcp=True): + # Native dhcp should be set for this subnet + self.assertTrue(enable_dhcp.called) + + def test_subnet_native_dhcp_subnet_disabled(self): + cfg.CONF.set_override('native_dhcp_metadata', True, 'nsx_v3') + with self.network() as network: + with mock.patch.object(self.plugin, + '_enable_native_dhcp') as enable_dhcp,\ + self.subnet(network=network, enable_dhcp=False): + # Native dhcp should be set for this subnet + self.assertFalse(enable_dhcp.called) + + def test_subnet_native_dhcp_with_relay(self): + """Verify that the relay service is added to the router interface""" + self._enable_dhcp_relay() + with self.network() as network: + with mock.patch.object(self.plugin, + '_enable_native_dhcp') as enable_dhcp,\ + self.subnet(network=network, enable_dhcp=True): + # Native dhcp should not be set for this subnet + self.assertFalse(enable_dhcp.called) + class TestPortsV2(test_plugin.TestPortsV2, NsxV3PluginTestCaseMixin, test_bindings.PortBindingsTestCase, @@ -757,6 +797,29 @@ class TestPortsV2(test_plugin.TestPortsV2, NsxV3PluginTestCaseMixin, def test_create_port_anticipating_allocation(self): self.skipTest('Multiple fixed ips on a port are not supported') + def test_create_compute_port_with_relay_no_router(self): + """Compute port creation should fail + + if a network with dhcp relay is not connected to a router + """ + self._enable_dhcp_relay() + with self.network() as network, \ + self.subnet(network=network, enable_dhcp=True) as s1: + device_owner = constants.DEVICE_OWNER_COMPUTE_PREFIX + 'X' + data = {'port': { + 'network_id': network['network']['id'], + 'tenant_id': self._tenant_id, + 'name': 'port', + 'admin_state_up': True, + 'device_id': 'fake_device', + 'device_owner': device_owner, + 'fixed_ips': [{'subnet_id': s1['subnet']['id']}], + 'mac_address': '00:00:00:00:00:01'} + } + self.assertRaises(n_exc.InvalidInput, + self.plugin.create_port, + self.ctx, data) + class DHCPOptsTestCase(test_dhcpopts.TestExtraDhcpOpt, NsxV3PluginTestCaseMixin): @@ -1478,19 +1541,13 @@ class TestL3NatTestCase(L3NatTest, {'router': {'admin_state_up': False}}, expected_code=exc.HTTPBadRequest.code) - def test_router_dhcp_relay(self): - # Add the relay service to the config and availability zones - cfg.CONF.set_override('dhcp_relay_service', NSX_DHCP_RELAY_SRV, - 'nsx_v3') - mock_nsx_version = mock.patch.object( - self.plugin.nsxlib, 'feature_supported', return_value=True) - mock_nsx_version.start() - self.plugin.init_availability_zones() - for az in self.plugin.get_azs_list(): - az.translate_configured_names_to_uuids(self.plugin.nsxlib) - + def test_router_dhcp_relay_dhcp_enabled(self): + """Verify that the relay service is added to the router interface""" + self._enable_dhcp_relay() with self.network() as network: - with self.subnet(network=network) as s1,\ + with mock.patch.object(self.plugin, + 'validate_router_dhcp_relay'),\ + self.subnet(network=network, enable_dhcp=True) as s1,\ self.router() as r1,\ mock.patch.object(self.plugin.nsxlib.logical_router_port, 'update') as mock_update_port: @@ -1501,6 +1558,35 @@ class TestL3NatTestCase(L3NatTest, relay_service_uuid=NSX_DHCP_RELAY_SRV, subnets=mock.ANY) + def test_router_dhcp_relay_dhcp_disabled(self): + """Verify that the relay service is not added to the router interface + + If the subnet do not have enabled dhcp + """ + self._enable_dhcp_relay() + with self.network() as network: + with mock.patch.object(self.plugin, + 'validate_router_dhcp_relay'),\ + self.subnet(network=network, enable_dhcp=False) as s1,\ + self.router() as r1,\ + mock.patch.object(self.plugin.nsxlib.logical_router_port, + 'update') as mock_update_port: + self._router_interface_action('add', r1['router']['id'], + s1['subnet']['id'], None) + mock_update_port.assert_called_once_with( + mock.ANY, + relay_service_uuid=None, + subnets=mock.ANY) + + def test_router_dhcp_relay_no_ipam(self): + """Verify that a router cannot be created with relay and no ipam""" + # Add the relay service to the config and availability zones + self._enable_dhcp_relay() + self.assertRaises(n_exc.InvalidInput, + self.plugin_instance.create_router, + context.get_admin_context(), + {'router': {'name': 'rtr'}}) + class ExtGwModeTestCase(test_ext_gw_mode.ExtGwModeIntTestCase, L3NatTest):