Merge lp:~jtran/nova/import_pubkey into lp:~hudson-openstack/nova/trunk

[John Tran]

Added an EC2 API endpoint that'll allow import of public key. Prior, api only allowed generation of new keys.

[Devin Carlen (devcamcar)]

15 + if fingerprint:
16 + key['fingerprint'] = fingerprint

Is it useful to import a keypair with no fingerprint?

review: Needs Information

[John Tran (jtran)]

In our case, we only needed the pubkeys to be injected. We didn't need the fingerprint.

[Vish Ishaya (vishvananda)]

The fingerprint can be calculated pretty easily from the public key. Take the following code from crypto.py:

110 (out, err) = utils.execute('ssh-keygen', '-q', '-l', '-f',
111 '%s.pub' % (keyfile))
112 fingerprint = out.split(' ')[1]

On Mar 21, 2011, at 10:06 AM, John Tran wrote:

> In our case, we only needed the pubkeys to be injected. We didn't need the fingerprint.
> --
> https://code.launchpad.net/~jtran/nova/import_pubkey/+merge/54057
> You are subscribed to branch lp:nova.

[John Dewey (retr0h)]

Be sure to add yourself to Authors and http://wiki.openstack.org/Contributors, so if approved tarmac can pull them in.

See the prerequisites section of the How to Contribute Guide:
  http://wiki.openstack.org/HowToContribute

review: Needs Fixing

[John Dewey (retr0h)]

I suppose I should have marked my last review as "Needs Information".

[John Tran (jtran)]

Thanks, I've updated the Authors file.

[Jesse Andrews (anotherjesse)]

lgtm - we might want to move the fingerprint generation to a more central code path when we add keypair code into openstack api.

review: Approve

[Thierry Carrez (ttx)]

Feature is a bit late to the party, we just hit FeatureFreeze.

If you want this into Cactus rather than wait for Diablo, could you provide a quick benefit vs. risk of regression rationale, then request a specific FFe review from me ?

[Thierry Carrez (ttx)]

Linking a wishlist bug would also help.

[Thierry Carrez (ttx)]

review: Needs Information (ffe)

[John Tran (jtran)]

https://bugs.launchpad.net/nova/+bug/742602

[John Tran (jtran)]

This doesn't need to be in Cactus. It can wait for Diablo.

[termie (termie)]

linked to the bug, looks good to me, but I'll defer to Devin

review: Approve

[Thierry Carrez (ttx)]

OK, please set it to work in progress, you can repropose it once Diablo opens (in less than 3 weeks)

[Thierry Carrez (ttx)]

Merge window closed, please resubmit when Diablo opens (April 14).

review: Disapprove (ffe)

[justinsb (justin-fathomdb)]

I think the EC2 call is called "ImportKeyPair", so I think this the function name should be "import_key_pair" not "import_public_key". I'm also not sure how the arguments are being mapped. Is this compatible with EC2 clients?

[Thierry Carrez (ttx)]

The branch should be reproposed now that we are no longer frozen

review: Abstain (ffe)

[Devin Carlen (devcamcar)]

jtran - please repropose when ready.

[John Tran (jtran)]

> jtran - please repropose when ready.

I did that yesterday using the 'resubmit proposal' link. Was that the wrong way to do it?

[Devin Carlen (devcamcar)]

> > jtran - please repropose when ready.
>
> I did that yesterday using the 'resubmit proposal' link. Was that the wrong
> way to do it?

Hm, I think that should have worked. Maybe just try it again?

[Thierry Carrez (ttx)]

No need for FFe here.

review: Abstain (ffe)

[termie (termie)]

lgtm :)

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-05-12 19:51:03 +0000
+++ nova/api/ec2/cloud.py	2011-05-16 19:27:45 +0000
@@ -27,6 +27,8 @@
 import IPy
 import os
 import urllib
+import tempfile
+import shutil
 
 from nova import compute
 from nova import context
@@ -315,6 +317,27 @@
                 'keyMaterial': data['private_key']}
         # TODO(vish): when context is no longer an object, pass it here
 
