Merge lp:~gmb/maas/bug-1336617 into lp:~maas-committers/maas/trunk

[Graham Binns]

NodeGroupInterface now checks for invalid IP range bounds when saved. Previously, reversed bounds could cause an unhandled AddrFormatError.

[Gavin Panella (allenap)]

Looks good, but see my comments in the diff.

review: Approve

[Graham Binns (gmb)]

On 2 July 2014 14:40, Gavin Panella <email address hidden> wrote:
> Review: Approve
>
> Looks good, but see my comments in the diff.

Thanks for the review!

> Diff comments:
>
>> === modified file 'src/maasserver/models/nodegroupinterface.py'
>> --- src/maasserver/models/nodegroupinterface.py 2014-06-30 14:46:10 +0000
>> +++ src/maasserver/models/nodegroupinterface.py 2014-07-02 13:30:15 +0000
>> @@ -218,18 +218,49 @@
>> # form; validation breaks if we pass an IPAddress.
>> self.broadcast_ip = unicode(network.broadcast)
>>
>> + def clean_ip_range_bounds(self):
>> + """Ensure that the static and dynamic ranges have sane bounds."""
>> + if (self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED and
>
> This same condition appears already in 4 other places. What do you think of adding an `is_managed` property?

I think I noticed the same thing but was lazy. Gut Idee.

>> + (self.static_ip_range_low and self.static_ip_range_high)):
>
> Should do the `is not None` thing here, to avoid evaluating static_ip_range_{low,high} in a boolean context, which might not coincide with what you intend.

No; (static_)ip_range_(low|high) can be an empty string, too.

>> + 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")
>
> Any reason we don't reorder them automatically?

Yes. If I write:

 ip_range_low = 10.1.0.100
 ip_range_high = 10.1.0.15

How are we to know that I didn't mean to write low = 10.1.0.10?

>> + 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.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED 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_h...

[Gavin Panella (allenap)]

>>> + (self.static_ip_range_low and self.static_ip_range_high)):
>>
>> Should do the `is not None` thing here, to avoid evaluating
>> static_ip_range_{low,high} in a boolean context, which might not
>> coincide with what you intend.
>
> No; (static_)ip_range_(low|high) can be an empty string, too.

If you wanted to be a really good citizen you should check `thing is not
None and len(thing) != 0` but I think you can be forgiven here because
Django.

>
>>> + 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")
>>
>> Any reason we don't reorder them automatically?
>
> Yes. If I write:
>
> ip_range_low = 10.1.0.100
> ip_range_high = 10.1.0.15
>
> How are we to know that I didn't mean to write low = 10.1.0.10?

That's a good point.

[Graham Binns (gmb)]

On 3 July 2014 11:24, Gavin Panella <email address hidden> wrote:
> If you wanted to be a really good citizen you should check `thing is not
> None and len(thing) != 0` but I think you can be forgiven here because
> Django.

I am not, nor do I want to be, a good citizen.

=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2014-06-30 14:46:10 +0000
+++ src/maasserver/models/nodegroupinterface.py	2014-07-03 09:47:03 +0000
@@ -127,6 +127,11 @@
         else:
             return None
 
+    @property
+    def is_managed(self):
+        """Return true if this interface is managed by MAAS."""
+        return self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
+
     def display_management(self):
         """Return management status text as displayed to the user."""
         return NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[self.management]
@@ -152,7 +157,7 @@
 
     def clean_network_not_too_big(self):
         # If management is not 'UNMANAGED', refuse huge networks.
-        if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
+        if self.is_managed:
             network = self.network
             if network is not None:
                 if network.prefixlen < MINIMUM_NETMASK_BITS:
@@ -165,7 +170,7 @@
     def clean_network_config_if_managed(self):
         # If management is not 'UNMANAGED', all the network information
         # should be provided.
-        if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
+        if self.is_managed:
             mandatory_fields = [
                 'interface',
                 'subnet_mask',
@@ -218,18 +223,49 @@
             # form; validation breaks if we pass an IPAddress.
             self.broadcast_ip = unicode(network.broadcast)
 
+    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
+            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)
+
     def clean_ip_ranges(self):
         """Ensure that the static and dynamic ranges don't overlap."""
-        if (self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED and
-           (self.static_ip_range_low and self.static_ip_range_high)):
+        if (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)
+                static_ip_range_low, static_ip_range_high)
             dynamic_range = IPRange(
                 ip_range_low, ip_range_high)
 
@@ -260,4 +296,5 @@
         self.clean_network_not_too_big()
         self.clean_ips_in_network()
         self.clean_network_config_if_managed()
+        self.clean_ip_range_bounds()
         self.clean_ip_ranges()
 
=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
--- src/maasserver/models/tests/test_nodegroupinterface.py	2014-07-02 07:05:24 +0000
+++ src/maasserver/models/tests/test_nodegroupinterface.py	2014-07-03 09:47:03 +0000
@@ -273,3 +273,29 @@
             'static_ip_range_high': [message],
             }
         self.assertEqual(errors, exception.message_dict)
+
+    def test_clean_ip_range_bounds_checks_for_reversed_range_bounds(self):
+        network = IPNetwork("10.1.0.0/16")
+        interface = make_interface(network)
+        interface.ip_range_low = '10.1.0.2'
+        interface.ip_range_high = '10.1.0.1'
+        interface.static_ip_range_low = '10.1.0.10'
+        interface.static_ip_range_high = '10.1.0.9'
+        exception = self.assertRaises(
+            ValidationError, interface.full_clean)
+        message = "Lower bound %s is higher than upper bound %s"
+        errors = {
+            'ip_range_low': [
+                message % (interface.ip_range_low, interface.ip_range_high)],
+            'ip_range_high': [
+                message % (interface.ip_range_low, interface.ip_range_high)],
+            'static_ip_range_low': [
+                message % (
+                    interface.static_ip_range_low,
+                    interface.static_ip_range_high)],
+            'static_ip_range_high': [
+                message % (
+                    interface.static_ip_range_low,
+                    interface.static_ip_range_high)],
+            }
+        self.assertEqual(errors, exception.message_dict)