Merge lp:~allenap/maas/regiond-bind-on-all-interfaces into lp:~maas-committers/maas/trunk

Proposed by Gavin Panella
Status: Rejected
Rejected by: MAAS Lander
Proposed branch: lp:~allenap/maas/regiond-bind-on-all-interfaces
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 296 lines (+64/-39)
8 files modified
src/maasserver/eventloop.py (+29/-10)
src/maasserver/start_up.py (+2/-1)
src/maasserver/tests/test_eventloop.py (+3/-6)
src/maasserver/tests/test_plugin.py (+5/-1)
src/maasserver/tests/test_start_up.py (+3/-2)
src/maasserver/tests/test_webapp.py (+15/-12)
src/maasserver/webapp.py (+2/-4)
src/maastesting/protractor/runner.py (+5/-3)
To merge this branch: bzr merge lp:~allenap/maas/regiond-bind-on-all-interfaces
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Abstain
Review via email: mp+252159@code.launchpad.net

Commit message

Bind the web application on all IPv4 and IPv6 interfaces.

Previously only IPv4 interfaces were bound.

Description of the change

This is a resurrected old branch that I never quite finished.

To post a comment you must log in.
Revision history for this message
Mike Pontillo (mpontillo) wrote :

This is a little strange to me. Usually IPv6 sockets operate in IPv4-compatability mode, and all a service has to do in order to listen on *both* IPv4 and IPv6 sockets is to bind to `::`. So this branch looks like it may be more complex than needed. However, I haven't investigated how Twisted does it (and the Python code *may* be setting socket options that disable IPv4-compatibility within IPv6). So I neither approve nor disapprove, but I think it warrants more investigation.

review: Abstain
Revision history for this message
Gavin Panella (allenap) wrote :

Thanks for looking at this Mike. I didn't know about IPv4-compatibility mode for sockets. I'll give that a go!

Revision history for this message
Gavin Panella (allenap) wrote :

I did a little experiment:

  >>> import socket
  >>> s = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
  >>> s.bind(('::', 0))
  >>> s.listen(50)
  >>> s.getsockname()
  ('::', 39025, 0, 0)

Okay, looks good.

  $ lsof -i TCP:39026
  COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
  ipython 71207 gavin 5u IPv6 492194 0t0 TCP *:39025 (LISTEN)

It doesn't show IPv4 there, but as Mike says, this is a compatibility
mode so I'm not entirely surprised.

Next, in another shell:

  $ telnet localhost 39025
  Trying 127.0.0.1...
  Connected to localhost.
  ...

At the same time:

  >>> s.accept()
  (<socket._socketobject at 0x7fc88bc41c20>, ('::ffff:127.0.0.1', 35080, 0, 0))

Okay, so the server sees an IPv4 address mapped into the IPv6 address
space.

Again, in another shell:

  $ telnet ::1 39025
  Trying ::1...
  Connected to ::1.
  ...

Again, at the same time:

  >>> s.accept()
  (<socket._socketobject at 0x7fc88a87d0c0>, ('::1', 43349, 0, 0))

No surprises here.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Cool! Glad to see that this works as expected.

One usual 'gotcha' here is that if you want to get the IP address on the other side of the socket, you may get (for example) '::192.168.0.2' instead of '192.168.0.2', as you'd expect in IPv4. Not sure if we do this anywhere; maybe for the HTTP access logs. It may be smart enough to convert it to IPv4 if it detects and IPv4-compatible address.

There may be sysctls that affect the behavior of this, but obviously that's out of scope.

Revision history for this message
Gavin Panella (allenap) wrote :

My concern is that IPv4 connections have the IPv4 address mapped into the IPv6 address space. I'm not sure what consumes those addresses and I don't really want to have to do QA now to figure out what's going to break (or not). IPv6 is not yet a fully supported thing in MAAS, I think, so it might be safer to bind IPv4 and IPv6 separately, so that IPv4 continues to work exactly as now.

Mike, and anyone else who has IPv6 experience: what do you think?

Revision history for this message
Mike Pontillo (mpontillo) wrote :

