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

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
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
=== 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 @@
127 else:127 else:
128 return None128 return None
129129
130 @property
131 def is_managed(self):
132 """Return true if this interface is managed by MAAS."""
133 return self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
134
130 def display_management(self):135 def display_management(self):
131 """Return management status text as displayed to the user."""136 """Return management status text as displayed to the user."""
132 return NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[self.management]137 return NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[self.management]
@@ -152,7 +157,7 @@
152157
153 def clean_network_not_too_big(self):158 def clean_network_not_too_big(self):
154 # If management is not 'UNMANAGED', refuse huge networks.159 # If management is not 'UNMANAGED', refuse huge networks.
155 if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:160 if self.is_managed:
156 network = self.network161 network = self.network
157 if network is not None:162 if network is not None:
158 if network.prefixlen < MINIMUM_NETMASK_BITS:163 if network.prefixlen < MINIMUM_NETMASK_BITS:
@@ -165,7 +170,7 @@
165 def clean_network_config_if_managed(self):170 def clean_network_config_if_managed(self):
166 # If management is not 'UNMANAGED', all the network information171 # If management is not 'UNMANAGED', all the network information
167 # should be provided.172 # should be provided.
168 if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:173 if self.is_managed:
169 mandatory_fields = [174 mandatory_fields = [
170 'interface',175 'interface',
171 'subnet_mask',176 'subnet_mask',
@@ -218,18 +223,49 @@
218 # form; validation breaks if we pass an IPAddress.223 # form; validation breaks if we pass an IPAddress.
219 self.broadcast_ip = unicode(network.broadcast)224 self.broadcast_ip = unicode(network.broadcast)
220225
226 def clean_ip_range_bounds(self):
227 """Ensure that the static and dynamic ranges have sane bounds."""
228 if (self.is_managed and (self.static_ip_range_low and
229 self.static_ip_range_high)):
230 errors = {}
231 ip_range_low = IPAddress(self.ip_range_low)
232 ip_range_high = IPAddress(self.ip_range_high)
233 static_ip_range_low = IPAddress(self.static_ip_range_low)
234 static_ip_range_high = IPAddress(self.static_ip_range_high)
235
236 message_base = (
237 "Lower bound %s is higher than upper bound %s")
238 try:
239 IPRange(static_ip_range_low, static_ip_range_high)
240 except AddrFormatError:
241 message = (
242 message_base % (
243 self.static_ip_range_low, self.static_ip_range_high))
244 errors['static_ip_range_low'] = [message]
245 errors['static_ip_range_high'] = [message]
246 try:
247 IPRange(ip_range_low, ip_range_high)
248 except AddrFormatError:
249 message = (
250 message_base % (self.ip_range_low, self.ip_range_high))
251 errors['ip_range_low'] = [message]
252 errors['ip_range_high'] = [message]
253
254 if errors:
255 raise ValidationError(errors)
256
221 def clean_ip_ranges(self):257 def clean_ip_ranges(self):
222 """Ensure that the static and dynamic ranges don't overlap."""258 """Ensure that the static and dynamic ranges don't overlap."""
223 if (self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED and259 if (self.is_managed and (self.static_ip_range_low and
224 (self.static_ip_range_low and self.static_ip_range_high)):260 self.static_ip_range_high)):
261 errors = {}
225 ip_range_low = IPAddress(self.ip_range_low)262 ip_range_low = IPAddress(self.ip_range_low)
226 ip_range_high = IPAddress(self.ip_range_high)263 ip_range_high = IPAddress(self.ip_range_high)
227 static_ip_range_low = IPAddress(self.static_ip_range_low)264 static_ip_range_low = IPAddress(self.static_ip_range_low)
228 static_ip_range_high = IPAddress(self.static_ip_range_high)265 static_ip_range_high = IPAddress(self.static_ip_range_high)
229266
230 static_range = IPRange(267 static_range = IPRange(
231 static_ip_range_low,268 static_ip_range_low, static_ip_range_high)
232 static_ip_range_high)
233 dynamic_range = IPRange(269 dynamic_range = IPRange(
234 ip_range_low, ip_range_high)270 ip_range_low, ip_range_high)
235271
@@ -260,4 +296,5 @@
260 self.clean_network_not_too_big()296 self.clean_network_not_too_big()
261 self.clean_ips_in_network()297 self.clean_ips_in_network()
262 self.clean_network_config_if_managed()298 self.clean_network_config_if_managed()
299 self.clean_ip_range_bounds()
263 self.clean_ip_ranges()300 self.clean_ip_ranges()
264301
=== 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 @@
273 'static_ip_range_high': [message],273 'static_ip_range_high': [message],
274 }274 }
275 self.assertEqual(errors, exception.message_dict)275 self.assertEqual(errors, exception.message_dict)
276
277 def test_clean_ip_range_bounds_checks_for_reversed_range_bounds(self):
278 network = IPNetwork("10.1.0.0/16")
279 interface = make_interface(network)
280 interface.ip_range_low = '10.1.0.2'
281 interface.ip_range_high = '10.1.0.1'
282 interface.static_ip_range_low = '10.1.0.10'
283 interface.static_ip_range_high = '10.1.0.9'
284 exception = self.assertRaises(
285 ValidationError, interface.full_clean)
286 message = "Lower bound %s is higher than upper bound %s"
287 errors = {
288 'ip_range_low': [
289 message % (interface.ip_range_low, interface.ip_range_high)],
290 'ip_range_high': [
291 message % (interface.ip_range_low, interface.ip_range_high)],
292 'static_ip_range_low': [
293 message % (
294 interface.static_ip_range_low,
295 interface.static_ip_range_high)],
296 'static_ip_range_high': [
297 message % (
298 interface.static_ip_range_low,
299 interface.static_ip_range_high)],
300 }
301 self.assertEqual(errors, exception.message_dict)