Merge lp:~rvb/maas/bug-1070765-hostname into lp:maas/trunk

Proposed by Raphaël Badin on 2012-10-26
Status: Merged
Approved by: Raphaël Badin on 2012-10-30
Approved revision: 1301
Merged at revision: 1316
Proposed branch: lp:~rvb/maas/bug-1070765-hostname
Merge into: lp:maas/trunk
Diff against target: 240 lines (+73/-23)
8 files modified
src/maasserver/api.py (+2/-1)
src/maasserver/forms.py (+14/-1)
src/maasserver/models/dhcplease.py (+1/-5)
src/maasserver/tests/test_api.py (+19/-0)
src/maasserver/tests/test_dhcplease.py (+0/-15)
src/maasserver/tests/test_forms.py (+16/-1)
src/maasserver/utils/__init__.py (+6/-0)
src/maasserver/utils/tests/test_utils.py (+15/-0)
To merge this branch: bzr merge lp:~rvb/maas/bug-1070765-hostname
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) 2012-10-26 Approve on 2012-10-26
Review via email: mp+131539@code.launchpad.net

Commit message

Change the Node creation form to ignore the provided hostname if it looks like an ip-based hostname.

Description of the change

Change the Node creation form to ignore the provided hostname if it looks like an ip-based hostname.

I know that IP_BASED_HOSTNAME_REGEXP matches things like 999.999.999.999 (non-valid ip adresses) but I don't think it's a problem really.

Drive-by fix: move strip_domain in the utils package.

To post a comment you must log in.
lp:~rvb/maas/bug-1070765-hostname updated on 2012-10-26
1299. By Raphaël Badin on 2012-10-26

Fix typo.

Jeroen T. Vermeulen (jtv) wrote :

Typo in forms.py: “hode.” Also, same line (line 58 of the diff):

+ # Generate a hostname for this hode if provided the hostname is

I think right after “hode” it should say “if the provided hostname,” not “if provided the hostname.”

lp:~rvb/maas/bug-1070765-hostname updated on 2012-10-26
1300. By Raphaël Badin on 2012-10-26

Fix typo.

Raphaël Badin (rvb) wrote :

> Typo in forms.py: “hode.” Also, same line (line 58 of the diff):
>
> + # Generate a hostname for this hode if provided the hostname is
>
> I think right after “hode” it should say “if the provided hostname,” not “if
> provided the hostname.”

Thanks for spotting this, typos fixed.

Jeroen T. Vermeulen (jtv) wrote :

