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..5b3d576 100644
--- a/lpbuildd/proxy.py
+++ b/lpbuildd/proxy.py
@@ -3,6 +3,7 @@
 
 import base64
 import io
+import os
 from urllib.parse import urlparse
 
 from twisted.application import strports
@@ -214,8 +215,21 @@ 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."""
+
+        if self._use_fetch_service:
+            # TODO what to do if it is using the fetch service?
+            return []
+
         if not self.proxy_url:
             return []
         proxy_port = self._builder._config.get("builder", "proxyport")
@@ -232,6 +246,11 @@ class BuildManagerProxyMixin:
 
     def stopProxy(self):
         """Stop the local builder proxy, if necessary."""
+
+        if self._use_fetch_service:
+            # TODO what to do if it is using the fetch service?
+            return
+
         if self.proxy_service is None:
             return
         self.proxy_service.disownServiceParent()
@@ -243,6 +262,21 @@ class BuildManagerProxyMixin:
             return
         self._builder.log("Revoking proxy token...\n")
         try:
-            revoke_proxy_token(self.proxy_url, self.revocation_endpoint)
+            response = revoke_proxy_token(
+                self.proxy_url,
+                self.revocation_endpoint,
+                self._use_fetch_service,
+            )
         except RevokeProxyTokenError as e:
             self._builder.log(f"{e}\n")
+            return
+
+        # When using the fetch service, we want to retrieve and store the
+        # metadata we receive at the end of the session.
+        # This file will be uploaded when we run the `gatherResults` method
+        if self._use_fetch_service:
+            output_path = os.path.join(
+                "/build", self.name, "fetch-service-metadata.json"
+            )
+            with self.backend.open(output_path, mode="wb") as f:
+                f.write(response.content)
diff --git a/lpbuildd/snap.py b/lpbuildd/snap.py
index 02390d4..765bdbf 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)
 
@@ -141,7 +144,14 @@ class SnapBuildManager(BuildManagerProxyMixin, DebianBuildManager):
                 # `.comp` files are the binary result of building snap
                 # components, see spec SD149.
                 if entry.endswith(
-                    (".snap", ".manifest", ".debug", ".dpkg.yaml", ".comp")
+                    (
+                        ".snap",
+                        ".manifest",
+                        ".debug",
+                        ".dpkg.yaml",
+                        ".comp",
+                        ".json",
+                    )
                 ):
                     self.addWaitingFileFromBackend(path)
         if self.build_source_tarball:
diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index d9052f2..ef74958 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):
@@ -227,9 +233,18 @@ class BuildSnap(
             self.args.disable_proxy_after_pull
             and self.args.upstream_proxy_url
             and self.args.revocation_endpoint
+            and not self.args.use_fetch_service
         ):
             logger.info("Revoking proxy token...")
             try:
+                # TODO Should we revoke the proxy token even when we are
+                # using the fetch service here? If there were 2 revocation
+                # endpoints (one for revoking the token, another to end the
+                # session and get the metadata), then we would revoke the token
+                # here, and only end the session later. Given currently, it's
+                # only one endpoint, then we would closing the session and
+                # getting the metadata here, which doesn't seem right.
+                # Note: regardless of the 
                 revoke_proxy_token(
                     self.args.upstream_proxy_url, self.args.revocation_endpoint
                 )
diff --git a/lpbuildd/tests/test_snap.py b/lpbuildd/tests/test_snap.py
index 9bef60a..bd5fe12 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,33 @@ 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}";
+        )
+
+        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}";
+        )
+        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}";, request.url
+        )
+    
+    #TODO add tests about the writting of the metadata file for fetch service
diff --git a/lpbuildd/tests/test_util.py b/lpbuildd/tests/test_util.py
index 005d42b..ea3d8e4 100644
--- a/lpbuildd/tests/test_util.py
+++ b/lpbuildd/tests/test_util.py
@@ -1,9 +1,12 @@
 # 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 +62,44 @@ 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 """
+
+        proxy_url = f"https://session_id:test-token@host:port";
+        revocation_endpoint = "https://builder.api.endpoint/session_id";
+        response_metadata = {"metadata": "test"}
+
+        responses.add(
+            responses.DELETE, revocation_endpoint, json=response_metadata
+        )
+
+        response = revoke_proxy_token(
+            proxy_url,
+            revocation_endpoint,
+            use_fetch_service=True,
+        )
+        self.assertEqual(
+            f"Basic test-token", response.request.headers["Authorization"]
+        )
+        self.assertEqual(response_metadata, response.json())
diff --git a/lpbuildd/util.py b/lpbuildd/util.py
index eb35f01..e3188b1 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
+        return requests.delete(
+            revocation_endpoint,
+            headers={'Authorization': f'Basic {token}'},
+            timeout=15,
         )
     except requests.RequestException as e:
         raise RevokeProxyTokenError(url.username, e)