No node interface settings for classic drivers

Classic drivers have their interfaces (except for
network and storage) pre-determined. Unlike dynamic
drivers, it is not possible to change these interfaces
for nodes with a classic driver. If that is attempted
(when creating or updating a node), an exception
MustBeNone is raised (and HTTP status 400 is returned).

So that we don't break existing ironic clusters that
have nodes with classic drivers and interfaces specified,
a warning is logged (instead of raising an exception)
when a TaskManager is created.

Change-Id: I290b10f735d0da9710d1ee3b50c3252f73956428
Partial-Bug: #1524745

ironic/common/driver_factory.py

@@ -55,7 +55,17 @@ def build_driver_for_task(task, driver_name=None):
     driver_name = driver_name or node.driver
 
     driver_or_hw_type = get_driver_or_hardware_type(driver_name)
-    check_and_update_node_interfaces(node, driver_or_hw_type=driver_or_hw_type)
+    try:
+        check_and_update_node_interfaces(
+            node, driver_or_hw_type=driver_or_hw_type)
+    except exception.MustBeNone as e:
+        # NOTE(rloo). This was raised because nodes with classic drivers
+        #             cannot have any interfaces (except for network and
+        #             storage) set. However, there was a small window
+        #             where this was possible so instead of breaking those
+        #             users totally, we'll spam them with warnings instead.
+        LOG.warning(_LW('%s They will be ignored. To avoid this warning, '
+                        'please set them to None.'), e)
 
     bare_driver = driver_base.BareDriver()
     _attach_interfaces_to_driver(bare_driver, node, driver_or_hw_type)
@@ -225,6 +235,8 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None):
     :raises: NoValidDefaultForInterface if the default value cannot be
              calculated and is not provided in the configuration
     :raises: DriverNotFound if the node's driver or hardware type is not found
+    :raises: MustBeNone if one or more of the node's interface
+             fields were specified when they should not be.
     """
     if driver_or_hw_type is None:
         driver_or_hw_type = get_driver_or_hardware_type(node.driver)
@@ -237,6 +249,25 @@ def check_and_update_node_interfaces(node, driver_or_hw_type=None):
         # Only network and storage interfaces are dynamic for classic drivers
         factories = ['network', 'storage']
 
+    # These are interfaces that cannot be specified via the node. E.g.,
+    # for classic drivers, none are allowed except for network & storage.
+    not_allowed_ifaces = driver_base.ALL_INTERFACES - set(factories)
+
+    bad_interface_fields = []
+    for iface in not_allowed_ifaces:
+        field_name = '%s_interface' % iface
+        # NOTE(dtantsur): objects raise NotImplementedError on accessing fields
+        # that are known, but missing from an object. Thus, we cannot just use
+        # getattr(node, field_name, None) here.
+        if field_name in node:
+            impl_name = getattr(node, field_name)
+            if impl_name is not None:
+                bad_interface_fields.append(field_name)
+
+    if bad_interface_fields:
+        raise exception.MustBeNone(node=node.uuid, driver=node.driver,
+                                   node_fields=','.join(bad_interface_fields))
+
     # Result - whether the node object was modified
     result = False
 

ironic/common/exception.py

@@ -350,6 +350,11 @@ class NoValidDefaultForInterface(InvalidParameterValue):
                  "value found for %(interface_type)s interface.")
 
 
+class MustBeNone(InvalidParameterValue):
+    _msg_fmt = _("For node %(node)s with driver %(driver)s, these node "
+                 "fields must be set to None: %(node_fields)s.")
+
+
 class ImageNotFound(NotFound):
     _msg_fmt = _("Image %(image_id)s could not be found.")
 

ironic/conductor/manager.py

@@ -96,10 +96,12 @@ class ConductorManager(base_manager.BaseConductorManager):
         self.power_state_sync_count = collections.defaultdict(int)
 
     @METRICS.timer('ConductorManager.create_node')
+    # No need to add these since they are subclasses of InvalidParameterValue:
+    #     InterfaceNotFoundInEntrypoint
+    #     IncompatibleInterface,
+    #     NoValidDefaultForInterface
+    #     MustBeNone
     @messaging.expected_exceptions(exception.InvalidParameterValue,
-                                   exception.InterfaceNotFoundInEntrypoint,
-                                   exception.IncompatibleInterface,
-                                   exception.NoValidDefaultForInterface,
                                    exception.DriverNotFound)
     def create_node(self, context, node_obj):
         """Create a node in database.
