Merge lp:~cloudbuilders/nova/os-keypair-integration into lp:~hudson-openstack/nova/trunk

[Jesse Andrews]

Accept keypair when you launch a new server. These properties would be stored along with the other server properties in the database (like they are currently for ec2 api).

[Joshua McKenty (joshua-mckenty)]

lgtm

review: Approve

[Brian Waldon (bcwaldon)]

I definitely like the idea. Currently, we can set up a key in nova-manage and it will be added to the server without having to specify it on server create (at least that's how I understand it). It appears we lose that ability with this change. I just want to make sure it is what we want.

review: Needs Information

[Jesse Andrews (anotherjesse)]

bcwaldon - that is correct - this change does that.

We are working on updating novaclient..

   https://github.com/rackspace/python-novaclient/blob/master/novaclient/v1_1/keypairs.py

already has support for managing keypairs via api.

This is making it so you can launch via the api with the keypair as well, and will result in us making a pull request to novaclient to launch with a keypair...

Having the hack to inject the first key is not something we will want in Diablo... You?

[Brian Waldon (bcwaldon)]

Okay, so per our offline discussion, it looks like you are going to limit this to just v1.1

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Jesse Andrews (anotherjesse)]

Can someone do a branch where they add 1.1 only cleanly?

[Thierry Carrez (ttx)]

Any reason why we'd need that feature in Diablo ?

review: Needs Information (ffe)

[Jesse Andrews (anotherjesse)]

ttx - how do you login to instances?

[Ben McGraw (mcgrue)]

lgtm

review: Approve

[Thierry Carrez (ttx)]

anotherjesse: I was just after a rationale -- at this point in time, if a needed feature isn't there but is badly needed, there is a flaw in our design process.

review: Approve (ffe)

[Brian Waldon (bcwaldon)]

As for making this v1.1-only, I think you just need to move line 92 into a version-specific view builder in that same module. We can't really do much about versioning create_instance_helper at the moment.

[lp:~cloudbuilders/nova/os-keypair-integration]

1462. By Jesse Andrews on 2011-08-26

merge trunk

1463. By Jesse Andrews on 2011-08-27

v1.0 of server create injects first users keypair

[Jesse Andrews (anotherjesse)]

> As for making this v1.1-only, I think you just need to move line 92 into a
> version-specific view builder in that same module. We can't really do much
> about versioning create_instance_helper at the moment.

A view is what happens on the way out (response) not for determining how to add a keypair on create.

I did so by adding a create method to the ControllerV10 but the tests now fail - because it seems that my use of super breaks stubs?

[Brian Waldon (bcwaldon)]

For the create request:

ControllerV10 and ControllerV11 can have a _get_keypair method that the Controller.create (superclass) can call. It's probably the super call from the Controller subclasses screwing up your stubs.

For the create/update/show response:

Move the key_name line (126) into ViewBuilderV11._build_extra

[lp:~cloudbuilders/nova/os-keypair-integration]

1464. By Vish Ishaya on 2011-08-31

merged trunk

1465. By Vish Ishaya on 2011-08-31

moved key_name per review

1466. By Vish Ishaya on 2011-08-31

fix keypairs stubs

1467. By Vish Ishaya on 2011-08-31

change to use _get_key_name to retrieve the key

[Vish Ishaya (vishvananda)]

bcwaldon: I changed the code to use a _get_key_name() method which is I think what you intended in the irc discussion. Let me know if it looks like what you intended.

[lp:~cloudbuilders/nova/os-keypair-integration]

1468. By Vish Ishaya on 2011-08-31

expect key_name attribute in 1.1

[Brian Waldon (bcwaldon)]

Sorry to be so confusing on IRC! This is exactly what I wanted. I'm good with this once you resolve the merge conflicts.

review: Approve

[lp:~cloudbuilders/nova/os-keypair-integration]

1469. By Vish Ishaya on 2011-09-01

merge trunk, fix tests

=== modified file 'nova/api/openstack/create_instance_helper.py'
--- nova/api/openstack/create_instance_helper.py	2011-08-30 22:05:39 +0000
+++ nova/api/openstack/create_instance_helper.py	2011-09-01 04:57:26 +0000
@@ -19,7 +19,6 @@
 from webob import exc
 from xml.dom import minidom
 
-from nova import db
 from nova import exception
 from nova import flags
 from nova import log as logging
@@ -74,20 +73,17 @@
         if not 'server' in body:
             raise exc.HTTPUnprocessableEntity()
 
+        context = req.environ['nova.context']
         server_dict = body['server']
-        context = req.environ['nova.context']
         password = self.controller._get_server_admin_password(server_dict)
 
