Merge lp:~cjwatson/launchpad-buildd/local-snap-proxy into lp:launchpad-buildd

[Colin Watson]

Add a local unauthenticated proxy on port 8222, which proxies through to
the remote authenticated proxy. This should allow running a wider range
of network clients, since some of them apparently don't support
authenticated proxies very well.

[Colin Watson (cjwatson)]

See also https://bugs.launchpad.net/snapcraft/+bug/1682534.

[Adam Collard (adam-collard)]

drive by review

[lp:~cjwatson/launchpad-buildd/local-snap-proxy]

217. By Colin Watson on 2017-08-04

Remove dead code.

[Kevin W Monroe (kwmonroe)]

Aside from the merge conflicts, the spirit of this gets a big +1 from me. I'm running into all sorts of trouble with java projects (mvn, ant, ivy, gradle) and their poor support for auth'd proxies. I think an unauth'd alternative would be super useful.

[lp:~cjwatson/launchpad-buildd/local-snap-proxy]

218. By Colin Watson on 2018-02-27

Merge trunk.

219. By Colin Watson on 2018-02-27

Drop now-unnecessary compatibility with Twisted < 14.0.0.

[Brett Sutton (bsutton)]

Just to add my voice.
This is currently blocking a number of apps I'm trying to build in the store plus anyone that uses a remote part for Apache Tomcat with SSL that I've built.

[William Grant (wgrant)]

This seems to work fine with some manual testing.

However, this won't directly work for snap builds any more: that --proxy-url is going to point to the container rather than the host.

review: Approve (code)

[lp:~cjwatson/launchpad-buildd/local-snap-proxy]

220. By Colin Watson on 2018-06-05

Merge trunk.

221. By Colin Watson on 2018-06-07

Build-depend on curl for proxy tests.

222. By Colin Watson on 2018-06-07

Fix version in upgrade-config.

223. By Colin Watson on 2018-06-07

Remove duplicate --proxy-url option.

224. By Colin Watson on 2018-06-07

Point --proxy-url to the LXD host.

225. By Colin Watson on 2018-06-10

Close LP #1690834 and #1753340.

=== modified file 'buildd-genconfig'
--- buildd-genconfig	2016-12-09 18:04:00 +0000
+++ buildd-genconfig	2018-06-10 12:47:59 +0000
@@ -37,6 +37,11 @@
                   metavar="FILE",
                   default="/usr/share/launchpad-buildd/template-buildd-slave.conf")
 
+parser.add_option("--snap-proxy-port", dest="SNAPPROXYPORT",
+                  help="the port the local snap proxy binds to",
+                  metavar="PORT",
+                  default="8222")
+
 (options, args) = parser.parse_args()
 
 template = open(options.TEMPLATE, "r").read()
@@ -46,6 +51,7 @@
     "@BINDHOST@": options.BINDHOST,
     "@ARCHTAG@": options.ARCHTAG,
     "@BINDPORT@": options.BINDPORT,
+    "@SNAPPROXYPORT@": options.SNAPPROXYPORT,
     }
 
 for replacement_key in replacements:
@@ -53,4 +59,3 @@
                                 replacements[replacement_key])
 
 print(template.strip())
-
 
