launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/1.2-bug-1070774 into lp:maas/1.2.

Commit message:
Disallow changes to Node.hostname on nodes that are currently allocated.  The change would confuse Juju, which will still refer to the node by its old hostname.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1070774 in MAAS: "The hostname of a node can still be changed once the node is in use"
  https://bugs.launchpad.net/maas/+bug/1070774

For more details, see:
https://code.launchpad.net/~jtv/maas/1.2-bug-1070774/+merge/131612

As discussed with Raphaël.  Meant to be forward-ported to trunk as well.

Jeroen
-- 
https://code.launchpad.net/~jtv/maas/1.2-bug-1070774/+merge/131612
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/1.2-bug-1070774 into lp:maas/1.2.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-26 10:20:19 +0000
+++ src/maasserver/api.py	2012-10-26 13:15:28 +0000
@@ -503,10 +503,19 @@
             The default is 'false'.
         :type power_parameters_skip_validation: basestring
         """
-
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
         data = get_overrided_query_dict(model_to_dict(node), request.data)
+
+        # Don't allow the hostname to be changed if the node is
+        # currently allocated.  Juju knows the node by its old name, so
+        # changing the name would confuse things.
+        data_hostname = data.get('hostname', node.hostname)
+        hostname_changed = (data_hostname != node.hostname)
+        if hostname_changed and node.status == NODE_STATUS.ALLOCATED:
+            raise ValidationError(
+                "Can't change hostname to %s: node is in use." % data_hostname)
+
         Form = get_node_edit_form(request.user)
         form = Form(data, instance=node)
         if form.is_valid():

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-26 10:20:19 +0000
+++ src/maasserver/tests/test_api.py	2012-10-26 13:15:28 +0000
@@ -1310,6 +1310,41 @@
 
         self.assertEqual(httplib.OK, response.status_code)
 
+    def test_PUT_refuses_to_update_hostname_on_allocated_node(self):
+        # A node's hostname can't be changed while it's in use.  Juju
+        # may still want to refer to the node by its existing hostname.
+        old_name = factory.make_name('old-hostname')
+        new_name = factory.make_name('new-hostname')
+        node = factory.make_node(
+            owner=self.logged_in_user, hostname=old_name,
+            status=NODE_STATUS.ALLOCATED)
+        response = self.client.put(
+            self.get_node_uri(node), {'hostname': new_name})
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        expected_text = (
+            "Can't change hostname to %s: node is in use." % new_name)
+        self.assertEqual(
+            (httplib.BAD_REQUEST, expected_text),
+            (response.status_code, response.content))
+        self.assertEqual(old_name, reload_object(node).hostname)
+
+    def test_PUT_accepts_unchanged_hostname_on_allocated_node(self):
+        node = factory.make_node(
+            owner=self.logged_in_user, status=NODE_STATUS.ALLOCATED)
+        old_name = node.hostname
+        response = self.client.put(
+            self.get_node_uri(node), {'hostname': node.hostname})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(old_name, reload_object(node).hostname)
+
+    def test_PUT_accepts_omitted_hostname_on_allocated_node(self):
+        node = factory.make_node(
+            owner=self.logged_in_user, status=NODE_STATUS.ALLOCATED)
+        old_name = node.hostname
+        response = self.client.put(self.get_node_uri(node), {})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(old_name, reload_object(node).hostname)
+
     def test_PUT_admin_can_change_power_type(self):
         self.become_admin()
         original_power_type = factory.getRandomChoice(