-        key_name = None
-        key_data = None
-        # TODO(vish): Key pair access should move into a common library
-        #             instead of being accessed directly from the db.
-        key_pairs = db.key_pair_get_all_by_user(context.elevated(),
-                                                context.user_id)
-        if key_pairs:
-            key_pair = key_pairs[0]
-            key_name = key_pair['name']
-            key_data = key_pair['public_key']
+        if not 'name' in server_dict:
+            msg = _("Server name is not defined")
+            raise exc.HTTPBadRequest(explanation=msg)
+
+        name = server_dict['name']
+        self._validate_server_name(name)
+        name = name.strip()
 
         image_href = self.controller._image_ref_from_req_data(body)
         # If the image href was generated by nova api, strip image_href
@@ -133,12 +129,10 @@
             msg = _("Invalid flavorRef provided.")
             raise exc.HTTPBadRequest(explanation=msg)
 
-        if not 'name' in server_dict:
-            msg = _("Server name is not defined")
-            raise exc.HTTPBadRequest(explanation=msg)
-
         zone_blob = server_dict.get('blob')
 
+        # optional openstack extensions:
+        key_name = server_dict.get('key_name')
         user_data = server_dict.get('user_data')
         self._validate_user_data(user_data)
 
@@ -176,7 +170,6 @@
                                   display_name=name,
                                   display_description=name,
                                   key_name=key_name,
-                                  key_data=key_data,
                                   metadata=server_dict.get('metadata', {}),
                                   access_ip_v4=server_dict.get('accessIPv4'),
                                   access_ip_v6=server_dict.get('accessIPv6'),
@@ -199,6 +192,9 @@
         except exception.FlavorNotFound as error:
             msg = _("Invalid flavorRef provided.")
             raise exc.HTTPBadRequest(explanation=msg)
+        except exception.KeypairNotFound as error:
+            msg = _("Invalid key_name provided.")
+            raise exc.HTTPBadRequest(explanation=msg)
         except exception.SecurityGroupNotFound as error:
             raise exc.HTTPBadRequest(explanation=unicode(error))
         except RemoteError as err:

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-08-24 17:16:37 +0000
+++ nova/api/openstack/servers.py	2011-09-01 04:57:26 +0000
@@ -22,6 +22,7 @@
 import webob
 
 from nova import compute
+from nova import db
 from nova import exception
 from nova import flags
 from nova import log as logging
@@ -141,10 +142,16 @@
         except exception.NotFound:
             raise exc.HTTPNotFound()
 
+    def _get_key_name(self, req, body):
+        """ Get default keypair if not set """
+        raise NotImplementedError()
+
     def create(self, req, body):
         """ Creates a new server for a given user """
+        if 'server' in body:
+            body['server']['key_name'] = self._get_key_name(req, body)
+
         extra_values = None
-        result = None
         extra_values, instances = self.helper.create_instance(
                 req, body, self.compute_api.create)
 
@@ -562,6 +569,13 @@
             raise exc.HTTPNotFound()
         return webob.Response(status_int=202)
 
+    def _get_key_name(self, req, body):
+        context = req.environ["nova.context"]
+        keypairs = db.key_pair_get_all_by_user(context,
+                                               context.user_id)
+        if keypairs:
+            return keypairs[0]['name']
+
     def _image_ref_from_req_data(self, data):
         return data['server']['imageId']
 
@@ -632,6 +646,10 @@
         except exception.NotFound:
             raise exc.HTTPNotFound()
 
+    def _get_key_name(self, req, body):
+        if 'server' in body:
+            return body['server'].get('key_name')
+
     def _image_ref_from_req_data(self, data):
         try:
             return data['server']['imageRef']

=== modified file 'nova/api/openstack/views/servers.py'
--- nova/api/openstack/views/servers.py	2011-08-25 15:11:51 +0000
+++ nova/api/openstack/views/servers.py	2011-09-01 04:57:26 +0000
@@ -183,6 +183,7 @@
     def _build_extra(self, response, inst):
         self._build_links(response, inst)
         response['uuid'] = inst['uuid']
+        response['key_name'] = inst.get('key_name', '')
         self._build_config_drive(response, inst)
 
     def _build_links(self, response, inst):

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-08-19 20:03:24 +0000
+++ nova/tests/api/openstack/fakes.py	2011-09-01 04:57:26 +0000
@@ -107,13 +107,20 @@
     def key_pair(context, user_id):
         return [dict(name='key', public_key='public_key')]
 
+    def one_key_pair(context, user_id, name):
+        if name == 'key':
+            return dict(name='key', public_key='public_key')
+        else:
+            raise exc.KeypairNotFound(user_id=user_id, name=name)
+
     def no_key_pair(context, user_id):
         return []
 
     if have_key_pair:
