launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/nodekey-token-fk into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/nodekey-token-fk/+merge/94766

Now that we need to look up the Token for a NodeKey, add a foreign key.  Not really _needed_ as such, but will speed things up.  And let's face it, NodeKey was already effectively a linking table.  Its existing "key" attribute becomes more of a cache.

Also, the node, token, and key are now all unique.  Maybe at some point we'll drop NodeKey altogether and make it a foreign key from Node to Token.  For now it's still comforting to have the extra flexibility of a separate table.
-- 
https://code.launchpad.net/~jtv/maas/nodekey-token-fk/+merge/94766
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/nodekey-token-fk into lp:maas.

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-24 14:15:34 +0000
+++ src/metadataserver/models.py	2012-02-27 13:43:18 +0000
@@ -29,33 +29,82 @@
     BinaryField,
     )
 from metadataserver.nodeinituser import get_node_init_user
-from piston.models import KEY_SIZE
+from piston.models import (
+    KEY_SIZE,
+    Token,
+    )
 
 
 class NodeKeyManager(Manager):
-    """Utility for the collection of NodeKeys."""
+    """Utility for the collection of NodeKeys.
+
+    Each Node that needs to access the metadata service will have its own
+    OAuth token, tied to the dedicated "node-init" user.  Each node will see
+    just its own meta-data when it accesses the serviec.
+
+    NodeKeyManager is what connects those nodes to their respective tokens.
+
+    There's two parts to using NodeKey and NodeKeyManager:
+
+    1. get_token_for_node(node) gives you a token that the node can then
+        access the metadata service with.  From the "token" that this
+        returns, the node will need to know node.key, node.secret, and
+        node.consumer.key for its credentials.
+
+    2. get_node_for_key(key) takes the token.key (which will be in the
+        http Authorization header of a metadata request as "oauth_token")
+        and looks up the associated Node.
+    """
 
     def create_token(self, node):
         """Create an OAuth token for a given node.
 
-        The node will be able to use these credentials for accessing the
-        metadata service.  It will see its own, custom metadata.
-
         :param node: The system that is to be allowed access to the metadata
             service.
         :type node: Node
-        :return: Token for the node to use.  It will belong to the
-            maas-init-node user.  If passed the token's key,
-            `get_node_for_key` will return `node`.
+        :return: Token for the node to use.
         :rtype: piston.models.Token
         """
         token = create_auth_token(get_node_init_user())
-        self.create(node=node, key=token.key)
+        self.create(node=node, token=token, key=token.key)
         return token
 
+    def get_token_for_node(self, node):
+        """Find node's OAuth token, or if it doesn't have one, create it.
+
+        This implicitly grants cloud-init on the node access to the metadata
+        service.
+
+        Barring exceptions, this will always hold:
+
+            get_node_for_key(get_token_for_node(node).key) == node
+
+        :param node: The node that needs an oauth token for access to the
+            metadata service.
+        :type node: Node
+        :return: An OAuth token, belonging to the node-init user, but
+            uniquely associated with this node.
+        :rtype: piston.models.Token
+        """
+        existing_nodekey = self.filter(node=node)
+        assert len(existing_nodekey) in (0, 1), (
+            "Found %d keys for node (expected at most one)."
+            % len(existing_nodekey))
+        if len(existing_nodekey) == 0:
+            return self.create_token(node)
+        else:
+            [nodekey] = existing_nodekey
+            return nodekey.token
+
     def get_node_for_key(self, key):
         """Find the Node that `key` was created for.
 
+        Barring exceptions, this will always hold:
+
+            get_token_for_node(get_node_for_key(key)).key == key
+
+        :param key: The key part of a node's OAuth token.
+        :type key: basestring
         :raise NodeKey.DoesNotExist: if `key` is not associated with any
             node.
         """
@@ -72,8 +121,10 @@
 
     objects = NodeKeyManager()
 
-    node = ForeignKey(Node, null=False, editable=False)
-    key = CharField(max_length=KEY_SIZE, null=False, editable=False)
+    node = ForeignKey(Node, null=False, editable=False, unique=True)
+    token = ForeignKey(Token, null=False, editable=False, unique=True)
+    key = CharField(
+        max_length=KEY_SIZE, null=False, editable=False, unique=True)
 
 
 class NodeUserDataManager(Manager):

=== modified file 'src/metadataserver/tests/test_models.py'
--- src/metadataserver/tests/test_models.py	2012-02-24 14:03:02 +0000
+++ src/metadataserver/tests/test_models.py	2012-02-27 13:43:18 +0000
@@ -38,6 +38,31 @@
         self.assertRaises(
             NodeKey.DoesNotExist, NodeKey.objects.get_node_for_key, non_key)
 
+    def test_get_token_for_node_creates_token(self):
+        node = factory.make_node()
+        token = NodeKey.objects.get_token_for_node(node)
+        self.assertEqual(node, NodeKey.objects.get_node_for_key(token.key))
+
+    def test_get_token_for_node_returns_existing_token(self):
+        node = factory.make_node()
+        original_token = NodeKey.objects.get_token_for_node(node)
+        repeated_token = NodeKey.objects.get_token_for_node(node)
+        self.assertEqual(original_token, repeated_token)
+
+    def test_get_token_for_node_inverts_get_node_for_key(self):
+        node = factory.make_node()
+        self.assertEqual(
+            node,
+            NodeKey.objects.get_node_for_key(
+                NodeKey.objects.get_token_for_node(node).key))
+
+    def test_get_node_for_key_inverts_get_token_for_node(self):
+        key = NodeKey.objects.get_token_for_node(factory.make_node()).key
+        self.assertEqual(
+            key,
+            NodeKey.objects.get_token_for_node(
+                NodeKey.objects.get_node_for_key(key)).key)
+
 
 class TestNodeUserDataManager(TestCase):
     """Test NodeUserDataManager."""