Merge lp:~rvb/maas/1081701-c-bis into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix the value of 'server_host' in the preseed.

This fixes a bug in the preseed generation. I just spotted that the preseed was still using "server_host = get_maas_facing_server_host()" to populate the 'server_host' field instead of "server_host = get_maas_facing_server_host(nodegroup)".

This was not caught by the tests in place for a stupid reason: "self.assertThat(preseed, MatchesAll(*[Contains(ng_url), Not(Contains(maas_url))]))" did not caught the problem because the value of 'server_host' is the hostname part of 'maas_url' but since it went trough urlparse.urlparse, it was lowercased. To avoid that kind of problem in the future, I created a utility method to create hostname (factory.make_hostname) which will return lowercase hostnames.

To fix that I had to refactor the preseed code a bit: instead of passing the url around, we pass the nodegroup. I think it's all for the best: extracting maas_url from nodegroup is done only in the preseed code.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-26 16:50:14 +0000
+++ src/maasserver/api.py	2012-11-27 16:03:23 +0000
@@ -1630,7 +1630,8 @@
         # The node's hostname may include a domain, but we ignore that
         # and use the one from the nodegroup instead.
         hostname = strip_domain(node.hostname)
-        domain = node.nodegroup.name
+        nodegroup = node.nodegroup
+        domain = nodegroup.name
     else:
         try:
             pxelinux_arch = request.GET['arch']
@@ -1665,8 +1666,7 @@
             subarch = pxelinux_subarch
 
         nodegroup = find_nodegroup(request)
-        base_url = nodegroup.maas_url if nodegroup is not None else None
-        preseed_url = compose_enlistment_preseed_url(base_url=base_url)
+        preseed_url = compose_enlistment_preseed_url(nodegroup=nodegroup)
         hostname = 'maas-enlist'
         domain = Config.objects.get_config('enlistment_domain')
 
@@ -1683,7 +1683,7 @@
         extra_kernel_opts = None
 
     purpose = get_boot_purpose(node)
-    server_address = get_maas_facing_server_address()
+    server_address = get_maas_facing_server_address(nodegroup=nodegroup)
     cluster_address = get_mandatory_param(request.GET, "local")
 
     params = KernelParameters(
 
=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-11-23 14:35:53 +0000
+++ src/maasserver/preseed.py	2012-11-27 16:03:23 +0000
@@ -38,24 +38,26 @@
 GENERIC_FILENAME = 'generic'
 
 
-def get_enlist_preseed(base_url=''):
+def get_enlist_preseed(nodegroup=None):
     """Return the enlistment preseed.
 
+    :param nodegroup: The nodegroup used to generate the preseed.
     :return: The rendered preseed string.
     :rtype: basestring.
     """
     return render_enlistment_preseed(
-        PRESEED_TYPE.ENLIST, base_url=base_url)
-
-
-def get_enlist_userdata(base_url=''):
+        PRESEED_TYPE.ENLIST, nodegroup=nodegroup)
+
+
+def get_enlist_userdata(nodegroup=None):
     """Return the enlistment preseed.
 
+    :param nodegroup: The nodegroup used to generate the preseed.
     :return: The rendered enlistment user-data string.
     :rtype: basestring.
     """
     return render_enlistment_preseed(
-        PRESEED_TYPE.ENLIST_USERDATA, base_url=base_url)
+        PRESEED_TYPE.ENLIST_USERDATA, nodegroup=nodegroup)
 
 
 def get_preseed(node):
@@ -209,21 +211,21 @@
     return parsed_url.hostname, parsed_url.path
 
 
-def get_preseed_context(release='', base_url=''):
+def get_preseed_context(release='', nodegroup=None):
     """Return the node-independent context dictionary to be used to render
     preseed templates.
 
-    :param node: See `get_preseed_filenames`.
-    :param prefix: See `get_preseed_filenames`.
     :param release: See `get_preseed_filenames`.
+    :param nodegroup: The nodegroup used to generate the preseed.
     :return: The context dictionary.
     :rtype: dict.
     """
