Merge lp:~rackspace-titan/nova/osapi-pass-update-lp744567 into lp:~hudson-openstack/nova/trunk

[Brian Waldon]

Moving server update adminPass support to be v1.0-specific
OS API servers update tests actually assert and pass now
Enforcing server name being a string of length > 0

[Brian Lamar (blamar)]

Is the "isinstance(name, basestring)" needed or can you just cast the name to a string? When would it not be a string?

review: Needs Information

[Brian Waldon (bcwaldon)]

> Is the "isinstance(name, basestring)" needed or can you just cast the name to
> a string? When would it not be a string?

I typically operate under the assumption that the users of the api will make mistakes. Would it make sense to cast a dict as the server name? It would be easily correctable, but should we prevent it from happening in the first place? The v1.1 spec doesn't seem to offer any help.

[Jay Pipes (jaypipes)]

Hi!

19 + if not isinstance(name, basestring) or name == '':
20 + return exc.HTTPBadRequest()

Not going to weigh in on Brian Lamar's question, but I would say that if you stick with raising an HTTPBadRequest in this way, that you should, at a minimum, provide an error message. I'd hate to receive a standard 400 Bad Request and have no idea why ;)

something like this would work:

if not isinstance(name, basestring):
    msg = _("Name supplied for server (%r) is not a string or unicode") % name
    return exc.HTTPBadRequest(msg)
elif name.strip() == '':
    msg = _("Null string supplied as server name")
    return exc.HTTPBadRequest(msg)

Other than that nit, looks good, and good work adding tests.

Cheers!
jay

review: Needs Fixing

[Jay Pipes (jaypipes)]

ooh, please add a test case for the nullstring and non-string args. sorry, meant to mention that above...

review: Needs Fixing

[Brian Waldon (bcwaldon)]

Jay,

I incorporated your suggestions into my code. However, this branch now exceeds the bug it is aimed at fixing. I started copying my server name validation to create, too, but I thought I should get your opinion on how to accomplish that. Is it okay to do that within this branch?

[Jay Pipes (jaypipes)]

In this case, you are tightening existing code with better error checking. It's always a bit of a choice, but in this case, the additional code blocks doing checks are fine, the patch size is still very manageable, and you are fixing things that came up during the review process (not randomly changing code in other modules that don't have anything to do with your bug or something like that...).

And more/better test cases are always appreciated, regardless.

So, yes, this is perfectly fine with me.

review: Approve

[Jay Pipes (jaypipes)]

Brian, if you'd to put the same validation in the create method (and possibly have a _validate_name() method or similar, my opinion is to do so in this patch. The additional code won't be more than a few lines of code, and the patch size is minimal.

[Brian Waldon (bcwaldon)]

> Brian, if you'd to put the same validation in the create method (and possibly
> have a _validate_name() method or similar, my opinion is to do so in this
> patch. The additional code won't be more than a few lines of code, and the
> patch size is minimal.

Great. I'll get this done right now.

[Brian Waldon (bcwaldon)]

Jay,

I would appreciate it if you could look over the last couple of revisions. It's a pretty straightforward patch.

[Brian Lamar (blamar)]

review: Approve

[Jay Pipes (jaypipes)]

Nice work, Brian.

review: Approve

[Sandy Walsh (sandy-walsh)]

Seems reasonable to me :)

Tests are happy.

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-03-25 18:36:54 +0000
+++ nova/api/openstack/servers.py	2011-03-29 15:19:38 +0000
@@ -150,6 +150,15 @@
             injected_files = self._get_injected_files(personality)
 
         flavor_id = self._flavor_id_from_req_data(env)
