From 4d06a423386d8af340d4ad4086da554df66a179f Mon Sep 17 00:00:00 2001
From: Zhenguo Niu <Niu.ZGlinux@gmail.com>
Date: Wed, 30 Mar 2016 17:35:00 +0800
Subject: [PATCH] Correct api version check conditional for node.name

Currently the name acceptable check is under the condition of
"if name", so if you specify name to an empty value like "",
that will skip the API version check.

Change-Id: I85dc67ea7ac4f38801dc4675f85fac5e1fc1b364
Closes-Bug: #1563694
---
 ironic/api/controllers/v1/node.py             |  4 +-
 ironic/tests/unit/api/v1/test_nodes.py        | 39 +++++++++++++++++++
 ...itional-for-nodename-439bebc02fb5493d.yaml |  5 +++
 3 files changed, 46 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/correct-api-version-check-conditional-for-nodename-439bebc02fb5493d.yaml

diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index 381eb3ce68..1f637774f3 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -1208,7 +1208,7 @@ class NodesController(rest.RestController):
             e.code = http_client.BAD_REQUEST
             raise e
 
-        if node.name:
+        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)
@@ -1259,7 +1259,7 @@ class NodesController(rest.RestController):
                 msg % node_ident, status_code=http_client.CONFLICT)
 
         name = api_utils.get_patch_value(patch, '/name')
-        if name:
+        if name is not None:
             error_msg = _("Node %(node)s: Cannot change name to invalid "
                           "name '%(name)s'") % {'node': node_ident,
                                                 'name': name}
diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py
index 4a2b410f1f..41a7b8150f 100644
--- a/ironic/tests/unit/api/v1/test_nodes.py
+++ b/ironic/tests/unit/api/v1/test_nodes.py
@@ -1269,6 +1269,29 @@ class TestPatch(test_api_base.BaseApiTest):
         self.assertEqual(http_client.BAD_REQUEST, response.status_code)
         self.assertTrue(response.json['error_message'])
 
+    def test_patch_add_name_empty_invalid(self):
+        test_name = ''
+        response = self.patch_json('/nodes/%s' % self.node_no_name.uuid,
+                                   [{'path': '/name',
+                                     'op': 'add',
+                                     '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.assertTrue(response.json['error_message'])
+
+    def test_patch_add_name_empty_not_acceptable(self):
+        test_name = ''
+        response = self.patch_json('/nodes/%s' % self.node_no_name.uuid,
+                                   [{'path': '/name',
+                                     'op': 'add',
+                                     'value': test_name}],
+                                   expect_errors=True)
+        self.assertEqual('application/json', response.content_type)
+        self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code)
+        self.assertTrue(response.json['error_message'])
+
     def test_patch_name_replace_ok(self):
         self.mock_update_node.return_value = self.node
         test_name = 'guido-van-rossum'
@@ -1380,6 +1403,22 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual(urlparse.urlparse(response.location).path,
                          expected_location)
 
+    def test_create_node_name_empty_invalid(self):
+        ndict = test_api_utils.post_get_test_node(name='')
+        response = self.post_json('/nodes', ndict,
+                                  headers={api_base.Version.string: "1.10"},
+                                  expect_errors=True)
+        self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+        self.assertEqual('application/json', response.content_type)
+        self.assertTrue(response.json['error_message'])
+
+    def test_create_node_name_empty_not_acceptable(self):
+        ndict = test_api_utils.post_get_test_node(name='')
+        response = self.post_json('/nodes', ndict, expect_errors=True)
+        self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int)
+        self.assertEqual('application/json', response.content_type)
+        self.assertTrue(response.json['error_message'])
+
     def test_create_node_default_state_none(self):
         ndict = test_api_utils.post_get_test_node()
         response = self.post_json('/nodes', ndict,
diff --git a/releasenotes/notes/correct-api-version-check-conditional-for-nodename-439bebc02fb5493d.yaml b/releasenotes/notes/correct-api-version-check-conditional-for-nodename-439bebc02fb5493d.yaml
new file mode 100644
index 0000000000..ef9ed8a313
--- /dev/null
+++ b/releasenotes/notes/correct-api-version-check-conditional-for-nodename-439bebc02fb5493d.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - Correct api version check conditional for node.name to address
+    an issue that we could set node name to '' using API version lower than
+    1.5, where node names were introduced.