Merge lp:~julian-edwards/maas/enlist-nodegroup-bug-1274926 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Add a new parameter for node enlisting called "autodetect_nodegroup", which will avoid an error being returned if the nodegroup is not manually specified. Previously, non-node API users calling this would cause the nodegroup detection to be based on the caller's IP, which is wrong.

[Jeroen T. Vermeulen (jtv)]

Sensible change. I assume autodetection itself is already tested, in a test that needed no change. One thing that's a bit awkward (and I do see the XXX about it) is that the semantics of the autodetect_nodegroup parameter are only explained in the test. I think the representation of booleans is not very highly standardised, so it may be worth a note in the docstring even if the docstring does not cover all parameters.

For the error message in api.py, saying that the nodegroup parameter is missing may be too strong. There's a choice to be made. How about "one of nodegroup or autodetect_nodegroup must be specified"?

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for reviewing.

On Tuesday 11 Feb 2014 07:00:06 you wrote:
> Review: Approve
>
> Sensible change. I assume autodetection itself is already tested, in a test
> that needed no change. One thing that's a bit awkward (and I do see the
> XXX about it) is that the semantics of the autodetect_nodegroup parameter
> are only explained in the test. I think the representation of booleans is
> not very highly standardised, so it may be worth a note in the docstring
> even if the docstring does not cover all parameters.

I considered it but it felt daft to add just that one param and none of the
others, which is why I XXXed it for later! I'll stick it in the comment for
later inclusion in a decent docstring.

> For the error message in api.py, saying that the nodegroup parameter is
> missing may be too strong. There's a choice to be made. How about "one of
> nodegroup or autodetect_nodegroup must be specified"?

The code doesn't enforce that though, it just enforces the former without the
latter.

