From faead8980a16c733e1bdc4077c996528d7c31e7e Mon Sep 17 00:00:00 2001 From: Vladyslav Drok Date: Mon, 4 Apr 2016 12:10:38 +0300 Subject: [PATCH] Fix API node name updates This change fixes an issue when it is possible to specify name multiple times in the patch, and only first of them is checked for correctness. Instead, all of them should be checked, as stated in JSON PATCH RFC https://tools.ietf.org/html/rfc6902. Closes-Bug: #1566731 Change-Id: I4301876d9d4b060c69616b893dcdbc36e41a6369 --- ironic/api/controllers/v1/node.py | 36 ++++++++++--------- ironic/api/controllers/v1/utils.py | 18 +++++++--- ironic/tests/unit/api/v1/test_nodes.py | 33 ++++++++++++++++- ironic/tests/unit/api/v1/test_utils.py | 25 ++++++++----- ...pi-node-name-updates-f3813295472795be.yaml | 6 ++++ 5 files changed, 87 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/fix-api-node-name-updates-f3813295472795be.yaml 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