launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/enlist-kernel-cmd-line into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/enlist-kernel-cmd-line/+merge/111632

This branch adds the enlistment preseed url to the kernel arguments in the PXE config file.

= Pre-imp =

This was briefly discussed with Jeroen and Gavin on IRC.

= Notes =

I didn't bother escaping the url that is now added on the kernel command line.  The existing code didn't seem to bother and we also have complete control over the passed url and the query string arguments.  Anyway, that part of the code should definitely be extracted into a separate method and escaping (if it's really needed) should be done there in a centralized fashion.

There is a catch here.  A big one.  This branch breaks the test suite and another branch will need to be done and landed right after this one lands.  The tests in src/maas/tests/test_maas_import_pxe_files.py now don't pass because now, the command 'generate_enlistment_pxe' loads up src/maasserver/urls.py (because it uses Django's 'reverse' method).  In urls.py, the call to setup_maas_avahi_service publishes the "maas_name." when it's imported.  Since the test runs the command inside a script (and not directly with call_command), when Django runs the avahi stuff, it's not within the context of a Django test case and it blows up because no database can be reached.

Now, this, in fact shows that the avahi modules publishes the 'maas_name' every time a command which imports src/maasserver/urls.py is run.  That's clearly something that should be fixed.
-- 
https://code.launchpad.net/~rvb/maas/enlist-kernel-cmd-line/+merge/111632
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/enlist-kernel-cmd-line into lp:maas.

=== modified file 'src/maasserver/management/commands/generate_enlistment_pxe.py'
--- src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-22 13:45:32 +0000
+++ src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-22 16:01:19 +0000
@@ -23,6 +23,7 @@
 from optparse import make_option
 
 from django.core.management.base import BaseCommand
+from maasserver.utils import absolute_reverse
 from provisioningserver.pxe.pxeconfig import PXEConfig
 
 
@@ -47,6 +48,10 @@
     def handle(self, arch=None, subarch='generic', release=None,
                pxe_target_dir=None, **kwargs):
         image_path = '/maas/%s/%s/%s/install' % (arch, subarch, release)
+        enlistment_url = absolute_reverse(
+            'metadata-enlist-preseed',
+            query_dict={'op': 'get_enlist_preseed'},
+            kwargs={'version': 'latest'})
         # TODO: This needs to go somewhere more appropriate, and
         # probably contain more appropriate options.
         kernel_opts = ' '.join([
@@ -62,6 +67,7 @@
             'priority=critical',
             'local=en_US',
             'netcfg/choose_interface=auto',
+            'auto url=%s' % enlistment_url,
             ])
         template_args = {
             'menutitle': "Enlisting with MAAS",

=== modified file 'src/maasserver/tests/test_commands_generate_enlistment_pxe.py'
--- src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-20 16:56:42 +0000
+++ src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-22 16:01:19 +0000
@@ -18,6 +18,8 @@
 from maasserver.enum import ARCHITECTURE_CHOICES
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
+from maasserver.utils import absolute_reverse
+from maastesting.matchers import ContainsAll
 from provisioningserver.pxe.pxeconfig import PXEConfig
 
 
@@ -32,12 +34,21 @@
             'generate_enlistment_pxe', arch=arch, release=release,
             pxe_target_dir=tftpdir)
         # This produces a "default" PXE config file in the right place.
-        # It refers to the kernel and initrd for the requested
-        # architecture and release.
         result_path = os.path.join(
             tftpdir, arch, 'generic', 'pxelinux.cfg', 'default')
         with open(result_path) as result_file:
             contents = result_file.read()
-        self.assertIn(
-            '/'.join(['/maas', arch, 'generic', release, 'install', 'linux']),
-            contents)
+        enlistment_url = absolute_reverse(
+            'metadata-enlist-preseed',
+            query_dict={'op': 'get_enlist_preseed'},
+            kwargs={'version': 'latest'})
+        self.assertThat(
+            contents,
+            ContainsAll([
+                # The file refers to the kernel and initrd for the
+                # requested architecture and release.
+                '/'.join([
+                    '/maas', arch, 'generic', release, 'install', 'linux']),
+                # The file contains the enlistment url from the metadata API.
+                'auto url=%s' % enlistment_url,
+                ]))

=== modified file 'src/maasserver/tests/test_utils.py'
--- src/maasserver/tests/test_utils.py	2012-06-20 18:32:54 +0000
+++ src/maasserver/tests/test_utils.py	2012-06-22 16:01:19 +0000
@@ -12,6 +12,8 @@
 __metaclass__ = type
 __all__ = []
 
+from urllib import urlencode
+
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from maasserver.enum import NODE_STATUS_CHOICES
@@ -69,6 +71,13 @@
         expected_url = settings.DEFAULT_MAAS_URL + reverse('settings')
         self.assertEqual(expected_url, absolute_url)
 
+    def test_absolute_reverse_uses_query_string(self):
+        self.patch(settings, 'DEFAULT_MAAS_URL', '')
+        parameters = {factory.getRandomString(): factory.getRandomString()}
+        absolute_url = absolute_reverse('settings', query_dict=parameters)
+        expected_url = '%s?%s' % (reverse('settings'), urlencode(parameters))
+        self.assertEqual(expected_url, absolute_url)
+
     def test_absolute_reverse_uses_kwargs(self):
         node = factory.make_node()
         self.patch(settings, 'DEFAULT_MAAS_URL', '')

=== modified file 'src/maasserver/utils.py'
--- src/maasserver/utils.py	2012-06-20 15:59:47 +0000
+++ src/maasserver/utils.py	2012-06-22 16:01:19 +0000
@@ -17,6 +17,7 @@
     'map_enum',
     ]
 
+from urllib import urlencode
 from urlparse import urljoin
 
 from django.conf import settings
@@ -61,7 +62,7 @@
     }
 
 
-def absolute_reverse(view_name, *args, **kwargs):
+def absolute_reverse(view_name, query_dict=None, *args, **kwargs):
     """Return the absolute URL (i.e. including the URL scheme specifier and
     the network location of the MAAS server).  Internally this method simply
     calls Django's 'reverse' method and prefixes the result of that call with
@@ -69,8 +70,13 @@
 
     :param view_name: Django's view function name/reference or URL pattern
         name for which to compute the absolute URL.
+    :param query_dict: Optional dictionary containing parameters for the
+        query string.
     :param args: Positional arguments for Django's 'reverse' method.
     :param kwargs: Named arguments for Django's 'reverse' method.
     """
-    return urljoin(
+    url = urljoin(
         settings.DEFAULT_MAAS_URL, reverse(view_name, *args, **kwargs))
+    if query_dict is not None:
+        url += '?%s' % urlencode(query_dict)
+    return url