From d1cd215c66eaa205bc63a5405835e0ef28dadc20 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Mon, 12 Feb 2018 13:47:09 +0000 Subject: [PATCH] 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 | 3 ++ ironic/conductor/utils.py | 37 ++++++++++++++ ironic/tests/unit/conductor/test_manager.py | 22 +++++++++ ironic/tests/unit/conductor/test_utils.py | 48 +++++++++++++++++++ ...date-instance-traits-525dd3150aa6afa2.yaml | 9 ++++ 5 files changed, 119 insertions(+) create mode 100644 releasenotes/notes/validate-instance-traits-525dd3150aa6afa2.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 951f7c73ee..95eb8e1dbb 100644 --- a/ironic/conductor/manager.py +++ b/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: diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index a23390d5f9..e2837d8178 100644 --- a/ironic/conductor/utils.py +++ b/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) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 7a2d426081..3a66485473 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/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') diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index e5db5a44af..e9d9cfa884 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/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) diff --git a/releasenotes/notes/validate-instance-traits-525dd3150aa6afa2.yaml b/releasenotes/notes/validate-instance-traits-525dd3150aa6afa2.yaml new file mode 100644 index 0000000000..a8fb604092 --- /dev/null +++ b/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 + `_ for details.