Merge lp:~allenap/maas/bug-1081701-b into lp:~maas-committers/maas/trunk

[Gavin Panella]

Update the node group's MAAS URL field when contacted by a starting cluster.

[Raphaël Badin (rvb)]

Looks generally ok but I'm afraid I've got a few questions about the specifics of this branch :).

[0]

8 - form.save()
9 + nodegroup = form.save()
10 + update_nodegroup_maas_url(nodegroup, request)

I don't really see the need for that: the maas_url field will be updated when the cluster will have been accepted and will try to connect again so why bother updating it also when the nodegroup is created (also, at this stage, this might be a malicious request)?

[1]

23 elif existing_nodegroup.status == NODEGROUP_STATUS.PENDING:
24 + update_nodegroup_maas_url(existing_nodegroup, request)

Again, why the need to update nodegroup.maas_url here? We only need it after the cluster controller has been accepted (and tries to connect) and that is already taken care of by the code you've added after " if existing_nodegroup.status == NODEGROUP_STATUS.ACCEPTED:".

[3]

We need to have a test to make sure that when the master nodegroup registers itself, the value of maas_url is not changed and is still the empty string. This is fairly important since the master nodegroup runs on the same machine as the region controller and is configured to reach the region controller through the localhost interface. When dealing with the master nodegroup, we need to use DEFAULT_MAAS_URL where we would have used nodegroup.maas_url otherwise. All the code I've written in the fixes for c), d) and e) assumes that if nodegroup.maas_url is '', then DEFAULT_MAAS_URL should be used.

[4]

49 + self.assertIn(
50 + response.status_code,
51 + {code for code in httplib.responses if code // 100 == 2})

If this fails, the error message is not going to be really explicit, it would be good to have the content of the response in the failure message.

[5]

116 +class TestUpdateNodeGroupMAASURL(TransactionTestCase):
The test for update_nodegroup_maas_url should be in a transaction.

Why? I don't see anything in there that justifies the usage of a TransactionTestCase over the regular (Django-based) TestCase.

review: Needs Information

[Raphaël Badin (rvb)]

Yes, I had a point named [2] but then I ditched it and forgot to update the numbering :)

[Gavin Panella (allenap)]

> Looks generally ok but I'm afraid I've got a few questions about the specifics
> of this branch :).

Thanks for the review!

>
> [0]
>
> 8       -                    form.save()
> 9       +                    nodegroup = form.save()
> 10      +                    update_nodegroup_maas_url(nodegroup, request)
>
> I don't really see the need for that:  the maas_url field will be updated when
> the cluster will have been accepted and will try to connect again so why
> bother updating it also when the nodegroup is created (also, at this stage,
> this might be a malicious request)?

You're right. I was thinking "it might not restart before it's
accepted", but that doesn't matter, because it will call register
again before it starts.

Fixed.

>
> [1]
>
> 23                   elif existing_nodegroup.status ==
> NODEGROUP_STATUS.PENDING:
> 24      +                update_nodegroup_maas_url(existing_nodegroup,
> request)
>
> Again, why the need to update nodegroup.maas_url here?  We only need it after
> the cluster controller has been accepted (and tries to connect) and that is
> already taken care of by the code you've added after " if
> existing_nodegroup.status == NODEGROUP_STATUS.ACCEPTED:".

As for [0].

>
> [3]
>
> We need to have a test to make sure that when the master nodegroup registers
> itself,

Do you mean, that it calls op=register, or is there some other
mechanism? test_register_master_nodegroup_does_not_update_maas_url
tests the former already I think.

>         the value of maas_url is not changed and is still the empty string.
> This is fairly important since the master nodegroup runs on the same machine
> as the region controller and is configured to reach the region controller
> through the localhost interface.  When dealing with the master nodegroup, we
> need to use DEFAULT_MAAS_URL where we would have used nodegroup.maas_url
> otherwise.  All the code I've written in the fixes for c), d) and e) assumes
> that if nodegroup.maas_url is '', then DEFAULT_MAAS_URL should be used.

I've added an assertion to
test_register_master_nodegroup_does_not_update_maas_url that tests the
model too.