+
+        if not 'name' in env['server']:
+            msg = _("Server name is not defined")
+            return exc.HTTPBadRequest(msg)
+
+        name = env['server']['name']
+        self._validate_server_name(name)
+        name = name.strip()
+
         try:
             (inst,) = self.compute_api.create(
                 context,
@@ -157,8 +166,8 @@
                 image_id,
                 kernel_id=kernel_id,
                 ramdisk_id=ramdisk_id,
-                display_name=env['server']['name'],
-                display_description=env['server']['name'],
+                display_name=name,
+                display_description=name,
                 key_name=key_name,
                 key_data=key_data,
                 metadata=metadata,
@@ -246,20 +255,33 @@
 
         ctxt = req.environ['nova.context']
         update_dict = {}
-        if 'adminPass' in inst_dict['server']:
-            update_dict['admin_pass'] = inst_dict['server']['adminPass']
-            try:
-                self.compute_api.set_admin_password(ctxt, id)
-            except exception.TimeoutException:
-                return exc.HTTPRequestTimeout()
+
         if 'name' in inst_dict['server']:
-            update_dict['display_name'] = inst_dict['server']['name']
+            name = inst_dict['server']['name']
+            self._validate_server_name(name)
+            update_dict['display_name'] = name.strip()
+
+        self._parse_update(ctxt, id, inst_dict, update_dict)
+
         try:
             self.compute_api.update(ctxt, id, **update_dict)
         except exception.NotFound:
             return faults.Fault(exc.HTTPNotFound())
+
         return exc.HTTPNoContent()
 
+    def _validate_server_name(self, value):
+        if not isinstance(value, basestring):
+            msg = _("Server name is not a string or unicode")
+            raise exc.HTTPBadRequest(msg)
+
+        if value.strip() == '':
+            msg = _("Server name is an empty string")
+            raise exc.HTTPBadRequest(msg)
+
+    def _parse_update(self, context, id, inst_dict, update_dict):
+        pass
+
     @scheduler_api.redirect_handler
     def action(self, req, id):
         """Multi-purpose method used to reboot, rebuild, or
@@ -566,6 +588,14 @@
     def _limit_items(self, items, req):
         return common.limited(items, req)
 
+    def _parse_update(self, context, server_id, inst_dict, update_dict):
+        if 'adminPass' in inst_dict['server']:
+            update_dict['admin_pass'] = inst_dict['server']['adminPass']
+            try:
+                self.compute_api.set_admin_password(context, server_id)
+            except exception.TimeoutException:
+                return exc.HTTPRequestTimeout()
+
 
 class ControllerV11(Controller):
     def _image_id_from_req_data(self, data):

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-03-28 18:31:12 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-03-29 15:19:38 +0000
@@ -392,6 +392,74 @@
         fakes.stub_out_key_pair_funcs(self.stubs, have_key_pair=False)
         self._test_create_instance_helper()
 
+    def test_create_instance_no_name(self):
+        self._setup_for_create_instance()
+
+        body = {
+            'server': {
+                'imageId': 3,
+                'flavorId': 1,
+                'metadata': {
+                    'hello': 'world',
+                    'open': 'stack',
+                },
+                'personality': {},
+            },
+        }
+
+        req = webob.Request.blank('/v1.0/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_create_instance_nonstring_name(self):
+        self._setup_for_create_instance()
+
+        body = {
+            'server': {
+                'name': 12,
+                'imageId': 3,
+                'flavorId': 1,
+                'metadata': {
+                    'hello': 'world',
+                    'open': 'stack',
+                },
+                'personality': {},
+            },
+        }
+
+        req = webob.Request.blank('/v1.0/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_create_instance_whitespace_name(self):
+        self._setup_for_create_instance()
+
+        body = {
+            'server': {
+                'name': '    ',
+                'imageId': 3,
+                'flavorId': 1,
+                'metadata': {
+                    'hello': 'world',
+                    'open': 'stack',
+                },
+                'personality': {},
+            },
+        }
+
+        req = webob.Request.blank('/v1.0/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
     def test_create_instance_v11(self):
         self._setup_for_create_instance()
 
@@ -448,39 +516,82 @@
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 422)
 
-    def test_update_bad_params(self):
-        """ Confirm that update is filtering params """
-        inst_dict = dict(cat='leopard', name='server_test', adminPass='bacon')
-        self.body = json.dumps(dict(server=inst_dict))
-
-        def server_update(context, id, params):
-            self.update_called = True
-            filtered_dict = dict(name='server_test', admin_pass='bacon')
-            self.assertEqual(params, filtered_dict)
-
-        self.stubs.Set(nova.db.api, 'instance_update',
-            server_update)
-
-        req = webob.Request.blank('/v1.0/servers/1')
-        req.method = 'PUT'
-        req.body = self.body
-        req.get_response(fakes.wsgi_app())
-
-    def test_update_server(self):
-        inst_dict = dict(name='server_test', adminPass='bacon')
-        self.body = json.dumps(dict(server=inst_dict))
-
-        def server_update(context, id, params):
-            filtered_dict = dict(name='server_test', admin_pass='bacon')
-            self.assertEqual(params, filtered_dict)
-
-        self.stubs.Set(nova.db.api, 'instance_update',
-            server_update)
-
-        req = webob.Request.blank('/v1.0/servers/1')
-        req.method = 'PUT'
-        req.body = self.body
-        req.get_response(fakes.wsgi_app())
+    def test_update_nonstring_name(self):
+        """ Confirm that update is filtering params """
+        inst_dict = dict(name=12, adminPass='bacon')
+        self.body = json.dumps(dict(server=inst_dict))
+
+        req = webob.Request.blank('/v1.0/servers/1')
+        req.method = 'PUT'
+        req.content_type = "application/json"
+        req.body = self.body
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_update_whitespace_name(self):
+        """ Confirm that update is filtering params """
+        inst_dict = dict(name='   ', adminPass='bacon')
+        self.body = json.dumps(dict(server=inst_dict))
+
+        req = webob.Request.blank('/v1.0/servers/1')
+        req.method = 'PUT'
+        req.content_type = "application/json"
+        req.body = self.body
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_update_null_name(self):
+        """ Confirm that update is filtering params """
+        inst_dict = dict(name='', adminPass='bacon')
+        self.body = json.dumps(dict(server=inst_dict))
+
+        req = webob.Request.blank('/v1.0/servers/1')
+        req.method = 'PUT'
+        req.content_type = "application/json"
+        req.body = self.body
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_update_server_v10(self):
+        inst_dict = dict(name='server_test', adminPass='bacon')
+        self.body = json.dumps(dict(server=inst_dict))
+
+        def server_update(context, id, params):
+            filtered_dict = dict(
+                display_name='server_test',
+                admin_pass='bacon',
+            )
+            self.assertEqual(params, filtered_dict)
+            return filtered_dict
+
+        self.stubs.Set(nova.db.api, 'instance_update',
+            server_update)
+
+        req = webob.Request.blank('/v1.0/servers/1')
+        req.method = 'PUT'
+        req.content_type = "application/json"
+        req.body = self.body
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 204)
+
+    def test_update_server_adminPass_ignored_v11(self):
+        inst_dict = dict(name='server_test', adminPass='bacon')
+        self.body = json.dumps(dict(server=inst_dict))
+
+        def server_update(context, id, params):
+            filtered_dict = dict(display_name='server_test')
+            self.assertEqual(params, filtered_dict)
+            return filtered_dict
+
+        self.stubs.Set(nova.db.api, 'instance_update',
+            server_update)
+
+        req = webob.Request.blank('/v1.1/servers/1')
+        req.method = 'PUT'
+        req.content_type = "application/json"
+        req.body = self.body
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 204)
 
     def test_create_backup_schedules(self):
         req = webob.Request.blank('/v1.0/servers/1/backup_schedule')