Merge "NSX|V3 Add validations to DHCP relay"

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

vmware_nsx/plugins/nsx_v3/plugin.py

@@ -1394,7 +1394,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)):
@@ -1411,8 +1410,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,
+                        dhcp_relay = self.get_network_az_by_net_id(
-                                                 created_subnet)
+                            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 "
@@ -1861,6 +1864,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:
@@ -2232,6 +2281,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):
@@ -2674,6 +2724,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)
@@ -3013,9 +3065,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(
@@ -3373,7 +3434,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,
@@ -3381,7 +3448,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

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):
@@ -1479,19 +1542,13 @@ class TestL3NatTestCase(L3NatTest,
                          {'router': {'admin_state_up': False}},
                          expected_code=exc.HTTPBadRequest.code)
 
-    def test_router_dhcp_relay(self):
+    def test_router_dhcp_relay_dhcp_enabled(self):
-        # Add the relay service to the config and availability zones
+        """Verify that the relay service is added to the router interface"""
-        cfg.CONF.set_override('dhcp_relay_service', NSX_DHCP_RELAY_SRV,
+        self._enable_dhcp_relay()
-                              '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)
-
         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:
@@ -1502,6 +1559,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):