I'll land this now and we can discuss a different message later, I gotta run.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/enlist-nodegroup-bug-1274926 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com trusty Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com trusty Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty/main Sources [1,057 kB]
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com trusty/universe Sources [6,414 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages [1,315 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages [5,878 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 14.7 MB in 5s (2,680 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential curl daemontools distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops py...

[Julian Edwards (julian-edwards)]

So it looks like tests for creating new nodes on the api are split between
test_api_enlistment.py and test_api_nodes.py.

:/

[Raphaël Badin (rvb)]

As discussed this morning, this change means earlier versions of maas-enlist will fail to register nodes (because the API call will not contain the 'autodetect_nodegroup' parameter). This means we won't be able to enlist/commission nodes using pre-Trusty series. In conjunction with bug 1279310, this restricts the number of series that can be used for enlistment down to zero :). (Note that we have a manual workaround (see the bug description)).

[Jorge Niedbalski (niedbalski)]

Hello guys,

Any way to workaround this ( using maasclient api) with pre-trusty versions?

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-02-11 08:23:05 +0000
+++ src/maasserver/api.py	2014-02-11 23:37:14 +0000
@@ -514,6 +514,13 @@
         # If 'nodegroup' is not explicitely specified, get the origin of the
         # request to figure out which nodegroup the new node should be
         # attached to.
+        if request.data.get('autodetect_nodegroup', None) is None:
+            # We insist on this to protect command-line API users who
+            # are manually enlisting nodes.  You can't use the origin's
+            # IP address to indicate in which nodegroup the new node belongs.
+            raise ValidationError(
+                "'autodetect_nodegroup' must be specified if 'nodegroup' "
+                "parameter missing")
         nodegroup = find_nodegroup(request)
         if nodegroup is not None:
             altered_query_data['nodegroup'] = nodegroup
@@ -550,6 +557,11 @@
         the enlistment is done by a non-admin, the node is held in the
         "Declared" state for approval by a MAAS admin.
         """
+        # XXX 2014-02-11 bug=1278685
+        # There's no documentation here on what parameters can be passed!
+
+        # Note that request.autodetect_nodegroup is treated as a
+        # boolean; its presence indicates True.
         return create_node(request)
 
     @operation(idempotent=True)

=== modified file 'src/maasserver/tests/test_api_enlistment.py'
--- src/maasserver/tests/test_api_enlistment.py	2014-02-01 04:04:37 +0000
+++ src/maasserver/tests/test_api_enlistment.py	2014-02-11 23:37:14 +0000
@@ -54,6 +54,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'architecture': architecture,
                 'power_type': 'ether_wake',
@@ -76,6 +77,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname,
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -105,6 +107,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname,
                 'architecture': architecture,
                 'power_type': 'ether_wake',
@@ -125,6 +128,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'architecture': architecture.split('/')[0],
                 'power_type': 'ether_wake',
@@ -148,6 +152,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'architecture': architecture.split('/')[0],
                 'subarchitecture': architecture.split('/')[1],
@@ -171,6 +176,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'architecture': architecture,
                 'subarchitecture': architecture.split('/')[1],
@@ -192,6 +198,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'architecture': architecture,
                 'after_commissioning_action': (
@@ -209,6 +216,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname,
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'mac_addresses': [factory.getRandomMACAddress()],
@@ -223,6 +231,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': architecture,
                 'power_type': 'ether_wake',
                 'mac_addresses': [factory.getRandomMACAddress()],
@@ -247,6 +256,26 @@
             "Unrecognised signature: POST None",
             response.content)
 
+    def test_POST_new_fails_if_autodetect_nodegroup_required(self):
+        # If new() is called with no nodegroup, we require the client to
+        # explicitly also supply autodetect_nodegroup (with any value)
+        # to force the autodetection. If it's not supplied then an error
+        # is raised.
+        architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+        response = self.client.post(
+            reverse('nodes_handler'),
+            {
+                'op': 'new',
+                'architecture': architecture,
+                'power_type': 'ether_wake',
+                'mac_addresses': [factory.getRandomMACAddress()],
+            })
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        self.assertIn('text/plain', response['Content-Type'])
+        self.assertEqual(
+            "'autodetect_nodegroup' must be specified if 'nodegroup' "
+             "parameter missing", response.content)
+
     def test_POST_fails_if_mac_duplicated(self):
         # Mac Addresses should be unique.
         mac = 'aa:bb:cc:dd:ee:ff'
@@ -256,6 +285,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': architecture,
                 'hostname': factory.getRandomString(),
                 'mac_addresses': [mac],
@@ -275,6 +305,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'invalid_operation',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid'],
             })
@@ -291,6 +322,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid'],
             })
@@ -309,6 +341,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': 'diane',
                 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
                 'architecture': 'invalid-architecture',
@@ -342,6 +375,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname_with_domain,
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -366,6 +400,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname_without_domain,
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -388,6 +423,7 @@
             reverse('nodes_handler'),
             data={
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.make_name('hostname'),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -407,6 +443,7 @@
             reverse('nodes_handler'),
             data={
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.make_name('hostname'),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -438,6 +475,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'after_commissioning_action': (
@@ -470,6 +508,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'hostname': factory.getRandomString(),
                 'after_commissioning_action': (
@@ -535,6 +574,7 @@
         response = self.client.post(
             reverse('nodes_handler'), {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
                 'power_parameters': json.dumps(
@@ -556,6 +596,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'after_commissioning_action': (
@@ -592,6 +633,7 @@
         response = self.client.post(
             reverse('nodes_handler'), {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
                 'mac_addresses': ['00:11:22:33:44:55'],
@@ -609,6 +651,7 @@
         response = self.client.post(
             reverse('nodes_handler'), {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
                 'power_parameters_mac_address': new_mac_address,
@@ -628,6 +671,7 @@
         response = self.client.post(
             reverse('nodes_handler'), {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': hostname,
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -651,6 +695,7 @@
         response = self.client.post(
             reverse('nodes_handler'), {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
                 'power_parameters_param': param,
@@ -673,6 +718,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',
@@ -692,6 +738,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': 'ether_wake',

=== modified file 'src/maasserver/tests/test_api_nodes.py'
--- src/maasserver/tests/test_api_nodes.py	2014-02-10 11:34:47 +0000
+++ src/maasserver/tests/test_api_nodes.py	2014-02-11 23:37:14 +0000
@@ -152,6 +152,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': architecture,
                 'after_commissioning_action': (
@@ -168,6 +169,7 @@
             reverse('nodes_handler'),
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'after_commissioning_action': (
@@ -1085,6 +1087,7 @@
             '/api/1.0/nodes/MAAS/api/1.0/nodes/',
             {
                 'op': 'new',
+                'autodetect_nodegroup': '1',
                 'hostname': factory.getRandomString(),
                 'architecture': architecture,
                 'mac_addresses': ['aa:bb:cc:dd:ee:ff'],