-    server_host = get_maas_facing_server_host()
+    server_host = get_maas_facing_server_host(nodegroup=nodegroup)
     main_archive_hostname, main_archive_directory = get_hostname_and_path(
         Config.objects.get_config('main_archive'))
     ports_archive_hostname, ports_archive_directory = get_hostname_and_path(
         Config.objects.get_config('ports_archive'))
+    base_url = nodegroup.maas_url if nodegroup is not None else None
     return {
         'main_archive_hostname': main_archive_hostname,
         'main_archive_directory': main_archive_directory,
@@ -242,7 +244,6 @@
     preseed templates.
 
     :param node: See `get_preseed_filenames`.
-    :param prefix: See `get_preseed_filenames`.
     :param release: See `get_preseed_filenames`.
     :return: The context dictionary.
     :rtype: dict.
@@ -261,16 +262,17 @@
     }
 
 
-def render_enlistment_preseed(prefix, release='', base_url=''):
+def render_enlistment_preseed(prefix, release='', nodegroup=None):
     """Return the enlistment preseed.
 
     :param prefix: See `get_preseed_filenames`.
     :param release: See `get_preseed_filenames`.
+    :param nodegroup: The nodegroup used to generate the preseed.
     :return: The rendered preseed string.
     :rtype: basestring.
     """
     template = load_preseed_template(None, prefix, release)
-    context = get_preseed_context(release, base_url=base_url)
+    context = get_preseed_context(release, nodegroup=nodegroup)
     return template.substitute(**context)
 
 
