launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/different-mac-address-format into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/different-mac-address-format/+merge/117134

PXELINUX is very specific about which paths it checks, and if there's
a failure other than "not found" for a particular file then it will
keep retrying, assuming things will get better.

If we call the API pxeconfig view with a weird looking MAC address, it
raises a 5xx error. However, I don't want to suppress this and just
say "file not found" in case it's a genuine failure. Instead I've
taken the approach of only forwarding very specific files from the
TFTP server to the API.

Also, PXELINUX passes an ARP HTYPE field in the filename it requests;
we'd forgotten about that.

-- 
https://code.launchpad.net/~allenap/maas/different-mac-address-format/+merge/117134
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/different-mac-address-format into lp:maas.

=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py	2012-07-24 17:47:48 +0000
+++ src/maastesting/factory.py	2012-07-27 20:52:21 +0000
@@ -82,9 +82,10 @@
         return str(IPAddress(
             random.randint(network.first, network.last)))
 
-    def getRandomMACAddress(self):
+    def getRandomMACAddress(self, delimiter=b":"):
+        assert isinstance(delimiter, bytes)
         octets = islice(self.random_octets, 6)
-        return b":".join(format(octet, b"02x") for octet in octets)
+        return delimiter.join(format(octet, b"02x") for octet in octets)
 
     def getRandomDate(self, year=2011):
         start = time.mktime(datetime.datetime(year, 1, 1).timetuple())

=== modified file 'src/maastesting/tests/test_factory.py'
--- src/maastesting/tests/test_factory.py	2012-07-24 17:47:48 +0000
+++ src/maastesting/tests/test_factory.py	2012-07-27 20:52:21 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from datetime import datetime
+from itertools import count
 import os.path
 from random import randint
 
@@ -73,6 +74,11 @@
         for hex_octet in mac_address.split(":"):
             self.assertTrue(0 <= int(hex_octet, 16) <= 255)
 
+    def test_getRandomMACAddress_alternative_delimiter(self):
+        self.patch(factory, "random_octets", count(0x3a))
+        mac_address = factory.getRandomMACAddress(delimiter=b"-")
+        self.assertEqual("3a-3b-3c-3d-3e-3f", mac_address)
+
     def test_make_file_creates_file(self):
         self.assertThat(factory.make_file(self.make_dir()), FileExists())
 

=== modified file 'src/provisioningserver/pxe/tests/test_tftppath.py'
--- src/provisioningserver/pxe/tests/test_tftppath.py	2012-07-23 13:27:56 +0000
+++ src/provisioningserver/pxe/tests/test_tftppath.py	2012-07-27 20:52:21 +0000
@@ -42,7 +42,7 @@
         subarch = factory.make_name('subarch')
         name = factory.make_name('config')
         self.assertEqual(
-            '/maas/%s/%s/pxelinux.cfg/%s' % (arch, subarch, name),
+            '/maas/%s/%s/pxelinux.cfg/01-%s' % (arch, subarch, name),
             compose_config_path(arch, subarch, name))
 
     def test_compose_config_path_does_not_include_tftp_root(self):

=== modified file 'src/provisioningserver/pxe/tftppath.py'
--- src/provisioningserver/pxe/tftppath.py	2012-07-23 10:24:39 +0000
+++ src/provisioningserver/pxe/tftppath.py	2012-07-27 20:52:21 +0000
@@ -25,6 +25,7 @@
     return '/'.join(['/maas', arch, subarch, 'pxelinux.0'])
 
 
+# TODO: move this; it is now only used for testing.
 def compose_config_path(arch, subarch, name):
     """Compose the TFTP path for a PXE configuration file.
 
@@ -37,9 +38,11 @@
     :return: Path for the corresponding PXE config file as exposed over
         TFTP.
     """
-    # Not using os.path.join: this is a TFTP path, not a native path.
-    # Yes, in practice for us they're the same.
-    return '/'.join(['/maas', arch, subarch, 'pxelinux.cfg', name])
+    # Not using os.path.join: this is a TFTP path, not a native path. Yes, in
+    # practice for us they're the same. The '01-' before the name is the ARP
+    # HTYPE (hardware type) that PXELINUX sends. Here we assume it's always
+    # Ethernet.
+    return '/'.join(['/maas', arch, subarch, 'pxelinux.cfg', '01-' + name])
 
 
 def compose_image_path(arch, subarch, release, purpose):

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-07-27 13:27:54 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-07-27 20:52:21 +0000
@@ -71,7 +71,7 @@
         components = {
             "arch": factory.make_name("arch"),
             "subarch": factory.make_name("subarch"),
-            "mac": factory.getRandomMACAddress(),
+            "mac": factory.getRandomMACAddress(b"-"),
             }
         config_path = compose_config_path(
             arch=components["arch"], subarch=components["subarch"],
@@ -130,7 +130,7 @@
         # file path (arch, subarch, name) into the configured generator URL.
         arch = factory.make_name("arch").encode("ascii")
         subarch = factory.make_name("subarch").encode("ascii")
-        mac = factory.getRandomMACAddress()
+        mac = factory.getRandomMACAddress(b"-")
         kernel = factory.make_name("kernel").encode("ascii")
         initrd = factory.make_name("initrd").encode("ascii")
         menu_title = factory.make_name("menu-title").encode("ascii")
@@ -178,7 +178,7 @@
         # a Deferred that will yield a BytesReader.
         arch = factory.make_name("arch").encode("ascii")
         subarch = factory.make_name("subarch").encode("ascii")
-        mac = factory.getRandomMACAddress()
+        mac = factory.getRandomMACAddress(b"-")
         config_path = compose_config_path(arch, subarch, mac)
         backend = TFTPBackend(self.make_dir(), b"http://example.com/";)
 

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-07-27 13:27:54 +0000
+++ src/provisioningserver/tftp.py	2012-07-27 20:52:21 +0000
@@ -15,6 +15,7 @@
     ]
 
 from io import BytesIO
+from itertools import repeat
 import re
 from urllib import urlencode
 from urlparse import (
@@ -50,14 +51,27 @@
     """A partially dynamic read-only TFTP server.
 
     Requests for PXE configurations are forwarded to a configurable URL. See
-    `re_config_file`.
+    `re_config_file` and `re_mac_address`.
+
+    This must be very selective about which requests to forward, because
+    failures cause the boot process to halt. This is why the expression for
+    matching the MAC address is so narrowly defined; PXELINUX attempts to
+    fetch files at many similar paths, and this must respond to only one
+    pattern.
     """
 
     get_page = staticmethod(getPage)
 
+    # This is how PXELINUX represents a MAC address. See
+    # http://www.syslinux.org/wiki/index.php/PXELINUX.
+    re_mac_address = re.compile(
+        "-".join(repeat(r'[0-9a-f]{2}', 6)))
+
+    # The "01-" before the MAC address is the ARP HTYPE field (hardware
+    # type). Here we assume it's always Ethernet.
     re_config_file = re.compile(
-        r'^/?maas/(?P<arch>[^/]+)/(?P<subarch>[^/]+)/'
-        r'pxelinux[.]cfg/(?P<mac>[^/]+)$')
+        r'^/?maas/(?P<arch>[^/]+)/(?P<subarch>[^/]+)/pxelinux[.]cfg'
+        r'/01-(?P<mac>%s)$' % re_mac_address.pattern)
 
     def __init__(self, base_path, generator_url):
         """