Merge lp:~allenap/maas/rpc-get-secrets into lp:~maas-committers/maas/trunk

[Gavin Panella]

GetSecrets RPC call to allow cluster access to the API.

This is based on Graham's GetSecrets work, which was vetoed by Julian. However, there is *too much* work to do to convert the code in provisioningserver.tags to using RPC right now. I am blocked on needing this, and removing Celery from the cluster is blocked on needing this. Please do not veto this branch in the same way that Graham's was.

[lp:~allenap/maas/rpc-get-secrets]

2933. By Gavin Panella on 2014-09-08

Tests for get_api_credentials.

[Julian Edwards (julian-edwards)]

I'm not going to block it, but I'm not going to approve it either. Out of
interest, what constitutes too much work here?

The tags stuff is also blocked by the problem we talked about yesterday,
namely the GIL block. As I said in the other review I don't think the process
pipe will help as the reader blocks until it closes, so it needs twisted IO
instead (which is why ampoule was created I suspect).

I think that we need to think harder about the architecture of these jobs
rather than throwing RPC at everything. We might want to include API
parameters as part of the RPC call-out to do the job, for example, especially
in the case of the long running jobs where we don't want to wait around. We
can then reactor.spawnProcess() and leave it running and get notified when it
finishes.

[Graham Binns (gmb)]

review: Approve

[Graham Binns (gmb)]

