launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/no-public-keys-is-not-an-error into lp:maas.

Commit message:
If a node requests its public-keys but there are none, just return a success response without any keys.  The tracebacks were distracting, and may or may not have affected commissioning.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1058313 in MAAS: "MAASAPINotFound: Unknown metadata attribute: public-keys  during commissioning"
  https://bugs.launchpad.net/maas/+bug/1058313

For more details, see:
https://code.launchpad.net/~jtv/maas/no-public-keys-is-not-an-error/+merge/127969

Julian ran into the traceback when commissioning.  Commissioning was broken for apparently unrelated reasons, but we might as well eliminate the error from our list of suspects.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/no-public-keys-is-not-an-error/+merge/127969
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/no-public-keys-is-not-an-error into lp:maas.

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-10-04 00:01:47 +0000
+++ src/metadataserver/api.py	2012-10-04 09:55:25 +0000
@@ -22,10 +22,7 @@
 
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
-from django.http import (
-    HttpResponse,
-    HttpResponseNotFound,
-    )
+from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from maasserver.api import (
     extract_oauth_key,
@@ -337,10 +334,8 @@
 
     def public_keys(self, node, version, item):
         """ Produce public-keys attribute."""
-        keys = SSHKey.objects.get_keys_for_user(user=node.owner)
-        if not keys:
-            raise MAASAPINotFound("No registered public keys")
-        return make_list_response(keys)
+        return make_list_response(
+            SSHKey.objects.get_keys_for_user(user=node.owner))
 
 
 class UserDataHandler(MetadataViewHandler):
@@ -367,6 +362,7 @@
     data = {
         'instance-id': 'i-maas-enlistment',
         'local-hostname': "maas-enlisting-node",
+        'public-keys': "",
     }
 
     def read(self, request, version, item=None):
@@ -374,12 +370,13 @@
 
         # Requesting the list of attributes, not any particular attribute.
         if item is None or len(item) == 0:
-            return make_list_response(sorted(self.data.keys()))
+            keys = sorted(self.data.keys())
+            # There's nothing in public-keys, so we don't advertise it.
+            # But cloud-init does ask for it and it's not worth logging
+            # a traceback for.
+            keys.remove('public-keys')
+            return make_list_response(keys)
 
-        # Enlistment asks for SSH keys.  We don't give it any, but it's
-        # not an error either.
-        if item == 'public-keys':
-            return HttpResponseNotFound("No SSH keys available for this node.")
         if item not in self.data:
             raise MAASAPINotFound("Unknown metadata attribute: %s" % item)
 

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-10-04 00:01:47 +0000
+++ src/metadataserver/tests/test_api.py	2012-10-04 09:55:25 +0000
@@ -272,11 +272,13 @@
         self.assertIn(
             'public-keys', response.content.decode('ascii').split('\n'))
 
-    def test_public_keys_for_node_without_public_keys_returns_not_found(self):
+    def test_public_keys_for_node_without_public_keys_returns_empty(self):
         url = reverse('metadata-meta-data', args=['latest', 'public-keys'])
         client = self.make_node_client()
         response = client.get(url)
-        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+        self.assertEqual(
+            (httplib.OK, ''),
+            (response.status_code, response.content))
 
     def test_public_keys_for_node_returns_list_of_keys(self):
         user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
@@ -675,15 +677,15 @@
         # just insist content is non-empty. It doesn't matter what it is.
         self.assertTrue(response.content)
 
-    def test_public_keys_returns_404_but_does_not_raise_exception(self):
+    def test_public_keys_returns_empty(self):
         # An enlisting node has no SSH keys, but it does request them
-        # (bug 1058313).  The request should fail, but without the log
-        # noise of an exception.
+        # (bug 1058313).  If the node insists, we give it the empty
+        # list.
         md_url = reverse(
             'enlist-metadata-meta-data', args=['latest', 'public-keys'])
         response = self.client.get(md_url)
         self.assertEqual(
-            (httplib.NOT_FOUND, "No SSH keys available for this node."),
+            (httplib.OK, ""),
             (response.status_code, response.content))
 
     def test_metadata_bogus_is_404(self):