diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 6491c9a590..6de1c83771 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1019,21 +1019,24 @@ class NodesController(rest.RestController): except exception.InstanceNotFound: return [] - def _check_name_acceptable(self, name, error_msg): - """Checks if a node 'name' is acceptable, it does not return a value. + def _check_names_acceptable(self, names, error_msg): + """Checks all node 'name's are acceptable, it does not return a value. This function will raise an exception for unacceptable names. - :param name: node name - :param error_msg: error message in case of wsme.exc.ClientSideError + :param names: list of node names to check + :param error_msg: error message in case of wsme.exc.ClientSideError, + should contain %(name)s placeholder. :raises: exception.NotAcceptable :raises: wsme.exc.ClientSideError """ if not api_utils.allow_node_logical_names(): raise exception.NotAcceptable() - if not api_utils.is_valid_node_name(name): - raise wsme.exc.ClientSideError( - error_msg, status_code=http_client.BAD_REQUEST) + for name in names: + if not api_utils.is_valid_node_name(name): + raise wsme.exc.ClientSideError( + error_msg % {'name': name}, + status_code=http_client.BAD_REQUEST) def _update_changed_fields(self, node, rpc_node): """Update rpc_node based on changed fields in a node. @@ -1213,10 +1216,9 @@ class NodesController(rest.RestController): e.code = http_client.BAD_REQUEST raise e - if (node.name != wtypes.Unset and node.name is not None): - error_msg = _("Cannot create node with invalid name " - "%(name)s") % {'name': node.name} - self._check_name_acceptable(node.name, error_msg) + if node.name != wtypes.Unset and node.name is not None: + error_msg = _("Cannot create node with invalid name '%(name)s'") + self._check_names_acceptable([node.name], error_msg) node.provision_state = api_utils.initial_node_provision_state() new_node = objects.Node(pecan.request.context, @@ -1263,12 +1265,12 @@ class NodesController(rest.RestController): raise wsme.exc.ClientSideError( msg % node_ident, status_code=http_client.CONFLICT) - name = api_utils.get_patch_value(patch, '/name') - if name is not None: - error_msg = _("Node %(node)s: Cannot change name to invalid " - "name '%(name)s'") % {'node': node_ident, - 'name': name} - self._check_name_acceptable(name, error_msg) + names = api_utils.get_patch_values(patch, '/name') + if len(names): + error_msg = (_("Node %s: Cannot change name to invalid name ") + % node_ident) + error_msg += "'%(name)s'" + self._check_names_acceptable(names, error_msg) try: node_dict = rpc_node.as_dict() # NOTE(lucasagomes): diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 5a7f474874..fedf0da14d 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -79,10 +79,20 @@ def apply_jsonpatch(doc, patch): return jsonpatch.apply_patch(doc, jsonpatch.JsonPatch(patch)) -def get_patch_value(patch, path): - for p in patch: - if p['path'] == path and p['op'] != 'remove': - return p['value'] +def get_patch_values(patch, path): + """Get the patch values corresponding to the specified path. + + If there are multiple values specified for the same path + (for example the patch is [{'op': 'add', 'path': '/name', 'value': 'abc'}, + {'op': 'add', 'path': '/name', 'value': 'bca'}]) + return all of them in a list (preserving order). + + :param patch: HTTP PATCH request body. + :param path: the path to get the patch values for. + :returns: list of values for the specified path in the patch. + """ + return [p['value'] for p in patch + if p['path'] == path and p['op'] != 'remove'] def allow_node_logical_names(): diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index 41a7b8150f..0097d37bfb 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/ironic/tests/unit/api/v1/test_nodes.py @@ -1316,6 +1316,37 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.BAD_REQUEST, response.status_code) self.assertTrue(response.json['error_message']) + def test_patch_update_name_twice_both_invalid(self): + test_name_1 = 'Windows ME' + test_name_2 = 'Guido Van Error' + response = self.patch_json('/nodes/%s' % self.node.uuid, + [{'path': '/name', + 'op': 'add', + 'value': test_name_1}, + {'path': '/name', + 'op': 'replace', + 'value': test_name_2}], + headers={api_base.Version.string: "1.5"}, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_code) + self.assertIn(test_name_1, response.json['error_message']) + + def test_patch_update_name_twice_second_invalid(self): + test_name = 'Guido Van Error' + response = self.patch_json('/nodes/%s' % self.node.uuid, + [{'path': '/name', + 'op': 'add', + 'value': 'node-0'}, + {'path': '/name', + 'op': 'replace', + 'value': test_name}], + headers={api_base.Version.string: "1.5"}, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_code) + self.assertIn(test_name, response.json['error_message']) + def test_patch_duplicate_name(self): node = obj_utils.create_test_node(self.context, uuid=uuidutils.generate_uuid()) @@ -1331,7 +1362,7 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.CONFLICT, response.status_code) self.assertTrue(response.json['error_message']) - @mock.patch.object(api_node.NodesController, '_check_name_acceptable') + @mock.patch.object(api_node.NodesController, '_check_names_acceptable') def test_patch_name_remove_ok(self, cna_mock): self.mock_update_node.return_value = self.node response = self.patch_json('/nodes/%s' % self.node.uuid, diff --git a/ironic/tests/unit/api/v1/test_utils.py b/ironic/tests/unit/api/v1/test_utils.py index 66f9c18665..1b77c5b29c 100644 --- a/ironic/tests/unit/api/v1/test_utils.py +++ b/ironic/tests/unit/api/v1/test_utils.py @@ -56,23 +56,30 @@ class TestApiUtils(base.TestCase): utils.validate_sort_dir, 'fake-sort') - def test_get_patch_value_no_path(self): + def test_get_patch_values_no_path(self): patch = [{'path': '/name', 'op': 'update', 'value': 'node-0'}] path = '/invalid' - value = utils.get_patch_value(patch, path) - self.assertIsNone(value) + values = utils.get_patch_values(patch, path) + self.assertEqual([], values) - def test_get_patch_value_remove(self): + def test_get_patch_values_remove(self): patch = [{'path': '/name', 'op': 'remove'}] path = '/name' - value = utils.get_patch_value(patch, path) - self.assertIsNone(value) + values = utils.get_patch_values(patch, path) + self.assertEqual([], values) - def test_get_patch_value_success(self): + def test_get_patch_values_success(self): patch = [{'path': '/name', 'op': 'replace', 'value': 'node-x'}] path = '/name' - value = utils.get_patch_value(patch, path) - self.assertEqual('node-x', value) + values = utils.get_patch_values(patch, path) + self.assertEqual(['node-x'], values) + + def test_get_patch_values_multiple_success(self): + patch = [{'path': '/name', 'op': 'replace', 'value': 'node-x'}, + {'path': '/name', 'op': 'replace', 'value': 'node-y'}] + path = '/name' + values = utils.get_patch_values(patch, path) + self.assertEqual(['node-x', 'node-y'], values) def test_check_for_invalid_fields(self): requested = ['field_1', 'field_3'] diff --git a/releasenotes/notes/fix-api-node-name-updates-f3813295472795be.yaml b/releasenotes/notes/fix-api-node-name-updates-f3813295472795be.yaml new file mode 100644 index 0000000000..24ee6aeb59 --- /dev/null +++ b/releasenotes/notes/fix-api-node-name-updates-f3813295472795be.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Remove the possibility to set incorrect node name by specifying multiple + add/replace operations in patch request. Since this version, all the values + specified in the patch for name are checked, in order to conform to + JSON PATCH RFC https://tools.ietf.org/html/rfc6902. \ No newline at end of file