Code review comment for lp:~gmb/maas/bug-1336617

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_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 +291,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-02 13:30:15 +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)
>>
>
>
> --
> https://code.launchpad.net/~gmb/maas/bug-1336617/+merge/225325
> You are the owner of lp:~gmb/maas/bug-1336617.