Merge lp:~justin-fathomdb/nova/bug732204 into lp:~hudson-openstack/nova/trunk

[justinsb]

Keypairs are not required in the OpenStack API; don't require them!

[Dan Prince (dan-prince)]

LGTM.

review: Approve

[Jay Pipes (jaypipes)]

Do we have a test case that stresses the code path where no key pair is present?

review: Needs Information

[justinsb (justin-fathomdb)]

Well, I have a testing branch that is failing right now because it needs a
key pair:
https://code.launchpad.net/~justin-fathomdb/nova/test-openstack-servers

So we will (hopefully) have a test soon, but it's not in this MP.

[Jay Pipes (jaypipes)]

I understand about the other branch, but can we add a test case to this branch that stresses the code path of not having a key pair, or is that not possible?

[justinsb (justin-fathomdb)]

I'll see if I can rustle something up...

[justinsb (justin-fathomdb)]

Added test with and without keypairs. Test fails without the patch.

[Jay Pipes (jaypipes)]

danke! lgtm.

review: Approve

[Rick Harris (rconradharris)]

Nice job, Justin. Ran the code w/ and w/o fix, tests worked as expected.

Small nits:

> 66 + def _test_create_instance_helper(self, with_key_pair)

`with_key_pair` isn't used.

> 78 + fakes.stub_out_key_pair_funcs(self.stubs, False)

Test might be a bit more readable if kwarg passing was used:

    fakes.stub_out_key_pair_funcs(self.stubs, have_key_pair=False)

review: Needs Fixing

[justinsb (justin-fathomdb)]

Thanks Rick - obviously wasn't paying 100% attention there....

