openstack/ironic
Code Issues Proposed changes

Fix API node name updates

Browse Source 
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
This commit is contained in:
Vladyslav Drok 2016-04-04 12:10:38 +03:00
parent 38e554c77a
commit faead8980a
5 changed files with 87 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
ironic/api/controllers/v1/node.py
View File

@ -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):

18
ironic/api/controllers/v1/utils.py
View File

@ -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():

33
ironic/tests/unit/api/v1/test_nodes.py
View File

@ -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,

25
ironic/tests/unit/api/v1/test_utils.py
View File

@ -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']

6
releasenotes/notes/fix-api-node-name-updates-f3813295472795be.yaml Normal file
View File

@ -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.