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

Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: 2498
Merged at revision: 2504
Proposed branch: lp:~gmb/maas/bug-1336617
Merge into: lp:maas/trunk
Diff against target: 127 lines (+69/-6)
2 files modified
src/maasserver/models/nodegroupinterface.py (+43/-6)
src/maasserver/models/tests/test_nodegroupinterface.py (+26/-0)
To merge this branch: bzr merge lp:~gmb/maas/bug-1336617
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+225325@code.launchpad.net

Commit message

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

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

Looks good, but see my comments in the diff.

review: Approve
Revision history for this message
Graham Binns (gmb) wrote :
Download full text (5.4 KiB)

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

Read more...

lp:~gmb/maas/bug-1336617 updated
2498. By Graham Binns

Added an is_managed property to NodeGroupInterface.

Revision history for this message
Gavin Panella (allenap) wrote :

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

Revision history for this message
Graham Binns (gmb) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/nodegroupinterface.py'
2--- src/maasserver/models/nodegroupinterface.py 2014-06-30 14:46:10 +0000
3+++ src/maasserver/models/nodegroupinterface.py 2014-07-03 09:47:03 +0000
4@@ -127,6 +127,11 @@
5 else:
6 return None
7
8+ @property
9+ def is_managed(self):
10+ """Return true if this interface is managed by MAAS."""
11+ return self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
12+
13 def display_management(self):
14 """Return management status text as displayed to the user."""
15 return NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[self.management]
16@@ -152,7 +157,7 @@
17
18 def clean_network_not_too_big(self):
19 # If management is not 'UNMANAGED', refuse huge networks.
20- if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
21+ if self.is_managed:
22 network = self.network
23 if network is not None:
24 if network.prefixlen < MINIMUM_NETMASK_BITS:
25@@ -165,7 +170,7 @@
26 def clean_network_config_if_managed(self):
27 # If management is not 'UNMANAGED', all the network information
28 # should be provided.
29- if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
30+ if self.is_managed:
31 mandatory_fields = [
32 'interface',
33 'subnet_mask',
34@@ -218,18 +223,49 @@
35 # form; validation breaks if we pass an IPAddress.
36 self.broadcast_ip = unicode(network.broadcast)
37
38+ def clean_ip_range_bounds(self):
39+ """Ensure that the static and dynamic ranges have sane bounds."""
40+ if (self.is_managed and (self.static_ip_range_low and
41+ self.static_ip_range_high)):
42+ errors = {}
43+ ip_range_low = IPAddress(self.ip_range_low)
44+ ip_range_high = IPAddress(self.ip_range_high)
45+ static_ip_range_low = IPAddress(self.static_ip_range_low)
46+ static_ip_range_high = IPAddress(self.static_ip_range_high)
47+
48+ message_base = (
49+ "Lower bound %s is higher than upper bound %s")
50+ try:
51+ IPRange(static_ip_range_low, static_ip_range_high)
52+ except AddrFormatError:
53+ message = (
54+ message_base % (
55+ self.static_ip_range_low, self.static_ip_range_high))
56+ errors['static_ip_range_low'] = [message]
57+ errors['static_ip_range_high'] = [message]
58+ try:
59+ IPRange(ip_range_low, ip_range_high)
60+ except AddrFormatError:
61+ message = (
62+ message_base % (self.ip_range_low, self.ip_range_high))
63+ errors['ip_range_low'] = [message]
64+ errors['ip_range_high'] = [message]
65+
66+ if errors:
67+ raise ValidationError(errors)
68+
69 def clean_ip_ranges(self):
70 """Ensure that the static and dynamic ranges don't overlap."""
71- if (self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED and
72- (self.static_ip_range_low and self.static_ip_range_high)):
73+ if (self.is_managed and (self.static_ip_range_low and
74+ self.static_ip_range_high)):
75+ errors = {}
76 ip_range_low = IPAddress(self.ip_range_low)
77 ip_range_high = IPAddress(self.ip_range_high)
78 static_ip_range_low = IPAddress(self.static_ip_range_low)
79 static_ip_range_high = IPAddress(self.static_ip_range_high)
80
81 static_range = IPRange(
82- static_ip_range_low,
83- static_ip_range_high)
84+ static_ip_range_low, static_ip_range_high)
85 dynamic_range = IPRange(
86 ip_range_low, ip_range_high)
87
88@@ -260,4 +296,5 @@
89 self.clean_network_not_too_big()
90 self.clean_ips_in_network()
91 self.clean_network_config_if_managed()
92+ self.clean_ip_range_bounds()
93 self.clean_ip_ranges()
94
95=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
96--- src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-02 07:05:24 +0000
97+++ src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-03 09:47:03 +0000
98@@ -273,3 +273,29 @@
99 'static_ip_range_high': [message],
100 }
101 self.assertEqual(errors, exception.message_dict)
102+
103+ def test_clean_ip_range_bounds_checks_for_reversed_range_bounds(self):
104+ network = IPNetwork("10.1.0.0/16")
105+ interface = make_interface(network)
106+ interface.ip_range_low = '10.1.0.2'
107+ interface.ip_range_high = '10.1.0.1'
108+ interface.static_ip_range_low = '10.1.0.10'
109+ interface.static_ip_range_high = '10.1.0.9'
110+ exception = self.assertRaises(
111+ ValidationError, interface.full_clean)
112+ message = "Lower bound %s is higher than upper bound %s"
113+ errors = {
114+ 'ip_range_low': [
115+ message % (interface.ip_range_low, interface.ip_range_high)],
116+ 'ip_range_high': [
117+ message % (interface.ip_range_low, interface.ip_range_high)],
118+ 'static_ip_range_low': [
119+ message % (
120+ interface.static_ip_range_low,
121+ interface.static_ip_range_high)],
122+ 'static_ip_range_high': [
123+ message % (
124+ interface.static_ip_range_low,
125+ interface.static_ip_range_high)],
126+ }
127+ self.assertEqual(errors, exception.message_dict)