Merge lp:~allenap/maas/pass-addresses-to-tftp-backend into lp:~maas-committers/maas/trunk

[Gavin Panella]

Use the node-facing address of the cluster controller as the iSCSI host.

The cluster and node addresses are passed over to the pxeconfig view.

[Julian Edwards (julian-edwards)]

306 + # Send the local and remote endpoint addresses.
307 + local_host, local_port = get("local", (None, None))
308 + if local_host is not None:
309 + params["local"] = local_host
310 + remote_host, remote_port = get("remote", (None, None))
311 + if remote_host is not None:
312 + params["remote"] = remote_host

As discussed, get rid of the conditionals here, just try to poke in the IP data since it's mandatory in the API. If it fails, we want to fail early.

Everything else looks tip top!

review: Approve

=== modified file 'contrib/python-tx-tftp/tftp/protocol.py'
--- contrib/python-tx-tftp/tftp/protocol.py	2012-07-05 14:36:27 +0000
+++ contrib/python-tx-tftp/tftp/protocol.py	2012-10-30 09:11:22 +0000
@@ -12,6 +12,7 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 from twisted.internet.protocol import DatagramProtocol
 from twisted.python import log
+from twisted.python.context import call
 
 
 class TFTP(DatagramProtocol):
@@ -48,11 +49,22 @@
 
     @inlineCallbacks
     def _startSession(self, datagram, addr, mode):
+        # Set up a call context so that we can pass extra arbitrary
+        # information to interested backends without adding extra call
+        # arguments, or switching to using a request object, for example.
+        context = {}
+        if self.transport is not None:
+            # Add the local and remote addresses to the call context.
+            local = self.transport.getHost()
+            context["local"] = local.host, local.port
+            context["remote"] = addr
         try:
             if datagram.opcode == OP_WRQ:
-                fs_interface = yield self.backend.get_writer(datagram.filename)
+                fs_interface = yield call(
+                    context, self.backend.get_writer, datagram.filename)
             elif datagram.opcode == OP_RRQ:
-                fs_interface = yield self.backend.get_reader(datagram.filename)
+                fs_interface = yield call(
+                    context, self.backend.get_reader, datagram.filename)
         except Unsupported, e:
             self.transport.write(ERRORDatagram.from_code(ERR_ILLEGAL_OP,
                                     str(e)).to_wire(), addr)
 
=== modified file 'contrib/python-tx-tftp/tftp/test/test_protocol.py'
--- contrib/python-tx-tftp/tftp/test/test_protocol.py	2012-07-25 19:59:37 +0000
+++ contrib/python-tx-tftp/tftp/test/test_protocol.py	2012-10-30 09:11:22 +0000
@@ -11,9 +11,11 @@
 from tftp.netascii import NetasciiReceiverProxy, NetasciiSenderProxy
 from tftp.protocol import TFTP
 from twisted.internet import reactor
-from twisted.internet.defer import Deferred
+from twisted.internet.address import IPv4Address
+from twisted.internet.defer import Deferred, inlineCallbacks
 from twisted.internet.protocol import DatagramProtocol
 from twisted.internet.task import Clock
+from twisted.python import context
 from twisted.python.filepath import FilePath
 from twisted.test.proto_helpers import StringTransport
 from twisted.trial import unittest
@@ -50,7 +52,8 @@
 
     def setUp(self):
         self.clock = Clock()
-        self.transport = FakeTransport(hostAddress=('127.0.0.1', self.port))
+        self.transport = FakeTransport(
+            hostAddress=IPv4Address('UDP', '127.0.0.1', self.port))
 
     def test_malformed_datagram(self):
         tftp = TFTP(BackendFactory(), _clock=self.clock)
@@ -247,3 +250,71 @@
         self.clock.advance(1)
         self.assertTrue(d.called)
         self.assertTrue(IWriter.providedBy(d.result.backend))