@@ -114,6 +116,8 @@ class ConductorManager(base_manager.BaseConductorManager):
         :raises: NoValidDefaultForInterface if no default can be calculated
                  for some interfaces, and explicit values must be provided.
         :raises: InvalidParameterValue if some fields fail validation.
+        :raises: MustBeNone if one or more of the node's interface
+                 fields were specified when they should not be.
         :raises: DriverNotFound if the driver or hardware type is not found.
         """
         LOG.debug("RPC create_node called for node %s.", node_obj.uuid)
@@ -122,12 +126,14 @@ class ConductorManager(base_manager.BaseConductorManager):
         return node_obj
 
     @METRICS.timer('ConductorManager.update_node')
+    # No need to add these since they are subclasses of InvalidParameterValue:
+    #     InterfaceNotFoundInEntrypoint
+    #     IncompatibleInterface,
+    #     NoValidDefaultForInterface
+    #     MustBeNone
     @messaging.expected_exceptions(exception.InvalidParameterValue,
                                    exception.NodeLocked,
                                    exception.InvalidState,
-                                   exception.InterfaceNotFoundInEntrypoint,
-                                   exception.IncompatibleInterface,
-                                   exception.NoValidDefaultForInterface,
                                    exception.DriverNotFound)
     def update_node(self, context, node_obj):
         """Update a node with the supplied data.
@@ -140,7 +146,8 @@ class ConductorManager(base_manager.BaseConductorManager):
         :param node_obj: a changed (but not saved) node object.
         :raises: NoValidDefaultForInterface if no default can be calculated
                  for some interfaces, and explicit values must be provided.
