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

[Raphaël Badin]

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

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.

[Jeroen T. Vermeulen (jtv)]

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

[Raphaël Badin (rvb)]

> 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)]

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)]

> 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)]

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!

[Raphaël Badin (rvb)]

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

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-26 10:56:06 +0000
+++ src/maasserver/api.py	2012-10-29 09:54:36 +0000
@@ -165,6 +165,7 @@
 from maasserver.utils import (
     absolute_reverse,
     map_enum,
+    strip_domain,
     )
 from maasserver.utils.orm import get_one
 from piston.handler import (
@@ -1800,7 +1801,7 @@
         preseed_url = compose_preseed_url(node)
         # The node's hostname may include a domain, but we ignore that
         # and use the one from the nodegroup instead.
-        hostname = node.hostname.split('.', 1)[0]
+        hostname = strip_domain(node.hostname)
         domain = node.nodegroup.name
     else:
         try:
 
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-10-24 16:07:00 +0000
+++ src/maasserver/forms.py	2012-10-29 09:54:36 +0000
@@ -31,6 +31,7 @@
 
 import collections
 import json
+import re
 
 from django import forms
 from django.contrib import messages
@@ -80,6 +81,7 @@
 from maasserver.models.nodegroup import NODEGROUP_CLUSTER_NAME_TEMPLATE
 from maasserver.node_action import compile_node_actions
 from maasserver.power_parameters import POWER_TYPE_PARAMETERS
+from maasserver.utils import strip_domain
 from provisioningserver.enum import (
     POWER_TYPE,
     POWER_TYPE_CHOICES,
@@ -319,6 +321,9 @@
         node.nodegroup = form_value
 
 
+IP_BASED_HOSTNAME_REGEXP = re.compile('\d{1,3}-\d{1,3}-\d{1,3}-\d{1,3}')
+
+
 class WithMACAddressesMixin:
     """A form mixin which dynamically adds a MultipleMACAddressField to the
     list of fields.  This mixin also overrides the 'save' method to persist
@@ -374,7 +379,15 @@
         node.save()
         for mac in self.cleaned_data['mac_addresses']:
             node.add_mac_address(mac)
-        if self.cleaned_data['hostname'] == "":
+        hostname = self.cleaned_data['hostname']
+        stripped_hostname = strip_domain(hostname)
+        # Generate a hostname for this node if the provided hostname is
+        # IP-based (because this means that this name comes from a DNS
+        # reverse query to the MAAS DNS) or an empty string.
+        generate_hostname = (
+            hostname == "" or
+            IP_BASED_HOSTNAME_REGEXP.match(stripped_hostname) != None)
+        if generate_hostname:
             node.set_mac_based_hostname(self.cleaned_data['mac_addresses'][0])
         return node
 
 
=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-10-09 10:30:10 +0000
+++ src/maasserver/models/dhcplease.py	2012-10-29 09:54:36 +0000
@@ -25,11 +25,7 @@
 from maasserver import DefaultMeta
 from maasserver.fields import MACAddressField
 from maasserver.models.cleansave import CleanSave
-
-
-def strip_domain(hostname):
-    """Return `hostname` with the domain part removed."""
-    return hostname.split('.', 1)[0]
+from maasserver.utils import strip_domain
 
 
 class DHCPLeaseManager(Manager):
 
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-26 10:56:06 +0000
+++ src/maasserver/tests/test_api.py	2012-10-29 09:54:36 +0000
@@ -381,6 +381,25 @@
         [diane] = Node.objects.filter(hostname='diane')
         self.assertEqual(architecture, diane.architecture)
 
+    def test_POST_new_generates_hostname_if_ip_based_hostname(self):
+        hostname = '192-168-5-19.domain'
+        response = self.client.post(
+            self.get_uri('nodes/'),
+            {
+                'op': 'new',
+                'hostname': hostname,
+                'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+                'after_commissioning_action':
+                    NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
+                'mac_addresses': [factory.getRandomMACAddress()],
+            })
+        parsed_result = json.loads(response.content)
+
+        self.assertEqual(httplib.OK, response.status_code)
+        system_id = parsed_result.get('system_id')
+        node = Node.objects.get(system_id=system_id)
+        self.assertNotEqual(hostname, node.hostname)
+
     def test_POST_new_creates_node_with_power_parameters(self):
         # We're setting power parameters so we disable start_commissioning to
         # prevent anything from attempting to issue power instructions.
 
=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py	2012-10-09 10:30:10 +0000
+++ src/maasserver/tests/test_dhcplease.py	2012-10-29 09:54:36 +0000
@@ -14,7 +14,6 @@
 
 from maasserver import dns
 from maasserver.models import DHCPLease
-from maasserver.models.dhcplease import strip_domain
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.utils import ignore_unused
@@ -47,20 +46,6 @@
         self.assertEqual(mac, lease.mac)
 
 
-class TestUtitilies(TestCase):
-
-    def test_strip_domain(self):
-        input_and_results = [
-            ('name.domain',  'name'),
-            ('name', 'name'),
-            ('name.domain.what', 'name'),
-            ('name..domain', 'name'),
-            ]
-        inputs = [input for input, _ in input_and_results]
-        results = [result for _, result in input_and_results]
-        self.assertEqual(results, map(strip_domain, inputs))
-
-
 class TestDHCPLeaseManager(TestCase):
     """Tests for :class:`DHCPLeaseManager`."""
 
 
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-10-07 11:00:05 +0000
+++ src/maasserver/tests/test_forms.py	2012-10-29 09:54:36 +0000
@@ -111,14 +111,17 @@
         return query_dict
 
     def make_params(self, mac_addresses=None, architecture=None,
-                    nodegroup=None):
+                    hostname=None, nodegroup=None):
         if mac_addresses is None:
             mac_addresses = [factory.getRandomMACAddress()]
         if architecture is None:
             architecture = factory.getRandomEnum(ARCHITECTURE)
+        if hostname is None:
+            hostname = factory.make_name('hostname')
         params = {
             'mac_addresses': mac_addresses,
             'architecture': architecture,
+            'hostname': hostname,
         }
         if nodegroup is not None:
             params['nodegroup'] = nodegroup
@@ -217,6 +220,18 @@
         form.save()
         self.assertEqual(original_nodegroup, reload_object(node).nodegroup)
 
+    def test_form_without_hostname_generates_hostname(self):
+        form = NodeWithMACAddressesForm(self.make_params(hostname=''))
+        node = form.save()
+        self.assertTrue(len(node.hostname) > 0)
+
+    def test_form_with_ip_based_hostname_generates_hostname(self):
+        ip_based_hostname = '192-168-12-10.domain'
+        form = NodeWithMACAddressesForm(
+            self.make_params(hostname=ip_based_hostname))
+        node = form.save()
+        self.assertNotEqual(ip_based_hostname, node.hostname)
+
 
 class TestOptionForm(ConfigForm):
     field1 = forms.CharField(label="Field 1", max_length=10)
 
=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2012-08-24 10:28:29 +0000
+++ src/maasserver/utils/__init__.py	2012-10-29 09:54:36 +0000
@@ -15,6 +15,7 @@
     'get_db_state',
     'ignore_unused',
     'map_enum',
+    'strip_domain',
     ]
 
 from urllib import urlencode
@@ -82,3 +83,8 @@
     if query is not None:
         url += '?%s' % urlencode(query, doseq=True)
     return url
+
+
+def strip_domain(hostname):
+    """Return `hostname` with the domain part removed."""
+    return hostname.split('.', 1)[0]
 
=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2012-06-26 16:36:10 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-10-29 09:54:36 +0000
@@ -23,6 +23,7 @@
     absolute_reverse,
     get_db_state,
     map_enum,
+    strip_domain,
     )
 from maastesting.testcase import TestCase
 
@@ -104,3 +105,17 @@
             NODE_STATUS_CHOICES, but_not=[status])
         node.status = another_status
         self.assertEqual(status, get_db_state(node, 'status'))
+
+
+class TestStripDomain(TestCase):
+
+    def test_strip_domain(self):
+        input_and_results = [
+            ('name.domain',  'name'),
+            ('name', 'name'),
+            ('name.domain.what', 'name'),
+            ('name..domain', 'name'),
+            ]
+        inputs = [input for input, _ in input_and_results]
+        results = [result for _, result in input_and_results]
+        self.assertEqual(results, map(strip_domain, inputs))