+
+
+class CapturedContext(Exception):
+    """A donkey, to carry the call context back up the stack."""
+
+    def __init__(self, args, names):
+        super(CapturedContext, self).__init__(*args)
+        self.context = {name: context.get(name) for name in names}
+
+
+class ContextCapturingBackend(object):
+    """A fake `IBackend` that raises `CapturedContext`.
+
+    Calling `get_reader` or `get_writer` raises a `CapturedContext` exception,
+    which captures the values of the call context for the given `names`.
+    """
+
+    def __init__(self, *names):
+        self.names = names
+
+    def get_reader(self, file_name):
+        raise CapturedContext(("get_reader", file_name), self.names)
+
+    def get_writer(self, file_name):
+        raise CapturedContext(("get_writer", file_name), self.names)
+
+
+class HostTransport(object):
+    """A fake `ITransport` that only responds to `getHost`."""
+
+    def __init__(self, host):
+        self.host = host
+
+    def getHost(self):
+        return IPv4Address("UDP", *self.host)
+
+
+class BackendCallingContext(unittest.TestCase):
+
+    def setUp(self):
+        super(BackendCallingContext, self).setUp()
+        self.backend = ContextCapturingBackend("local", "remote")
+        self.tftp = TFTP(self.backend)
+        self.tftp.transport = HostTransport(("12.34.56.78", 1234))
+
+    @inlineCallbacks
+    def test_context_rrq(self):
+        rrq_datagram = RRQDatagram('nonempty', 'NetASCiI', {})
+        rrq_addr = ('127.0.0.1', 1069)
+        error = yield self.assertFailure(
+            self.tftp._startSession(rrq_datagram, rrq_addr, "octet"),
+            CapturedContext)
+        self.assertEqual(("get_reader", rrq_datagram.filename), error.args)
+        self.assertEqual(
+            {"local": self.tftp.transport.host, "remote": rrq_addr},
+            error.context)
+
+    @inlineCallbacks
+    def test_context_wrq(self):
+        wrq_datagram = WRQDatagram('nonempty', 'NetASCiI', {})
+        wrq_addr = ('127.0.0.1', 1069)
+        error = yield self.assertFailure(
+            self.tftp._startSession(wrq_datagram, wrq_addr, "octet"),
+            CapturedContext)
+        self.assertEqual(("get_writer", wrq_datagram.filename), error.args)
+        self.assertEqual(
+            {"local": self.tftp.transport.host, "remote": wrq_addr},
+            error.context)
 
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-26 10:56:06 +0000
+++ src/maasserver/api.py	2012-10-30 09:11:22 +0000
@@ -1792,6 +1792,8 @@
     :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
         'armhf').
     :param subarch: Subarchitecture name (in the pxelinux namespace).
