launchpad-reviewers team mailing list archive

[Ines Almeida <mp+462875@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master.

Commit message:
WIP: Update end-of-session code for snaps that use the fetch service

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad-buildd/+git/launchpad-buildd/+merge/462875

This is WIP code with some TODOs and open questions to answer.
The goal is to get early feedback and direction.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master.

diff --git a/lpbuildd/proxy.py b/lpbuildd/proxy.py
index ea33dda..b56d890 100644
--- a/lpbuildd/proxy.py
+++ b/lpbuildd/proxy.py
@@ -214,8 +214,20 @@ class BuilderProxyFactory(http.HTTPFactory):
 
 
 class BuildManagerProxyMixin:
+
+    @property
+    def _use_fetch_service(self):
+        return (
+            hasattr(self, 'use_fetch_service')
+            and getattr(self, "use_fetch_service")
+        )
+
     def startProxy(self):
-        """Start the local builder proxy, if necessary."""
+        """Start the local builder proxy, if necessary.
+
+        This starts an internal proxy that stands before the Builder
+        Proxy/Fetch Service. It is important for certain build types.
+        """
         if not self.proxy_url:
             return []
         proxy_port = self._builder._config.get("builder", "proxyport")
@@ -243,6 +255,10 @@ class BuildManagerProxyMixin:
             return
         self._builder.log("Revoking proxy token...\n")
         try:
-            revoke_proxy_token(self.proxy_url, self.revocation_endpoint)
+            revoke_proxy_token(
+                self.proxy_url,
+                self.revocation_endpoint,
+                self._use_fetch_service,
+            )
         except RevokeProxyTokenError as e:
             self._builder.log(f"{e}\n")
diff --git a/lpbuildd/snap.py b/lpbuildd/snap.py
index 02390d4..c5b3205 100644
--- a/lpbuildd/snap.py
+++ b/lpbuildd/snap.py
@@ -38,6 +38,7 @@ class SnapBuildManager(BuildManagerProxyMixin, DebianBuildManager):
         self.branch = extra_args.get("branch")
         self.git_repository = extra_args.get("git_repository")
         self.git_path = extra_args.get("git_path")
+        self.use_fetch_service = extra_args.get("use_fetch_service")
         self.proxy_url = extra_args.get("proxy_url")
         self.revocation_endpoint = extra_args.get("revocation_endpoint")
         self.build_source_tarball = extra_args.get(
@@ -100,6 +101,8 @@ class SnapBuildManager(BuildManagerProxyMixin, DebianBuildManager):
         if self.target_architectures:
             for arch in self.target_architectures:
                 args.extend(["--target-arch", arch])
+        if self.use_fetch_service:
+            args.append("--use_fetch_service")
         args.append(self.name)
         self.runTargetSubProcess("buildsnap", *args)
 
diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index d9052f2..d649fad 100644
--- a/lpbuildd/target/build_snap.py
+++ b/lpbuildd/target/build_snap.py
@@ -102,6 +102,12 @@ class BuildSnap(
             action="store_true",
             help="disable proxy access after the pull phase has finished",
         )
+        parser.add_argument(
+            "--use_fetch_service",
+            default=False,
+            action="store_true",
+            help="use the fetch service instead of the old builder proxy",
+        )
         parser.add_argument("name", help="name of snap to build")
 
     def install_svn_servers(self):
@@ -231,7 +237,9 @@ class BuildSnap(
             logger.info("Revoking proxy token...")
             try:
                 revoke_proxy_token(
-                    self.args.upstream_proxy_url, self.args.revocation_endpoint
+                    self.args.upstream_proxy_url,
+                    self.args.revocation_endpoint,
+                    self.args.use_fetch_service,
                 )
             except RevokeProxyTokenError as e:
                 logger.info(str(e))
diff --git a/lpbuildd/tests/test_snap.py b/lpbuildd/tests/test_snap.py
index 9bef60a..fb72924 100644
--- a/lpbuildd/tests/test_snap.py
+++ b/lpbuildd/tests/test_snap.py
@@ -754,6 +754,13 @@ class TestSnapBuildManagerIteration(TestCase):
         yield self.startBuild(args, expected_options)
 
     @defer.inlineCallbacks
+    def test_iterate_use_fetch_service(self):
+        # The build manager can be told to use the fetch service as its proxy.
+        args = {"use_fetch_service": True}
+        expected_options = ["--use_fetch_service"]
+        yield self.startBuild(args, expected_options)
+
+    @defer.inlineCallbacks
     def test_iterate_disable_proxy_after_pull(self):
         self.builder._config.set("builder", "proxyport", "8222")
         args = {
@@ -874,3 +881,32 @@ class TestSnapBuildManagerIteration(TestCase):
         # XXX cjwatson 2023-02-07: Ideally we'd check the timeout as well,
         # but the version of responses in Ubuntu 20.04 doesn't store it
         # anywhere we can get at it.
+
+    @responses.activate
+    def test_revokeProxyToken_fetch_service(self):
+        session_id = "123"
+        token = "test_token"
+
+        responses.add(
+            "DELETE", f"http://fetch-service.example/{session_id}/token";
+        )
+
+        self.buildmanager.backend_name = "fake"
+        self.buildmanager.initiate(
+            {}, "chroot.tar.gz", {"name": "test-snap", "series": "xenial"}
+        )
+        self.buildmanager.use_fetch_service = True
+        self.buildmanager.revocation_endpoint = (
+            f"http://fetch-service.example/{session_id}/token";
+        )
+        self.buildmanager.proxy_url = f"http://session_id:{token}@proxy.example";
+
+        self.buildmanager.revokeProxyToken()
+
+        self.assertEqual(1, len(responses.calls))
+        request = responses.calls[0].request
+        self.assertEqual(f"Basic {token}", request.headers["Authorization"])
+        self.assertEqual(
+            f"http://fetch-service.example/{session_id}/token";, request.url
+        )
+    
diff --git a/lpbuildd/tests/test_util.py b/lpbuildd/tests/test_util.py
index 005d42b..bfa48c3 100644
--- a/lpbuildd/tests/test_util.py
+++ b/lpbuildd/tests/test_util.py
@@ -1,9 +1,17 @@
 # Copyright 2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import base64
+import responses
+
 from testtools import TestCase
 
-from lpbuildd.util import get_arch_bits, set_personality, shell_escape
+from lpbuildd.util import (
+    get_arch_bits,
+    revoke_proxy_token,
+    set_personality,
+    shell_escape,
+)
 
 
 class TestShellEscape(TestCase):
@@ -59,3 +67,41 @@ class TestSetPersonality(TestCase):
             ["linux64", "sbuild"],
             set_personality(["sbuild"], "amd64", series="trusty"),
         )
+
+
+class TestRevokeToken(TestCase):
+
+    @responses.activate
+    def test_revoke_proxy_token(self):
+        """Proxy token revocation uses the right authentication"""
+
+        proxy_url = "https://username:password@host:port";
+        revocation_endpoint = "https://builder.api.endpoint.token";
+        token = base64.b64encode(b"username:password").decode()
+
+        responses.add(responses.DELETE, revocation_endpoint)
+
+        response = revoke_proxy_token(proxy_url, revocation_endpoint)
+        self.assertEqual(
+            f"Basic {token}", response.request.headers["Authorization"]
+        )
+
+    @responses.activate
+    def test_revoke_fetch_service_token(self):
+        """Proxy token revocation for the fetch service, uses the right
+        authentication and returns metadata """
+
+        token = "test-token"
+        proxy_url = f"https://session_id:{token}@host:port";
+        revocation_endpoint = "https://builder.api.endpoint/session_id";
+
+        responses.add(responses.DELETE, revocation_endpoint)
+
+        response = revoke_proxy_token(
+            proxy_url,
+            revocation_endpoint,
+            use_fetch_service=True,
+        )
+        self.assertEqual(
+            f"Basic {token}", response.request.headers["Authorization"]
+        )
diff --git a/lpbuildd/util.py b/lpbuildd/util.py
index eb35f01..fa99ddd 100644
--- a/lpbuildd/util.py
+++ b/lpbuildd/util.py
@@ -1,6 +1,7 @@
 # Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import base64
 import os
 import subprocess
 import sys
@@ -68,15 +69,37 @@ class RevokeProxyTokenError(Exception):
         return f"Unable to revoke token for {self.username}: {self.exception}"
 
 
-def revoke_proxy_token(proxy_url, revocation_endpoint):
+def revoke_proxy_token(proxy_url, revocation_endpoint, use_fetch_service=False):
     """Revoke builder proxy token.
 
+    The proxy_url for the current Builder Proxy:
+    http://{username}:{password}@{host}:{port}
+
+    We use the username-password combo for authentication.
+
+    The proxy_url for the fetch service:
+    http://{session_id}:{token}@{host}:{port}
+
+    We use the token for authentication.
+
     :raises RevokeProxyTokenError: if attempting to revoke the token failed.
     """
     url = urlparse(proxy_url)
+
+    # For the fetch service URL case, the {session_id}:{token} will be parsed
+    # as `username` and `password` respectively
+    if use_fetch_service:
+        token = url.password
+    else:
+        token = base64.b64encode(
+            f"{url.username}:{url.password}".encode()
+        ).decode()
+
     try:
         requests.delete(
-            revocation_endpoint, auth=(url.username, url.password), timeout=15
+            revocation_endpoint,
+            headers={'Authorization': f'Basic {token}'},
+            timeout=15,
         )
     except requests.RequestException as e:
         raise RevokeProxyTokenError(url.username, e)