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

Proposed by Gavin Panella on 2012-10-31
Status: Merged
Approved by: Gavin Panella on 2012-10-31
Approved revision: 1324
Merged at revision: 1322
Proposed branch: lp:~allenap/maas/one-tftp-server-per-interface
Merge into: lp:maas/trunk
Diff against target: 233 lines (+131/-14)
4 files modified
src/provisioningserver/plugin.py (+12/-4)
src/provisioningserver/tests/test_plugin.py (+49/-10)
src/provisioningserver/tests/test_utils.py (+59/-0)
src/provisioningserver/utils.py (+11/-0)
To merge this branch: bzr merge lp:~allenap/maas/one-tftp-server-per-interface
Reviewer Review Type Date Requested Status
John A Meinel Approve on 2012-10-31
Julian Edwards (community) 2012-10-31 Approve on 2012-10-31
Review via email: mp+132252@code.launchpad.net

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.

To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :

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

review: Approve
John A Meinel (jameinel) wrote :

-----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
1323. By Gavin Panella on 2012-10-31

Express the TFTP service assertions using matchers instead of iterating.

1324. By Gavin Panella on 2012-10-31

Use symbols from netifaces instead of magic numbers.

Gavin Panella (allenap) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/plugin.py'
2--- src/provisioningserver/plugin.py 2012-08-16 08:07:06 +0000
3+++ src/provisioningserver/plugin.py 2012-10-31 11:59:23 +0000
4@@ -19,6 +19,7 @@
5 OOPSService,
6 )
7 from provisioningserver.tftp import TFTPBackend
8+from provisioningserver.utils import get_all_interface_addresses
9 from tftp.protocol import TFTP
10 from twisted.application import internet
11 from twisted.application.internet import (
12@@ -150,10 +151,17 @@
13 def _makeTFTPService(self, tftp_config):
14 """Create the dynamic TFTP service."""
15 backend = TFTPBackend(tftp_config["root"], tftp_config["generator"])
16- factory = TFTP(backend)
17- tftp_service = internet.UDPServer(tftp_config["port"], factory)
18- tftp_service.setName("tftp")
19- return tftp_service
20+ # Create a UDP server individually for each discovered network
21+ # interface, so that we can detect the interface via which we have
22+ # received a datagram.
23+ tftp_services = MultiService()
24+ tftp_services.setName("tftp")
25+ for address in get_all_interface_addresses():
26+ tftp_service = internet.UDPServer(
27+ tftp_config["port"], TFTP(backend), interface=address)
28+ tftp_service.setName(address)
29+ tftp_service.setServiceParent(tftp_services)
30+ return tftp_services
31
32 def makeService(self, options):
33 """Construct a service."""
34
35=== modified file 'src/provisioningserver/tests/test_plugin.py'
36--- src/provisioningserver/tests/test_plugin.py 2012-08-16 10:50:16 +0000
37+++ src/provisioningserver/tests/test_plugin.py 2012-10-31 11:59:23 +0000
38@@ -17,6 +17,7 @@
39
40 from maastesting.factory import factory
41 from maastesting.testcase import TestCase
42+from provisioningserver import plugin
43 from provisioningserver.plugin import (
44 Options,
45 ProvisioningRealm,
46@@ -29,6 +30,11 @@
47 AsynchronousDeferredRunTest,
48 )
49 from testtools.matchers import (
50+ AfterPreprocessing,
51+ AllMatch,
52+ Equals,
53+ IsInstance,
54+ MatchesAll,
55 MatchesException,
56 Raises,
57 )
58@@ -120,6 +126,13 @@
59
60 def test_tftp_service(self):
61 # A TFTP service is configured and added to the top-level service.
62+ interfaces = [
63+ factory.getRandomIPAddress(),
64+ factory.getRandomIPAddress(),
65+ ]
66+ self.patch(
67+ plugin, "get_all_interface_addresses",
68+ lambda: interfaces)
69 config = {
70 "tftp": {
71 "generator": "http://candlemass/solitude",
72@@ -131,17 +144,43 @@
73 options["config-file"] = self.write_config(config)
74 service_maker = ProvisioningServiceMaker("Harry", "Hill")
75 service = service_maker.makeService(options)
76- tftp_service = service.getServiceNamed("tftp")
77- self.assertIsInstance(tftp_service, UDPServer)
78- port, protocol = tftp_service.args
79- self.assertEqual(config["tftp"]["port"], port)
80- self.assertIsInstance(protocol, TFTP)
81- self.assertIsInstance(protocol.backend, TFTPBackend)
82+ tftp_services = service.getServiceNamed("tftp")
83+ # The "tftp" service is a multi-service containing UDP servers for
84+ # each interface defined by get_all_interface_addresses().
85+ self.assertIsInstance(tftp_services, MultiService)
86+ services = [
87+ tftp_services.getServiceNamed(interface)
88+ for interface in interfaces
89+ ]
90+ expected_backend = MatchesAll(
91+ IsInstance(TFTPBackend),
92+ AfterPreprocessing(
93+ lambda backend: backend.base.path,
94+ Equals(config["tftp"]["root"])),
95+ AfterPreprocessing(
96+ lambda backend: backend.generator_url.geturl(),
97+ Equals(config["tftp"]["generator"])))
98+ expected_protocol = MatchesAll(
99+ IsInstance(TFTP),
100+ AfterPreprocessing(
101+ lambda protocol: protocol.backend,
102+ expected_backend))
103+ expected_service = MatchesAll(
104+ IsInstance(UDPServer),
105+ AfterPreprocessing(
106+ lambda service: len(service.args),
107+ Equals(2)),
108+ AfterPreprocessing(
109+ lambda service: service.args[0], # port
110+ Equals(config["tftp"]["port"])),
111+ AfterPreprocessing(
112+ lambda service: service.args[1], # protocol
113+ expected_protocol))
114+ self.assertThat(services, AllMatch(expected_service))
115+ # Only the interface used for each service differs.
116 self.assertEqual(
117- (config["tftp"]["root"],
118- config["tftp"]["generator"]),
119- (protocol.backend.base.path,
120- protocol.backend.generator_url.geturl()))
121+ [service.kwargs for service in services],
122+ [{"interface": interface} for interface in interfaces])
123
124
125 class TestSingleUsernamePasswordChecker(TestCase):
126
127=== modified file 'src/provisioningserver/tests/test_utils.py'
128--- src/provisioningserver/tests/test_utils.py 2012-09-25 05:21:24 +0000
129+++ src/provisioningserver/tests/test_utils.py 2012-10-31 11:59:23 +0000
130@@ -35,11 +35,18 @@
131 from maastesting.fakemethod import FakeMethod
132 from maastesting.testcase import TestCase
133 from mock import Mock
134+import netifaces
135+from netifaces import (
136+ AF_LINK,
137+ AF_INET,
138+ AF_INET6,
139+ )
140 import provisioningserver
141 from provisioningserver.utils import (
142 ActionScript,
143 atomic_write,
144 AtomicWriteScript,
145+ get_all_interface_addresses,
146 get_mtime,
147 incremental_write,
148 maas_custom_config_markers,
149@@ -59,6 +66,58 @@
150 from testtools.testcase import ExpectedException
151
152
153+class TestInterfaceFunctions(TestCase):
154+ """Tests for functions relating to network interfaces."""
155+
156+ example_interfaces = {
157+ 'eth0': {
158+ AF_LINK: [{'addr': '00:1d:ba:86:aa:fe',
159+ 'broadcast': 'ff:ff:ff:ff:ff:ff'}],
160+ },
161+ 'lo': {
162+ AF_INET: [{'addr': '127.0.0.1',
163+ 'netmask': '255.0.0.0',
164+ 'peer': '127.0.0.1'}],
165+ AF_INET6: [{'addr': '::1',
166+ 'netmask': 'ff:ff:ff:ff:ff:ff'}],
167+ AF_LINK: [{'addr': '00:00:00:00:00:00',
168+ 'peer': '00:00:00:00:00:00'}],
169+ },
170+ 'lxcbr0': {
171+ AF_INET: [{'addr': '10.0.3.1',
172+ 'broadcast': '10.0.3.255',
173+ 'netmask': '255.255.255.0'}],
174+ AF_INET6: [{'addr': 'fe80::9894:6fff:fe8b:22%lxcbr0',
175+ 'netmask': 'ffff:ffff:ffff:ffff::'}],
176+ AF_LINK: [{'addr': '9a:94:6f:8b:00:22',
177+ 'broadcast': 'ff:ff:ff:ff:ff:ff'}]},
178+ 'tun0': {
179+ AF_INET: [{'addr': '10.99.244.250',
180+ 'netmask': '255.255.255.255',
181+ 'peer': '10.99.244.249'}],
182+ },
183+ 'wlan0': {
184+ AF_INET: [{'addr': '10.155.1.159',
185+ 'broadcast': '10.155.31.255',
186+ 'netmask': '255.255.224.0'}],
187+ AF_INET6: [{'addr': 'fe80::221:5dff:fe85:d2e4%wlan0',
188+ 'netmask': 'ffff:ffff:ffff:ffff::'}],
189+ AF_LINK: [{'addr': '00:21:5d:85:dAF_INET:e4',
190+ 'broadcast': 'ff:ff:ff:ff:ff:ff'}],
191+ },
192+ }
193+
194+ def test_get_all_interface_addresses(self):
195+ # get_all_interface_addresses() returns the IPv4 addresses associated
196+ # with each of the network devices present on the system, as reported
197+ # by netifaces. IPv6 is ignored.
198+ self.patch(netifaces, "interfaces", self.example_interfaces.keys)
199+ self.patch(netifaces, "ifaddresses", self.example_interfaces.get)
200+ self.assertEqual(
201+ ["127.0.0.1", "10.0.3.1", "10.99.244.250", "10.155.1.159"],
202+ list(get_all_interface_addresses()))
203+
204+
205 class TestSafe(TestCase):
206 """Test `Safe`."""
207
208
209=== modified file 'src/provisioningserver/utils.py'
210--- src/provisioningserver/utils.py 2012-10-11 10:47:57 +0000
211+++ src/provisioningserver/utils.py 2012-10-31 11:59:23 +0000
212@@ -26,6 +26,7 @@
213 from argparse import ArgumentParser
214 import errno
215 from functools import wraps
216+import netifaces
217 import os
218 from os import fdopen
219 from pipes import quote
220@@ -460,3 +461,13 @@
221 atomic_write(
222 content, args.filename, overwrite=not args.no_overwrite,
223 mode=mode)
224+
225+
226+def get_all_interface_addresses():
227+ """For each network interface, yield its IPv4 address."""
228+ for interface in netifaces.interfaces():
229+ addresses = netifaces.ifaddresses(interface)
230+ if netifaces.AF_INET in addresses:
231+ for inet_address in addresses[netifaces.AF_INET]:
232+ if "addr" in inet_address:
233+ yield inet_address["addr"]