+    :param local: The IP address of the cluster controller.
+    :param remote: The IP address of the booting node.
     """
     node = get_node_from_mac_string(request.GET.get('mac', None))
 
@@ -1846,11 +1848,12 @@
 
     purpose = get_boot_purpose(node)
     server_address = get_maas_facing_server_address()
+    cluster_address = get_mandatory_param(request.GET, "local")
 
     params = KernelParameters(
         arch=arch, subarch=subarch, release=series, purpose=purpose,
         hostname=hostname, domain=domain, preseed_url=preseed_url,
-        log_host=server_address, fs_host=server_address)
+        log_host=server_address, fs_host=cluster_address)
 
     return HttpResponse(
         json.dumps(params._asdict()),
 
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-26 10:56:06 +0000
+++ src/maasserver/tests/test_api.py	2012-10-30 09:11:22 +0000
@@ -3202,11 +3202,16 @@
 
 class TestPXEConfigAPI(AnonAPITestCase):
 
+    def get_default_params(self):
+        return {
+            "local": factory.getRandomIPAddress(),
+            "remote": factory.getRandomIPAddress(),
+            }
+
     def get_mac_params(self):
-        return {'mac': factory.make_mac_address().mac_address}
-
-    def get_default_params(self):
-        return dict()
+        params = self.get_default_params()
+        params['mac'] = factory.make_mac_address().mac_address
+        return params
 
     def get_pxeconfig(self, params=None):
         """Make a request to `pxeconfig`, and return its response dict."""
@@ -3240,7 +3245,8 @@
             ContainsAll(KernelParameters._fields))
 
     def test_pxeconfig_returns_data_for_known_node(self):
-        response = self.client.get(reverse('pxeconfig'), self.get_mac_params())
+        params = self.get_mac_params()
+        response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(httplib.OK, response.status_code)
 
     def test_pxeconfig_returns_no_content_for_unknown_node(self):
@@ -3252,6 +3258,7 @@
         architecture = factory.getRandomEnum(ARCHITECTURE)
         arch, subarch = architecture.split('/')
         params = dict(
+            self.get_default_params(),
             mac=factory.getRandomMACAddress(delimiter=b'-'),
             arch=arch,
             subarch=subarch)
@@ -3280,15 +3287,19 @@
         full_hostname = '.'.join([host, domain])
         node = factory.make_node(hostname=full_hostname)
         mac = factory.make_mac_address(node=node)
-        pxe_config = self.get_pxeconfig(params={'mac': mac.mac_address})
+        params = self.get_default_params()
+        params['mac'] = mac.mac_address
+        pxe_config = self.get_pxeconfig(params)
         self.assertEqual(host, pxe_config.get('hostname'))
         self.assertNotIn(domain, pxe_config.values())
 
     def test_pxeconfig_uses_nodegroup_domain_for_node(self):
         mac = factory.make_mac_address()
+        params = self.get_default_params()
+        params['mac'] = mac
         self.assertEqual(
             mac.node.nodegroup.name,
-            self.get_pxeconfig({'mac': mac.mac_address}).get('domain'))
+            self.get_pxeconfig(params).get('domain'))
 
     def get_without_param(self, param):
         """Request a `pxeconfig()` response, but omit `param` from request."""
@@ -3353,6 +3364,13 @@
             fake_boot_purpose,
             json.loads(response.content)["purpose"])
 
+    def test_pxeconfig_returns_fs_host_as_cluster_controller(self):
+        # The kernel parameter `fs_host` points to the cluster controller
+        # address, which is passed over within the `local` parameter.
+        params = self.get_default_params()
+        kernel_params = KernelParameters(**self.get_pxeconfig(params))
+        self.assertEqual(params["local"], kernel_params.fs_host)
+
 
 class TestNodeGroupsAPI(APIv10TestMixin, MultipleUsersScenarios, TestCase):
     scenarios = [
 
=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-10-02 10:53:22 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-10-30 09:11:22 +0000
@@ -35,6 +35,7 @@
     inlineCallbacks,
     succeed,
     )
+from twisted.python import context
 from zope.interface.verify import verifyObject
 
 
@@ -213,6 +214,16 @@
         mac = factory.getRandomMACAddress(b"-")
         config_path = compose_config_path(mac)
         backend = TFTPBackend(self.make_dir(), b"http://example.com/")
+        # python-tx-tftp sets up call context so that backends can discover
+        # more about the environment in which they're running.
+        call_context = {
+            "local": (
+                factory.getRandomIPAddress(),
+                factory.getRandomPort()),
+            "remote": (
+                factory.getRandomIPAddress(),
+                factory.getRandomPort()),
+            }
 
         @partial(self.patch, backend, "get_config_reader")
         def get_config_reader(params):
@@ -220,9 +231,16 @@
             params_json_reader = BytesReader(params_json)
             return succeed(params_json_reader)
 
-        reader = yield backend.get_reader(config_path)
+        reader = yield context.call(
+            call_context, backend.get_reader, config_path)
         output = reader.read(10000)
-        expected_params = dict(mac=mac)
+        # The addresses provided by python-tx-tftp in the call context are
+        # passed over the wire as address:port strings.
+        expected_params = {
+            "mac": mac,
+            "local": call_context["local"][0],  # address only.
+            "remote": call_context["remote"][0],  # address only.
+            }
         observed_params = json.loads(output)
         self.assertEqual(expected_params, observed_params)
 
 
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-10-03 08:50:17 +0000
+++ src/provisioningserver/tftp.py	2012-10-30 09:11:22 +0000
@@ -34,6 +34,7 @@
     IReader,
     )
 from tftp.errors import FileNotFound
+from twisted.python.context import get
 from twisted.web.client import getPage
 import twisted.web.error
 from zope.interface import implementer
@@ -211,6 +212,11 @@
                 for key, value in config_file_match.groupdict().items()
                 if value is not None
                 }
+            # Send the local and remote endpoint addresses.
+            local_host, local_port = get("local", (None, None))
+            params["local"] = local_host
+            remote_host, remote_port = get("remote", (None, None))
+            params["remote"] = remote_host
             d = self.get_config_reader(params)
             d.addErrback(self.get_page_errback, file_name)
             return d