Merge lp:~gmb/maas/fix-my-stupidity into lp:~maas-committers/maas/trunk

[Graham Binns]

Fix TestNodeGroupInterface.test_clean_ip_ranges_works_with_ipv6_ranges()
so that it no longer uses factory.make_ip_range().

Fix TestNodeGroupInterface.test_clean_ip_ranges_works_with_ipv6_ranges()
so that it no longer uses factory.make_ip_range().

make_ip_range() makes no guarantees about the size of the range, other
than that the lower bound will be lower than the upper. So it was
possible for the returned IP range to span only two addresses, which
meant that when test_clean_ip_ranges_works_with_ipv6_ranges added 1 to
the lowest address in the range and subtracted 1 from the highest - in
order to definte a range within a range - it would end up just reversing
the bounds.

To fix this, I've made test_clean_ip_ranges_works_with_ipv6_ranges()
just use get_random_ipv6_network(), which it was already doing, and
which returns a network with a minimum size of 8.

[Jeroen T. Vermeulen (jtv)]

Yup, that should do it. Maybe make_ip_range should take a "minimum size" argument, too, but that would be harder to implement I suppose.

By the way, clean_ip_ranges looks as if it could be a lot prettier if it just started with "if I have nothing to do here, return right away."

review: Approve

[Graham Binns (gmb)]

> Yup, that should do it. Maybe make_ip_range should take a "minimum size"
> argument, too, but that would be harder to implement I suppose.

Yeah.

> By the way, clean_ip_ranges looks as if it could be a lot prettier if it just
> started with "if I have nothing to do here, return right away."

Agreed. I'll change that as a drive-by.

=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2014-07-03 09:42:15 +0000
+++ src/maasserver/models/nodegroupinterface.py	2014-07-03 17:00:27 +0000
@@ -225,70 +225,76 @@
 
     def clean_ip_range_bounds(self):
         """Ensure that the static and dynamic ranges have sane bounds."""
