Validate instance_info.traits against node traits

The ironic node traits spec calls out that traits added to
instance_info.traits should be validated against the node's traits.  All
traits in instance_info.traits should exist in the node's traits.  This
protects us against race conditions between traits being removed from a
node in ironic, and the node's resource provider's traits being updated
in placement.

This change adds validation to do_node_deploy() and
validate_driver_interfaces() in the conductor manager, ensuring that all
instance traits are also node traits.

Change-Id: I956f8285fe428b2bdf8822e4a308f5c2a1675836
Closes-Bug: #1755146
Related-Bug: #1722194

ironic/conductor/manager.py

@@ -822,6 +822,7 @@ class ConductorManager(base_manager.BaseConductorManager):
             try:
                 task.driver.power.validate(task)
                 task.driver.deploy.validate(task)
+                utils.validate_instance_info_traits(task.node)
             except exception.InvalidParameterValue as e:
                 raise exception.InstanceDeployFailure(
                     _("Failed to validate deploy or power info for node "
@@ -1869,6 +1870,8 @@ class ConductorManager(base_manager.BaseConductorManager):
                 if iface:
                     try:
                         iface.validate(task)
+                        if iface_name == 'deploy':
+                            utils.validate_instance_info_traits(task.node)
                         result = True
                     except (exception.InvalidParameterValue,
                             exception.UnsupportedDriverExtension) as e:

ironic/conductor/utils.py

@@ -16,6 +16,7 @@ from oslo_config import cfg
 from oslo_log import log
 from oslo_service import loopingcall
 from oslo_utils import excutils
+import six
 
 from ironic.common import exception
 from ironic.common.i18n import _
@@ -744,3 +745,39 @@ def remove_node_rescue_password(node, save=True):
         node.instance_info = instance_info
         if save:
             node.save()
+
+
+def validate_instance_info_traits(node):
+    """Validate traits in instance_info.
+
+    All traits in instance_info must also exist as node traits.
+
+    :param node: an Ironic node object.
+    :raises: InvalidParameterValue if the instance traits are badly formatted,
+        or contain traits that are not set on the node.
+    """
+
+    def invalid():
+        err = (_("Error parsing traits from Node %(node)s instance_info "
+                 "field. A list of strings is expected.")
+               % {"node": node.uuid})
+        raise exception.InvalidParameterValue(err)
+
+    if not node.instance_info.get('traits'):
+        return
+    instance_traits = node.instance_info['traits']
+    if not isinstance(instance_traits, list):
+        invalid()
+    if not all(isinstance(t, six.string_types) for t in instance_traits):
+        invalid()
+
+    # TODO(mgoddard): Remove the obj_attr_is_set() call in Rocky
+    # when all node objects will have a traits field.
+    node_traits = (node.traits.get_trait_names()
+                   if node.obj_attr_is_set('traits') else [])
+    missing = set(instance_traits) - set(node_traits)
+    if missing:
+        err = (_("Cannot specify instance traits that are not also set on the "
+                 "node. Node %(node)s is missing traits %(traits)s") %
+               {"node": node.uuid, "traits": ", ".join(missing)})
+        raise exception.InvalidParameterValue(err)

ironic/tests/unit/conductor/test_manager.py

@@ -1225,6 +1225,11 @@ class ServiceDoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin,
                                                 mock_iwdi):
         self._test_do_node_deploy_validate_fail(mock_validate, mock_iwdi)
 
+    @mock.patch.object(conductor_utils, 'validate_instance_info_traits')
+    def test_do_node_deploy_traits_validate_fail(self, mock_validate,
+                                                 mock_iwdi):
+        self._test_do_node_deploy_validate_fail(mock_validate, mock_iwdi)
+
     def test_do_node_deploy_partial_ok(self, mock_iwdi):
         mock_iwdi.return_value = False
         self._start_service()
@@ -3407,6 +3412,23 @@ class MiscTestCase(mgr_utils.ServiceSetUpMixin, mgr_utils.CommonMixIn,
 
             mock_iwdi.assert_called_once_with(self.context, node.instance_info)
 
+    @mock.patch.object(images, 'is_whole_disk_image')
+    def test_validate_driver_interfaces_validation_fail_instance_traits(
+            self, mock_iwdi):
+        mock_iwdi.return_value = False
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          network_interface='noop')
+        with mock.patch(
+            'ironic.conductor.utils.validate_instance_info_traits'
+        ) as ii_traits:
+            reason = 'fake reason'
+            ii_traits.side_effect = exception.InvalidParameterValue(reason)
+            ret = self.service.validate_driver_interfaces(self.context,
+                                                          node.uuid)
+            self.assertFalse(ret['deploy']['result'])
+            self.assertEqual(reason, ret['deploy']['reason'])
+            mock_iwdi.assert_called_once_with(self.context, node.instance_info)
+
     @mock.patch.object(manager.ConductorManager, '_fail_if_in_state',
                        autospec=True)
     @mock.patch.object(manager.ConductorManager, '_mapped_to_this_conductor')

ironic/tests/unit/conductor/test_utils.py

@@ -1766,3 +1766,51 @@ class MiscTestCase(db_base.DbTestCase):
 
     def test_remove_node_rescue_password_save_false(self):
         self._test_remove_node_rescue_password(save=False)
+
+
+class ValidateInstanceInfoTraitsTestCase(tests_base.TestCase):
+
+    def setUp(self):
+        super(ValidateInstanceInfoTraitsTestCase, self).setUp()
+        self.node = obj_utils.get_test_node(self.context, driver='fake',
+                                            traits=['trait1', 'trait2'])
+
+    def test_validate_instance_info_traits_no_instance_traits(self):
+        conductor_utils.validate_instance_info_traits(self.node)
+
+    def test_validate_instance_info_traits_empty_instance_traits(self):
+        self.node.instance_info['traits'] = []
+        conductor_utils.validate_instance_info_traits(self.node)
+
+    def test_parse_instance_info_traits_invalid_type(self):
+        self.node.instance_info['traits'] = 'not-a-list'
+        self.assertRaisesRegex(exception.InvalidParameterValue,
+                               'Error parsing traits from Node',
+                               conductor_utils.validate_instance_info_traits,
+                               self.node)
+
+    def test_parse_instance_info_traits_invalid_trait_type(self):
+        self.node.instance_info['traits'] = ['trait1', {}]
+        self.assertRaisesRegex(exception.InvalidParameterValue,
+                               'Error parsing traits from Node',
+                               conductor_utils.validate_instance_info_traits,
+                               self.node)
+
+    def test_validate_instance_info_traits(self):
+        self.node.instance_info['traits'] = ['trait1', 'trait2']
+        conductor_utils.validate_instance_info_traits(self.node)
+
+    def test_validate_instance_info_traits_missing(self):
+        self.node.instance_info['traits'] = ['trait1', 'trait3']
+        self.assertRaisesRegex(exception.InvalidParameterValue,
+                               'Cannot specify instance traits that are not',
+                               conductor_utils.validate_instance_info_traits,
+                               self.node)
+
+    def test_validate_instance_info_traits_no_node_traits(self):
+        self.node.instance_info['traits'] = ['trait1', 'trait2']
+        delattr(self.node, 'traits')
+        self.assertRaisesRegex(exception.InvalidParameterValue,
+                               'Cannot specify instance traits that are not',
+                               conductor_utils.validate_instance_info_traits,
+                               self.node)

releasenotes/notes/validate-instance-traits-525dd3150aa6afa2.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Fixes an issue where a node's ``instance_info.traits`` field could be
+    incorrectly formatted, or contain traits that are not traits of the node.
+    When validating drivers and prior to deployment, the Bare Metal service now
+    validates that a node's traits include all the traits in its
+    ``instance_info.traits`` field.  See `bug 1755146
+    <https://bugs.launchpad.net/ironic/+bug/1755146>`_ for details.