One question: should the hostname regex perhaps contain the final dot that separates the hostname proper from the domain name? (If so, of course it needs to be escaped or it'll match any character).

Jeroen

review: Approve
Raphaël Badin (rvb) wrote :

> One question: should the hostname regex perhaps contain the final dot that
> separates the hostname proper from the domain name? (If so, of course it
> needs to be escaped or it'll match any character).

I don't think so, we want to store only the hostname in that field (in the long run, but we have to cope with the fact that existing installations will have a fqdn in there).

Julian Edwards (julian-edwards) wrote :

58 + # Generate a hostname for this node if the provided hostname is
59 + # IP-based or an empty string.

You need to say *why* you are doing this, or it will look rather confusing when you come back to look in a year!

lp:~rvb/maas/bug-1070765-hostname updated on 2012-10-29
1301. By Raphaël Badin on 2012-10-29

Improve comment.

Raphaël Badin (rvb) wrote :

> You need to say *why* you are doing this, or it will look rather confusing
> when you come back to look in a year!

Right, done.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api.py'
2--- src/maasserver/api.py 2012-10-26 10:56:06 +0000
3+++ src/maasserver/api.py 2012-10-29 09:54:36 +0000
4@@ -165,6 +165,7 @@
5 from maasserver.utils import (
6 absolute_reverse,
7 map_enum,
8+ strip_domain,
9 )
10 from maasserver.utils.orm import get_one
11 from piston.handler import (
12@@ -1800,7 +1801,7 @@
13 preseed_url = compose_preseed_url(node)
14 # The node's hostname may include a domain, but we ignore that
15 # and use the one from the nodegroup instead.
16- hostname = node.hostname.split('.', 1)[0]
17+ hostname = strip_domain(node.hostname)
18 domain = node.nodegroup.name
19 else:
20 try:
21
22=== modified file 'src/maasserver/forms.py'
23--- src/maasserver/forms.py 2012-10-24 16:07:00 +0000
24+++ src/maasserver/forms.py 2012-10-29 09:54:36 +0000
25@@ -31,6 +31,7 @@
26
27 import collections
28 import json
29+import re
30
31 from django import forms
32 from django.contrib import messages
33@@ -80,6 +81,7 @@
34 from maasserver.models.nodegroup import NODEGROUP_CLUSTER_NAME_TEMPLATE
35 from maasserver.node_action import compile_node_actions
36 from maasserver.power_parameters import POWER_TYPE_PARAMETERS
37+from maasserver.utils import strip_domain
38 from provisioningserver.enum import (
39 POWER_TYPE,
40 POWER_TYPE_CHOICES,
41@@ -319,6 +321,9 @@
42 node.nodegroup = form_value
43
44
45+IP_BASED_HOSTNAME_REGEXP = re.compile('\d{1,3}-\d{1,3}-\d{1,3}-\d{1,3}')
46+
47+
48 class WithMACAddressesMixin:
49 """A form mixin which dynamically adds a MultipleMACAddressField to the
50 list of fields. This mixin also overrides the 'save' method to persist
51@@ -374,7 +379,15 @@
52 node.save()
53 for mac in self.cleaned_data['mac_addresses']:
54 node.add_mac_address(mac)
55- if self.cleaned_data['hostname'] == "":
56+ hostname = self.cleaned_data['hostname']
57+ stripped_hostname = strip_domain(hostname)
58+ # Generate a hostname for this node if the provided hostname is
59+ # IP-based (because this means that this name comes from a DNS
60+ # reverse query to the MAAS DNS) or an empty string.
61+ generate_hostname = (
62+ hostname == "" or
63+ IP_BASED_HOSTNAME_REGEXP.match(stripped_hostname) != None)
64+ if generate_hostname:
65 node.set_mac_based_hostname(self.cleaned_data['mac_addresses'][0])
66 return node
67
68
69=== modified file 'src/maasserver/models/dhcplease.py'
70--- src/maasserver/models/dhcplease.py 2012-10-09 10:30:10 +0000
71+++ src/maasserver/models/dhcplease.py 2012-10-29 09:54:36 +0000
72@@ -25,11 +25,7 @@
73 from maasserver import DefaultMeta
74 from maasserver.fields import MACAddressField
75 from maasserver.models.cleansave import CleanSave
76-
77-
78-def strip_domain(hostname):
79- """Return `hostname` with the domain part removed."""
80- return hostname.split('.', 1)[0]
81+from maasserver.utils import strip_domain
82
83
84 class DHCPLeaseManager(Manager):
85
86=== modified file 'src/maasserver/tests/test_api.py'
87--- src/maasserver/tests/test_api.py 2012-10-26 10:56:06 +0000
88+++ src/maasserver/tests/test_api.py 2012-10-29 09:54:36 +0000
89@@ -381,6 +381,25 @@
90 [diane] = Node.objects.filter(hostname='diane')
91 self.assertEqual(architecture, diane.architecture)
92
93+ def test_POST_new_generates_hostname_if_ip_based_hostname(self):
94+ hostname = '192-168-5-19.domain'
95+ response = self.client.post(
96+ self.get_uri('nodes/'),
97+ {
98+ 'op': 'new',
99+ 'hostname': hostname,
100+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
101+ 'after_commissioning_action':
102+ NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
103+ 'mac_addresses': [factory.getRandomMACAddress()],
104+ })
105+ parsed_result = json.loads(response.content)
106+
107+ self.assertEqual(httplib.OK, response.status_code)
108+ system_id = parsed_result.get('system_id')
109+ node = Node.objects.get(system_id=system_id)
110+ self.assertNotEqual(hostname, node.hostname)
111+
112 def test_POST_new_creates_node_with_power_parameters(self):
113 # We're setting power parameters so we disable start_commissioning to
114 # prevent anything from attempting to issue power instructions.
115
116=== modified file 'src/maasserver/tests/test_dhcplease.py'
117--- src/maasserver/tests/test_dhcplease.py 2012-10-09 10:30:10 +0000
118+++ src/maasserver/tests/test_dhcplease.py 2012-10-29 09:54:36 +0000
119@@ -14,7 +14,6 @@
120
121 from maasserver import dns
122 from maasserver.models import DHCPLease
123-from maasserver.models.dhcplease import strip_domain
124 from maasserver.testing.factory import factory
125 from maasserver.testing.testcase import TestCase
126 from maasserver.utils import ignore_unused
127@@ -47,20 +46,6 @@
128 self.assertEqual(mac, lease.mac)
129
130
131-class TestUtitilies(TestCase):
132-
133- def test_strip_domain(self):
134- input_and_results = [
135- ('name.domain', 'name'),
136- ('name', 'name'),
137- ('name.domain.what', 'name'),
138- ('name..domain', 'name'),
139- ]
140- inputs = [input for input, _ in input_and_results]
141- results = [result for _, result in input_and_results]
142- self.assertEqual(results, map(strip_domain, inputs))
143-
144-
145 class TestDHCPLeaseManager(TestCase):
146 """Tests for :class:`DHCPLeaseManager`."""
147
148
149=== modified file 'src/maasserver/tests/test_forms.py'
150--- src/maasserver/tests/test_forms.py 2012-10-07 11:00:05 +0000
151+++ src/maasserver/tests/test_forms.py 2012-10-29 09:54:36 +0000
152@@ -111,14 +111,17 @@
153 return query_dict
154
155 def make_params(self, mac_addresses=None, architecture=None,
156- nodegroup=None):
157+ hostname=None, nodegroup=None):
158 if mac_addresses is None:
159 mac_addresses = [factory.getRandomMACAddress()]
160 if architecture is None:
161 architecture = factory.getRandomEnum(ARCHITECTURE)
162+ if hostname is None:
163+ hostname = factory.make_name('hostname')
164 params = {
165 'mac_addresses': mac_addresses,
166 'architecture': architecture,
167+ 'hostname': hostname,
168 }
169 if nodegroup is not None:
170 params['nodegroup'] = nodegroup
171@@ -217,6 +220,18 @@
172 form.save()
173 self.assertEqual(original_nodegroup, reload_object(node).nodegroup)
174
175+ def test_form_without_hostname_generates_hostname(self):
176+ form = NodeWithMACAddressesForm(self.make_params(hostname=''))
177+ node = form.save()
178+ self.assertTrue(len(node.hostname) > 0)
179+
180+ def test_form_with_ip_based_hostname_generates_hostname(self):
181+ ip_based_hostname = '192-168-12-10.domain'
182+ form = NodeWithMACAddressesForm(
183+ self.make_params(hostname=ip_based_hostname))
184+ node = form.save()
185+ self.assertNotEqual(ip_based_hostname, node.hostname)
186+
187
188 class TestOptionForm(ConfigForm):
189 field1 = forms.CharField(label="Field 1", max_length=10)
190
191=== modified file 'src/maasserver/utils/__init__.py'
192--- src/maasserver/utils/__init__.py 2012-08-24 10:28:29 +0000
193+++ src/maasserver/utils/__init__.py 2012-10-29 09:54:36 +0000
194@@ -15,6 +15,7 @@
195 'get_db_state',
196 'ignore_unused',
197 'map_enum',
198+ 'strip_domain',
199 ]
200
201 from urllib import urlencode
202@@ -82,3 +83,8 @@
203 if query is not None:
204 url += '?%s' % urlencode(query, doseq=True)
205 return url
206+
207+
208+def strip_domain(hostname):
209+ """Return `hostname` with the domain part removed."""
210+ return hostname.split('.', 1)[0]
211
212=== modified file 'src/maasserver/utils/tests/test_utils.py'
213--- src/maasserver/utils/tests/test_utils.py 2012-06-26 16:36:10 +0000
214+++ src/maasserver/utils/tests/test_utils.py 2012-10-29 09:54:36 +0000
215@@ -23,6 +23,7 @@
216 absolute_reverse,
217 get_db_state,
218 map_enum,
219+ strip_domain,
220 )
221 from maastesting.testcase import TestCase
222
223@@ -104,3 +105,17 @@
224 NODE_STATUS_CHOICES, but_not=[status])
225 node.status = another_status
226 self.assertEqual(status, get_db_state(node, 'status'))
227+
228+
229+class TestStripDomain(TestCase):
230+
231+ def test_strip_domain(self):
232+ input_and_results = [
233+ ('name.domain', 'name'),
234+ ('name', 'name'),
235+ ('name.domain.what', 'name'),
236+ ('name..domain', 'name'),
237+ ]
238+ inputs = [input for input, _ in input_and_results]
239+ results = [result for _, result in input_and_results]
240+ self.assertEqual(results, map(strip_domain, inputs))