-        stubs.Set(nova.db, 'key_pair_get_all_by_user', key_pair)
+        stubs.Set(nova.db.api, 'key_pair_get_all_by_user', key_pair)
+        stubs.Set(nova.db.api, 'key_pair_get', one_key_pair)
     else:
-        stubs.Set(nova.db, 'key_pair_get_all_by_user', no_key_pair)
+        stubs.Set(nova.db.api, 'key_pair_get_all_by_user', no_key_pair)
 
 
 def stub_out_image_service(stubs):

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-08-31 23:07:29 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-09-01 04:57:26 +0000
@@ -155,7 +155,7 @@
                   public_addresses=None, host=None,
                   vm_state=None, task_state=None,
                   reservation_id="", uuid=FAKE_UUID, image_ref="10",
-                  flavor_id="1", interfaces=None, name=None,
+                  flavor_id="1", interfaces=None, name=None, key_name='',
                   access_ipv4=None, access_ipv6=None):
     metadata = []
     metadata.append(InstanceMetadata(key='seq', value=id))
@@ -171,6 +171,11 @@
     if host is not None:
         host = str(host)
 
+    if key_name:
+        key_data = 'FAKE'
+    else:
+        key_data = ''
+
     # ReservationID isn't sent back, hack it in there.
     server_name = name or "server%s" % id
     if reservation_id != "":
@@ -187,8 +192,8 @@
         "kernel_id": "",
         "ramdisk_id": "",
         "launch_index": 0,
-        "key_name": "",
-        "key_data": "",
+        "key_name": key_name,
+        "key_data": key_data,
         "vm_state": vm_state or vm_states.BUILDING,
         "task_state": task_state,
         "memory_mb": 0,
@@ -350,6 +355,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "10",
                     "links": [
@@ -517,6 +523,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "10",
                     "links": [
@@ -611,6 +618,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "10",
                     "links": [
@@ -1746,6 +1754,36 @@
         self.assertEqual('1.2.3.4', server['accessIPv4'])
         self.assertEqual('fead::1234', server['accessIPv6'])
 
+    def test_create_instance_v1_1_invalid_key_name(self):
+        self._setup_for_create_instance()
+
+        image_href = 'http://localhost/v1.1/images/2'
+        flavor_ref = 'http://localhost/flavors/3'
+        body = dict(server=dict(
+            name='server_test', imageRef=image_href, flavorRef=flavor_ref,
+            key_name='nonexistentkey'))
+        req = webob.Request.blank('/v1.1/fake/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_v1_1_valid_key_name(self):
+        self._setup_for_create_instance()
+
+        image_href = 'http://localhost/v1.1/images/2'
+        flavor_ref = 'http://localhost/flavors/3'
+        body = dict(server=dict(
+            name='server_test', imageRef=image_href, flavorRef=flavor_ref,
+            key_name='key'))
+        req = webob.Request.blank('/v1.1/fake/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, 202)
+
     def test_create_instance_v1_1_invalid_flavor_href(self):
         self._setup_for_create_instance()
 
@@ -3617,6 +3655,7 @@
                 "id": 1,
                 "uuid": self.instance['uuid'],
                 "name": "test_server",
+                "key_name": '',
                 "links": [
                     {
                         "rel": "self",
@@ -3640,6 +3679,7 @@
                 "id": 1,
                 "uuid": self.instance['uuid'],
                 "name": "test_server",
+                "key_name": '',
                 "config_drive": None,
                 "links": [
                     {
@@ -3673,6 +3713,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "5",
                     "links": [
@@ -3727,6 +3768,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "5",
                     "links": [
@@ -3778,6 +3820,7 @@
                 "created": "2010-10-10T12:00:00Z",
                 "progress": 0,
                 "name": "test_server",
+                "key_name": "",
                 "status": "BUILD",
                 "hostId": '',
                 "image": {
@@ -3833,6 +3876,7 @@
                 "created": "2010-10-10T12:00:00Z",
                 "progress": 0,
                 "name": "test_server",
+                "key_name": "",
                 "status": "BUILD",
                 "hostId": '',
                 "image": {
@@ -3895,6 +3939,7 @@
                 "accessIPv4": "",
                 "accessIPv6": "",
                 "hostId": '',
+                "key_name": '',
                 "image": {
                     "id": "5",
                     "links": [
@@ -3961,6 +4006,7 @@
                 "name": "test_server",
                 "status": "BUILD",
                 "hostId": 'e4d909c290d0fb1ca068ffaddf22cbd0',
+                "key_name": '',
                 "accessIPv4": "1.2.3.4",
                 "accessIPv6": "fead::1234",
                 "image": {