Merge lp:~jtv/maas/hide-disable_ipv4-if-no-ipv6 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Hide a node's “Disable IPv4 on deployment” checkbox if its cluster is not configured to manage an IPv6 network.

When the option is hidden, it's present on the form (so that the API will continue to support it) but with a HiddenInput widget instead of the default CheckboxInput widget for boolean settings. That's what the test checks for. The label also matters (it shows up even if the widget is "hidden") but it's nowhere near as important as getting the widget right.

The check doesn't go into too much detail: it doesn't care whether the node is actually connected to any of the managed IPv6 networks, only that they're there. The Node edit form is meant to configure what we know about the node, so in principle, we might have to hide or reveal that checkbox actively as the user edits the form. That's probably more trouble than it's worth.

Also, in reality, the option isn't shown on the Add Node form. I left the option to change that open, and provided sensible (I hope) behaviour.

Jeroen

[Gavin Panella (allenap)]

Looks good.

What happens when there's a managed IPv6 network, the user disabled IPv4 on a node, then the IPv6 network is removed? Will MAAS still try to disable IPv4 on the node? If so, does this change remove the means to fix that?

review: Needs Information

[Jeroen T. Vermeulen (jtv)]

In that case, the disable_ipv4 setting will remain on the node, but the node will ignore it.

[Gavin Panella (allenap)]

That's grand.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~jtv/maas/hide-disable_ipv4-if-no-ipv6 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [59.7 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [46.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [10.8 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [146 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [49.1 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/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
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [125 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [86.2 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [337 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [209 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Fetched 1,130 kB in 0s (1,802 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi python-...

[Jeroen T. Vermeulen (jtv)]

Argh. Stuck behind that JS lint that snuck in while I was fixing the other JS lint...

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-10-03 01:50:39 +0000
+++ src/maasserver/forms.py	2014-10-03 04:54:19 +0000
@@ -150,7 +150,10 @@
     )
 from metadataserver.fields import Bin
 from metadataserver.models import CommissioningScript
-from netaddr import IPAddress
+from netaddr import (
+    IPAddress,
+    valid_ipv6,
+    )
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.network import REVEAL_IPv6
 from provisioningserver.rpc.exceptions import (
@@ -308,6 +311,14 @@
     return None, None
 
 
+def contains_managed_ipv6_interface(interfaces):
+    """Does any of a list of cluster interfaces manage a IPv6 subnet?"""
+    return any(
+        interface.manages_static_range() and valid_ipv6(interface.ip)
+        for interface in interfaces
+        )
+
+
 class NodeForm(MAASModelForm):
 
     def __init__(self, request=None, *args, **kwargs):
@@ -322,10 +333,25 @@
             # Offer choice of nodegroup.
             self.fields['nodegroup'] = NodeGroupFormField(
                 required=False, empty_label="Default (master)")
-        self.set_up_architecture_field()
-        self.set_up_osystem_and_distro_series_fields(kwargs.get('instance'))
 
         if not REVEAL_IPv6:
+            # We're not showing the IPv6 feature to the user.  Hide the ability
+            # to disable IPv4 on a node.
+            allow_disable_ipv4 = False
+        elif self.new_node:
+            # Permit disabling of IPv4 if at least one cluster supports IPv6.
+            allow_disable_ipv4 = contains_managed_ipv6_interface(
+                NodeGroupInterface.objects.all())
+        else:
+            # Permit disabling of IPv4 if at least one interface on the cluster
+            # supports IPv6.
+            allow_disable_ipv4 = contains_managed_ipv6_interface(
+                self.instance.nodegroup.nodegroupinterface_set.all())
+
+        self.set_up_architecture_field()
+        self.set_up_osystem_and_distro_series_fields(kwargs.get('instance'))
+
+        if not allow_disable_ipv4:
             # Hide the disable_ipv4 field until support works properly.  The
             # API will still support the field, but it won't be visible.
             # This hidden field absolutely needs an empty label, because its

=== modified file 'src/maasserver/tests/test_forms_node.py'
--- src/maasserver/tests/test_forms_node.py	2014-09-29 13:21:42 +0000
+++ src/maasserver/tests/test_forms_node.py	2014-10-03 04:54:19 +0000
@@ -15,6 +15,10 @@
 __all__ = []
 
 from crochet import TimeoutError
+from django.forms import (
+    CheckboxInput,
+    HiddenInput,
+    )
 from maasserver import forms
 from maasserver.clusterrpc.power_parameters import get_power_type_choices
 from maasserver.clusterrpc.testing.osystems import (
@@ -32,6 +36,7 @@
     NodeForm,
     pick_default_architecture,
     )
+import maasserver.forms as forms_module
 from maasserver.testing.architecture import (
     make_usable_architecture,
     patch_usable_architectures,
@@ -412,6 +417,57 @@
         node = form.save()
         self.assertEqual(setting, node.disable_ipv4)
 
+    def test_shows_disable_ipv4_if_IPv6_revealed_and_configured(self):
+        self.patch(forms_module, 'REVEAL_IPv6', True)
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        factory.make_NodeGroupInterface(
+            node.nodegroup, network=factory.make_ipv6_network(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        form = NodeForm(
+            instance=node,
+            data={'architecture': make_usable_architecture(self)})
+        self.assertIsInstance(
+            form.fields['disable_ipv4'].widget, CheckboxInput)
+
+    def test_hides_disable_ipv4_if_IPv6_not_revealed(self):
+        self.patch(forms_module, 'REVEAL_IPv6', False)
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        factory.make_NodeGroupInterface(
+            node.nodegroup, network=factory.make_ipv6_network(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        form = NodeForm(
+            instance=node,
+            data={'architecture': make_usable_architecture(self)})
+        self.assertIsInstance(form.fields['disable_ipv4'].widget, HiddenInput)
+
+    def test_hides_disable_ipv4_if_IPv6_not_configured(self):
+        self.patch(forms_module, 'REVEAL_IPv6', True)
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        factory.make_NodeGroupInterface(
+            node.nodegroup, network=factory.make_ipv6_network(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        form = NodeForm(
+            instance=node,
+            data={'architecture': make_usable_architecture(self)})
+        self.assertIsInstance(form.fields['disable_ipv4'].widget, HiddenInput)
+
+    def test_shows_disable_ipv4_on_new_node_if_any_cluster_supports_it(self):
+        self.patch(forms_module, 'REVEAL_IPv6', True)
+        factory.make_NodeGroupInterface(
+            factory.make_NodeGroup(), network=factory.make_ipv6_network(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        form = NodeForm(data={'architecture': make_usable_architecture(self)})
+        self.assertIsInstance(
+            form.fields['disable_ipv4'].widget, CheckboxInput)
+
+    def test_hides_disable_ipv4_on_new_node_if_no_cluster_supports_it(self):
+        self.patch(forms_module, 'REVEAL_IPv6', True)
+        factory.make_NodeGroupInterface(
+            factory.make_NodeGroup(), network=factory.make_ipv6_network(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        form = NodeForm(data={'architecture': make_usable_architecture(self)})
+        self.assertIsInstance(form.fields['disable_ipv4'].widget, HiddenInput)
+
 
 class TestAdminNodeForm(MAASServerTestCase):