+    def import_public_key(self, context, key_name, public_key,
+                         fingerprint=None):
+        LOG.audit(_("Import key %s"), key_name, context=context)
+        key = {}
+        key['user_id'] = context.user_id
+        key['name'] = key_name
+        key['public_key'] = public_key
+        if fingerprint is None:
+            tmpdir = tempfile.mkdtemp()
+            pubfile = os.path.join(tmpdir, 'temp.pub')
+            fh = open(pubfile, 'w')
+            fh.write(public_key)
+            fh.close()
+            (out, err) = utils.execute('ssh-keygen', '-q', '-l', '-f',
+                                       '%s' % (pubfile))
+            fingerprint = out.split(' ')[1]
+            shutil.rmtree(tmpdir)
+        key['fingerprint'] = fingerprint
+        db.key_pair_create(context, key)
+        return True
+
     def delete_key_pair(self, context, key_name, **kwargs):
         LOG.audit(_("Delete key pair %s"), key_name, context=context)
         try:

=== added directory 'nova/tests/public_key'
=== added file 'nova/tests/public_key/dummy.fingerprint'
--- nova/tests/public_key/dummy.fingerprint	1970-01-01 00:00:00 +0000
+++ nova/tests/public_key/dummy.fingerprint	2011-05-16 19:27:45 +0000
@@ -0,0 +1,1 @@
+1c:87:d1:d9:32:fd:62:3c:78:2b:c0:ad:c0:15:88:df

=== added file 'nova/tests/public_key/dummy.pub'
--- nova/tests/public_key/dummy.pub	1970-01-01 00:00:00 +0000
+++ nova/tests/public_key/dummy.pub	2011-05-16 19:27:45 +0000
@@ -0,0 +1,1 @@
+ssh-dss AAAAB3NzaC1kc3MAAACBAMGJlY9XEIm2X234pdO5yFWMp2JuOQx8U0E815IVXhmKxYCBK9ZakgZOIQmPbXoGYyV+mziDPp6HJ0wKYLQxkwLEFr51fAZjWQvRss0SinURRuLkockDfGFtD4pYJthekr/rlqMKlBSDUSpGq8jUWW60UJ18FGooFpxR7ESqQRx/AAAAFQC96LRglaUeeP+E8U/yblEJocuiWwAAAIA3XiMR8Skiz/0aBm5K50SeQznQuMJTyzt9S9uaz5QZWiFu69hOyGSFGw8fqgxEkXFJIuHobQQpGYQubLW0NdaYRqyE/Vud3JUJUb8Texld6dz8vGemyB5d1YvtSeHIo8/BGv2msOqR3u5AZTaGCBD9DhpSGOKHEdNjTtvpPd8S8gAAAIBociGZ5jf09iHLVENhyXujJbxfGRPsyNTyARJfCOGl0oFV6hEzcQyw8U/ePwjgvjc2UizMWLl8tsb2FXKHRdc2v+ND3Us+XqKQ33X3ADP4FZ/+Oj213gMyhCmvFTP0u5FmHog9My4CB7YcIWRuUR42WlhQ2IfPvKwUoTk3R+T6Og== www-data@mk

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-05-12 19:51:03 +0000
+++ nova/tests/test_cloud.py	2011-05-16 19:27:45 +0000
@@ -354,6 +354,36 @@
         self.assertTrue(filter(lambda k: k['keyName'] == 'test1', keys))
         self.assertTrue(filter(lambda k: k['keyName'] == 'test2', keys))
 
+    def test_import_public_key(self):
+        # test when user provides all values
+        result1 = self.cloud.import_public_key(self.context,
+                                               'testimportkey1',
+                                               'mytestpubkey',
+                                               'mytestfprint')
+        self.assertTrue(result1)
+        keydata = db.key_pair_get(self.context,
+                                  self.context.user.id,
+                                  'testimportkey1')
+        self.assertEqual('mytestpubkey', keydata['public_key'])
+        self.assertEqual('mytestfprint', keydata['fingerprint'])
+        # test when user omits fingerprint
+        pubkey_path = os.path.join(os.path.dirname(__file__), 'public_key')
+        f = open(pubkey_path + '/dummy.pub', 'r')
+        dummypub = f.readline().rstrip()
+        f.close
+        f = open(pubkey_path + '/dummy.fingerprint', 'r')
+        dummyfprint = f.readline().rstrip()
+        f.close
+        result2 = self.cloud.import_public_key(self.context,
+                                               'testimportkey2',
+                                               dummypub)
+        self.assertTrue(result2)
+        keydata = db.key_pair_get(self.context,
+                                  self.context.user.id,
+                                  'testimportkey2')
+        self.assertEqual(dummypub, keydata['public_key'])
+        self.assertEqual(dummyfprint, keydata['fingerprint'])
+
     def test_delete_key_pair(self):
         self._create_key('test')
         self.cloud.delete_key_pair(self.context, 'test')