Merge lp:~rackspace-titan/nova/password-on-server-create into lp:~hudson-openstack/nova/trunk

[Mark Washenberger]

Support admin password when specified in server create requests.

[Jay Pipes (jaypipes)]

lgtm. nice work.

review: Approve

[Brian Waldon (bcwaldon)]

A docstring that makes it clear what _get_server_admin_password does would make me feel better. If someone came to the module and saw that function name, they may think it would retrieve the adminPass from the actual instance, while it might actually generate something random.

review: Needs Fixing

[Mark Washenberger (markwash)]

I hope these docstrings fit the bill.

[Brian Waldon (bcwaldon)]

Sorry for being such a quibbler!

review: Approve

[Rick Harris (rconradharris)]

If I'm reading the logic correctly, it looks like you need one more test case for complete coverage:

def test_create_instance_without_admin_pass_v11(self):
    """Ensure that V11 will generate a random password if adminPass isn't passed in with request"""

...code above...

body = {
    'server': {
        'name': 'server_test',
        'imageRef': imageRef,
        'flavorRef': flavorRef,
    },
}

...code below...

review: Needs Fixing

[Mark Washenberger (markwash)]

While I'd almost never object to more coverage, I think this case is covered by the test_create_instance_v11 testcase.

Thanks!

[Rick Harris (rconradharris)]

You're right, I should have poked around the file a bit more.

Thanks.

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/api/openstack/servers.py

[Mark Washenberger (markwash)]

Fixed merge conflicts. Sorry.

[Jay Pipes (jaypipes)]

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-04-08 00:32:34 +0000
+++ nova/api/openstack/servers.py	2011-04-18 21:46:11 +0000
@@ -118,6 +118,8 @@
 
         context = req.environ['nova.context']
 
+        password = self._get_server_admin_password(env['server'])
+
         key_name = None
         key_data = None
         key_pairs = auth_manager.AuthManager.get_key_pairs(context)
@@ -180,7 +182,6 @@
 
         builder = self._get_view_builder(req)
         server = builder.build(inst, is_detail=True)
-        password = utils.generate_password(16)
         server['server']['adminPass'] = password
         self.compute_api.set_admin_password(context, server['server']['id'],
                                             password)
@@ -242,6 +243,10 @@
         # if the original error is okay, just reraise it
         raise error
 
+    def _get_server_admin_password(self, server):
+        """ Determine the admin password for a server on creation """
+        return utils.generate_password(16)
+
     @scheduler_api.redirect_handler
     def update(self, req, id):
         """ Updates the server name or password """
@@ -648,6 +653,16 @@
     def _limit_items(self, items, req):
         return common.limited_by_marker(items, req)
 
+    def _get_server_admin_password(self, server):
+        """ Determine the admin password for a server on creation """
+        password = server.get('adminPass')
+        if password is None:
+            return utils.generate_password(16)
+        if not isinstance(password, basestring) or password == '':
+            msg = _("Invalid adminPass")
+            raise exc.HTTPBadRequest(msg)
+        return password
+
     def get_default_xmlns(self, req):
         return common.XML_NS_V11
 

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-04-18 20:40:16 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-04-18 21:46:11 +0000
@@ -613,6 +613,70 @@
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 400)
 
+    def test_create_instance_with_admin_pass_v10(self):
+        self._setup_for_create_instance()
+
+        body = {
+            'server': {
+                'name': 'test-server-create',
+                'imageId': 3,
+                'flavorId': 1,
+                'adminPass': 'testpass',
+            },
+        }
+
+        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())
+        res = json.loads(res.body)
+        self.assertNotEqual(res['server']['adminPass'],
+                            body['server']['adminPass'])
+
+    def test_create_instance_with_admin_pass_v11(self):
+        self._setup_for_create_instance()
+
+        imageRef = 'http://localhost/v1.1/images/2'
+        flavorRef = 'http://localhost/v1.1/flavors/3'
+        body = {
+            'server': {
+                'name': 'server_test',
+                'imageRef': imageRef,
+                'flavorRef': flavorRef,
+                'adminPass': 'testpass',
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers['content-type'] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        server = json.loads(res.body)['server']
+        self.assertEqual(server['adminPass'], body['server']['adminPass'])
+
+    def test_create_instance_with_empty_admin_pass_v11(self):
+        self._setup_for_create_instance()
+
+        imageRef = 'http://localhost/v1.1/images/2'
+        flavorRef = 'http://localhost/v1.1/flavors/3'
+        body = {
+            'server': {
+                'name': 'server_test',
+                'imageRef': imageRef,
+                'flavorRef': flavorRef,
+                'adminPass': '',
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/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_update_no_body(self):
         req = webob.Request.blank('/v1.0/servers/1')
         req.method = 'PUT'