@@ -284,15 +286,19 @@
     :rtype: basestring.
     """
     template = load_preseed_template(node, prefix, release)
-    base_url = node.nodegroup.maas_url
-    context = get_preseed_context(release, base_url=base_url)
+    nodegroup = node.nodegroup
+    context = get_preseed_context(release, nodegroup=nodegroup)
     context.update(get_node_preseed_context(node, release))
     return template.substitute(**context)
 
 
-def compose_enlistment_preseed_url(base_url=''):
-    """Compose enlistment preseed URL."""
+def compose_enlistment_preseed_url(nodegroup=None):
+    """Compose enlistment preseed URL.
+
+    :param nodegroup: The nodegroup used to generate the preseed.
+    """
     # Always uses the latest version of the metadata API.
+    base_url = nodegroup.maas_url if nodegroup is not None else None
     version = 'latest'
     return absolute_reverse(
         'metadata-enlist-preseed', args=[version],
 
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-27 09:51:35 +0000
+++ src/maasserver/tests/test_api.py	2012-11-27 16:03:23 +0000
@@ -48,7 +48,10 @@
     EnvironmentVariableFixture,
     Fixture,
     )
-from maasserver import api
+from maasserver import (
+    api,
+    server_address,
+    )
 from maasserver.api import (
     describe,
     DISPLAYED_NODEGROUP_FIELDS,
@@ -3438,9 +3441,11 @@
 
     def test_pxeconfig_enlistment_preseed_url_detects_request_origin(self):
         self.silence_get_ephemeral_name()
-        ng_url = 'http://%s' % factory.make_name('host')
+        hostname = factory.make_hostname()
+        ng_url = 'http://%s' % hostname
         network = IPNetwork("10.1.1/24")
         ip = factory.getRandomIPInNetwork(network)
+        self.patch(server_address, 'gethostbyname', Mock(return_value=ip))
         factory.make_node_group(maas_url=ng_url, network=network)
         params = self.get_default_params()
 
@@ -3451,6 +3456,24 @@
             json.loads(response.content)["preseed_url"],
             StartsWith(ng_url))
 
+    def test_pxeconfig_enlistment_log_host_url_detects_request_origin(self):
+        self.silence_get_ephemeral_name()
+        hostname = factory.make_hostname()
+        ng_url = 'http://%s' % hostname
+        network = IPNetwork("10.1.1/24")
+        ip = factory.getRandomIPInNetwork(network)
+        mock = self.patch(
+            server_address, 'gethostbyname', Mock(return_value=ip))
+        factory.make_node_group(maas_url=ng_url, network=network)
+        params = self.get_default_params()
+
+        # Simulate that the request targets ip by setting 'SERVER_NAME'.
+        response = self.client.get(
+            reverse('pxeconfig'), params, SERVER_NAME=ip)
+        self.assertEqual(
+            (ip, hostname),
+            (json.loads(response.content)["log_host"], mock.call_args[0][0]))
+
     def test_pxeconfig_has_preseed_url_for_known_node(self):
         params = self.get_mac_params()
         node = MACAddress.objects.get(mac_address=params['mac']).node
@@ -3463,6 +3486,7 @@
         ng_url = 'http://%s' % factory.make_name('host')
         network = IPNetwork("10.1.1/24")
         ip = factory.getRandomIPInNetwork(network)
+        self.patch(server_address, 'gethostbyname', Mock(return_value=ip))
         nodegroup = factory.make_node_group(maas_url=ng_url, network=network)
         params = self.get_mac_params()
         node = MACAddress.objects.get(mac_address=params['mac']).node
 
=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2012-11-26 14:41:53 +0000
+++ src/maasserver/tests/test_dns.py	2012-11-27 16:03:23 +0000
@@ -91,7 +91,7 @@
         ip = factory.getRandomIPAddress()
         resolver = FakeMethod(result=ip)
         self.patch(server_address, 'gethostbyname', resolver)
-        hostname = factory.getRandomString().lower()
+        hostname = factory.make_hostname()
         self.patch_DEFAULT_MAAS_URL_with_random_values(hostname=hostname)
         self.assertEqual(
             (ip, [(hostname, )]),
@@ -118,7 +118,7 @@
         ip = factory.getRandomIPAddress()
         resolver = FakeMethod(result=ip)
         self.patch(server_address, 'gethostbyname', resolver)
-        hostname = factory.getRandomString().lower()
+        hostname = factory.make_hostname()
         maas_url = 'http://%s' % hostname
         nodegroup = factory.make_node_group(maas_url=maas_url)
         self.assertEqual(
 
=== modified file 'src/maasserver/tests/test_preseed.py'
--- src/maasserver/tests/test_preseed.py	2012-11-23 14:35:53 +0000
+++ src/maasserver/tests/test_preseed.py	2012-11-27 16:03:23 +0000
@@ -323,7 +323,8 @@
 
     def test_get_preseed_context_contains_keys(self):
         release = factory.getRandomString()
-        context = get_preseed_context(None, release)
+        nodegroup = factory.make_node_group(maas_url=factory.getRandomString())
+        context = get_preseed_context(release, nodegroup)
         self.assertItemsEqual(
             ['release', 'metadata_enlist_url', 'server_host', 'server_url',
             'main_archive_hostname', 'main_archive_directory',
@@ -339,8 +340,8 @@
         ports_archive = make_url('ports_archive')
         Config.objects.set_config('main_archive', main_archive)
         Config.objects.set_config('ports_archive', ports_archive)
-        context = get_preseed_context(
-            factory.make_node(), factory.getRandomString())
+        nodegroup = factory.make_node_group(maas_url=factory.getRandomString())
+        context = get_preseed_context(factory.make_node(), nodegroup)
         parsed_main_archive = urlparse(main_archive)
         parsed_ports_archive = urlparse(ports_archive)
         self.assertEqual(
@@ -402,9 +403,9 @@
         self.assertIsInstance(preseed, str)
 
     def test_get_preseed_uses_nodegroup_maas_url(self):
-        ng_url = 'http://%s' % factory.make_name('host')
+        ng_url = 'http://%s' % factory.make_hostname()
         ng = factory.make_node_group(maas_url=ng_url)
-        maas_url = 'http://%s' % factory.make_name('host')
+        maas_url = 'http://%s' % factory.make_hostname()
         node = factory.make_node(
             nodegroup=ng, status=NODE_STATUS.COMMISSIONING)
         self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
@@ -423,11 +424,12 @@
         self.assertIsInstance(preseed, str)
 
     def test_get_preseed_uses_nodegroup_maas_url(self):
-        ng_url = 'http://%s' % factory.make_name('host')
-        maas_url = 'http://%s' % factory.make_name('host')
+        ng_url = 'http://%s' % factory.make_hostname()
+        maas_url = 'http://%s' % factory.make_hostname()
         self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
+        nodegroup = factory.make_node_group(maas_url=ng_url)
         preseed = render_enlistment_preseed(
-            PRESEED_TYPE.ENLIST, "precise", base_url=ng_url)
+            PRESEED_TYPE.ENLIST, "precise", nodegroup=nodegroup)
         self.assertThat(
             preseed, MatchesAll(*[Contains(ng_url), Not(Contains(maas_url))]))
 
@@ -461,7 +463,7 @@
 class TestPreseedProxy(TestCase):
 
     def test_preseed_uses_default_proxy(self):
-        server_host = factory.getRandomString().lower()
+        server_host = factory.make_hostname()
         url = 'http://%s:%d/%s' % (
             server_host, factory.getRandomPort(), factory.getRandomString())
         self.patch(settings, 'DEFAULT_MAAS_URL', url)
 
=== modified file 'src/maasserver/tests/test_server_address.py'
--- src/maasserver/tests/test_server_address.py	2012-11-26 14:41:53 +0000
+++ src/maasserver/tests/test_server_address.py	2012-11-27 16:03:23 +0000
@@ -24,7 +24,7 @@
 class TestServerAddress(TestCase):
 
     def make_hostname(self):
-        return '%s.example.com' % factory.make_name('host').lower()
+        return '%s.example.com' % factory.make_hostname()
 
     def set_DEFAULT_MAAS_URL(self, hostname=None, with_port=False):
         """Patch DEFAULT_MAAS_URL to be a (partly) random URL."""
@@ -49,7 +49,7 @@
         self.assertEqual(ip, server_address.get_maas_facing_server_host())
 
     def test_get_maas_facing_server_host_returns_nodegroup_maas_url(self):
-        hostname = factory.make_name('host').lower()
+        hostname = factory.make_hostname()
         maas_url = 'http://%s' % hostname
         nodegroup = factory.make_node_group(maas_url=maas_url)
         self.assertEqual(
@@ -82,7 +82,7 @@
         ip = factory.getRandomIPAddress()
         resolver = FakeMethod(result=ip)
         self.patch(server_address, 'gethostbyname', resolver)
-        hostname = self.make_hostname().lower()
+        hostname = self.make_hostname()
         self.set_DEFAULT_MAAS_URL(hostname=hostname)
         self.assertEqual(
             (ip, [(hostname, )]),
 
=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py	2012-09-10 14:50:56 +0000
+++ src/maastesting/factory.py	2012-11-27 16:03:23 +0000
@@ -148,6 +148,13 @@
         return sep.join(
             filter(None, [prefix, self.getRandomString(size=size)]))
 
+    def make_hostname(self, prefix='host', *args, **kwargs):
+        """Generate a random hostname.
+
+        The returned hostname is lowercase because python's urlparse
+        implicitely lowercases the hostnames."""
+        return self.make_name(prefix=prefix, *args, **kwargs).lower()
+
     def make_names(self, *prefixes):
         """Generate random names.
 
 
=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-11-26 08:52:01 +0000
+++ src/metadataserver/api.py	2012-11-27 16:03:23 +0000
@@ -395,9 +395,8 @@
     def get_enlist_preseed(self, request, version=None):
         """Render and return a preseed script for enlistment."""
         nodegroup = find_nodegroup(request)
-        base_url = nodegroup.maas_url if nodegroup is not None else None
         return HttpResponse(
-            get_enlist_preseed(base_url=base_url), mimetype="text/plain")
+            get_enlist_preseed(nodegroup=nodegroup), mimetype="text/plain")
 
     @operation(idempotent=True)
     def get_preseed(self, request, version=None, system_id=None):