Merge lp:~rvb/maas/bug-1065406 into lp:maas/trunk

[Raphaël Badin]

Generate a dhcp key (if nodegroup,dhcp_key is empty) before we write the dhcp_config.

Make sure we have a valid dhcp_key before we write the dhcp_config.

= Pre-imp =

Discussed with Jeroen.

= Notes =

The trick in ensure_dhcp_key is a bit messy but I see no other way to do this: ensure_dhcp_key is called from configure_dhcp which is hooked-up to NodeGroup.post_save so it cannot trigger a pose_save signal itself.

[Jeroen T. Vermeulen (jtv)]

Good stuff. Just this:

3 + def ensure_dhcp_key(self):
24 + """If self.dhcp_key is empty: create a valid dhcp key.
25 +
26 + This method persists the dhcp key without triggering the model
27 + signals (pre_save/post_save/etc)."""

Not just create: set it on the nodegroup. Maybe "ensure that this nodegroup has a dhcp key"?

28 + if self.dhcp_key == '':
29 + dhcp_key = generate_omapi_key()
30 + self.dhcp_key = dhcp_key
31 + # Persist the dhcp_key without triggering the signals.
32 + NodeGroup.objects.filter(id=self.id).update(dhcp_key=dhcp_key)

Don't forget to say why it's important that the signal doesn't fire!

[Jeroen T. Vermeulen (jtv)]

Hmmm that was meant to be an Approve. Thought I'd clicked that.

review: Approve

[lp:~rvb/maas/bug-1065406]

1259. By Raphaël Badin on 2012-10-11

Fix docstring.

[Raphaël Badin (rvb)]

Thanks for the review.

I've updated the docstring of ensure_dhcp_key.

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2012-09-25 14:45:59 +0000
+++ src/maasserver/dhcp.py	2012-10-11 11:01:21 +0000
@@ -48,6 +48,10 @@
     if not is_dhcp_managed(nodegroup):
         return
 
+    # Make sure this nodegroup has a key to communicate with the dhcp
+    # server.
+    nodegroup.ensure_dhcp_key()
+
     # Use the server's address (which is where the central TFTP
     # server is) for the next_server setting.  We'll want to proxy
     # it on the local worker later, and then we can use

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-10-04 07:55:27 +0000
+++ src/maasserver/models/nodegroup.py	2012-10-11 11:01:21 +0000
@@ -201,6 +201,19 @@
                 nodegroup=self).exclude(
                     management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED))
 
+    def ensure_dhcp_key(self):
+        """Ensure that this nodegroup has a dhcp key.
+
+        This method persists the dhcp key without triggering the model
+        signals (pre_save/post_save/etc) because it's called from
+        dhcp.configure_dhcp which, in turn, it called from the post_save
+        signal of NodeGroup."""
+        if self.dhcp_key == '':
+            dhcp_key = generate_omapi_key()
+            self.dhcp_key = dhcp_key
+            # Persist the dhcp_key without triggering the signals.
+            NodeGroup.objects.filter(id=self.id).update(dhcp_key=dhcp_key)
+
     @property
     def work_queue(self):
         """The name of the queue for tasks specific to this nodegroup."""

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2012-09-30 21:19:08 +0000
+++ src/maasserver/tests/test_dhcp.py	2012-10-11 11:01:21 +0000
@@ -29,6 +29,7 @@
 from netaddr import IPNetwork
 from provisioningserver import tasks
 from testresources import FixtureResource
+from testtools.matchers import EndsWith
 
 
 class TestDHCP(TestCase):
@@ -107,6 +108,15 @@
             mocked_check_call.call_args[0][0],
             ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
 
+    def test_configure_dhcp_is_called_with_valid_dhcp_key(self):
+        self.patch(dhcp, 'write_dhcp_config')
+        self.patch(settings, "DHCP_CONNECT", True)
+        nodegroup = factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED, dhcp_key='')
+        configure_dhcp(nodegroup)
+        args, kwargs = dhcp.write_dhcp_config.apply_async.call_args
+        self.assertThat(kwargs['kwargs']['omapi_key'], EndsWith('=='))
+
     def test_dhcp_config_gets_written_when_nodegroup_becomes_active(self):
         nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
         self.patch(settings, "DHCP_CONNECT", True)

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-10-08 12:50:32 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-10-11 11:01:21 +0000
@@ -38,6 +38,7 @@
     )
 from testresources import FixtureResource
 from testtools.matchers import (
+    EndsWith,
     GreaterThan,
     MatchesStructure,
     )
@@ -324,3 +325,26 @@
             status=factory.getRandomEnum(NODEGROUP_STATUS))
         nodegroup.reject()
         self.assertEqual(nodegroup.status, NODEGROUP_STATUS.REJECTED)
+
+    def test_ensure_dhcp_key_creates_key(self):
+        nodegroup = factory.make_node_group(dhcp_key='')
+        nodegroup.ensure_dhcp_key()
+        # Check that the dhcp_key is not empty and looks
+        # valid.
+        self.assertThat(nodegroup.dhcp_key, EndsWith("=="))
+        # The key is persisted.
+        self.assertThat(
+            reload_object(nodegroup).dhcp_key, EndsWith("=="))
+
+    def test_ensure_dhcp_key_preserves_existing_key(self):
+        key = factory.make_name('dhcp-key')
+        nodegroup = factory.make_node_group(dhcp_key=key)
+        nodegroup.ensure_dhcp_key()
+        self.assertEqual(key, nodegroup.dhcp_key)
+
+    def test_ensure_dhcp_key_creates_different_keys(self):
+        nodegroup1 = factory.make_node_group(dhcp_key='')
+        nodegroup2 = factory.make_node_group(dhcp_key='')
+        nodegroup1.ensure_dhcp_key()
+        nodegroup2.ensure_dhcp_key()
+        self.assertNotEqual(nodegroup1.dhcp_key, nodegroup2.dhcp_key)