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

Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: 2509
Merged at revision: 2508
Proposed branch: lp:~gmb/maas/fix-my-stupidity
Merge into: lp:maas/trunk
Diff against target: 175 lines (+77/-65)
2 files modified
src/maasserver/models/nodegroupinterface.py (+65/-59)
src/maasserver/models/tests/test_nodegroupinterface.py (+12/-6)
To merge this branch: bzr merge lp:~gmb/maas/fix-my-stupidity
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+225516@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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
Revision history for this message
Graham Binns (gmb) wrote :

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

lp:~gmb/maas/fix-my-stupidity updated
2509. By Graham Binns

Quick exit-early change for clean_ip_range(s|bounds).

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-07-03 09:42:15 +0000
3+++ src/maasserver/models/nodegroupinterface.py 2014-07-03 17:00:27 +0000
4@@ -225,70 +225,76 @@
5
6 def clean_ip_range_bounds(self):
7 """Ensure that the static and dynamic ranges have sane bounds."""
8- if (self.is_managed and (self.static_ip_range_low and
9+ if not (self.is_managed and (self.static_ip_range_low and
10 self.static_ip_range_high)):
11- errors = {}
12- ip_range_low = IPAddress(self.ip_range_low)
13- ip_range_high = IPAddress(self.ip_range_high)
14- static_ip_range_low = IPAddress(self.static_ip_range_low)
15- static_ip_range_high = IPAddress(self.static_ip_range_high)
16-
17- message_base = (
18- "Lower bound %s is higher than upper bound %s")
19- try:
20- IPRange(static_ip_range_low, static_ip_range_high)
21- except AddrFormatError:
22- message = (
23- message_base % (
24- self.static_ip_range_low, self.static_ip_range_high))
25- errors['static_ip_range_low'] = [message]
26- errors['static_ip_range_high'] = [message]
27- try:
28- IPRange(ip_range_low, ip_range_high)
29- except AddrFormatError:
30- message = (
31- message_base % (self.ip_range_low, self.ip_range_high))
32- errors['ip_range_low'] = [message]
33- errors['ip_range_high'] = [message]
34-
35- if errors:
36- raise ValidationError(errors)
37+ # Exit early with nothing to do.
38+ return
39+
40+ errors = {}
41+ ip_range_low = IPAddress(self.ip_range_low)
42+ ip_range_high = IPAddress(self.ip_range_high)
43+ static_ip_range_low = IPAddress(self.static_ip_range_low)
44+ static_ip_range_high = IPAddress(self.static_ip_range_high)
45+
46+ message_base = (
47+ "Lower bound %s is higher than upper bound %s")
48+ try:
49+ IPRange(static_ip_range_low, static_ip_range_high)
50+ except AddrFormatError:
51+ message = (
52+ message_base % (
53+ self.static_ip_range_low, self.static_ip_range_high))
54+ errors['static_ip_range_low'] = [message]
55+ errors['static_ip_range_high'] = [message]
56+ try:
57+ IPRange(ip_range_low, ip_range_high)
58+ except AddrFormatError:
59+ message = (
60+ message_base % (self.ip_range_low, self.ip_range_high))
61+ errors['ip_range_low'] = [message]
62+ errors['ip_range_high'] = [message]
63+
64+ if errors:
65+ raise ValidationError(errors)
66
67 def clean_ip_ranges(self):
68 """Ensure that the static and dynamic ranges don't overlap."""
69- if (self.is_managed and (self.static_ip_range_low and
70+ if not (self.is_managed and (self.static_ip_range_low and
71 self.static_ip_range_high)):
72- errors = {}
73- ip_range_low = IPAddress(self.ip_range_low)
74- ip_range_high = IPAddress(self.ip_range_high)
75- static_ip_range_low = IPAddress(self.static_ip_range_low)
76- static_ip_range_high = IPAddress(self.static_ip_range_high)
77-
78- static_range = IPRange(
79- static_ip_range_low, static_ip_range_high)
80- dynamic_range = IPRange(
81- ip_range_low, ip_range_high)
82-
83- # This is a bit unattractive, but we can't use IPSet for
84- # large networks - it's far too slow. What we actually care
85- # about is whether the lows and highs of the static range
86- # fall within the dynamic range and vice-versa, which
87- # IPRange gives us.
88- networks_overlap = (
89- static_ip_range_low in dynamic_range or
90- static_ip_range_high in dynamic_range or
91- ip_range_low in static_range or
92- ip_range_high in static_range
93- )
94- if networks_overlap:
95- message = "Static and dynamic IP ranges may not overlap."
96- errors = {
97- 'ip_range_low': [message],
98- 'ip_range_high': [message],
99- 'static_ip_range_low': [message],
100- 'static_ip_range_high': [message],
101- }
102- raise ValidationError(errors)
103+ # Nothing to do; bail out.
104+ return
105+
106+ errors = {}
107+ ip_range_low = IPAddress(self.ip_range_low)
108+ ip_range_high = IPAddress(self.ip_range_high)
109+ static_ip_range_low = IPAddress(self.static_ip_range_low)
110+ static_ip_range_high = IPAddress(self.static_ip_range_high)
111+
112+ static_range = IPRange(
113+ static_ip_range_low, static_ip_range_high)
114+ dynamic_range = IPRange(
115+ ip_range_low, ip_range_high)
116+
117+ # This is a bit unattractive, but we can't use IPSet for
118+ # large networks - it's far too slow. What we actually care
119+ # about is whether the lows and highs of the static range
120+ # fall within the dynamic range and vice-versa, which
121+ # IPRange gives us.
122+ networks_overlap = (
123+ static_ip_range_low in dynamic_range or
124+ static_ip_range_high in dynamic_range or
125+ ip_range_low in static_range or
126+ ip_range_high in static_range
127+ )
128+ if networks_overlap:
129+ message = "Static and dynamic IP ranges may not overlap."
130+ errors = {
131+ 'ip_range_low': [message],
132+ 'ip_range_high': [message],
133+ 'static_ip_range_low': [message],
134+ 'static_ip_range_high': [message],
135+ }
136+ raise ValidationError(errors)
137
138 def clean_fields(self, *args, **kwargs):
139 super(NodeGroupInterface, self).clean_fields(*args, **kwargs)
140
141=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
142--- src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-03 09:56:26 +0000
143+++ src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-03 17:00:27 +0000
144@@ -28,7 +28,10 @@
145 from maasserver.testing import reload_object
146 from maasserver.testing.factory import factory
147 from maasserver.testing.testcase import MAASServerTestCase
148-from netaddr import IPNetwork
149+from netaddr import (
150+ IPAddress,
151+ IPNetwork,
152+ )
153 from provisioningserver.utils.network import make_network
154
155
156@@ -237,11 +240,14 @@
157 def test_clean_ip_ranges_works_with_ipv6_ranges(self):
158 network = factory.get_random_ipv6_network()
159 interface = make_interface(network)
160- dynamic_low, dynamic_high = factory.make_ipv6_range(network)
161- interface.ip_range_low = unicode(dynamic_low)
162- interface.ip_range_high = unicode(dynamic_high)
163- interface.static_ip_range_low = unicode(dynamic_low + 1)
164- interface.static_ip_range_high = unicode(dynamic_high - 1)
165+ interface.ip_range_low = unicode(
166+ IPAddress(network.first))
167+ interface.ip_range_high = unicode(
168+ IPAddress(network.last))
169+ interface.static_ip_range_low = unicode(
170+ IPAddress(network.first + 1))
171+ interface.static_ip_range_high = unicode(
172+ IPAddress(network.last - 1))
173 exception = self.assertRaises(
174 ValidationError, interface.full_clean)
175 message = "Static and dynamic IP ranges may not overlap."