MAAS

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

  1. validate-ipv4-ipv6-mix
  2. Merge into trunk
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 3015
Proposed branch: lp:~jtv/maas/validate-ipv4-ipv6-mix
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 85 lines (+26/-2)
3 files modified
src/maasserver/models/network.py (+12/-1)
src/maasserver/models/tests/test_network.py (+12/-0)
src/maasserver/models/tests/test_node.py (+2/-1)
To merge this branch: bzr merge lp:~jtv/maas/validate-ipv4-ipv6-mix
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+234938@code.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Looks good.

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (18.9 KiB)

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...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
250250
251 dns_servers = CharField(251 dns_servers = CharField(
252 blank=True, editable=True, null=True, max_length=255,252 blank=True, editable=True, null=True, max_length=255,
253 help_text="Space separated list of DNS server addresses that this "253 help_text="Space-separated list of DNS server addresses that this "
254 "network may use.")254 "network may use.")
255255
256 vlan_tag = PositiveSmallIntegerField(256 vlan_tag = PositiveSmallIntegerField(
@@ -349,10 +349,21 @@
349 raise ValidationError(349 raise ValidationError(
350 {'netmask': ["This netmask leaves no room for IP addresses."]})350 {'netmask': ["This netmask leaves no room for IP addresses."]})
351351
352 def clean_default_gateway(self):
353 if self.default_gateway not in (None, ''):
354 gateway_addr = IPAddress(self.default_gateway)
355 ip_addr = IPAddress(self.ip)
356 if gateway_addr.version != ip_addr.version:
357 message = (
358 "You can't mix IPv4 and IPv6 anywhere in the same network "
359 "definition.")
360 raise ValidationError({'default_gateway': [message]})
361
352 def clean_fields(self, *args, **kwargs):362 def clean_fields(self, *args, **kwargs):
353 super(Network, self).clean_fields(*args, **kwargs)363 super(Network, self).clean_fields(*args, **kwargs)
354 self.clean_vlan_tag()364 self.clean_vlan_tag()
355 self.clean_netmask()365 self.clean_netmask()
366 self.clean_default_gateway()
356367
357 def clean(self):368 def clean(self):
358 super(Network, self).clean()369 super(Network, self).clean()
359370
=== 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 @@
28 VLANSpecifier,28 VLANSpecifier,
29 )29 )
30from maasserver.testing.factory import factory30from maasserver.testing.factory import factory
31from maasserver.testing.orm import reload_object
31from maasserver.testing.testcase import MAASServerTestCase32from maasserver.testing.testcase import MAASServerTestCase
32from mock import sentinel33from mock import sentinel
33from netaddr import (34from netaddr import (
@@ -384,6 +385,17 @@
384 network.netmask = '255.255.255.0'385 network.netmask = '255.255.255.0'
385 self.assertRaises(ValidationError, network.save)386 self.assertRaises(ValidationError, network.save)
386387
388 def test_default_gateway_validation_errors_on_mixed_IPv4_and_IPv6(self):
389 network = factory.make_Network(network=factory.make_ipv6_network())
390 network.default_gateway = factory.make_ipv4_address()
391 self.assertRaises(ValidationError, network.save)
392
393 def test_default_gateway_validation_accepts_blank(self):
394 network = factory.make_Network(network=factory.make_ipv6_network())
395 network.default_gateway = None
396 network.save()
397 self.assertIsNone(reload_object(network).default_gateway)
398
387 def test_unicode_returns_cidr_if_tag_is_zero(self):399 def test_unicode_returns_cidr_if_tag_is_zero(self):
388 cidr = '10.9.0.0/16'400 cidr = '10.9.0.0/16'
389 network = factory.make_Network(network=IPNetwork(cidr))401 network = factory.make_Network(network=IPNetwork(cidr))
390402
=== 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 @@
1167 def test_mac_addresses_on_managed_interfaces_returns_only_managed(self):1167 def test_mac_addresses_on_managed_interfaces_returns_only_managed(self):
1168 node = factory.make_node_with_mac_attached_to_nodegroupinterface(1168 node = factory.make_node_with_mac_attached_to_nodegroupinterface(
1169 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)1169 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
1170 mac_with_interface = node.get_primary_mac()
11701171
1171 mac_with_no_interface = factory.make_MACAddress(node=node)1172 mac_with_no_interface = factory.make_MACAddress(node=node)
1172 unmanaged_interface = factory.make_NodeGroupInterface(1173 unmanaged_interface = factory.make_NodeGroupInterface(
@@ -1177,7 +1178,7 @@
1177 ignore_unused(mac_with_no_interface, mac_with_unmanaged_interface)1178 ignore_unused(mac_with_no_interface, mac_with_unmanaged_interface)
11781179
1179 observed = node.mac_addresses_on_managed_interfaces()1180 observed = node.mac_addresses_on_managed_interfaces()
1180 self.assertItemsEqual([node.get_primary_mac()], observed)1181 self.assertItemsEqual([mac_with_interface], observed)
11811182
1182 def test_mac_addresses_on_managed_interfaces_returns_empty_if_none(self):1183 def test_mac_addresses_on_managed_interfaces_returns_empty_if_none(self):
1183 node = factory.make_Node(mac=True)1184 node = factory.make_Node(mac=True)