Merge lp:~allenap/maas/one-tftp-server-per-interface into lp:~maas-committers/maas/trunk

[Gavin Panella]

Start a TFTP server for each IPv4 address configured on a cluster controller's interfaces.

Previously there was a single server on all interfaces, which made it tough to figure out on which interface a particular datagram was received.

[Julian Edwards (julian-edwards)]

Awesome! Just one comment as usual - can you squish the multiple assertions into a single one?

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/31/2012 09:30 AM, Gavin Panella wrote:
> Gavin Panella has proposed merging
> lp:~allenap/maas/one-tftp-server-per-interface into lp:maas.
>
> Commit message: Start a TFTP server for each IPv4 address
> configured on a cluster controller's interfaces.
>
> Previously there was a single server on all interfaces, which made
> it tough to figure out on which interface a particular datagram
> was received.
>
>
> Requested reviews: Launchpad code reviewers (launchpad-reviewers)
> Related bugs: Bug #1068843 in MAAS: "maas-cluster-controller
> doesn't have images for provisioning"
> https://bugs.launchpad.net/maas/+bug/1068843
>
> For more details, see:
> https://code.launchpad.net/~allenap/maas/one-tftp-server-per-interface/+merge/132252
>
>
>
+ example_interfaces = {
+ 'eth0': {
+ 17: [{'addr': '00:1d:ba:86:aa:fe',
+ 'broadcast': 'ff:ff:ff:ff:ff:ff'}],
+ },
+ 'lo': {
+ 2: [{'addr': '127.0.0.1',
+ 'netmask': '255.0.0.0',
+ 'peer': '127.0.0.1'}],
+ 10: [{'addr': '::1',
+ 'netmask': 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff'}],
+ 17: [{'addr': '00:00:00:00:00:00',
+ 'peer': '00:00:00:00:00:00'}],
+ },

The 2, 10, 17 values seem particularly magical. Is it possible to do
something ilke:

tcpv4 = 2
tcpv6 = 17
...

And then use those constants instead of raw numbers?

 review: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCQ5hUACgkQJdeBCYSNAANfAwCfZ2E2Tz27Iw6ZbtwPTKbaHvyd
wcYAoLe9lehi1shIBcizSrznFA+sXmQE
=Qc3e
-----END PGP SIGNATURE-----

review: Approve

[Gavin Panella (allenap)]

Thank you both for the reviews. I've addressed all your points.

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2012-08-16 08:07:06 +0000
+++ src/provisioningserver/plugin.py	2012-10-31 11:59:23 +0000
@@ -19,6 +19,7 @@
     OOPSService,
     )
 from provisioningserver.tftp import TFTPBackend
+from provisioningserver.utils import get_all_interface_addresses
 from tftp.protocol import TFTP
 from twisted.application import internet
 from twisted.application.internet import (
@@ -150,10 +151,17 @@
     def _makeTFTPService(self, tftp_config):
         """Create the dynamic TFTP service."""
         backend = TFTPBackend(tftp_config["root"], tftp_config["generator"])
-        factory = TFTP(backend)
-        tftp_service = internet.UDPServer(tftp_config["port"], factory)
-        tftp_service.setName("tftp")
-        return tftp_service
+        # Create a UDP server individually for each discovered network
+        # interface, so that we can detect the interface via which we have
+        # received a datagram.
+        tftp_services = MultiService()
+        tftp_services.setName("tftp")
+        for address in get_all_interface_addresses():
+            tftp_service = internet.UDPServer(
+                tftp_config["port"], TFTP(backend), interface=address)
+            tftp_service.setName(address)
+            tftp_service.setServiceParent(tftp_services)
+        return tftp_services
 
     def makeService(self, options):
         """Construct a service."""
 
=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2012-08-16 10:50:16 +0000
+++ src/provisioningserver/tests/test_plugin.py	2012-10-31 11:59:23 +0000
@@ -17,6 +17,7 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
+from provisioningserver import plugin
 from provisioningserver.plugin import (
     Options,
     ProvisioningRealm,
@@ -29,6 +30,11 @@
     AsynchronousDeferredRunTest,
     )
 from testtools.matchers import (
+    AfterPreprocessing,
+    AllMatch,
+    Equals,
+    IsInstance,
+    MatchesAll,
     MatchesException,
     Raises,
     )
@@ -120,6 +126,13 @@
 
     def test_tftp_service(self):
         # A TFTP service is configured and added to the top-level service.
+        interfaces = [
+            factory.getRandomIPAddress(),
+            factory.getRandomIPAddress(),
+            ]
+        self.patch(
+            plugin, "get_all_interface_addresses",
+            lambda: interfaces)
         config = {
             "tftp": {
                 "generator": "http://candlemass/solitude",
@@ -131,17 +144,43 @@
         options["config-file"] = self.write_config(config)
         service_maker = ProvisioningServiceMaker("Harry", "Hill")
         service = service_maker.makeService(options)
-        tftp_service = service.getServiceNamed("tftp")
-        self.assertIsInstance(tftp_service, UDPServer)
-        port, protocol = tftp_service.args
-        self.assertEqual(config["tftp"]["port"], port)
-        self.assertIsInstance(protocol, TFTP)
-        self.assertIsInstance(protocol.backend, TFTPBackend)
+        tftp_services = service.getServiceNamed("tftp")
+        # The "tftp" service is a multi-service containing UDP servers for
+        # each interface defined by get_all_interface_addresses().
+        self.assertIsInstance(tftp_services, MultiService)
+        services = [
+            tftp_services.getServiceNamed(interface)
+            for interface in interfaces
+            ]
+        expected_backend = MatchesAll(
+            IsInstance(TFTPBackend),
+            AfterPreprocessing(
+                lambda backend: backend.base.path,
+                Equals(config["tftp"]["root"])),
+            AfterPreprocessing(
+                lambda backend: backend.generator_url.geturl(),
+                Equals(config["tftp"]["generator"])))
+        expected_protocol = MatchesAll(
+            IsInstance(TFTP),
+            AfterPreprocessing(
+                lambda protocol: protocol.backend,
+                expected_backend))
+        expected_service = MatchesAll(
+            IsInstance(UDPServer),
+            AfterPreprocessing(
+                lambda service: len(service.args),
+                Equals(2)),
+            AfterPreprocessing(
+                lambda service: service.args[0],  # port
+                Equals(config["tftp"]["port"])),
+            AfterPreprocessing(
+                lambda service: service.args[1],  # protocol
+                expected_protocol))
+        self.assertThat(services, AllMatch(expected_service))
+        # Only the interface used for each service differs.
         self.assertEqual(
-            (config["tftp"]["root"],
-             config["tftp"]["generator"]),
-            (protocol.backend.base.path,
-             protocol.backend.generator_url.geturl()))
+            [service.kwargs for service in services],
+            [{"interface": interface} for interface in interfaces])
 
 
 class TestSingleUsernamePasswordChecker(TestCase):
 
=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-09-25 05:21:24 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-10-31 11:59:23 +0000
@@ -35,11 +35,18 @@
 from maastesting.fakemethod import FakeMethod
 from maastesting.testcase import TestCase
 from mock import Mock
+import netifaces
+from netifaces import (
+    AF_LINK,
+    AF_INET,
+    AF_INET6,
+    )
 import provisioningserver
 from provisioningserver.utils import (
     ActionScript,
     atomic_write,
     AtomicWriteScript,
+    get_all_interface_addresses,
     get_mtime,
     incremental_write,
     maas_custom_config_markers,
@@ -59,6 +66,58 @@
 from testtools.testcase import ExpectedException
 
 
+class TestInterfaceFunctions(TestCase):
+    """Tests for functions relating to network interfaces."""
+
+    example_interfaces = {
+        'eth0': {
+            AF_LINK: [{'addr': '00:1d:ba:86:aa:fe',
+                       'broadcast': 'ff:ff:ff:ff:ff:ff'}],
+            },
+        'lo': {
+            AF_INET: [{'addr': '127.0.0.1',
+                       'netmask': '255.0.0.0',
+                       'peer': '127.0.0.1'}],
+            AF_INET6: [{'addr': '::1',
+                        'netmask': 'ff:ff:ff:ff:ff:ff'}],
+            AF_LINK: [{'addr': '00:00:00:00:00:00',
+                       'peer': '00:00:00:00:00:00'}],
+            },
+        'lxcbr0': {
+            AF_INET: [{'addr': '10.0.3.1',
+                       'broadcast': '10.0.3.255',
+                       'netmask': '255.255.255.0'}],
+            AF_INET6: [{'addr': 'fe80::9894:6fff:fe8b:22%lxcbr0',
+                        'netmask': 'ffff:ffff:ffff:ffff::'}],
+            AF_LINK: [{'addr': '9a:94:6f:8b:00:22',
+                       'broadcast': 'ff:ff:ff:ff:ff:ff'}]},
+        'tun0': {
+            AF_INET: [{'addr': '10.99.244.250',
+                       'netmask': '255.255.255.255',
+                       'peer': '10.99.244.249'}],
+            },
+        'wlan0': {
+            AF_INET: [{'addr': '10.155.1.159',
+                       'broadcast': '10.155.31.255',
+                       'netmask': '255.255.224.0'}],
+            AF_INET6: [{'addr': 'fe80::221:5dff:fe85:d2e4%wlan0',
+                        'netmask': 'ffff:ffff:ffff:ffff::'}],
+            AF_LINK: [{'addr': '00:21:5d:85:dAF_INET:e4',
+                       'broadcast': 'ff:ff:ff:ff:ff:ff'}],
+            },
+        }
+
+    def test_get_all_interface_addresses(self):
+        # get_all_interface_addresses() returns the IPv4 addresses associated
+        # with each of the network devices present on the system, as reported
+        # by netifaces. IPv6 is ignored.
+        self.patch(netifaces, "interfaces", self.example_interfaces.keys)
+        self.patch(netifaces, "ifaddresses", self.example_interfaces.get)
+        self.assertEqual(
+            ["127.0.0.1", "10.0.3.1", "10.99.244.250", "10.155.1.159"],
+            list(get_all_interface_addresses()))
+
+
 class TestSafe(TestCase):
     """Test `Safe`."""
 
 
=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-10-11 10:47:57 +0000
+++ src/provisioningserver/utils.py	2012-10-31 11:59:23 +0000
@@ -26,6 +26,7 @@
 from argparse import ArgumentParser
 import errno
 from functools import wraps
+import netifaces
 import os
 from os import fdopen
 from pipes import quote
@@ -460,3 +461,13 @@
         atomic_write(
             content, args.filename, overwrite=not args.no_overwrite,
             mode=mode)
+
+
+def get_all_interface_addresses():
+    """For each network interface, yield its IPv4 address."""
+    for interface in netifaces.interfaces():
+        addresses = netifaces.ifaddresses(interface)
+        if netifaces.AF_INET in addresses:
+            for inet_address in addresses[netifaces.AF_INET]:
+                if "addr" in inet_address:
+                    yield inet_address["addr"]