=== modified file 'debian/changelog'
--- debian/changelog	2018-06-05 02:06:04 +0000
+++ debian/changelog	2018-06-10 12:47:59 +0000
@@ -10,6 +10,10 @@
   * Refactor VCS operations from lpbuildd.target.build_snap out to a module
     that can be used by other targets.
   * Allow checking out a git tag rather than a branch (LP: #1687078).
+  * Add a local unauthenticated proxy on port 8222, which proxies through to
+    the remote authenticated proxy.  This should allow running a wider range
+    of network clients, since some of them apparently don't support
+    authenticated proxies very well (LP: #1690834, #1753340).
 
  -- Colin Watson <[e-mail]>  Tue, 08 May 2018 10:36:22 +0100
 
 
=== modified file 'debian/control'
--- debian/control	2017-11-27 17:14:19 +0000
+++ debian/control	2018-06-10 12:47:59 +0000
@@ -5,6 +5,7 @@
 Standards-Version: 3.9.5
 Build-Depends: apt-utils,
                bzr,
+               curl,
                debhelper (>= 9~),
                dh-exec,
                dh-python,
 
=== modified file 'debian/postinst'
--- debian/postinst	2017-08-29 13:19:35 +0000
+++ debian/postinst	2018-06-10 12:47:59 +0000
@@ -14,7 +14,7 @@
 
 make_buildd()
 {
- /usr/share/launchpad-buildd/buildd-genconfig --name=default --host=0.0.0.0 --port=8221 > \
+ /usr/share/launchpad-buildd/buildd-genconfig --name=default --host=0.0.0.0 --port=8221 --snap-proxy-port=8222 > \
   /etc/launchpad-buildd/default
  echo Default buildd created.
 }
 
=== modified file 'debian/upgrade-config'
--- debian/upgrade-config	2016-12-09 18:05:21 +0000
+++ debian/upgrade-config	2018-06-10 12:47:59 +0000
@@ -197,6 +197,17 @@
     in_file.close()
     out_file.close()
 
+def upgrade_to_162():
+    print("Upgrading %s to version 162" % conf_file)
+    os.rename(conf_file, conf_file + "-prev162~")
+
+    with open(conf_file + "-prev162~", "r") as in_file:
+        with open(conf_file, "w") as out_file:
+            out_file.write(in_file.read())
+            out_file.write(
+                "\n[snapmanager]\n"
+                "proxyport = 8222\n")
+
 if __name__ == "__main__":
     old_version = re.sub(r'[~-].*', '', old_version)
     if apt_pkg.version_compare(old_version, "12") < 0:
@@ -223,3 +234,5 @@
         upgrade_to_126()
     if apt_pkg.version_compare(old_version, "127") < 0:
         upgrade_to_127()
+    if apt_pkg.version_compare(old_version, "162") < 0:
+        upgrade_to_162()
 
=== modified file 'lpbuildd/buildd-slave.tac'
--- lpbuildd/buildd-slave.tac	2018-02-27 13:25:36 +0000
+++ lpbuildd/buildd-slave.tac	2018-06-10 12:47:59 +0000
@@ -51,6 +51,7 @@
 application.addComponent(
     RotatableFileLogObserver(options.get('logfile')), ignoreClass=1)
 builddslaveService = service.IServiceCollection(application)
+slave.slave.service = builddslaveService
 
 root = resource.Resource()
 root.putChild('rpc', slave)
 
=== modified file 'lpbuildd/snap.py'
--- lpbuildd/snap.py	2018-04-21 10:04:37 +0000
+++ lpbuildd/snap.py	2018-06-10 12:47:59 +0000
@@ -5,9 +5,39 @@
 
 __metaclass__ = type
 
+import base64
+import io
 import json
 import os
 import sys
+try:
+    from urllib.error import (
+        HTTPError,
+        URLError,
+        )
+    from urllib.parse import urlparse
+    from urllib.request import (
+        Request,
+        urlopen,
+        )
+except ImportError:
+    from urllib2 import (
+        HTTPError,
+        Request,
+        URLError,
+        urlopen,
+        )
+    from urlparse import urlparse
+
+from twisted.application import strports
+from twisted.internet import reactor
+from twisted.internet.interfaces import IHalfCloseableProtocol
+from twisted.python.compat import intToBytes
+from twisted.web import (
+    http,
+    proxy,
+    )
+from zope.interface import implementer
 
 from lpbuildd.debian import (
     DebianBuildManager,
@@ -21,6 +51,195 @@
 RETCODE_FAILURE_BUILD = 201
 
 
+class SnapProxyClient(proxy.ProxyClient):
+
+    def __init__(self, command, rest, version, headers, data, father):
+        proxy.ProxyClient.__init__(
+            self, command, rest, version, headers, data, father)
+        # Why doesn't ProxyClient at least store this?
+        self.version = version
+        # We must avoid calling self.father.finish in the event that its
+        # connection was already lost, i.e. if the original client
+        # disconnects first (which is particularly likely in the case of
+        # CONNECT).
+        d = self.father.notifyFinish()
+        d.addBoth(self.requestFinished)
+
+    def connectionMade(self):
+        proxy.ProxyClient.connectionMade(self)
+        self.father.setChildClient(self)
+
+    def sendCommand(self, command, path):
+        # For some reason, HTTPClient.sendCommand doesn't preserve the
+        # protocol version.
+        self.transport.writeSequence(
+            [command, b' ', path, b' ', self.version, b'\r\n'])
+
+    def handleEndHeaders(self):
+        self.father.handleEndHeaders()
+
+    def sendData(self, data):
+        self.transport.write(data)
+
+    def endData(self):
+        if self.transport is not None:
+            self.transport.loseWriteConnection()
+
+    def requestFinished(self, result):
+        self._finished = True
+        self.transport.loseConnection()
+
+
+class SnapProxyClientFactory(proxy.ProxyClientFactory):
+
+    protocol = SnapProxyClient
+
+
+class SnapProxyRequest(http.Request):
+
+    child_client = None
+    _request_buffer = None
+    _request_data_done = False
+
+    def setChildClient(self, child_client):
+        self.child_client = child_client
+        if self._request_buffer is not None:
+            self.child_client.sendData(self._request_buffer.getvalue())
+            self._request_buffer = None
+        if self._request_data_done:
+            self.child_client.endData()
+
+    def allHeadersReceived(self, command, path, version):
+        # Normally done in `requestReceived`, but we disable that since it
+        # does other things we don't want.
+        self.method, self.uri, self.clientproto = command, path, version
+        self.client = self.channel.transport.getPeer()
+        self.host = self.channel.transport.getHost()
+
+        remote_parsed = urlparse(self.channel.factory.remote_url)
+        request_parsed = urlparse(path)
+        headers = self.getAllHeaders().copy()
+        if b"host" not in headers and request_parsed.netloc:
+            headers[b"host"] = request_parsed.netloc
+        if remote_parsed.username:
+            auth = (remote_parsed.username + ":" +
+                    remote_parsed.password).encode("ASCII")
+            authHeader = b"Basic " + base64.b64encode(auth)
+            headers[b"proxy-authorization"] = authHeader
+        self.client_factory = SnapProxyClientFactory(
+            command, path, version, headers, b"", self)
+        reactor.connectTCP(
+            remote_parsed.hostname, remote_parsed.port, self.client_factory)
+
+    def requestReceived(self, command, path, version):
+        # We do most of our work in `allHeadersReceived` instead.
+        pass
+
+    def rawDataReceived(self, data):
+        if self.child_client is not None:
+            if not self._request_data_done:
+                self.child_client.sendData(data)
+        else:
+            if self._request_buffer is None:
+                self._request_buffer = io.BytesIO()
+            self._request_buffer.write(data)
+
+    def handleEndHeaders(self):
+        # Cut-down version of Request.write.  We must avoid switching to
+        # chunked encoding for the sake of CONNECT; since our actual
+        # response data comes from another proxy, we can cut some corners.
+        if self.startedWriting:
+            return
+        self.startedWriting = 1
+        l = []
+        l.append(
+            self.clientproto + b" " + intToBytes(self.code) + b" " +
+            self.code_message + b"\r\n")
+        for name, values in self.responseHeaders.getAllRawHeaders():
+            for value in values:
+                l.extend([name, b": ", value, b"\r\n"])
+        l.append(b"\r\n")
+        self.transport.writeSequence(l)
+
+    def write(self, data):
+        if self.channel is not None:
+            self.channel.resetTimeout()
+        http.Request.write(self, data)
+
+    def endData(self):
+        if self.child_client is not None:
+            self.child_client.endData()
+        self._request_data_done = True
+
+
+@implementer(IHalfCloseableProtocol)
+class SnapProxy(http.HTTPChannel):
+    """A channel that streams request data.
+
+    The stock HTTPChannel isn't quite suitable for our needs, because it
+    expects to read the entire request data before passing control to the
+    request.  This doesn't work well for CONNECT.
+    """
+
+    requestFactory = SnapProxyRequest
+
+    def checkPersistence(self, request, version):
+        # ProxyClient.__init__ forces "Connection: close".
+        return False
+
+    def allHeadersReceived(self):
+        http.HTTPChannel.allHeadersReceived(self)
+        self.requests[-1].allHeadersReceived(
+            self._command, self._path, self._version)
+        if self._command == b"CONNECT":
+            # This is a lie, but we don't want HTTPChannel to decide that
+            # the request is finished just because a CONNECT request
+            # (naturally) has no Content-Length.
+            self.length = -1
+
+    def rawDataReceived(self, data):
+        self.resetTimeout()
+        if self.requests:
+            self.requests[-1].rawDataReceived(data)
+
+    def readConnectionLost(self):
+        for request in self.requests:
+            request.endData()
+
+    def writeConnectionLost(self):
+        pass
+
+
+class SnapProxyFactory(http.HTTPFactory):
+
+    protocol = SnapProxy
+
+    def __init__(self, manager, remote_url, *args, **kwargs):
+        http.HTTPFactory.__init__(self, *args, **kwargs)
+        self.manager = manager
+        self.remote_url = remote_url
+
+    def log(self, request):
+        # Log requests to the build log rather than to Twisted.
+        # Reimplement log formatting because there's no point logging the IP
+        # here.
+        referrer = http._escape(request.getHeader(b"referer") or b"-")
+        agent = http._escape(request.getHeader(b"user-agent") or b"-")
+        line = (
+            u'%(timestamp)s "%(method)s %(uri)s %(protocol)s" '
+            u'%(code)d %(length)s "%(referrer)s" "%(agent)s"\n' % {
+                'timestamp': self._logDateTime,
+                'method': http._escape(request.method),
+                'uri': http._escape(request.uri),
+                'protocol': http._escape(request.clientproto),
+                'code': request.code,
+                'length': request.sentLength or "-",
+                'referrer': referrer,
+                'agent': agent,
+                })
+        self.manager._slave.log(line.encode("UTF-8"))
+
+
 class SnapBuildState(DebianBuildState):
     BUILD_SNAP = "BUILD_SNAP"
 
@@ -47,9 +266,49 @@
         self.revocation_endpoint = extra_args.get("revocation_endpoint")
         self.build_source_tarball = extra_args.get(
             "build_source_tarball", False)
+        self.proxy_service = None
 
         super(SnapBuildManager, self).initiate(files, chroot, extra_args)
 
+    def startProxy(self):
+        """Start the local snap proxy, if necessary."""
+        if not self.proxy_url:
+            return []
+        proxy_port = self._slave._config.get("snapmanager", "proxyport")
+        proxy_factory = SnapProxyFactory(self, self.proxy_url, timeout=60)
+        self.proxy_service = strports.service(proxy_port, proxy_factory)
+        self.proxy_service.setServiceParent(self._slave.service)
+        if self.backend_name == "lxd":
+            proxy_host = self.backend.ipv4_network.ip
+        else:
+            proxy_host = "localhost"
+        return ["--proxy-url", "http://{}:{}/".format(proxy_host, proxy_port)]
+
+    def stopProxy(self):
+        """Stop the local snap proxy, if necessary."""
+        if self.proxy_service is None:
+            return
+        self.proxy_service.disownServiceParent()
+        self.proxy_service = None
+
+    def revokeProxyToken(self):
+        """Revoke builder proxy token."""
+        if not self.revocation_endpoint:
+            return
+        self._slave.log("Revoking proxy token...\n")
+        url = urlparse(self.proxy_url)
+        auth = "{}:{}".format(url.username, url.password)
+        headers = {
+            "Authorization": "Basic {}".format(base64.b64encode(auth))
+            }
+        req = Request(self.revocation_endpoint, None, headers)
+        req.get_method = lambda: "DELETE"
+        try:
+            urlopen(req)
+        except (HTTPError, URLError) as e:
+            self._slave.log(
+                "Unable to revoke token for %s: %s" % (url.username, e))
+
     def status(self):
         status_path = get_build_path(self.home, self._buildid, "status")
         try:
@@ -78,8 +337,7 @@
                 file=sys.stderr)
         if self.build_url:
             args.extend(["--build-url", self.build_url])
-        if self.proxy_url:
-            args.extend(["--proxy-url", self.proxy_url])
+        args.extend(self.startProxy())
         if self.revocation_endpoint:
             args.extend(["--revocation-endpoint", self.revocation_endpoint])
         if self.branch is not None:
@@ -95,6 +353,8 @@
 
     def iterate_BUILD_SNAP(self, retcode):
         """Finished building the snap."""
+        self.stopProxy()
+        self.revokeProxyToken()
         if retcode == RETCODE_SUCCESS:
             self.gatherResults()
             print("Returning build status: OK")
 
=== modified file 'lpbuildd/target/build_snap.py'
--- lpbuildd/target/build_snap.py	2018-06-05 02:06:04 +0000
+++ lpbuildd/target/build_snap.py	2018-06-10 12:47:59 +0000
@@ -5,30 +5,11 @@
 
 __metaclass__ = type
 
-import base64
 from collections import OrderedDict
 import json
 import logging
 import os.path
 import sys
-try:
-    from urllib.error import (
-        HTTPError,
-        URLError,
-        )
-    from urllib.request import (
-        Request,
-        urlopen,
-        )
-    from urllib.parse import urlparse
-except ImportError:
-    from urllib2 import (
-        HTTPError,
-        Request,
-        URLError,
-        urlopen,
-        )
-    from urlparse import urlparse
 
 from lpbuildd.target.operation import Operation
 from lpbuildd.target.vcs import VCSOperationMixin
@@ -206,21 +187,6 @@
             cwd=os.path.join("/build", self.args.name),
             env=env)
 
-    def revoke_token(self):
-        """Revoke builder proxy token."""
-        logger.info("Revoking proxy token...")
-        url = urlparse(self.args.proxy_url)
-        auth = '{}:{}'.format(url.username, url.password)
-        headers = {
-            'Authorization': 'Basic {}'.format(base64.b64encode(auth))
-            }
-        req = Request(self.args.revocation_endpoint, None, headers)
-        req.get_method = lambda: 'DELETE'
-        try:
-            urlopen(req)
-        except (HTTPError, URLError):
-            logger.exception('Unable to revoke token for %s', url.username)
-
     def run(self):
         try:
             self.install()
@@ -234,7 +200,4 @@
         except Exception:
             logger.exception('Build failed')
             return RETCODE_FAILURE_BUILD
-        finally:
-            if self.args.revocation_endpoint is not None:
-                self.revoke_token()
         return 0
 
=== modified file 'lpbuildd/tests/test_snap.py'
--- lpbuildd/tests/test_snap.py	2018-04-21 10:04:00 +0000
+++ lpbuildd/tests/test_snap.py	2018-06-10 12:47:59 +0000
@@ -10,10 +10,25 @@
     TempDir,
     )
 from testtools import TestCase
