Merge "Fix API node name updates"

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):
+    def _check_names_acceptable(self, names, error_msg):
-        """Checks if a node 'name' is acceptable, it does not return a value.
+        """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 names: list of node names to check
-        :param error_msg: error message in case of wsme.exc.ClientSideError
+        :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):
+        for name in names:
-            raise wsme.exc.ClientSideError(
+            if not api_utils.is_valid_node_name(name):
-                error_msg, status_code=http_client.BAD_REQUEST)
+                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):
+        if node.name != wtypes.Unset and node.name is not None:
-            error_msg = _("Cannot create node with invalid name "
+            error_msg = _("Cannot create node with invalid name '%(name)s'")
-                          "%(name)s") % {'name': node.name}
+            self._check_names_acceptable([node.name], error_msg)
-            self._check_name_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')
+        names = api_utils.get_patch_values(patch, '/name')
-        if name is not None:
+        if len(names):
-            error_msg = _("Node %(node)s: Cannot change name to invalid "
+            error_msg = (_("Node %s: Cannot change name to invalid name ")
-                          "name '%(name)s'") % {'node': node_ident,
+                         % node_ident)
-                                                'name': name}
+            error_msg += "'%(name)s'"
-            self._check_name_acceptable(name, error_msg)
+            self._check_names_acceptable(names, error_msg)
         try:
             node_dict = rpc_node.as_dict()
             # NOTE(lucasagomes):

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):
+def get_patch_values(patch, path):
-    for p in patch:
+    """Get the patch values corresponding to the specified path.
-        if p['path'] == path and p['op'] != 'remove':
+
-            return p['value']
+    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():

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,

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)
+        values = utils.get_patch_values(patch, path)
-        self.assertIsNone(value)
+        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)
+        values = utils.get_patch_values(patch, path)
-        self.assertIsNone(value)
+        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)
+        values = utils.get_patch_values(patch, path)
-        self.assertEqual('node-x', value)
+        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']

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.