[Rick Harris (rconradharris)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~justin-fathomdb/nova/bug732204 into lp:nova failed. Below is the output from the failed tests.

AccountsTest
    test_account_create OK
    test_account_delete OK
    test_account_update OK
    test_get_account OK
AdminAPITest
    test_admin_disabled OK
    test_admin_enabled OK
APITest
    test_exceptions_are_converted_to_faults OK
Test
    test_authorize_token OK
    test_authorize_user OK
    test_bad_token OK
    test_bad_user_bad_key OK
    test_bad_user_good_key OK
    test_no_user OK
    test_token_expiry OK
TestFunctional
    test_token_doesnotexist OK
    test_token_expiry OK
TestLimiter
    test_authorize_token OK
LimiterTest
    test_limiter_custom_max_limit OK
    test_limiter_limit_and_offset OK
    test_limiter_limit_medium OK
    test_limiter_limit_over_max OK
    test_limiter_limit_zero OK
    test_limiter_negative_limit OK
    test_limiter_negative_offset OK
    test_limiter_nothing OK
    test_limiter_offset_bad OK
    test_limiter_offset_blank OK
    test_limiter_offset_medium OK
    test_limiter_offset_over_max OK
    test_limiter_offset_zero OK
TestFaults
    test_fault_parts OK
    test_raise OK
    test_retry_header OK
FlavorsTest
    test_get_flavor_by_id OK
    test_get_flavor_list OK
GlanceImageServiceTest
    test_create OK
    test_create_and_show_non_existing_image OK
    test_delete OK
    test_update OK
ImageControllerWithGlanceServiceTest
    test_get_image_details OK
    test_get_image_index OK
LocalImageServiceTest
    test_create OK
    test_create_and_show_non_existing_image OK
    test_delete...

[justinsb (justin-fathomdb)]

Hopefully this will keep Jenkins happy.

And no, this time I wasn't using git-bzr :-)

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~justin-fathomdb/nova/bug732204 into lp:nova failed. Below is the output from the failed tests.

AccountsTest
    test_account_create OK
    test_account_delete OK
    test_account_update OK
    test_get_account OK
AdminAPITest
    test_admin_disabled OK
    test_admin_enabled OK
APITest
    test_exceptions_are_converted_to_faults OK
Test
    test_authorize_token OK
    test_authorize_user OK
    test_bad_token OK
    test_bad_user_bad_key OK
    test_bad_user_good_key OK
    test_no_user OK
    test_token_expiry OK
TestFunctional
    test_token_doesnotexist OK
    test_token_expiry OK
TestLimiter
    test_authorize_token OK
LimiterTest
    test_limiter_custom_max_limit OK
    test_limiter_limit_and_offset OK
    test_limiter_limit_medium OK
    test_limiter_limit_over_max OK
    test_limiter_limit_zero OK
    test_limiter_negative_limit OK
    test_limiter_negative_offset OK
    test_limiter_nothing OK
    test_limiter_offset_bad OK
    test_limiter_offset_blank OK
    test_limiter_offset_medium OK
    test_limiter_offset_over_max OK
    test_limiter_offset_zero OK
TestFaults
    test_fault_parts OK
    test_raise OK
    test_retry_header OK
FlavorsTest
    test_get_flavor_by_id OK
    test_get_flavor_list OK
GlanceImageServiceTest
    test_create OK
    test_create_and_show_non_existing_image OK
    test_delete OK
    test_update OK
ImageControllerWithGlanceServiceTest
    test_get_image_details OK
    test_get_image_index OK
LocalImageServiceTest
    test_create OK
    test_create_and_show_non_existing_image OK
    test_delete...

[Jay Pipes (jaypipes)]

cleaning this up locally...

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-03-11 19:49:32 +0000
+++ nova/api/openstack/servers.py	2011-03-16 00:01:22 +0000
@@ -146,10 +146,14 @@
             return faults.Fault(exc.HTTPUnprocessableEntity())
 
         context = req.environ['nova.context']
+
+        key_name = None
+        key_data = None
         key_pairs = auth_manager.AuthManager.get_key_pairs(context)
-        if not key_pairs:
-            raise exception.NotFound(_("No keypairs defined"))
-        key_pair = key_pairs[0]
+        if key_pairs:
+            key_pair = key_pairs[0]
+            key_name = key_pair['name']
+            key_data = key_pair['public_key']
 
         image_id = common.get_image_id_from_image_hash(self._image_service,
             context, env['server']['imageId'])
@@ -174,8 +178,8 @@
             ramdisk_id=ramdisk_id,
             display_name=env['server']['name'],
             display_description=env['server']['name'],
-            key_name=key_pair['name'],
-            key_data=key_pair['public_key'],
+            key_name=key_name,
+            key_data=key_data,
             metadata=metadata,
             onset_files=env.get('onset_files', []))
 

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-03-11 22:19:14 +0000
+++ nova/tests/api/openstack/fakes.py	2011-03-16 00:01:22 +0000
@@ -85,10 +85,17 @@
     return mapper
 
 
-def stub_out_key_pair_funcs(stubs):
+def stub_out_key_pair_funcs(stubs, have_key_pair=True):
     def key_pair(context, user_id):
         return [dict(name='key', public_key='public_key')]
-    stubs.Set(nova.db, 'key_pair_get_all_by_user', key_pair)
+
+    def no_key_pair(context, user_id):
+        return []
+
+    if have_key_pair:
+        stubs.Set(nova.db, 'key_pair_get_all_by_user', key_pair)
+    else:
+        stubs.Set(nova.db, '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-03-11 19:49:32 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-03-16 00:01:22 +0000
@@ -216,7 +216,8 @@
         servers = json.loads(res.body)['servers']
         self.assertEqual([s['id'] for s in servers], [1, 2])
 
-    def test_create_instance(self):
+    def _test_create_instance_helper(self):
+        """Shared implementation for tests below that create instance"""
         def instance_create(context, inst):
             return {'id': '1', 'display_name': 'server_test'}
 
@@ -271,6 +272,13 @@
 
         self.assertEqual(res.status_int, 200)
 
+    def test_create_instance(self):
+        self._test_create_instance_helper()
+
+    def test_create_instance_no_key_pair(self):
+        fakes.stub_out_key_pair_funcs(self.stubs, have_key_pair=False)
+        self._test_create_instance_helper()
+
     def test_update_no_body(self):
         req = webob.Request.blank('/v1.0/servers/1')
         req.method = 'PUT'