-        if (self.is_managed and (self.static_ip_range_low and
+        if not (self.is_managed and (self.static_ip_range_low and
             self.static_ip_range_high)):
-            errors = {}
-            ip_range_low = IPAddress(self.ip_range_low)
-            ip_range_high = IPAddress(self.ip_range_high)
-            static_ip_range_low = IPAddress(self.static_ip_range_low)
-            static_ip_range_high = IPAddress(self.static_ip_range_high)
-
-            message_base = (
-                "Lower bound %s is higher than upper bound %s")
-            try:
-                IPRange(static_ip_range_low, static_ip_range_high)
-            except AddrFormatError:
-                message = (
-                    message_base % (
-                        self.static_ip_range_low, self.static_ip_range_high))
-                errors['static_ip_range_low'] = [message]
-                errors['static_ip_range_high'] = [message]
-            try:
-                IPRange(ip_range_low, ip_range_high)
-            except AddrFormatError:
-                message = (
-                    message_base % (self.ip_range_low, self.ip_range_high))
-                errors['ip_range_low'] = [message]
-                errors['ip_range_high'] = [message]
-
-            if errors:
-                raise ValidationError(errors)
+            # Exit early with nothing to do.
+            return
+
+        errors = {}
+        ip_range_low = IPAddress(self.ip_range_low)
+        ip_range_high = IPAddress(self.ip_range_high)
+        static_ip_range_low = IPAddress(self.static_ip_range_low)
+        static_ip_range_high = IPAddress(self.static_ip_range_high)
+
+        message_base = (
+            "Lower bound %s is higher than upper bound %s")
+        try:
+            IPRange(static_ip_range_low, static_ip_range_high)
+        except AddrFormatError:
+            message = (
+                message_base % (
+                    self.static_ip_range_low, self.static_ip_range_high))
+            errors['static_ip_range_low'] = [message]
+            errors['static_ip_range_high'] = [message]
+        try:
+            IPRange(ip_range_low, ip_range_high)
+        except AddrFormatError:
+            message = (
+                message_base % (self.ip_range_low, self.ip_range_high))
+            errors['ip_range_low'] = [message]
+            errors['ip_range_high'] = [message]
+
+        if errors:
+            raise ValidationError(errors)
 
     def clean_ip_ranges(self):
         """Ensure that the static and dynamic ranges don't overlap."""
-        if (self.is_managed and (self.static_ip_range_low and
+        if not (self.is_managed and (self.static_ip_range_low and
             self.static_ip_range_high)):
-            errors = {}
-            ip_range_low = IPAddress(self.ip_range_low)
-            ip_range_high = IPAddress(self.ip_range_high)
-            static_ip_range_low = IPAddress(self.static_ip_range_low)
-            static_ip_range_high = IPAddress(self.static_ip_range_high)
-
-            static_range = IPRange(
-                static_ip_range_low, static_ip_range_high)
-            dynamic_range = IPRange(
-                ip_range_low, ip_range_high)
-
-            # This is a bit unattractive, but we can't use IPSet for
-            # large networks - it's far too slow. What we actually care
-            # about is whether the lows and highs of the static range
-            # fall within the dynamic range and vice-versa, which
-            # IPRange gives us.
-            networks_overlap = (
-                static_ip_range_low in dynamic_range or
-                static_ip_range_high in dynamic_range or
-                ip_range_low in static_range or
-                ip_range_high in static_range
-            )
-            if networks_overlap:
-                message = "Static and dynamic IP ranges may not overlap."
-                errors = {
-                    'ip_range_low': [message],
-                    'ip_range_high': [message],
-                    'static_ip_range_low': [message],
-                    'static_ip_range_high': [message],
-                    }
-                raise ValidationError(errors)
+            # Nothing to do; bail out.
+            return
+
+        errors = {}
+        ip_range_low = IPAddress(self.ip_range_low)
+        ip_range_high = IPAddress(self.ip_range_high)
+        static_ip_range_low = IPAddress(self.static_ip_range_low)
+        static_ip_range_high = IPAddress(self.static_ip_range_high)
+
+        static_range = IPRange(
+            static_ip_range_low, static_ip_range_high)
+        dynamic_range = IPRange(
+            ip_range_low, ip_range_high)
+
+        # This is a bit unattractive, but we can't use IPSet for
+        # large networks - it's far too slow. What we actually care
+        # about is whether the lows and highs of the static range
+        # fall within the dynamic range and vice-versa, which
+        # IPRange gives us.
+        networks_overlap = (
+            static_ip_range_low in dynamic_range or
+            static_ip_range_high in dynamic_range or
+            ip_range_low in static_range or
+            ip_range_high in static_range
+        )
+        if networks_overlap:
+            message = "Static and dynamic IP ranges may not overlap."
+            errors = {
+                'ip_range_low': [message],
+                'ip_range_high': [message],
+                'static_ip_range_low': [message],
+                'static_ip_range_high': [message],
+                }
+            raise ValidationError(errors)
 
     def clean_fields(self, *args, **kwargs):
         super(NodeGroupInterface, self).clean_fields(*args, **kwargs)

=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
--- src/maasserver/models/tests/test_nodegroupinterface.py	2014-07-03 09:56:26 +0000
+++ src/maasserver/models/tests/test_nodegroupinterface.py	2014-07-03 17:00:27 +0000
@@ -28,7 +28,10 @@
 from maasserver.testing import reload_object
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from netaddr import IPNetwork
+from netaddr import (
+    IPAddress,
+    IPNetwork,
+    )
 from provisioningserver.utils.network import make_network
 
 
@@ -237,11 +240,14 @@
     def test_clean_ip_ranges_works_with_ipv6_ranges(self):
         network = factory.get_random_ipv6_network()
         interface = make_interface(network)
-        dynamic_low, dynamic_high = factory.make_ipv6_range(network)
-        interface.ip_range_low = unicode(dynamic_low)
-        interface.ip_range_high = unicode(dynamic_high)
-        interface.static_ip_range_low = unicode(dynamic_low + 1)
-        interface.static_ip_range_high = unicode(dynamic_high - 1)
+        interface.ip_range_low = unicode(
+            IPAddress(network.first))
+        interface.ip_range_high = unicode(
+            IPAddress(network.last))
+        interface.static_ip_range_low = unicode(
+            IPAddress(network.first + 1))
+        interface.static_ip_range_high = unicode(
+            IPAddress(network.last - 1))
         exception = self.assertRaises(
             ValidationError, interface.full_clean)
         message = "Static and dynamic IP ranges may not overlap."