MAAS

Merge ~mpontillo/maas:disallow-slash-zero-subnets--bug-1672947 into maas:master

  1. Git
  2. lp:~mpontillo/maas
  3. disallow-slash-zero-subnets--bug-1672947
  4. Merge into master
Proposed by Mike Pontillo
Status: Merged
Approved by: Mike Pontillo
Approved revision: 8062000f79f5e9f6e338c9f62abd959dc55625c5
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:disallow-slash-zero-subnets--bug-1672947
Merge into: maas:master
Diff against target: 304 lines (+83/-27)
2 files modified
src/maasserver/forms/subnet.py (+15/-0)
src/maasserver/forms/tests/test_subnet.py (+68/-27)
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Approve
Review via email: mp+337027@code.launchpad.net

Commit message

LP: #1672947 - Disallow subnets with prefixlen=0

 * Add friendlier error message if an invalid CIDR is passed in.
 * Drive-by fix to use dict(form.errors), which provides nicer
   errors if tests fail.

To post a comment you must log in.
Revision history for this message
Andres Rodriguez (andreserl) wrote :

lgtm! i just have one question inline.

review: Approve
Revision history for this message
Mike Pontillo (mpontillo) wrote :

Thanks; see reply below.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/forms/subnet.py b/src/maasserver/forms/subnet.py
2index ce9e22e..74cd78f 100644
3--- a/src/maasserver/forms/subnet.py
4+++ b/src/maasserver/forms/subnet.py
5@@ -8,6 +8,7 @@ __all__ = [
6 ]
7
8 from django import forms
9+from django.core.exceptions import ValidationError
10 from maasserver.enum import RDNS_MODE_CHOICES
11 from maasserver.fields import IPListFormField
12 from maasserver.forms import MAASModelForm
13@@ -16,6 +17,10 @@ from maasserver.models.subnet import Subnet
14 from maasserver.models.vlan import VLAN
15 from maasserver.utils.forms import set_form_error
16 from maasserver.utils.orm import get_one
17+from netaddr import (
18+ AddrFormatError,
19+ IPNetwork,
20+)
21
22
23 class SubnetForm(MAASModelForm):
24@@ -86,6 +91,16 @@ class SubnetForm(MAASModelForm):
25 cleaned_data = self._clean_vlan(cleaned_data)
26 return cleaned_data
27
28+ def clean_cidr(self):
29+ data = self.cleaned_data['cidr']
30+ try:
31+ network = IPNetwork(data)
32+ if network.prefixlen == 0:
33+ raise ValidationError("Prefix length must be greater than 0.")
34+ except AddrFormatError:
35+ raise ValidationError("Required format: <network>/<prefixlen>.")
36+ return data
37+
38 def _clean_name(self, cleaned_data):
39 name = cleaned_data.get("name", None)
40 instance_name_and_cidr_match = (
41diff --git a/src/maasserver/forms/tests/test_subnet.py b/src/maasserver/forms/tests/test_subnet.py
42index 8c07c6a..16dcefc 100644
43--- a/src/maasserver/forms/tests/test_subnet.py
44+++ b/src/maasserver/forms/tests/test_subnet.py
45@@ -12,17 +12,20 @@ from maasserver.models.fabric import Fabric
46 from maasserver.testing.factory import factory
47 from maasserver.testing.testcase import MAASServerTestCase
48 from maasserver.utils.orm import reload_object
49-from testtools.matchers import MatchesStructure
50+from testtools.matchers import (
51+ Equals,
52+ MatchesStructure,
53+)
54
55
56 class TestSubnetForm(MAASServerTestCase):
57
58 def test__requires_cidr(self):
59 form = SubnetForm({})
60- self.assertFalse(form.is_valid(), form.errors)
61+ self.assertFalse(form.is_valid(), dict(form.errors))
62 self.assertEqual({
63 "cidr": ["This field is required."],
64- }, form.errors)
65+ }, dict(form.errors))
66
67 def test__rejects_provided_space_on_update(self):
68 space = factory.make_Space()
69@@ -30,24 +33,54 @@ class TestSubnetForm(MAASServerTestCase):
70 form = SubnetForm(instance=subnet, data={
71 "space": space.id
72 })
73- self.assertFalse(form.is_valid(), form.errors)
74+ self.assertFalse(form.is_valid(), dict(form.errors))
75 self.assertEqual({
76 "space": [
77 "Spaces may no longer be set on subnets. Set the space on the "
78 "underlying VLAN."
79- ]}, form.errors)
80+ ]}, dict(form.errors))
81
82 def test__rejects_space_on_create(self):
83 space = factory.make_Space()
84 form = SubnetForm({
85 "space": space.id, "cidr": factory._make_random_network()
86 })
87- self.assertFalse(form.is_valid(), form.errors)
88+ self.assertFalse(form.is_valid(), dict(form.errors))
89 self.assertEqual({
90 "space": [
91 "Spaces may no longer be set on subnets. Set the space on the "
92 "underlying VLAN."
93- ]}, form.errors)
94+ ]}, dict(form.errors))
95+
96+ def test__rejects_invalid_cidr(self):
97+ form = SubnetForm({
98+ 'cidr': 'ten dot zero dot zero dot zero slash zero'
99+ })
100+ self.assertFalse(form.is_valid(), dict(form.errors))
101+ self.assertEqual({
102+ "cidr": [
103+ "Required format: <network>/<prefixlen>."
104+ ]}, dict(form.errors))
105+
106+ def test__rejects_ipv4_cidr_with_zero_prefixlen(self):
107+ form = SubnetForm({
108+ 'cidr': '0.0.0.0/0'
109+ })
110+ self.assertFalse(form.is_valid(), dict(form.errors))
111+ self.assertEqual({
112+ "cidr": [
113+ "Prefix length must be greater than 0."
114+ ]}, dict(form.errors))
115+
116+ def test__rejects_ipv6_cidr_with_zero_prefixlen(self):
117+ form = SubnetForm({
118+ 'cidr': '::/0'
119+ })
120+ self.assertFalse(form.is_valid(), dict(form.errors))
121+ self.assertEqual({
122+ "cidr": [
123+ "Prefix length must be greater than 0."
124+ ]}, dict(form.errors))
125
126 def test__creates_subnet(self):
127 subnet_name = factory.make_name("subnet")
128@@ -69,7 +102,7 @@ class TestSubnetForm(MAASServerTestCase):
129 "gateway_ip": gateway_ip,
130 "dns_servers": ','.join(dns_servers),
131 })
132- self.assertTrue(form.is_valid(), form.errors)
133+ self.assertTrue(form.is_valid(), dict(form.errors))
134 subnet = form.save()
135 self.assertThat(
136 subnet, MatchesStructure.byEquality(
137@@ -77,6 +110,14 @@ class TestSubnetForm(MAASServerTestCase):
138 vlan=vlan, cidr=cidr, gateway_ip=gateway_ip,
139 dns_servers=dns_servers))
140
141+ def test__removes_host_bits_and_whitespace(self):
142+ form = SubnetForm({
143+ "cidr": ' 10.0.0.1/24 ',
144+ })
145+ self.assertTrue(form.is_valid(), dict(form.errors))
146+ subnet = form.save()
147+ self.assertThat(subnet.cidr, Equals('10.0.0.0/24'))
148+
149 def test__creates_subnet_name_equal_to_cidr(self):
150 vlan = factory.make_VLAN()
151 network = factory.make_ip4_or_6_network()
152@@ -85,7 +126,7 @@ class TestSubnetForm(MAASServerTestCase):
153 "vlan": vlan.id,
154 "cidr": cidr,
155 })
156- self.assertTrue(form.is_valid(), form.errors)
157+ self.assertTrue(form.is_valid(), dict(form.errors))
158 subnet = form.save()
159 self.assertThat(
160 subnet, MatchesStructure.byEquality(
161@@ -97,7 +138,7 @@ class TestSubnetForm(MAASServerTestCase):
162 form = SubnetForm({
163 "cidr": cidr,
164 })
165- self.assertTrue(form.is_valid(), form.errors)
166+ self.assertTrue(form.is_valid(), dict(form.errors))
167 subnet = form.save()
168 self.assertThat(
169 subnet, MatchesStructure.byEquality(
170@@ -113,7 +154,7 @@ class TestSubnetForm(MAASServerTestCase):
171 "fabric": fabric.id,
172 "vlan": None,
173 })
174- self.assertTrue(form.is_valid(), form.errors)
175+ self.assertTrue(form.is_valid(), dict(form.errors))
176 subnet = form.save()
177 self.assertThat(
178 subnet, MatchesStructure.byEquality(
179@@ -129,7 +170,7 @@ class TestSubnetForm(MAASServerTestCase):
180 "vid": vlan.vid,
181 "vlan": None,
182 })
183- self.assertTrue(form.is_valid(), form.errors)
184+ self.assertTrue(form.is_valid(), dict(form.errors))
185 subnet = form.save()
186 self.assertThat(
187 subnet, MatchesStructure.byEquality(
188@@ -146,7 +187,7 @@ class TestSubnetForm(MAASServerTestCase):
189 "vid": vlan.vid,
190 "vlan": None,
191 })
192- self.assertTrue(form.is_valid(), form.errors)
193+ self.assertTrue(form.is_valid(), dict(form.errors))
194 subnet = form.save()
195 self.assertThat(
196 subnet, MatchesStructure.byEquality(
197@@ -161,10 +202,10 @@ class TestSubnetForm(MAASServerTestCase):
198 "cidr": cidr,
199 "vid": vlan.vid,
200 })
201- self.assertFalse(form.is_valid(), form.errors)
202+ self.assertFalse(form.is_valid(), dict(form.errors))
203 self.assertEqual({
204 "vid": ["No VLAN with vid %s in default fabric." % vlan.vid]
205- }, form.errors)
206+ }, dict(form.errors))
207
208 def test__error_for_unknown_vid_in_fabric(self):
209 fabric = factory.make_Fabric()
210@@ -176,10 +217,10 @@ class TestSubnetForm(MAASServerTestCase):
211 "fabric": fabric.id,
212 "vid": vlan.vid,
213 })
214- self.assertFalse(form.is_valid(), form.errors)
215+ self.assertFalse(form.is_valid(), dict(form.errors))
216 self.assertEqual({
217 "vid": ["No VLAN with vid %s in fabric %s." % (vlan.vid, fabric)]
218- }, form.errors)
219+ }, dict(form.errors))
220
221 def test__error_for_vlan_not_in_fabric(self):
222 fabric = factory.make_Fabric()
223@@ -191,15 +232,15 @@ class TestSubnetForm(MAASServerTestCase):
224 "fabric": fabric.id,
225 "vlan": vlan.id,
226 })
227- self.assertFalse(form.is_valid(), form.errors)
228+ self.assertFalse(form.is_valid(), dict(form.errors))
229 self.assertEqual({
230 "vlan": ["VLAN %s is not in fabric %s." % (vlan, fabric)]
231- }, form.errors)
232+ }, dict(form.errors))
233
234 def test__doest_require_vlan_or_cidr_on_update(self):
235 subnet = factory.make_Subnet()
236 form = SubnetForm(instance=subnet, data={})
237- self.assertTrue(form.is_valid(), form.errors)
238+ self.assertTrue(form.is_valid(), dict(form.errors))
239
240 def test__updates_subnet(self):
241 new_name = factory.make_name("subnet")
242@@ -222,7 +263,7 @@ class TestSubnetForm(MAASServerTestCase):
243 "gateway_ip": new_gateway_ip,
244 "dns_servers": ','.join(new_dns_servers),
245 })
246- self.assertTrue(form.is_valid(), form.errors)
247+ self.assertTrue(form.is_valid(), dict(form.errors))
248 form.save()
249 subnet = reload_object(subnet)
250 self.assertThat(
251@@ -242,7 +283,7 @@ class TestSubnetForm(MAASServerTestCase):
252 "cidr": new_cidr,
253 "gateway_ip": new_gateway_ip,
254 })
255- self.assertTrue(form.is_valid(), form.errors)
256+ self.assertTrue(form.is_valid(), dict(form.errors))
257 form.save()
258 subnet = reload_object(subnet)
259 self.assertThat(
260@@ -257,7 +298,7 @@ class TestSubnetForm(MAASServerTestCase):
261 form = SubnetForm(instance=subnet, data={
262 "name": factory.make_name("subnet")
263 })
264- self.assertTrue(form.is_valid(), form.errors)
265+ self.assertTrue(form.is_valid(), dict(form.errors))
266 form.save()
267 subnet = reload_object(subnet)
268 self.assertEquals(dns_servers, subnet.dns_servers)
269@@ -268,7 +309,7 @@ class TestSubnetForm(MAASServerTestCase):
270 form = SubnetForm(instance=subnet, data={
271 "name": new_name,
272 })
273- self.assertTrue(form.is_valid(), form.errors)
274+ self.assertTrue(form.is_valid(), dict(form.errors))
275 form.save()
276 subnet = reload_object(subnet)
277 self.assertThat(
278@@ -282,7 +323,7 @@ class TestSubnetForm(MAASServerTestCase):
279 "gateway_ip": "",
280 "dns_servers": "",
281 })
282- self.assertTrue(form.is_valid(), form.errors)
283+ self.assertTrue(form.is_valid(), dict(form.errors))
284 form.save()
285 subnet = reload_object(subnet)
286 self.assertThat(
287@@ -296,7 +337,7 @@ class TestSubnetForm(MAASServerTestCase):
288 form = SubnetForm(instance=subnet, data={
289 "dns_servers": ','.join(dns_servers)
290 })
291- self.assertTrue(form.is_valid(), form.errors)
292+ self.assertTrue(form.is_valid(), dict(form.errors))
293 form.save()
294 subnet = reload_object(subnet)
295 self.assertEquals(dns_servers,
296@@ -309,7 +350,7 @@ class TestSubnetForm(MAASServerTestCase):
297 form = SubnetForm(instance=subnet, data={
298 "dns_servers": " ".join(dns_servers)
299 })
300- self.assertTrue(form.is_valid(), form.errors)
301+ self.assertTrue(form.is_valid(), dict(form.errors))
302 form.save()
303 subnet = reload_object(subnet)
304 self.assertEquals(dns_servers,

Subscribers

People subscribed via source and target branches