launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/bug-1065406 into lp:maas.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1065406 in MAAS: "Nodegroup object is created with an empty dhcp_key."
  https://bugs.launchpad.net/maas/+bug/1065406

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1065406/+merge/129131

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.
-- 
https://code.launchpad.net/~rvb/maas/bug-1065406/+merge/129131
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1065406 into lp:maas.

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2012-09-25 14:45:59 +0000
+++ src/maasserver/dhcp.py	2012-10-11 09:42:19 +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 09:42:19 +0000
@@ -201,6 +201,17 @@
                 nodegroup=self).exclude(
                     management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED))
 
+    def ensure_dhcp_key(self):
+        """If self.dhcp_key is empty: create a valid dhcp key.
+
+        This method persists the dhcp key without triggering the model
+        signals (pre_save/post_save/etc)."""
+        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 09:42:19 +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 09:42:19 +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)