+from testtools.content import text_content
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.internet import (
+    defer,
+    reactor,
+    utils,
+    )
+from twisted.web import (
+    http,
+    proxy,
+    resource,
+    server,
+    static,
+    )
 
 from lpbuildd.snap import (
     SnapBuildManager,
     SnapBuildState,
+    SnapProxyFactory,
     )
 from lpbuildd.tests.fakeslave import FakeSlave
 from lpbuildd.tests.matchers import HasWaitingFiles
@@ -35,6 +50,9 @@
 
 class TestSnapBuildManagerIteration(TestCase):
     """Run SnapBuildManager through its iteration steps."""
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
     def setUp(self):
         super(TestSnapBuildManagerIteration, self).setUp()
         self.working_dir = self.useFixture(TempDir()).path
@@ -229,3 +247,64 @@
         self.assertEqual(
             self.buildmanager.iterate, self.buildmanager.iterators[-1])
         self.assertFalse(self.slave.wasCalled("buildFail"))
+
+    def getListenerURL(self, listener):
+        port = listener.getHost().port
+        return b"http://localhost:%d/" % port
+
+    def startFakeRemoteEndpoint(self):
+        remote_endpoint = resource.Resource()
+        remote_endpoint.putChild("a", static.Data("a" * 1024, "text/plain"))
+        remote_endpoint.putChild("b", static.Data("b" * 65536, "text/plain"))
+        remote_endpoint_listener = reactor.listenTCP(
+            0, server.Site(remote_endpoint))
+        self.addCleanup(remote_endpoint_listener.stopListening)
+        return remote_endpoint_listener
+
+    def startFakeRemoteProxy(self):
+        remote_proxy_factory = http.HTTPFactory()
+        remote_proxy_factory.protocol = proxy.Proxy
+        remote_proxy_listener = reactor.listenTCP(0, remote_proxy_factory)
+        self.addCleanup(remote_proxy_listener.stopListening)
+        return remote_proxy_listener
+
+    def startLocalProxy(self, remote_url):
+        proxy_factory = SnapProxyFactory(
+            self.buildmanager, remote_url, timeout=60)
+        proxy_listener = reactor.listenTCP(0, proxy_factory)
+        self.addCleanup(proxy_listener.stopListening)
+        return proxy_listener
+
+    @defer.inlineCallbacks
+    def assertCommandSuccess(self, command, extra_env=None):
+        env = os.environ
+        if extra_env is not None:
+            env.update(extra_env)
+        out, err, code = yield utils.getProcessOutputAndValue(
+            command[0], command[1:], env=env, path=".")
+        if code != 0:
+            self.addDetail("stdout", text_content(out))
+            self.addDetail("stderr", text_content(err))
+            self.assertEqual(0, code)
+        defer.returnValue(out)
+
+    @defer.inlineCallbacks
+    def test_fetch_via_proxy(self):
+        remote_endpoint_listener = self.startFakeRemoteEndpoint()
+        remote_endpoint_url = self.getListenerURL(remote_endpoint_listener)
+        remote_proxy_listener = self.startFakeRemoteProxy()
+        proxy_listener = self.startLocalProxy(
+            self.getListenerURL(remote_proxy_listener))
+        out = yield self.assertCommandSuccess(
+            [b"curl", remote_endpoint_url + b"a"],
+            extra_env={b"http_proxy": self.getListenerURL(proxy_listener)})
+        self.assertEqual("a" * 1024, out)
+        out = yield self.assertCommandSuccess(
+            [b"curl", remote_endpoint_url + b"b"],
+            extra_env={b"http_proxy": self.getListenerURL(proxy_listener)})
+        self.assertEqual("b" * 65536, out)
+
+    # XXX cjwatson 2017-04-13: We should really test the HTTPS case as well,
+    # but it's hard to see how to test that in a way that's independent of
+    # the code under test since the stock twisted.web.proxy doesn't support
+    # CONNECT.
 
=== modified file 'template-buildd-slave.conf'
--- template-buildd-slave.conf	2015-05-11 06:09:19 +0000
+++ template-buildd-slave.conf	2018-06-10 12:47:59 +0000
@@ -12,3 +12,6 @@
 
 [translationtemplatesmanager]
 resultarchive = translation-templates.tar.gz
+
+[snapmanager]
+proxyport = @SNAPPROXYPORT@