>
> [4]
>
> 49      +        self.assertIn(
> 50      +            response.status_code,
> 51      +            {code for code in httplib.responses if code // 100 == 2})
>
> If this fails, the error message is not going to be really explicit, it would
> be good to have the content of the response in the failure message.

Fixed.

>
> [5]
>
> 116     +class TestUpdateNodeGroupMAASURL(TransactionTestCase):
> The test for update_nodegroup_maas_url should be in a transaction.
>
> Why?  I don't see anything in there that justifies the usage of a
> TransactionTestCase over the regular (Django-based) TestCase.

'Cause it updates the database? I suspect this is going to be another
WTFDjango moments.

[Raphaël Badin (rvb)]

> > [3]
> >
> > We need to have a test to make sure that when the master nodegroup registers
> > itself,
>
> Do you mean, that it calls op=register, or is there some other
> mechanism? test_register_master_nodegroup_does_not_update_maas_url
> tests the former already I think.

Oh yeah, you're right, I missed that test, my bad.

> > [5]
> >
> > 116     +class TestUpdateNodeGroupMAASURL(TransactionTestCase):
> > The test for update_nodegroup_maas_url should be in a transaction.
> >
> > Why?  I don't see anything in there that justifies the usage of a
> > TransactionTestCase over the regular (Django-based) TestCase.
>
> 'Cause it updates the database? I suspect this is going to be another
> WTFDjango moments.

Well, the transaction test case is only useful if the code you're testing explicitly uses transactions (see https://docs.djangoproject.com/en/dev/topics/testing/#django.test.TransactionTestCase for details).

[Raphaël Badin (rvb)]

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-26 08:52:01 +0000
+++ src/maasserver/api.py	2012-11-27 09:57:26 +0000
@@ -929,6 +929,7 @@
                     raise ValidationError(form.errors)
         else:
             if existing_nodegroup.status == NODEGROUP_STATUS.ACCEPTED:
+                update_nodegroup_maas_url(existing_nodegroup, request)
                 # The nodegroup exists and is validated, return the RabbitMQ
                 return get_celery_credentials()
             elif existing_nodegroup.status == NODEGROUP_STATUS.REJECTED:
@@ -938,6 +939,13 @@
                     "Awaiting admin approval.", status=httplib.ACCEPTED)
 
 
+def update_nodegroup_maas_url(nodegroup, request):
+    """Update `nodegroup.maas_url` from the given `request`."""
+    path = request.META["SCRIPT_NAME"]
+    nodegroup.maas_url = build_absolute_uri(request, path)
+    nodegroup.save()
+
+
 class NodeGroupsHandler(OperationsHandler):
     """Manage NodeGroups."""
     anonymous = AnonNodeGroupsHandler

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-26 08:52:01 +0000
+++ src/maasserver/tests/test_api.py	2012-11-27 09:57:26 +0000
@@ -144,6 +144,7 @@
 from testtools.matchers import (
     AfterPreprocessing,
     AllMatch,
+    Annotate,
     Contains,
     Equals,
     Is,
@@ -3782,6 +3783,87 @@
         self.assertIn('application/json', response['Content-Type'])
         self.assertEqual({'BROKER_URL': fake_broker_url}, parsed_result)
 
+    def assertSuccess(self, response):
+        """Assert that `response` was successful (i.e. HTTP 2xx)."""
+        self.assertThat(
+            {code for code in httplib.responses if code // 100 == 2},
+            Annotate(response, Contains(response.status_code)))
+
+    def test_register_new_nodegroup_does_not_record_maas_url(self):
+        # When registering a cluster, the URL with which the call was made
+        # (i.e. from the perspective of the cluster) is *not* recorded.
+        self.create_configured_master()
+        name = factory.make_name('name')
+        uuid = factory.getRandomUUID()
+        update_maas_url = self.patch(api, "update_nodegroup_maas_url")
+        response = self.client.post(
+            reverse('nodegroups_handler'),
+            {'op': 'register', 'name': name, 'uuid': uuid})
+        self.assertSuccess(response)
+        self.assertEqual([], update_maas_url.call_args_list)
+
+    def test_register_accepted_nodegroup_updates_maas_url(self):
+        # When registering an existing, accepted, cluster, the URL with which
+        # the call was made is updated.
+        self.create_configured_master()
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
+        update_maas_url = self.patch(api, "update_nodegroup_maas_url")
+        response = self.client.post(
+            reverse('nodegroups_handler'),
+            {'op': 'register', 'uuid': nodegroup.uuid})
+        self.assertSuccess(response)
+        update_maas_url.assert_called_once_with(nodegroup, ANY)
+
+    def test_register_pending_nodegroup_does_not_update_maas_url(self):
+        # When registering an existing, pending, cluster, the URL with which
+        # the call was made is *not* updated.
+        self.create_configured_master()
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
+        update_maas_url = self.patch(api, "update_nodegroup_maas_url")
+        response = self.client.post(
+            reverse('nodegroups_handler'),
+            {'op': 'register', 'uuid': nodegroup.uuid})
+        self.assertSuccess(response)
+        self.assertEqual([], update_maas_url.call_args_list)
+
+    def test_register_rejected_nodegroup_does_not_update_maas_url(self):
+        # When registering an existing, pending, cluster, the URL with which
+        # the call was made is *not* updated.
+        self.create_configured_master()
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.REJECTED)
+        update_maas_url = self.patch(api, "update_nodegroup_maas_url")
+        response = self.client.post(
+            reverse('nodegroups_handler'),
+            {'op': 'register', 'uuid': nodegroup.uuid})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertEqual([], update_maas_url.call_args_list)
+
+    def test_register_master_nodegroup_does_not_update_maas_url(self):
+        # When registering the master cluster, the URL with which the call was
+        # made is *not* updated.
+        name = factory.make_name('name')
+        update_maas_url = self.patch(api, "update_nodegroup_maas_url")
+        response = self.client.post(
+            reverse('nodegroups_handler'),
+            {'op': 'register', 'name': name, 'uuid': 'master'})
+        self.assertSuccess(response)
+        self.assertEqual([], update_maas_url.call_args_list)
+        # The new node group's maas_url field remains empty.
+        nodegroup = NodeGroup.objects.get(uuid='master')
+        self.assertEqual("", nodegroup.maas_url)
+
+
+class TestUpdateNodeGroupMAASURL(TestCase):
+    """Tests for `update_nodegroup_maas_url`."""
+
+    def test_update_from_request(self):
+        request_factory = RequestFactory(SCRIPT_NAME="/script")
+        request = request_factory.get(
+            "/script/path", SERVER_NAME="example.com")
+        nodegroup = factory.make_node_group()
+        api.update_nodegroup_maas_url(nodegroup, request)
+        self.assertEqual("http://example.com/script", nodegroup.maas_url)
+
 
 def dict_subset(obj, fields):
     """Return a dict of a subset of the fields/values of an object."""