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.

Unmerged revisions

217. By Colin Watson on 2017-08-04

Remove dead code.

216. By Colin Watson on 2017-04-13

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.

=== modified file 'bin/buildsnap'
--- bin/buildsnap	2017-07-28 11:15:51 +0000
+++ bin/buildsnap	2017-08-04 11:03:23 +0000
@@ -8,15 +8,16 @@
 
 __metaclass__ = type
 
+<<<<<<< TREE
 import base64
 import json
+=======
+>>>>>>> MERGE-SOURCE
 from optparse import OptionParser
 import os
 import subprocess
 import sys
 import traceback
-import urllib2
-from urlparse import urlparse
 
 from lpbuildd.util import (
     set_personality,
@@ -200,21 +201,6 @@
         self.run_build_command(
             ["snapcraft"], path=os.path.join("/build", self.name), env=env)
 
-    def revoke_token(self):
-        """Revoke builder proxy token."""
-        print("Revoking proxy token...")
-        url = urlparse(self.options.proxy_url)
-        auth = '{}:{}'.format(url.username, url.password)
-        headers = {
-            'Authorization': 'Basic {}'.format(base64.b64encode(auth))
-            }
-        req = urllib2.Request(self.options.revocation_endpoint, None, headers)
-        req.get_method = lambda: 'DELETE'
-        try:
-            urllib2.urlopen(req)
-        except (urllib2.HTTPError, urllib2.URLError) as e:
-            print('Unable to revoke token for %s: %s' % (url.username, e))
-
 
 def main():
     parser = OptionParser("%prog [options] NAME")
@@ -255,9 +241,6 @@
     except Exception:
         traceback.print_exc()
         return RETCODE_FAILURE_BUILD
-    finally:
-        if options.revocation_endpoint is not None:
-            builder.revoke_token()
     return RETCODE_SUCCESS
 
 

=== modified file 'buildd-genconfig'
--- buildd-genconfig	2016-12-09 18:04:00 +0000
+++ buildd-genconfig	2017-08-04 11:03:23 +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 'buildd-slave.tac'
--- buildd-slave.tac	2015-07-31 11:54:07 +0000
+++ buildd-slave.tac	2017-08-04 11:03:23 +0000
@@ -48,6 +48,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 'debian/changelog'
--- debian/changelog	2017-08-03 13:13:08 +0000
+++ debian/changelog	2017-08-04 11:03:23 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 launchpad-buildd (148) UNRELEASED; urgency=medium
 
   * Move the contents of /usr/share/launchpad-buildd/slavebin/ into bin/ in
@@ -60,6 +61,17 @@
 
  -- Colin Watson <[e-mail]>  Fri, 12 May 2017 16:47:57 +0100
 
+=======
+launchpad-buildd (143) UNRELEASED; urgency=medium
+
+  * 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 <[e-mail]>  Thu, 13 Apr 2017 18:02:20 +0100
+
+>>>>>>> MERGE-SOURCE
 launchpad-buildd (142) trusty; urgency=medium
 
   * lpbuildd.binarypackage: Pass DEB_BUILD_OPTIONS=noautodbgsym if we have

=== modified file 'debian/postinst'
--- debian/postinst	2015-11-25 15:00:43 +0000
+++ debian/postinst	2017-08-04 11:03:23 +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	2017-08-04 11:03:23 +0000
@@ -197,6 +197,17 @@
     in_file.close()
     out_file.close()
 
+def upgrade_to_143():
+    print("Upgrading %s to version 143" % conf_file)
+    os.rename(conf_file, conf_file + "-prev143~")
+
+    with open(conf_file + "-prev143~", "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, "143") < 0:
+        upgrade_to_143()

=== modified file 'lpbuildd/snap.py'
--- lpbuildd/snap.py	2017-04-03 12:30:15 +0000
+++ lpbuildd/snap.py	2017-08-04 11:03:23 +0000
@@ -5,10 +5,46 @@
 
 __metaclass__ = type
 
+<<<<<<< TREE
 import json
+=======
+import base64
+import io
+>>>>>>> MERGE-SOURCE
 import os
 import shutil
+<<<<<<< TREE
 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
+>>>>>>> MERGE-SOURCE
 
 from lpbuildd.debian import (
     DebianBuildManager,
@@ -22,6 +58,204 @@
 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
+        # Hack for compatibility with the old version of Twisted that
+        # Launchpad currently uses, for the benefit of tests.
+        try:
+            from twisted.web.http import _escape
+            self._log_escape = _escape
+        except ImportError:
+            self._log_escape = self._escape
+
+    def log(self, request):
+        # Log requests to the build log rather than to Twisted.
+        # Reimplement log formatting partly to make it easier to stay
+        # compatible with the old version of Twisted that Launchpad
+        # currently uses, and partly because there's no point logging the IP
+        # here.
+        referrer = self._log_escape(request.getHeader(b"referer") or b"-")
+        agent = self._log_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': self._log_escape(request.method),
+                'uri': self._log_escape(request.uri),
+                'protocol': self._log_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"
 
@@ -52,9 +286,11 @@
         self.git_path = extra_args.get("git_path")
         self.proxy_url = extra_args.get("proxy_url")
         self.revocation_endpoint = extra_args.get("revocation_endpoint")
+        self.proxy_service = None
 
         super(SnapBuildManager, self).initiate(files, chroot, extra_args)
 
+<<<<<<< TREE
     def status(self):
         status_path = get_build_path(self.home, self._buildid, "status")
         try:
@@ -68,6 +304,43 @@
                 file=sys.stderr)
         return {}
 
+=======
+    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)
+        return ["--proxy-url", "http://localhost:{}/".format(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))
+
+>>>>>>> MERGE-SOURCE
     def doRunBuild(self):
         """Run the process to build the snap."""
         args = [
@@ -75,8 +348,7 @@
             "--build-id", self._buildid,
             "--arch", self.arch_tag,
             ]
-        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:
@@ -90,6 +362,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/tests/test_snap.py'
--- lpbuildd/tests/test_snap.py	2017-07-28 13:57:47 +0000
+++ lpbuildd/tests/test_snap.py	2017-08-04 11:03:23 +0000
@@ -8,10 +8,25 @@
 import tempfile
 
 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
 
@@ -32,6 +47,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 = tempfile.mkdtemp()
@@ -174,3 +192,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	2017-08-04 11:03:23 +0000
@@ -12,3 +12,6 @@
 
 [translationtemplatesmanager]
 resultarchive = translation-templates.tar.gz
+
+[snapmanager]
+proxyport = @SNAPPROXYPORT@