launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/bug-1070765-hostname into lp:maas.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1070765-hostname/+merge/131539

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.
-- 
https://code.launchpad.net/~rvb/maas/bug-1070765-hostname/+merge/131539
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1070765-hostname into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-25 14:33:05 +0000
+++ src/maasserver/api.py	2012-10-26 08:11:31 +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-26 08:11:31 +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,14 @@
         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 hode if provided the hostname is
+        # IP-based 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-26 08:11:31 +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-25 14:33:05 +0000
+++ src/maasserver/tests/test_api.py	2012-10-26 08:11:31 +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-26 08:11:31 +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-26 08:11:31 +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-26 08:11:31 +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-26 08:11:31 +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))