Merge lp:~jtv/maas/validate-ipv4-ipv6-mix into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Validate that a Network's default_gateway (if set) is of the same address family as the Network itself. Disallow IPv4 gateways on IPv6 networks and vice versa.

I went through Network and NodeGroupInterface looking for situations where we might fail to detect impermissible mixed entry of IPv4/IPv6 addresses. The only cases I found were this one, and DNS server addresses. However we treat DNS server addresses as a black box, so I chose not to validate them in this way. ISC's DHCP server would not accept an IPv4 DNS server address on an IPv6 subnet or vice versa, but that's not to say the model shouldn't tolerate it. If the issue came up, we would probably want to filter unusable DNS server addresses where appropriate.

Jeroen

[Gavin Panella (allenap)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~jtv/maas/validate-ipv4-ipv6-mix 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]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:2 http://security.ubuntu.com trusty-security Release [59.7 kB]
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]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
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:5 http://security.ubuntu.com trusty-security/main Sources [44.3 kB]
Get:6 http://security.ubuntu.com trusty-security/universe Sources [10.8 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [140 kB]
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [47.0 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [120 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [84.0 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [319 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [202 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
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,088 kB in 0s (1,697 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-openss...

[Jeroen T. Vermeulen (jtv)]

Damn. I get that test failure very occasionally (a fraction of a percent of the times I run that test) even on trunk. Slightly more frequently on my branch than on trunk, I think. It looks like it may be a problem where get_primary_mac() makes a nondeterministic choice between MACAddress entries.

I'm including a tiny change to the test that seems to fix it. It's clear which MAC the call to get_primary_mac() was supposed to pick; I just changed the test to obtain it before the second MAC is added to the same node. It's not material to the test as such.

=== modified file 'src/maasserver/models/network.py'
--- src/maasserver/models/network.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/models/network.py	2014-09-17 10:54:47 +0000
@@ -250,7 +250,7 @@
 
     dns_servers = CharField(
         blank=True, editable=True, null=True, max_length=255,
-        help_text="Space separated list of DNS server addresses that this "
+        help_text="Space-separated list of DNS server addresses that this "
                   "network may use.")
 
     vlan_tag = PositiveSmallIntegerField(
@@ -349,10 +349,21 @@
             raise ValidationError(
                 {'netmask': ["This netmask leaves no room for IP addresses."]})
 
+    def clean_default_gateway(self):
+        if self.default_gateway not in (None, ''):
+            gateway_addr = IPAddress(self.default_gateway)
+            ip_addr = IPAddress(self.ip)
+            if gateway_addr.version != ip_addr.version:
+                message = (
+                    "You can't mix IPv4 and IPv6 anywhere in the same network "
+                    "definition.")
+                raise ValidationError({'default_gateway': [message]})
+
     def clean_fields(self, *args, **kwargs):
         super(Network, self).clean_fields(*args, **kwargs)
         self.clean_vlan_tag()
         self.clean_netmask()
+        self.clean_default_gateway()
 
     def clean(self):
         super(Network, self).clean()

=== modified file 'src/maasserver/models/tests/test_network.py'
--- src/maasserver/models/tests/test_network.py	2014-09-15 14:28:28 +0000
+++ src/maasserver/models/tests/test_network.py	2014-09-17 10:54:47 +0000
@@ -28,6 +28,7 @@
     VLANSpecifier,
     )
 from maasserver.testing.factory import factory
+from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
 from mock import sentinel
 from netaddr import (
@@ -384,6 +385,17 @@
         network.netmask = '255.255.255.0'
         self.assertRaises(ValidationError, network.save)
 
+    def test_default_gateway_validation_errors_on_mixed_IPv4_and_IPv6(self):
+        network = factory.make_Network(network=factory.make_ipv6_network())
+        network.default_gateway = factory.make_ipv4_address()
+        self.assertRaises(ValidationError, network.save)
+
+    def test_default_gateway_validation_accepts_blank(self):
+        network = factory.make_Network(network=factory.make_ipv6_network())
+        network.default_gateway = None
+        network.save()
+        self.assertIsNone(reload_object(network).default_gateway)
+
     def test_unicode_returns_cidr_if_tag_is_zero(self):
         cidr = '10.9.0.0/16'
         network = factory.make_Network(network=IPNetwork(cidr))

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-09-16 09:35:10 +0000
+++ src/maasserver/models/tests/test_node.py	2014-09-17 10:54:47 +0000
@@ -1167,6 +1167,7 @@
     def test_mac_addresses_on_managed_interfaces_returns_only_managed(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface(
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        mac_with_interface = node.get_primary_mac()
 
         mac_with_no_interface = factory.make_MACAddress(node=node)
         unmanaged_interface = factory.make_NodeGroupInterface(
@@ -1177,7 +1178,7 @@
         ignore_unused(mac_with_no_interface, mac_with_unmanaged_interface)
 
         observed = node.mac_addresses_on_managed_interfaces()
-        self.assertItemsEqual([node.get_primary_mac()], observed)
+        self.assertItemsEqual([mac_with_interface], observed)
 
     def test_mac_addresses_on_managed_interfaces_returns_empty_if_none(self):
         node = factory.make_Node(mac=True)