-
+        :raises: MustBeNone if one or more of the node's interface
+                 fields were specified when they should not be.
         """
         node_id = node_obj.uuid
         LOG.debug("RPC update_node called for node %s.", node_id)

ironic/tests/unit/api/v1/test_nodes.py

@@ -1815,6 +1815,22 @@ class TestPatch(test_api_base.BaseApiTest):
             self.assertEqual('application/json', response.content_type)
             self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code)
 
+    def test_update_classic_driver_interface_fields(self):
+        headers = {api_base.Version.string: '1.31'}
+        self.mock_update_node.side_effect = (
+            exception.MustBeNone('error'))
+        for field in api_utils.V31_FIELDS:
+            node = obj_utils.create_test_node(self.context,
+                                              uuid=uuidutils.generate_uuid())
+            response = self.patch_json('/nodes/%s' % node.uuid,
+                                       [{'path': '/%s' % field,
+                                         'value': 'fake',
+                                         'op': 'add'}],
+                                       headers=headers,
+                                       expect_errors=True)
+            self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+            self.assertEqual('application/json', response.content_type)
+
 
 def _create_node_locally(node):
     driver_factory.check_and_update_node_interfaces(node)
@@ -1890,10 +1906,13 @@ class TestPost(test_api_base.BaseApiTest):
 
     def test_create_node_specify_interfaces(self):
         headers = {api_base.Version.string: '1.31'}
+        for field in api_utils.V31_FIELDS:
+            cfg.CONF.set_override('enabled_%ss' % field, ['fake'])
         for field in api_utils.V31_FIELDS:
             node = {
                 'uuid': uuidutils.generate_uuid(),
-                field: 'fake'
+                field: 'fake',
+                'driver': 'fake-hardware'
             }
             result = self._test_create_node(headers=headers, **node)
             self.assertEqual('fake', result[field])
@@ -1906,6 +1925,21 @@ class TestPost(test_api_base.BaseApiTest):
                                       expect_errors=True)
             self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int)
 
+    def test_create_node_classic_driver_specify_interface(self):
+        headers = {api_base.Version.string: '1.31'}
+        for field in api_utils.V31_FIELDS:
+            node = {
+                'uuid': uuidutils.generate_uuid(),
+                field: 'fake',
+            }
+            ndict = test_api_utils.post_get_test_node(**node)
+            response = self.post_json('/nodes', ndict,
+                                      headers=headers,
+                                      expect_errors=True)
+            self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+            self.assertEqual('application/json', response.content_type)
+            self.assertTrue(response.json['error_message'])
+
     def test_create_node_name_empty_invalid(self):
         ndict = test_api_utils.post_get_test_node(name='')
         response = self.post_json('/nodes', ndict,

ironic/tests/unit/common/test_driver_factory.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 import mock
+from oslo_utils import uuidutils
 from stevedore import dispatch
 
 from ironic.common import driver_factory
@@ -32,7 +33,7 @@ class FakeEp(object):
     name = 'fake'
 
 
-class DriverLoadTestCase(base.TestCase):
+class DriverLoadTestCase(db_base.DbTestCase):
 
     def _fake_init_name_err(self, *args, **kwargs):
         kwargs['on_load_failure_callback'](None, FakeEp, NameError('aaa'))
@@ -91,6 +92,33 @@ class DriverLoadTestCase(base.TestCase):
             ['fake'], driver_factory.DriverFactory._extension_manager.names())
         self.assertTrue(mock_warn.called)
 
+    def test_build_driver_for_task(self):
+        node = obj_utils.create_test_node(self.context, driver='fake')
+        with task_manager.acquire(self.context, node.id) as task:
+            for iface in drivers_base.ALL_INTERFACES:
+                impl = getattr(task.driver, iface)
+                self.assertIsNotNone(impl)
+
+    @mock.patch.object(driver_factory, '_attach_interfaces_to_driver',
+                       autospec=True)
+    @mock.patch.object(driver_factory.LOG, 'warning', autospec=True)
+    def test_build_driver_for_task_incorrect(self, mock_warn, mock_attach):
+        # Cannot set these node interfaces for classic driver
+        no_set_interfaces = (drivers_base.ALL_INTERFACES -
+                             set(['network', 'storage']))
+        for iface in no_set_interfaces:
+            iface_name = '%s_interface' % iface
+            node_kwargs = {'uuid': uuidutils.generate_uuid(),
+                           iface_name: 'fake'}
+            node = obj_utils.create_test_node(self.context, driver='fake',
+                                              **node_kwargs)
+            with task_manager.acquire(self.context, node.id) as task:
+                mock_warn.assert_called_once_with(mock.ANY, mock.ANY)
+                mock_warn.reset_mock()
+                mock_attach.assert_called_once_with(mock.ANY, task.node,
+                                                    mock.ANY)
+                mock_attach.reset_mock()
+
 
 class WarnUnsupportedDriversTestCase(base.TestCase):
     @mock.patch.object(driver_factory.LOG, 'warning', autospec=True)
@@ -258,6 +286,21 @@ class CheckAndUpdateNodeInterfacesTestCase(db_base.DbTestCase):
                           driver_factory.check_and_update_node_interfaces,
                           node)
 
+    def test_classic_setting_interfaces(self):
+        # Cannot set these node interfaces for classic driver
+        no_set_interfaces = (drivers_base.ALL_INTERFACES -
+                             set(['network', 'storage']))
+        for iface in no_set_interfaces:
+            iface_name = '%s_interface' % iface
+            node_kwargs = {'uuid': uuidutils.generate_uuid(),
+                           iface_name: 'fake'}
+            node = obj_utils.create_test_node(self.context, driver='fake',
+                                              **node_kwargs)
+            self.assertRaisesRegex(
+                exception.InvalidParameterValue,
+                'driver fake.*%s' % iface_name,
+                driver_factory.check_and_update_node_interfaces, node)
+
 
 class DefaultInterfaceTestCase(db_base.DbTestCase):
     def setUp(self):

releasenotes/notes/nodes-classic-drivers-cannot-set-interfaces-620b37c4e5c88b80.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Nodes with classic drivers cannot have any interfaces (except for network
+    and storage) specified. HTTP status 400 is returned in these cases.