My recommendation would be to "bite the bullet" and use IPv6-compatibility mode as intended. If there are issues that occur, I feel like they would have occurred anyway, eventually (maybe in an IPv6-only environment). Plus, if you do it this way and bind to IPv4 and IPv6 separately, you may risk race conditions. (what if the IPv6 socket somehow binds first; maybe the address is already in use for a moment, etc - then you'll be in the same situation that you already fear.)

This RFC might be worth reading before we make a decision:

http://tools.ietf.org/html/rfc4038

There are a lot of aspects of the IPv6 transition to think about, but in particular these sections are relevant:

http://tools.ietf.org/html/rfc4038#section-4.3
http://tools.ietf.org/html/rfc4038#section-6

Since making this change could uncover some unexpected "gotchas", we might want to consider doing it right at the beginning of the next cycle, so we can catch any bugs.

3511. By Gavin Panella

Merge trunk, resolving conflicts.

3512. By Gavin Panella

Bind an AF_INET6 socket to '::', which should provide IPv4 compatibility.

Revision history for this message
Gavin Panella (allenap) wrote :

I've switched this to the IPv6 compatibility thing, i.e. only one socket. I've just sent if off for a run through CI.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

so if this doesn't land, IPv6 wont work?

Revision history for this message
Gavin Panella (allenap) wrote :

> so if this doesn't land, IPv6 wont work?

Yes. IPv6 support is somewhat experimental throughout MAAS, but without this it can definitely be classed as "not working".

3513. By Gavin Panella

Merge trunk, resolving lots of conflicts.

3514. By Gavin Panella

Get things working.

Revision history for this message
MAAS Lander (maas-lander) wrote :

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

3514. By Gavin Panella

Get things working.

3513. By Gavin Panella

Merge trunk, resolving lots of conflicts.

3512. By Gavin Panella

Bind an AF_INET6 socket to '::', which should provide IPv4 compatibility.

3511. By Gavin Panella

Merge trunk, resolving conflicts.

3510. By Gavin Panella

Run start_up() before starting *any* services.

3509. By Gavin Panella

Use @transactional so that serialization failures are retried.

3508. By Gavin Panella

Remove DEFAULT_PORT.

3507. By Gavin Panella

Merge trunk, resolving conflicts.

3506. By Gavin Panella

Merge trunk.

3505. By Gavin Panella

Bind the webapp to all interfaces.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/eventloop.py'
2--- src/maasserver/eventloop.py 2015-04-08 16:24:22 +0000
3+++ src/maasserver/eventloop.py 2015-04-17 10:22:44 +0000
4@@ -64,9 +64,8 @@
5 from twisted.application.service import MultiService
6 from twisted.internet import reactor
7 from twisted.internet.endpoints import AdoptedStreamServerEndpoint
8+from twisted.python.threadpool import ThreadPool
9
10-# Default port for regiond.
11-DEFAULT_PORT = 5240
12
13 logger = getLogger(__name__)
14
15@@ -151,13 +150,21 @@
16 return bootresources.ImportResourcesProgressService()
17
18
19-def make_WebApplicationService():
20+def make_WebApplicationService_for(family, address, port, threadpool):
21+ """Create a web application service for the given socket family.
22+
23+ :param family: Either `socket.AF_INET` or `socket.AF_INET6`.
24+ :param port: The TCP port on which to listen.
25+ :param threadpool: The `ThreadPool` to use.
26+
27+ :return: An instance of `WebApplicationService` connected to a socket
28+ that's listening on all interfaces.
29+ """
30 from maasserver.webapp import WebApplicationService
31- site_port = DEFAULT_PORT # config["port"]
32 # Make a socket with SO_REUSEPORT set so that we can run multiple web
33 # applications. This is easier to do from outside of Twisted as there's
34 # not yet official support for setting socket options.
35- s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
36+ s = socket.socket(family, socket.SOCK_STREAM)
37 s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
38 try:
39 s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1)
40@@ -170,17 +177,29 @@
41 # log message when we see this error, and a test for it.
42 if e.errno != ENOPROTOOPT:
43 raise e
44- s.bind(('0.0.0.0', site_port))
45- # Use a backlog of 50, which seems to be fairly common.
46- s.listen(50)
47+ s.bind((address, port))
48+ s.listen(50) # A backlog of 50 seems fairly common.
49 # Adopt this socket into Twisted's reactor.
50 site_endpoint = AdoptedStreamServerEndpoint(reactor, s.fileno(), s.family)
51- site_endpoint.port = site_port # Make it easy to get the port number.
52+ site_endpoint.port = port # Make it easy to get the port number.
53 site_endpoint.socket = s # Prevent garbage collection.
54- site_service = WebApplicationService(site_endpoint)
55+ site_service = WebApplicationService(site_endpoint, threadpool)
56 return site_service
57
58
59+def make_WebApplicationService(port=5240):
60+ """Create a web application meta-service.
61+
62+ :param port: The TCP port on which to listen.
63+
64+ :return: A `WebApplicationService` instance, for IPv6, but bound to
65+ ``::``, providing IPv4 compatibility.
66+ """
67+ threadpool = ThreadPool(name="webapp")
68+ return make_WebApplicationService_for(
69+ socket.AF_INET6, "::", port, threadpool)
70+
71+
72 class RegionEventLoop:
73 """An event loop running in a region controller process.
74
75
76=== modified file 'src/maasserver/start_up.py'
77--- src/maasserver/start_up.py 2015-04-07 23:09:12 +0000
78+++ src/maasserver/start_up.py 2015-04-17 10:22:44 +0000
79@@ -1,4 +1,4 @@
80-# Copyright 2012-2014 Canonical Ltd. This software is licensed under the
81+# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
82 # GNU Affero General Public License version 3 (see the file LICENSE).
83
84 """Start-up utilities for the MAAS server."""
85@@ -167,6 +167,7 @@
86 @synchronised(locks.startup)
87 def inner_start_up():
88 """Startup jobs that must run serialized w.r.t. other starting servers."""
89+
90 # Register our MAC data type with psycopg.
91 register_mac_type(connection.cursor())
92
93
94=== modified file 'src/maasserver/tests/test_eventloop.py'
95--- src/maasserver/tests/test_eventloop.py 2015-03-25 15:33:23 +0000
96+++ src/maasserver/tests/test_eventloop.py 2015-04-17 10:22:44 +0000
97@@ -24,7 +24,6 @@
98 nonces_cleanup,
99 webapp,
100 )
101-from maasserver.eventloop import DEFAULT_PORT
102 from maasserver.rpc import regionservice
103 from maasserver.testing.eventloop import RegionEventLoopFixture
104 from maasserver.utils.orm import transactional
105@@ -173,14 +172,12 @@
106 def test_make_WebApplicationService(self):
107 service = eventloop.make_WebApplicationService()
108 self.assertThat(service, IsInstance(webapp.WebApplicationService))
109- # The endpoint is set to port 5243 on localhost.
110+ # The endpoint is set to port 5240 on all interfaces.
111 self.assertThat(service.endpoint, MatchesStructure.byEquality(
112- reactor=reactor, addressFamily=socket.AF_INET))
113- self.assertThat(
114- service.endpoint.port, Equals(DEFAULT_PORT))
115+ reactor=reactor, addressFamily=socket.AF_INET6))
116 self.assertThat(
117 service.endpoint.socket.getsockname(),
118- Equals(("0.0.0.0", DEFAULT_PORT)))
119+ Equals(("::", 5240, 0, 0)))
120 # It is registered as a factory in RegionEventLoop.
121 self.assertIn(
122 eventloop.make_WebApplicationService,
123
124=== modified file 'src/maasserver/tests/test_plugin.py'
125--- src/maasserver/tests/test_plugin.py 2015-04-16 07:35:59 +0000
126+++ src/maasserver/tests/test_plugin.py 2015-04-17 10:22:44 +0000
127@@ -15,7 +15,10 @@
128 __all__ = []
129
130 import crochet
131-from maasserver import eventloop
132+from maasserver import (
133+ eventloop,
134+ start_up,
135+ )
136 from maasserver.plugin import (
137 Options,
138 RegionServiceMaker,
139@@ -60,6 +63,7 @@
140 Only the site service is created when no options are given.
141 """
142 self.patch_autospec(logger, "basicConfig")
143+ self.patch_autospec(start_up, "start_up")
144 options = Options()
145 service_maker = RegionServiceMaker("Harry", "Hill")
146 service = service_maker.makeService(options)
147
148=== modified file 'src/maasserver/tests/test_start_up.py'
149--- src/maasserver/tests/test_start_up.py 2015-04-07 23:09:12 +0000
150+++ src/maasserver/tests/test_start_up.py 2015-04-17 10:22:44 +0000
151@@ -75,8 +75,9 @@
152
153 def test_inner_start_up_runs_in_exclusion(self):
154 lock_checker = LockChecker()
155- self.patch(start_up, 'dns_update_all_zones', lock_checker)
156- start_up.inner_start_up()
157+ # Patch-out a function that's called inside inner_start_up().
158+ self.patch(start_up, "register_all_triggers", lock_checker)
159+ start_up.start_up()
160 self.assertEqual(1, lock_checker.call_count)
161 self.assertEqual(True, lock_checker.lock_was_held)
162
163
164=== modified file 'src/maasserver/tests/test_webapp.py'
165--- src/maasserver/tests/test_webapp.py 2015-04-02 13:21:10 +0000
166+++ src/maasserver/tests/test_webapp.py 2015-04-17 10:22:44 +0000
167@@ -24,9 +24,9 @@
168 start_up,
169 webapp,
170 )
171+from maasserver.utils import views
172 from maasserver.websockets.protocol import WebSocketFactory
173 from maastesting.factory import factory
174-from maastesting.matchers import MockCalledOnceWith
175 from maastesting.testcase import MAASTestCase
176 from provisioningserver.rpc.testing import TwistedLoggerFixture
177 from testtools.matchers import (
178@@ -66,15 +66,17 @@
179
180
181 class TestWebApplicationService(MAASTestCase):
182+
183 def make_endpoint(self):
184 return TCP4ServerEndpoint(reactor, 0, interface="localhost")
185
186 def make_webapp(self):
187 service_endpoint = self.make_endpoint()
188- service = webapp.WebApplicationService(service_endpoint)
189+ service = webapp.WebApplicationService(
190+ service_endpoint, ThreadPool(name=self.id()))
191 return service
192
193- def test__init_creates_site_and_threadpool(self):
194+ def test__init_creates_site_and_saves_threadpool(self):
195 service = self.make_webapp()
196 self.assertThat(service.site, IsInstance(Site))
197 self.assertThat(service.threadpool, IsInstance(ThreadPool))
198@@ -114,16 +116,17 @@
199
200 self.assertTrue(service.running)
201 self.assertTrue(service.threadpool.started)
202- self.assertThat(start_up.start_up, MockCalledOnceWith())
203 self.assertTrue(service.websocket.listener.connected())
204
205 def test__error_when_starting_is_logged(self):
206 service = self.make_webapp()
207 self.addCleanup(service.stopService)
208
209- start_up_error = factory.make_exception()
210- self.patch_autospec(start_up, "start_up")
211- start_up.start_up.return_value = defer.fail(start_up_error)
212+ # This will cause an error to be raised in
213+ # WebApplicationService.prepareApplication().
214+ start_up_error = factory.make_exception_type()
215+ self.patch_autospec(webapp, "WebApplicationHandler")
216+ webapp.WebApplicationHandler.side_effect = start_up_error
217
218 # The failure is logged.
219 with TwistedLoggerFixture() as logger:
220@@ -144,11 +147,11 @@
221 service = self.make_webapp()
222 self.addCleanup(service.stopService)
223
224- # start_up() isn't safe to call right now, but we only really care
225- # that it is called.
226- start_up_error = factory.make_exception()
227- self.patch_autospec(start_up, "start_up")
228- start_up.start_up.return_value = defer.fail(start_up_error)
229+ # This will cause an error to be raised in
230+ # WebApplicationService.prepareApplication().
231+ start_up_error = factory.make_exception_type()
232+ self.patch_autospec(webapp, "WebApplicationHandler")
233+ webapp.WebApplicationHandler.side_effect = start_up_error
234
235 # No error is returned.
236 service.startService()
237
238=== modified file 'src/maasserver/webapp.py'
239--- src/maasserver/webapp.py 2015-04-02 12:10:26 +0000
240+++ src/maasserver/webapp.py 2015-04-17 10:22:44 +0000
241@@ -32,7 +32,6 @@
242 from twisted.internet import reactor
243 from twisted.internet.threads import deferToThread
244 from twisted.python import log
245-from twisted.python.threadpool import ThreadPool
246 from twisted.web.resource import (
247 ErrorPage,
248 Resource,
249@@ -126,10 +125,10 @@
250 the web application.
251 """
252
253- def __init__(self, endpoint):
254+ def __init__(self, endpoint, threadpool):
255 self.site = Site(StartPage())
256 super(WebApplicationService, self).__init__(endpoint, self.site)
257- self.threadpool = ThreadPool(name=self.__class__.__name__)
258+ self.threadpool = threadpool
259 self.websocket = WebSocketFactory()
260
261 def prepareApplication(self):
262@@ -156,7 +155,6 @@
263 front-end configuration (i.e. Apache) so that there's no need to force
264 script names.
265 """
266-
267 root = Resource()
268 webapp = ResourceOverlay(
269 WSGIResource(reactor, self.threadpool, application))
270
271=== modified file 'src/maastesting/protractor/runner.py'
272--- src/maastesting/protractor/runner.py 2015-03-25 15:33:23 +0000
273+++ src/maastesting/protractor/runner.py 2015-04-17 10:22:44 +0000
274@@ -15,6 +15,7 @@
275 __all__ = [
276 ]
277
278+from functools import partial
279 import os
280 import signal
281 from subprocess import Popen
282@@ -101,10 +102,11 @@
283 "maas-regiond",
284 ]
285
286- # Change the DEFAULT_PORT so it can run along side of the
287- # development regiond.
288+ # Change the port that the webapp comes up on so it can run
289+ # alongside the development regiond.
290 from maasserver import eventloop
291- patch(eventloop, "DEFAULT_PORT", 5253)
292+ makeapp = partial(eventloop.make_WebApplicationService, port=5253)
293+ patch(eventloop, "make_WebApplicationService", makeapp)
294
295 # Start twistd.
296 try: