Merge lp:~blr/launchpad-buildd/snap_http_proxy into lp:launchpad-buildd

[Kit Randel]

Add http_proxy to buildsnap args.

Add http_proxy environment variable to buildsnap args, providing time limited external access. This branch also factors out buildsnap into distinct repo, pull, build and token revocation phases.

[lp:~blr/launchpad-buildd/snap_http_proxy]

175. By Kit Randel on 2015-10-04

Add http_proxy to buildsnap args.

[Colin Watson (cjwatson)]

It sounds like the direction we're going is indeed going to be to provide external access for the whole duration of the build, but could you please get in touch with Martin Albisetti to confirm that that's definitely what we have to do rather than separating out the pull phase and providing external access only to that as we'd previously intended?

review: Approve

[Kit Randel (blr)]

Thanks Colin, I'll catch up with Martin and make those changes.

On Mon, Oct 5, 2015 at 11:50 AM Colin Watson <email address hidden> wrote:

> Review: Approve
>
> It sounds like the direction we're going is indeed going to be to provide
> external access for the whole duration of the build, but could you please
> get in touch with Martin Albisetti to confirm that that's definitely what
> we have to do rather than separating out the pull phase and providing
> external access only to that as we'd previously intended?
>
> Diff comments:
>
> > === modified file 'lpbuildd/snap.py'
> > --- lpbuildd/snap.py 2015-07-31 11:54:07 +0000
> > +++ lpbuildd/snap.py 2015-10-04 22:12:07 +0000
> > @@ -52,6 +56,15 @@
> > "--build-id", self._buildid,
> > "--arch", self.arch_tag,
> > ]
> > + if self.proxy_token:
>
> Maybe worth checking here that all the necessary arguments were passed,
> just in case.
>
> > + proxy_env = (
> > + "http_proxy=http://{username}:{password}"
> > + "@{host}:{port}".format(
> > + username=self.build_cookie,
> > + password=self.proxy_token,
> > + host=self.proxy_host,
> > + port=self.proxy_port))
> > + args.insert(0, proxy_env)
>
> Rather than relying on shell parsing of environment variable prefixes to
> the command, could you please instead add an env=None keyword argument to
> runSubProcess, defaulting to os.environ if None, so that you can then do
> something like:
>
> env = dict(os.environ)
> if condition:
> env["http_proxy"] = blah
> ...
> self.runSubProcess(self.build_snap_path, args, env=env)
>
> > if self.branch is not None:
> > args.extend(["--branch", self.branch])
> > if self.git_repository is not None:
>
>
> --
>
> https://code.launchpad.net/~blr/launchpad-buildd/snap_http_proxy/+merge/273355
> You are the owner of lp:~blr/launchpad-buildd/snap_http_proxy.
>

[William Grant (wgrant)]

Rather than encoding in launchpad-buildd the knowledge that the username is the build cookie, I think it would make more sense for buildd-manager to simply pass in the full URL.

[lp:~blr/launchpad-buildd/snap_http_proxy]

176. By Kit Randel on 2015-10-05

* Pass env to runSubProcess.
* Expect proxy_url in build extra_args, rather than proxy_username, proxy_password, proxy_host and proxy_port.

177. By Kit Randel on 2015-10-05

* Add env (os.environ) kwarg to runSubProcess.
* Fix broken os.environ set.
* Fix associated snap test.

178. By Kit Randel on 2015-10-08

* Pass proxy_url as argument to snapcraft script.
* Add pull phase to snapcraft builder.

179. By Kit Randel on 2016-01-24

Merge devel.

180. By Kit Randel on 2016-01-28

Correct buildsnap proxy-url option.

181. By Kit Randel on 2016-01-29

Move repo collect into distinct phase.

182. By Kit Randel on 2016-01-29

Add doctring for repo.

183. By Kit Randel on 2016-01-29

Add debugging.

184. By Kit Randel on 2016-02-01

Add token revocation.

185. By Kit Randel on 2016-02-01

Reset ssl_verify.

186. By Kit Randel on 2016-02-01

Remove debugging.

187. By Kit Randel on 2016-02-02

Move revoke_token to finally block.

[Colin Watson (cjwatson)]

The auth token logging issue mentioned in comments must be fixed, but aside from that it's just minor adjustments and is fine to land. Thanks.

review: Needs Fixing

[lp:~blr/launchpad-buildd/snap_http_proxy]

188. By Kit Randel on 2016-02-03

Remove debugging print statement.

189. By Kit Randel on 2016-02-04

Sanitize proxy url in snap build logs.

[Colin Watson (cjwatson)]

review: Approve

[lp:~blr/launchpad-buildd/snap_http_proxy]

190. By Kit Randel on 2016-02-04

Set https_proxy envvar.

191. By Kit Randel on 2016-02-05

Ensure token revocation is conditioned on presense of key in extra_args.

=== modified file 'buildsnap'
--- buildsnap	2015-09-23 22:06:16 +0000
+++ buildsnap	2016-02-05 00:18:10 +0000
@@ -8,11 +8,14 @@
 
 __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,
@@ -85,6 +88,7 @@
         self.chroot(["/bin/sh", "-c", command], echo=echo)
 
     def install(self):
+        print("Running install phase...")
         deps = ["snapcraft"]
         if self.options.branch is not None:
             deps.append("bzr")
@@ -92,10 +96,15 @@
             deps.append("git")
         self.chroot(["apt-get", "-y", "install"] + deps)
 
-    def build(self):
+    def repo(self):
+        """Collect git or bzr branch."""
+        print("Running repo phase...")
         if self.options.branch is not None:
-            self.run_build_command([
-                "bzr", "branch", self.options.branch, self.name])
+            self.run_build_command(['ls', '/build'])
+            cmd = ["bzr", "branch", self.options.branch, self.name]
+            if not self.ssl_verify:
+                cmd.insert(1, "-Ossl.cert_reqs=none")
+            self.run_build_command(cmd)
         else:
             assert self.options.git_repository is not None
             assert self.options.git_path is not None
@@ -107,9 +116,40 @@
                 "git", "clone", "-b", self.options.git_path,
                 self.options.git_repository, self.name],
                 env=env)
-        self.run_build_command(
-            ["snapcraft", "assemble"], path=os.path.join("/build", self.name),
-            env={"SNAPCRAFT_LOCAL_SOURCES": "1"})
+
+    def pull(self):
+        """Run pull phase."""
+        print("Running pull phase...")
+        env = {"SNAPCRAFT_LOCAL_SOURCES": "1"}
+        if self.options.proxy_url:
+            env["http_proxy"] = self.options.proxy_url
+            https_url = self.options.proxy_url.replace('http://', 'https://')
+            env["https_proxy"] = https_url
+        self.run_build_command(
+            ["snapcraft", "pull"],
+            path=os.path.join("/build", self.name),
+            env=env)
+
+    def build(self):
+        """Run all build, stage and snap phases."""
+        print("Running build phase...")
+        self.run_build_command(
+            ["snapcraft", "all"], path=os.path.join("/build", self.name))
+
+    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:
+            print('Unable to revoke token for %s' % url.username)
 
 
 def main():
@@ -125,6 +165,9 @@
     parser.add_option(
         "--git-path", metavar="REF-PATH",
         help="build from this ref path in REPOSITORY")
+    parser.add_option("--proxy-url", help="builder proxy url")
+    parser.add_option("--revocation-endpoint",
+                      help="builder proxy token revocation endpoint")
     options, args = parser.parse_args()
     if (options.git_repository is None) != (options.git_path is None):
         parser.error(
@@ -143,10 +186,15 @@
         traceback.print_exc()
         return RETCODE_FAILURE_INSTALL
     try:
+        builder.repo()
+        builder.pull()
         builder.build()
     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 'lpbuildd/slave.py'
--- lpbuildd/slave.py	2015-11-04 13:35:36 +0000
+++ lpbuildd/slave.py	2016-02-05 00:18:10 +0000
@@ -123,7 +123,11 @@
         self.home = os.environ['HOME']
         self.abort_timeout = 120
 
-    def runSubProcess(self, command, args, iterate=None):
+    @property
+    def needs_sanitized_logs(self):
+        return self.is_archive_private
+
+    def runSubProcess(self, command, args, iterate=None, env=None):
         """Run a sub process capturing the results in the log."""
         if iterate is None:
             iterate = self.iterate
@@ -131,7 +135,7 @@
         self._slave.log("RUN: %s %r\n" % (command, args))
         childfds = {0: devnull.fileno(), 1: "r", 2: "r"}
         self._reactor.spawnProcess(
-            self._subprocess, command, args, env=os.environ,
+            self._subprocess, command, args, env=env,
             path=self.home, childFDs=childfds)
 
     def doUnpack(self):
@@ -165,7 +169,7 @@
 
         # Sanitize the URLs in the buildlog file if this is a build
         # in a private archive.
-        if self.is_archive_private:
+        if self.needs_sanitized_logs:
             self._slave.sanitizeBuildlog(self._slave.cachePath("buildlog"))
 
     def doMounting(self):
@@ -492,7 +496,7 @@
             if rlog is not None:
                 rlog.close()
 
-        if self.manager.is_archive_private:
+        if self.manager.needs_sanitized_logs:
             # This is a build in a private archive. We need to scrub
             # the URLs contained in the buildlog excerpt in order to
             # avoid leaking passwords.

=== modified file 'lpbuildd/snap.py'
--- lpbuildd/snap.py	2015-07-31 11:54:07 +0000
+++ lpbuildd/snap.py	2016-02-05 00:18:10 +0000
@@ -31,6 +31,10 @@
         super(SnapBuildManager, self).__init__(slave, buildid, **kwargs)
         self.build_snap_path = os.path.join(self._slavebin, "buildsnap")
 
+    @property
+    def needs_sanitized_logs(self):
+        return True
+
     def initiate(self, files, chroot, extra_args):
         """Initiate a build with a given set of files and chroot."""
         self.build_path = get_build_path(
@@ -42,6 +46,8 @@
         self.branch = extra_args.get("branch")
         self.git_repository = extra_args.get("git_repository")
         self.git_path = extra_args.get("git_path")
+        self.proxy_url = extra_args.get("proxy_url")
+        self.revocation_endpoint = extra_args.get("revocation_endpoint")
 
         super(SnapBuildManager, self).initiate(files, chroot, extra_args)
 
@@ -52,6 +58,10 @@
             "--build-id", self._buildid,
             "--arch", self.arch_tag,
             ]
+        if self.proxy_url:
+            args.extend(["--proxy-url", self.proxy_url])
+        if self.revocation_endpoint:
+            args.extend(["--revocation-endpoint", self.revocation_endpoint])
         if self.branch is not None:
             args.extend(["--branch", self.branch])
         if self.git_repository is not None:

=== modified file 'lpbuildd/tests/test_snap.py'
--- lpbuildd/tests/test_snap.py	2015-07-31 11:54:07 +0000
+++ lpbuildd/tests/test_snap.py	2016-02-05 00:18:10 +0000
@@ -22,7 +22,7 @@
         self.commands = []
         self.iterators = []
 
-    def runSubProcess(self, path, command, iterate=None):
+    def runSubProcess(self, path, command, iterate=None, env=None):
         self.commands.append([path] + command)
         if iterate is None:
             iterate = self.iterate