(Not sure if Jool's comments still stand in the light of the pipe fork stuff not running in the reactor thread… I suspect they do to a point, though).

[Julian Edwards (julian-edwards)]

If the pipe stuff prevents the IO block, then we're golden. I still think it might be easier to send API creds to each job that needs them though.

[Gavin Panella (allenap)]

> I'm not going to block it, but I'm not going to approve it either. Out
> of interest, what constitutes too much work here?

There are three functions in p.tags that correspond to API calls:

    def get_nodes_for_node_group(client, nodegroup_uuid):
        """Retrieve the UUIDs of nodes in a particular group.

    def get_details_for_nodes(client, nodegroup_uuid, system_ids):
        """Retrieve details for a set of nodes.

    def post_updated_nodes(client, tag_name, tag_definition, ...):
        """Update the nodes relevant for a particular tag.

The first and last of those could be rewritten as RPC calls as a
straight port. The middle one is the problem: it downloads many
megabytes (of XML) in a single call. Rewriting that to work over RPC
would mean inventing a mechanism for packing and unpacking the data over
the wire. That's square-nail-round-hammer territory, at least for now
when we'd have to rush a design and implementation.

This code works as it is right now. I can port the Celery tasks that
trigger it to RPC calls, but I think we should leave this code well
alone for now. It really isn't doing us any harm.

>
> The tags stuff is also blocked by the problem we talked about
> yesterday, namely the GIL block. As I said in the other review I don't
> think the process pipe will help as the reader blocks until it closes,
> so it needs twisted IO instead (which is why ampoule was created I
> suspect).

As discussed elsewhere, the process pipe (aka `pipefork` and
`objectfork`) needs to be run from a non-reactor thread. I'll add a
guard to prevent it from being used in the reactor thread.

>
> I think that we need to think harder about the architecture of these
> jobs rather than throwing RPC at everything. We might want to include
> API parameters as part of the RPC call-out to do the job, for example,
> especially in the case of the long running jobs where we don't want to
> wait around. We can then reactor.spawnProcess() and leave it running
> and get notified when it finishes.

I can live with passing the API parameters per-task rather than having a
GetSecrets call.

[Julian Edwards (julian-edwards)]

On Wednesday 10 Sep 2014 23:20:44 you wrote:
> The first and last of those could be rewritten as RPC calls as a
> straight port. The middle one is the problem: it downloads many
> megabytes (of XML) in a single call. Rewriting that to work over RPC
> would mean inventing a mechanism for packing and unpacking the data over
> the wire. That's square-nail-round-hammer territory, at least for now
> when we'd have to rush a design and implementation.
>
> This code works as it is right now. I can port the Celery tasks that
> trigger it to RPC calls, but I think we should leave this code well
> alone for now. It really isn't doing us any harm.

Fair enough, it sounds like we're better off with the API then.

> As discussed elsewhere, the process pipe (aka `pipefork` and
> `objectfork`) needs to be run from a non-reactor thread. I'll add a
> guard to prevent it from being used in the reactor thread.

Tiptop! Thanks.

> I can live with passing the API parameters per-task rather than having a
> GetSecrets call.

The more I think about it the more I like it. I indicates to the casual
viewer that this RPC will be using the API to return results, and that is a
special situation.

Cheers!

Unmerged revisions

2933. By Gavin Panella on 2014-09-08

Tests for get_api_credentials.

2932. By Gavin Panella on 2014-09-08

Rename an argument.

2931. By Gavin Panella on 2014-09-08

Move get_cluster_api_credentials() to the m.rpc.configuration module.

2930. By Gavin Panella on 2014-09-08

Merge lp:~gmb/maas/rpc-get-secrets, resolving several small conflicts.

=== modified file 'src/maasserver/rpc/configuration.py'
--- src/maasserver/rpc/configuration.py	2014-09-04 13:19:09 +0000
+++ src/maasserver/rpc/configuration.py	2014-09-08 22:06:15 +0000
@@ -13,14 +13,19 @@
 
 __metaclass__ = type
 __all__ = [
+    "get_api_credentials",
     "get_archive_mirrors",
     "get_proxies",
 ]
 
 from urlparse import urlparse
 
+from apiclient.creds import convert_tuple_to_string
 from maasserver.models.config import Config
+from maasserver.models.nodegroup import NodeGroup
+from maasserver.models.user import get_creds_tuple
 from maasserver.utils.async import transactional
+from provisioningserver.rpc import exceptions
 from provisioningserver.utils.twisted import synchronous
 
 
@@ -53,3 +58,15 @@
     if http_proxy is not None:
         http_proxy = urlparse(http_proxy)
     return {"http": http_proxy, "https": http_proxy}
+
+
+@synchronous
+@transactional
+def get_api_credentials(cluster_uuid):
+    """Return the API creds for a cluster as a string."""
+    try:
+        the_cluster = NodeGroup.objects.get_by_natural_key(cluster_uuid)
+    except NodeGroup.DoesNotExist:
+        raise exceptions.NoSuchCluster.from_uuid(cluster_uuid)
+    else:
+        return convert_tuple_to_string(get_creds_tuple(the_cluster.api_token))

=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2014-09-05 08:10:06 +0000
+++ src/maasserver/rpc/regionservice.py	2014-09-08 22:06:15 +0000
@@ -268,6 +268,17 @@
         d.addCallback(lambda node: {'system_id': node.system_id})
         return d
 
+    @region.GetSecrets.responder
+    def get_secrets(self, cluster_uuid):
+        """get_secrets()
+
+        Implementation of
+        :py:class:`~provisioningserver.rpc.region.GetSecrets`.
+        """
+        d = deferToThread(configuration.get_api_credentials, cluster_uuid)
+        d.addCallback(lambda creds: {b"credentials": creds})
+        return d
+
 
 @implementer(IConnection)
 class RegionServer(Region):

=== modified file 'src/maasserver/rpc/tests/test_configuration.py'
--- src/maasserver/rpc/tests/test_configuration.py	2014-09-04 13:19:09 +0000
+++ src/maasserver/rpc/tests/test_configuration.py	2014-09-08 22:06:15 +0000
@@ -16,16 +16,20 @@
 
 from urlparse import urlparse
 
+from apiclient.creds import convert_tuple_to_string
 from maasserver.models.config import Config
+from maasserver.models.user import get_creds_tuple
 from maasserver.rpc.configuration import (
+    get_api_credentials,
     get_archive_mirrors,
     get_proxies,
     )
-from maastesting.factory import factory
-from maastesting.testcase import MAASTestCase
-
-
-class TestGetArchiveMirrors(MAASTestCase):
+from maasserver.testing.factory import factory
+from maastesting.djangotestcase import TransactionTestCase
+from provisioningserver.rpc import exceptions
+
+
+class TestGetArchiveMirrors(TransactionTestCase):
 
     def test_returns_populated_dict_when_main_and_port_is_set(self):
         url = factory.make_parsed_url().geturl()
@@ -36,7 +40,7 @@
             get_archive_mirrors())
 
 
-class TestGetProxies(MAASTestCase):
+class TestGetProxies(TransactionTestCase):
 
     def test_returns_populated_dict_when_http_proxy_is_not_set(self):
         Config.objects.set_config("http_proxy", None)
@@ -50,3 +54,18 @@
         self.assertEqual(
             {"http": urlparse(url), "https": urlparse(url)},
             get_proxies())
+
+
+class TestGetAPICredentials(TransactionTestCase):
+
+    def test__gets_credentials_for_cluster(self):
+        nodegroup = factory.make_NodeGroup()
+        token = nodegroup.api_token
+        expected_credentials = convert_tuple_to_string(get_creds_tuple(token))
+        observed_credentials = get_api_credentials(nodegroup.uuid)
+        self.assertEqual(expected_credentials, observed_credentials)
+
+    def test__raises_exception_when_cluster_not_found(self):
+        self.assertRaises(
+            exceptions.NoSuchCluster,
+            get_api_credentials, factory.make_UUID())

=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2014-09-05 16:38:32 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-09-08 22:06:15 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = []
 
+
 from collections import defaultdict
 from contextlib import closing
 from itertools import product
@@ -23,6 +24,7 @@
 import threading
 from urlparse import urlparse
 
+from apiclient.creds import convert_tuple_to_string
 from crochet import wait_for_reactor
 from django.db import connection
 from django.db.utils import ProgrammingError
@@ -39,6 +41,7 @@
 from maasserver.models.event import Event
 from maasserver.models.eventtype import EventType
 from maasserver.models.node import Node
+from maasserver.models.user import get_creds_tuple
 from maasserver.rpc import regionservice
 from maasserver.rpc.regionservice import (
     Region,
@@ -50,7 +53,6 @@
 from maasserver.testing.architecture import make_usable_architecture
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
-from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.async import transactional
 from maastesting.djangotestcase import TransactionTestCase
 from maastesting.matchers import (
@@ -84,6 +86,7 @@
     GetBootSourcesV2,
     GetClusterInterfaces,
     GetProxies,
+    GetSecrets,
     Identify,
     ListNodePowerParameters,
     MarkNodeFailed,
@@ -793,7 +796,7 @@
         yield d.addErrback(check)
 
 
-class TestRegionServer(MAASServerTestCase):
+class TestRegionServer(TransactionTestCase):
 
     def test_interfaces(self):
         protocol = RegionServer()
@@ -1574,7 +1577,7 @@
         self.assertItemsEqual([], service._get_addresses())
 
 
-class TestRegionProtocol_ReportForeignDHCPServer(MAASTestCase):
+class TestRegionProtocol_ReportForeignDHCPServer(TransactionTestCase):
 
     def test_create_node_is_registered(self):
         protocol = Region()
@@ -1629,7 +1632,7 @@
             tasks.write_dhcp_config.apply_async, MockNotCalled())
 
 
-class TestRegionProtocol_GetClusterInterfaces(MAASTestCase):
+class TestRegionProtocol_GetClusterInterfaces(TransactionTestCase):
 
     def test_create_node_is_registered(self):
         protocol = Region()
@@ -1666,9 +1669,9 @@
             expected_interfaces, response["interfaces"])
 
 
-class TestRegionProtocol_CreateNode(MAASTestCase):
+class TestRegionProtocol_CreateNode(TransactionTestCase):
 
-    def test_create_node_is_registered(self):
+    def test__is_registered(self):
         protocol = Region()
         responder = protocol.locateResponder(
             CreateNode.commandName)
@@ -1680,10 +1683,9 @@
 
     @wait_for_reactor
     @inlineCallbacks
-    def test_calls_create_node_function(self):
-        create_node_function = self.patch(regionservice, 'create_node')
-        create_node_function.return_value = yield deferToThread(
-            self.create_node)
+    def test__calls_create_node_function(self):
+        create_node = self.patch_autospec(regionservice, 'create_node')
+        create_node.return_value = yield deferToThread(self.create_node)
 
         params = {
             'cluster_uuid': factory.make_name('uuid'),
@@ -1698,11 +1700,48 @@
         self.assertIsNotNone(response)
 
         self.assertThat(
-            create_node_function,
+            create_node,
             MockCalledOnceWith(
                 params['cluster_uuid'], params['architecture'],
                 params['power_type'], params['power_parameters'],
                 params['mac_addresses']))
         self.assertEqual(
-            create_node_function.return_value.system_id,
-            response['system_id'])
+            {"system_id": create_node.return_value.system_id},
+            response)
+
+
+class TestRegionProtocol_GetSecrets(TransactionTestCase):
+
+    def test_identify_is_registered(self):
+        protocol = Region()
+        responder = protocol.locateResponder(GetSecrets.commandName)
+        self.assertIsNot(responder, None)
+
+    @transactional
+    def create_cluster(self):
+        return factory.make_node_group()
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test_returns_api_secrets_for_cluster(self):
+        cluster = yield deferToThread(self.create_cluster)
+        expected_credentials = convert_tuple_to_string(
+            get_creds_tuple(cluster.api_token))
+
+        response = yield call_responder(
+            Region(), GetSecrets, {b'cluster_uuid': cluster.uuid})
+        self.assertEqual(
+            expected_credentials, response['credentials'])
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test_errors_if_cluster_not_found(self):
+        cluster_uuid = factory.make_UUID()
+        d = call_responder(
+            Region(), GetSecrets, {b'cluster_uuid': cluster_uuid})
+
+        def check(error):
+            self.assertIsInstance(error, Failure)
+            self.assertIsInstance(error.value, NoSuchCluster)
+
+        yield d.addErrback(check)

=== modified file 'src/provisioningserver/rpc/region.py'
--- src/provisioningserver/rpc/region.py	2014-09-05 09:15:57 +0000
+++ src/provisioningserver/rpc/region.py	2014-09-08 22:06:15 +0000
@@ -329,3 +329,20 @@
         ]
     response = []
     errors = []
+
+
+class GetSecrets(amp.Command):
+    """Get the API secrets for a cluster.
+
+    :since: 1.7
+    """
+
+    arguments = [
+        (b"cluster_uuid", amp.Unicode()),
+    ]
+    response = [
+        (b"credentials", amp.Unicode()),
+    ]
+    errors = {
+        NoSuchCluster: b"NoSuchCluster",
+    }