launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1041318 into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1041318 in MAAS: "Not finding PXE config when running with external dhcp"
  https://bugs.launchpad.net/maas/+bug/1041318

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1041318/+merge/121266

This may help Andres with his upgrade Q/A.  He is not running MAAS's DHCP server, and so PXE clients are directed to download “pxelinux.0” (at the TFTP root, not in the “maas” directory).

That works for pxelinux.0 itself, but not for the config files.  But it looks like the “maas” directory has been more or less abstracted away anyawy, so in this branch I make it optional.  It's mostly just a small change to the regex in the TFTPBackend.  That will still leave multi-architecture problems, but at least it will support i386 smoothly as a default.

I also added some missing negative tests for the regex.  There were no tests to establish that the config-path regex was not matching things it shouldn't.

(These changes still leave the problem that, as far as I can see, we're not serving the default PXE configs anywhere any more.)


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1041318/+merge/121266
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/bug-1041318 into lp:maas.

=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py	2012-08-23 22:45:54 +0000
+++ src/provisioningserver/pxe/config.py	2012-08-24 19:22:22 +0000
@@ -80,6 +80,9 @@
         parameters generated in another component (for example, see
         `TFTPBackend.get_config_reader`) won't cause this to break.
     """
+    if bootpath is None:
+        bootpath = ''
+
     template = get_pxe_template(
         kernel_params.purpose, kernel_params.arch,
         kernel_params.subarch)

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-08-24 16:23:15 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-08-24 19:22:22 +0000
@@ -23,7 +23,6 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
-import mock
 from provisioningserver.pxe.tftppath import compose_config_path
 from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
 from provisioningserver.tftp import (
@@ -78,7 +77,7 @@
         config_path = compose_config_path(components["mac"])
         return config_path, components
 
-    def test_re_config_file(self):
+    def test_re_config_file_is_compatible_with_config_path_generator(self):
         # The regular expression for extracting components of the file path is
         # compatible with the PXE config path generator.
         regex = TFTPBackend.re_config_file
@@ -110,6 +109,32 @@
         self.assertIsNotNone(match, config_path)
         self.assertEqual(args, match.groupdict())
 
+    def test_re_config_file_matches_classic_pxelinux_cfg(self):
+        # The default config path is simply "pxelinux.cfg" (without
+        # leading slash).  The regex matches this.
+        mac = 'aa-bb-cc-dd-ee-ff'
+        match = TFTPBackend.re_config_file.match('pxelinux.cfg/01-%s' % mac)
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict())
+
+    def test_re_config_file_matches_pxelinux_cfg_with_leading_slash(self):
+        mac = 'aa-bb-cc-dd-ee-ff'
+        match = TFTPBackend.re_config_file.match('/pxelinux.cfg/01-%s' % mac)
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict())
+
+    def test_re_config_file_does_not_match_non_config_file(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('maas/pxelinux.cfg/kernel'))
+
+    def test_re_config_file_does_not_match_file_in_root(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('01-aa-bb-cc-dd-ee-ff'))
+
+    def test_re_config_file_does_not_match_file_not_in_pxelinux_cfg(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('foo/01-aa-bb-cc-dd-ee-ff'))
+
 
 class TestTFTPBackend(TestCase):
     """Tests for `provisioningserver.tftp.TFTPBackend`."""

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-08-23 21:31:46 +0000
+++ src/provisioningserver/tftp.py	2012-08-24 19:22:22 +0000
@@ -85,11 +85,15 @@
     # always Ethernet.
     re_config_file = re.compile(
         r'''
+        # Optional leading slash(es).
         ^/?
-        (?P<bootpath>
-          maas     # Static namespacing.
-        )    # Capture boot directory.
-        /
+        # Optional boot path prefix (separated from the rest by slash):
+        (?:
+            (?P<bootpath>
+                maas
+            )
+            /
+        )?
         pxelinux[.]cfg    # PXELINUX expects this.
         /
         {htype:02x}    # ARP HTYPE.
@@ -170,7 +174,7 @@
         """See `IBackend.get_reader()`.
 
         If `file_name` matches `re_config_file` then the response is obtained
-        from a remote server. Otherwise the filesystem is used to service the
+        from a server. Otherwise the filesystem is used to service the
         response.
         """
         config_file_match = self.re_config_file.match(file_name)