launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21472
[Merge] lp:~cjwatson/launchpad-buildd/local-snap-proxy into lp:launchpad-buildd
Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/local-snap-proxy into lp:launchpad-buildd.
Commit message:
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.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/local-snap-proxy/+merge/322545
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/local-snap-proxy into lp:launchpad-buildd.
=== modified file 'buildd-genconfig'
--- buildd-genconfig 2016-12-09 18:04:00 +0000
+++ buildd-genconfig 2017-04-13 17:26:26 +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-04-13 17:26:26 +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 'buildsnap'
--- buildsnap 2017-02-10 14:53:12 +0000
+++ buildsnap 2017-04-13 17:26:26 +0000
@@ -8,14 +8,11 @@
__metaclass__ = type
-import base64
from optparse import OptionParser
import os
import subprocess
import sys
import traceback
-import urllib2
-from urlparse import urlparse
from lpbuildd.util import (
set_personality,
@@ -148,21 +145,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")
@@ -204,9 +186,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 'debian/changelog'
--- debian/changelog 2017-02-10 14:55:42 +0000
+++ debian/changelog 2017-04-13 17:26:26 +0000
@@ -1,3 +1,12 @@
+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 <cjwatson@xxxxxxxxxx> Thu, 13 Apr 2017 18:02:20 +0100
+
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-04-13 17:26:26 +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-04-13 17:26:26 +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 2016-08-30 12:47:03 +0000
+++ lpbuildd/snap.py 2017-04-13 17:26:26 +0000
@@ -3,8 +3,38 @@
__metaclass__ = type
+import base64
+import io
import os
import shutil
+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,
@@ -18,6 +48,208 @@
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
+ if self._command == b"CONNECT":
+ return False
+ else:
+ return http.HTTPChannel.checkPersistence(self, request, version)
+
+ 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"
@@ -48,9 +280,45 @@
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)
+ 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))
+
def doRunBuild(self):
"""Run the process to build the snap."""
args = [
@@ -58,8 +326,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:
@@ -73,6 +340,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 2016-08-30 12:47:03 +0000
+++ lpbuildd/tests/test_snap.py 2017-04-13 17:26:26 +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()
@@ -163,3 +181,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-04-13 17:26:26 +0000
@@ -12,3 +12,6 @@
[translationtemplatesmanager]
resultarchive = translation-templates.tar.gz
+
+[snapmanager]
+proxyport = @SNAPPROXYPORT@
Follow ups
