Merge ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master

[Ines Almeida]

Enable revoking a token for the fetch service

Handle calling the revocation_endpoint sent by Launchpad, both in the cases where we want to revoke the token after the snap pull phase, and at the end of the build

This handles calling the revocation endpoint against the fetch service, but not yet closing the session (that will be handled in another MP: https://code.launchpad.net/~ines-almeida/launchpad-buildd/+git/launchpad-buildd/+merge/463119)

[Ines Almeida (ines-almeida)]

In a meeting, decided that we will most likely need to have 2 endpoints for ending the session:
 - One for revoking the token
 - Another one to close the session

This MP is blocked until we settle that out.

[Ines Almeida (ines-almeida)]

Given we will have 2 endpoints (one for token revocation, another for session closing), I decided to separate this into 2 MPs:
 - This one will handle adding the logic to call the revocation endpoint to the fetch service
 - The next one will handle closing the fetch service session and saving the data (to be opened)

[Ines Almeida (ines-almeida)]

This MP has been updated after some conversations. The topic of how to authenticate the call to revoke the token is still on not decided on, so that part might change.

[Jürgen Gmach (jugmac00)]

Thanks! I added some comments. This is almost done, but let's try to use consistent, descriptive and RFC compliant naming for the hosts.

Do you think we need a feature flag for this? I do not think so, but you might have more insight.

[Ines Almeida (ines-almeida)]

Thanks you for the comments!

I don't think we need feature flag here simply because the feature flag in the launchpad code base should already do the same. The fetch service code in buildd will only be reached if the snap object has a `use_fetch_service` bool True - which will only be possible if the launchpad feature flag is ON.

As mentioned, I am only planning on merging this once the auth for the token revocation is defined in the meeting about the auth.

[Jürgen Gmach (jugmac00)]

@Ines I would love to have this merged asap, as both my and Simone's work base on this, and having this pre-requisite MPs juggling around is pretty cumbersome.

The code path for the fetch service cannot be triggered yet, so I do not see any issues with merging this - even without the auth being specced out fully.

[Jürgen Gmach (jugmac00)]

review: Approve

44d08b2... by Ines Almeida on 2024-04-16

Fix small linting issue in test_snap file

a541938... by Ines Almeida on 2024-04-16

Enable revoking a token for the fetch service

Handle calling the revocation_endpoint sent by Launchpad, both in the cases where we want to revoke the token after the snap pull phase, and at the end of the build

[Ines Almeida (ines-almeida)]

Updated the commit history to separate the linting fix from everything else.

diff --git a/lpbuildd/proxy.py b/lpbuildd/proxy.py
index ea33dda..1a05ff3 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, so build systems, which do not comply
+        with standard `http(s)_proxy` environment variables, would
+        still work with the builder proxy.
+        """
         if not self.proxy_url:
             return []
         proxy_port = self._builder._config.get("builder", "proxyport")
@@ -231,7 +243,7 @@ class BuildManagerProxyMixin:
         return ["--proxy-url", f"http://{proxy_host}:{proxy_port}/"]
 
     def stopProxy(self):
-        """Stop the local builder proxy, if necessary."""
+        """Stop the internal local proxy (see `startProxy`), if necessary."""
         if self.proxy_service is None:
             return
         self.proxy_service.disownServiceParent()
@@ -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..82470d5 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 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..bb0af3a 100644
--- a/lpbuildd/tests/test_snap.py
+++ b/lpbuildd/tests/test_snap.py
@@ -283,7 +283,8 @@ class TestSnapBuildManagerIteration(TestCase):
             "/build/test-snap/test-snap_0_all.snap", b"I am a snap package."
         )
         self.buildmanager.backend.add_file(
-            "/build/test-snap/test-snap+somecomponent_0.comp", b"I am a component."
+            "/build/test-snap/test-snap+somecomponent_0.comp",
+            b"I am a component.",
         )
 
         # After building the package, reap processes.
@@ -754,6 +755,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 +882,29 @@ 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"
+
+        responses.add(
+            "DELETE",
+            f"http://control.fetch-service.example/{session_id}/token",
+        )
+
+        self.buildmanager.use_fetch_service = True
+        self.buildmanager.revocation_endpoint = (
+            f"http://control.fetch-service.example/{session_id}/token"
+        )
+        self.buildmanager.proxy_url = (
+            "http://session_id:[e-mail]"
+        )
+
+        self.buildmanager.revokeProxyToken()
+
+        self.assertEqual(1, len(responses.calls))
+        request = responses.calls[0].request
+        self.assertEqual(
+            f"http://control.fetch-service.example/{session_id}/token",
+            request.url,
+        )
diff --git a/lpbuildd/tests/test_util.py b/lpbuildd/tests/test_util.py
index 005d42b..ab65f5f 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,47 @@ 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 = "http://username:[e-mail]"
+        revocation_endpoint = "http://proxy-auth.example/tokens/build_id"
+        token = base64.b64encode(b"username:password").decode()
+
+        responses.add(responses.DELETE, revocation_endpoint)
+
+        revoke_proxy_token(proxy_url, revocation_endpoint)
+        self.assertEqual(1, len(responses.calls))
+        request = responses.calls[0].request
+        self.assertEqual(
+            "http://proxy-auth.example/tokens/build_id", request.url
+        )
+        self.assertEqual(f"Basic {token}", request.headers["Authorization"])
+
+    @responses.activate
+    def test_revoke_fetch_service_token(self):
+        """Proxy token revocation for the fetch service"""
+
+        proxy_url = "http://session_id:[e-mail]"
+        revocation_endpoint = (
+            "http://control.fetch-service.example/session_id/token"
+        )
+
+        responses.add(responses.DELETE, revocation_endpoint)
+
+        revoke_proxy_token(
+            proxy_url,
+            revocation_endpoint,
+            use_fetch_service=True,
+        )
+
+        self.assertEqual(1, len(responses.calls))
+        request = responses.calls[0].request
+        self.assertEqual(
+            "http://control.fetch-service.example/session_id/token",
+            request.url,
+        )
diff --git a/lpbuildd/util.py b/lpbuildd/util.py
index eb35f01..664f92b 100644
--- a/lpbuildd/util.py
+++ b/lpbuildd/util.py
@@ -68,15 +68,34 @@ 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.
 
+    If not using the fetch service:
+        The proxy_url for the current Builder Proxy has the following format:
+        http://{username}:{password}@{host}:{port}
+
+        We use the username-password combo from the proxy_url for
+        authentication to revoke its token.
+
+    If using the fetch service:
+        The call to revoke a token does not require authentication.
+
+        XXX ines-almeida 2024-04-15: this might change depending on
+        conversations about fetch service authentication. We might decide to
+        instead use the token itself as the authentication.
+
     :raises RevokeProxyTokenError: if attempting to revoke the token failed.
     """
     url = urlparse(proxy_url)
+
+    auth = None
+    if not use_fetch_service:
+        auth = (url.username, url.password)
+
     try:
-        requests.delete(
-            revocation_endpoint, auth=(url.username, url.password), timeout=15
-        )
+        requests.delete(revocation_endpoint, auth=auth, timeout=15)
     except requests.RequestException as e:
         raise RevokeProxyTokenError(url.username, e)