Merge ~pelpsi/launchpad:fetch-service-session-API into launchpad:master

[Simone Pelosi]

Add fetch service session API

Update BuilderProxyMixin class to switch between
Fetch Service and classic Builder Proxy depending on
use_fetch_service flag.
Add test suite for Fetch Service and fetchservice.py mock class
for API.
Add Fetch Service API information on schema-lazr.conf.

[Ines Almeida (ines-almeida)]

Good work - left a few comments!
I'll leave the approval to Jürgen who is more into the details of the whole project.

[Simone Pelosi (pelpsi)]

Thank you for the review Ines, I'm so sorry I forgot to push the commit in which I removed all the debugging stuff or useless comments :P

[Jürgen Gmach (jugmac00)]

Thanks a lot for the MP! We are on the right track!

As we went with the approach to handle both cases inside a single mixin, we really, really want to have a good class docstring to explain that there are two proxies involved.

Also we should add a comment or a paragraph in the docstring which describes the way forward, that at one time hopefully the fetch service will replace the builder proxy.

Also, we need to be careful with naming - do not copy paste naming from the builder proxy. We should align this with what the starcraft folks are using.

[Jürgen Gmach (jugmac00)]

Thanks! Looks good now!

I would ask you to revert the refactoring when reading the configuration, or at very least put the refactoring and the new features in two separate commits.

Also, I left again the question where "FetchServiceURLMatcher" is used?

[Jürgen Gmach (jugmac00)]

Looks good now - please at least separate the MP into a refactoring commit and one implementing the new feature.

review: Approve

diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index be65e29..1ac121f 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -20,7 +20,10 @@ from typing import Dict, Generator
 
 from twisted.internet import defer
 
-from lp.buildmaster.downloader import RequestProxyTokenCommand
+from lp.buildmaster.downloader import (
+    RequestFetchServiceSessionCommand,
+    RequestProxyTokenCommand,
+)
 from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import BuildArgs
 from lp.services.config import config
@@ -31,14 +34,26 @@ def _get_proxy_config(name):
     return getattr(config.builddmaster, name) or getattr(config.snappy, name)
 
 
+def _get_value_from_config(key: str):
+    value = _get_proxy_config(key)
+    if not value:
+        raise CannotBuild(f"{key} is not configured.")
+    return value
+
+
 class BuilderProxyMixin:
     """Methods for handling builds with the Snap Build Proxy enabled."""
 
     @defer.inlineCallbacks
     def addProxyArgs(
-        self, args: BuildArgs, allow_internet: bool = True
+        self,
+        args: BuildArgs,
+        allow_internet: bool = True,
+        fetch_service: bool = False,
     ) -> Generator[None, Dict[str, str], None]:
-        if _get_proxy_config("builder_proxy_host") and allow_internet:
+        if not allow_internet:
+            return
+        if not fetch_service and _get_proxy_config("builder_proxy_host"):
             token = yield self._requestProxyToken()
             args["proxy_url"] = (
                 "http://{username}:{password}@{host}:{port}".format(
@@ -52,6 +67,20 @@ class BuilderProxyMixin:
                 endpoint=_get_proxy_config("builder_proxy_auth_api_endpoint"),
                 token=token["username"],
             )
+        elif fetch_service and _get_proxy_config("fetch_service_host"):
+            session = yield self._requestFetchServiceSession()
+            args["proxy_url"] = (
+                "http://{session_id}:{token}@{host}:{port}".format(
+                    session_id=session["id"],
+                    token=session["token"],
+                    host=_get_proxy_config("fetch_service_host"),
+                    port=_get_proxy_config("fetch_service_port"),
+                )
+            )
+            args["revocation_endpoint"] = "{endpoint}/session/{id}".format(
+                endpoint=_get_proxy_config("fetch_service_control_endpoint"),
+                id=session["id"],
+            )
 
     @defer.inlineCallbacks
     def _requestProxyToken(self):
@@ -86,3 +115,25 @@ class BuilderProxyMixin:
             proxy_username=proxy_username,
         )
         return token
+
+    @defer.inlineCallbacks
+    def _requestFetchServiceSession(self):
+        admin_username = _get_value_from_config(
+            "fetch_service_control_admin_username"
+        )
+        secret = _get_value_from_config("fetch_service_control_admin_secret")
+        url = _get_value_from_config("fetch_service_control_endpoint")
+        timestamp = int(time.time())
+        proxy_username = "{build_id}-{timestamp}".format(
+            build_id=self.build.build_cookie, timestamp=timestamp
+        )
+        auth_string = f"{admin_username}:{secret}".strip()
+        auth_header = b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
+
+        session = yield self._worker.process_pool.doWork(
+            RequestFetchServiceSessionCommand,
+            url=url,
+            auth_header=auth_header,
+            proxy_username=proxy_username,
+        )
+        return session
diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
index fc16852..7cf1c9b 100644
--- a/lib/lp/buildmaster/downloader.py
+++ b/lib/lp/buildmaster/downloader.py
@@ -9,6 +9,7 @@ anything from the rest of Launchpad.
 
 __all__ = [
     "DownloadCommand",
+    "RequestFetchServiceSessionCommand",
     "RequestProcess",
     "RequestProxyTokenCommand",
 ]
@@ -37,6 +38,28 @@ class DownloadCommand(amp.Command):
     }
 
 
+class RequestFetchServiceSessionCommand(amp.Command):
+    """Fetch service API Command subclass
+
+    It defines arguments, response values, and error conditions.
+    For reference:
+    https://docs.twisted.org/en/twisted-18.7.0/core/howto/amp.html
+    """
+
+    arguments = [
+        (b"url", amp.Unicode()),
+        (b"auth_header", amp.String()),
+        (b"proxy_username", amp.Unicode()),
+    ]
+    response = [
+        (b"id", amp.Unicode()),
+        (b"token", amp.Unicode()),
+    ]
+    errors = {
+        RequestException: b"REQUEST_ERROR",
+    }
+
+
 class RequestProxyTokenCommand(amp.Command):
     arguments = [
         (b"url", amp.Unicode()),
@@ -94,3 +117,24 @@ class RequestProcess(AMPChild):
             )
             response.raise_for_status()
             return response.json()
+
+    @RequestFetchServiceSessionCommand.responder
+    def requestFetchServiceSessionCommand(
+        self, url, auth_header, proxy_username
+    ):
+        with Session() as session:
+            session.trust_env = False
+            # XXX pelpsi: from ST108 and from what Claudio
+            # said `timeout` and `policy` are not mandatory now:
+            # `timeout` will never be mandatory and we don't pass
+            # it as parameter to the call.
+            # `policy` could be mandatory or optional in future
+            # (assuming `strict` as default), so for now it's better
+            # to pass it explicitly and set it as `permissive`.
+            response = session.post(
+                url,
+                headers={"Authorization": auth_header},
+                json={"username": proxy_username, "policy": "permissive"},
+            )
+            response.raise_for_status()
+            return response.json()
diff --git a/lib/lp/buildmaster/tests/fetchservice.py b/lib/lp/buildmaster/tests/fetchservice.py
new file mode 100644
index 0000000..ea70651
--- /dev/null
+++ b/lib/lp/buildmaster/tests/fetchservice.py
@@ -0,0 +1,124 @@
+# Copyright 2015-2024 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Fixtures for dealing with the build time Fetch Service http proxy."""
+
+import json
+import uuid
+from textwrap import dedent
+from urllib.parse import urlsplit
+
+import fixtures
+from testtools.matchers import Equals, HasLength, MatchesStructure
+from twisted.internet import defer, endpoints, reactor
+from twisted.python.compat import nativeString
+from twisted.web import resource, server
+
+from lp.services.config import config
+
+
+class FetchServiceAuthAPITokensResource(resource.Resource):
+    """A test session resource for the fetch service authentication API."""
+
+    isLeaf = True
+
+    def __init__(self):
+        resource.Resource.__init__(self)
+        self.requests = []
+
+    def render_POST(self, request):
+        content = json.loads(request.content.read().decode("UTF-8"))
+        self.requests.append(
+            {
+                "method": request.method,
+                "uri": request.uri,
+                "headers": dict(request.requestHeaders.getAllRawHeaders()),
+                "json": content,
+            }
+        )
+        return json.dumps(
+            {
+                "id": "1",
+                "token": uuid.uuid4().hex,
+            }
+        ).encode("UTF-8")
+
+
+class InProcessFetchServiceAuthAPIFixture(fixtures.Fixture):
+    """A fixture that mocks the Fetch Service authentication API.
+
+    Users of this fixture must call the `start` method, which returns a
+    `Deferred`, and arrange for that to get back to the reactor.  This is
+    necessary because the basic fixture API does not allow `setUp` to return
+    anything.  For example:
+
+        class TestSomething(TestCase):
+
+            run_tests_with = AsynchronousDeferredRunTest.make_factory(
+                timeout=30)
+
+            @defer.inlineCallbacks
+            def setUp(self):
+                super().setUp()
+                yield self.useFixture(
+                    InProcessFetchServiceAuthAPIFixture()
+                ).start()
+    """
+
+    @defer.inlineCallbacks
+    def start(self):
+        root = resource.Resource()
+        self.sessions = FetchServiceAuthAPITokensResource()
+        root.putChild(b"sessions", self.sessions)
+        endpoint = endpoints.serverFromString(reactor, nativeString("tcp:0"))
+        site = server.Site(self.sessions)
+        self.addCleanup(site.stopFactory)
+        port = yield endpoint.listen(site)
+        self.addCleanup(port.stopListening)
+        config.push(
+            "in-process-fetch-service-api-fixture",
+            dedent(
+                """
+                [builddmaster]
+                fetch_service_control_admin_secret: admin-secret
+                fetch_service_control_admin_username: admin-launchpad.test
+                fetch_service_control_endpoint: http://{host}:{port}/session
+                fetch_service_host: {host}
+                fetch_service_port: {port}
+                """
+            ).format(host=port.getHost().host, port=port.getHost().port),
+        )
+        self.addCleanup(config.pop, "in-process-fetch-service-api-fixture")
+
+
+# This class is not used yet but it could be
+# useful for future tests
+class FetchServiceURLMatcher(MatchesStructure):
+    """Check that a string is a valid url for fetch service."""
+
+    def __init__(self):
+        super().__init__(
+            scheme=Equals("http"),
+            id=Equals(1),
+            token=HasLength(32),
+            hostname=Equals(config.builddmaster.fetch_service_host),
+            port=Equals(config.builddmaster.fetch_service_port),
+            path=Equals(""),
+        )
+
+    def match(self, matchee):
+        super().match(urlsplit(matchee))
+
+
+class RevocationEndpointMatcher(Equals):
+    """Check that a string is a valid endpoint for fetch
+    service session revocation.
+    """
+
+    def __init__(self, session_id):
+        super().__init__(
+            "{endpoint}/session/{session_id}".format(
+                endpoint=config.builddmaster.fetch_service_control_endpoint,
+                session_id=session_id,
+            )
+        )
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index ac68bd4..bbfe8b8 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -167,6 +167,24 @@ builder_proxy_host: none
 # Builder http proxy port
 builder_proxy_port: none
 
+# Admin secret for requesting sessions from the fetch service.
+# For a mojo deployed proxy service, the secret is generated and will reside in
+# /srv/mojo/${MOJO_PROJECT}/${SERIES}/${MOJO_WORKSPACE}/local/admin-api-secret
+fetch_service_control_admin_secret: none
+
+# Admin username for fetch service.
+fetch_service_control_admin_username: none
+
+# Endpoint for fetch service authentication service
+fetch_service_control_endpoint: none
+
+# Fetch service host, it could be either a single instance 
+# or a load balancer in front
+fetch_service_host: none
+
+# Fetch service port
+fetch_service_port: none
+
 [canonical]
 # datatype: boolean
 show_tracebacks: False
@@ -1854,6 +1872,24 @@ builder_proxy_host: none
 # Deprecated in favour of the same key in the builddmaster section.
 builder_proxy_port: none
 
+# Admin secret for requesting sessions from the fetch service.
+# For a mojo deployed proxy service, the secret is generated and will reside in
+# /srv/mojo/${MOJO_PROJECT}/${SERIES}/${MOJO_WORKSPACE}/local/admin-api-secret
+fetch_service_control_admin_secret: none
+
+# Admin username for fetch service.
+fetch_service_control_admin_username: none
+
+# Endpoint for fetch service authentication service
+fetch_service_control_endpoint: none
+
+# Fetch service host, it could be either a single instance 
+# or a load balancer in front
+fetch_service_host: none
+
+# Fetch service port
+fetch_service_port: none
+
 # Optional sources.list entry to send to build workers when doing snap
 # package builds.  Use this form so that the series is set:
 # deb http://foo %(series)s main
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index 66ae7cc..58fdd5f 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -116,7 +116,9 @@ class SnapBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
         """
         build: ISnapBuild = self.build
         args: BuildArgs = yield super().extraBuildArgs(logger=logger)
-        yield self.addProxyArgs(args, build.snap.allow_internet)
+        yield self.addProxyArgs(
+            args, build.snap.allow_internet, build.snap.use_fetch_service
+        )
         args["name"] = build.snap.store_name or build.snap.name
         channels = build.channels or {}
         if "snapcraft" not in channels:
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 5ec5804..2ccc18d 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -51,6 +51,9 @@ from lp.buildmaster.tests.builderproxy import (
     ProxyURLMatcher,
     RevocationEndpointMatcher,
 )
+from lp.buildmaster.tests.fetchservice import (
+    InProcessFetchServiceAuthAPIFixture,
+)
 from lp.buildmaster.tests.mock_workers import (
     MockBuilder,
     OkWorker,
@@ -72,6 +75,7 @@ from lp.services.statsd.tests import StatsMixin
 from lp.services.webapp import canonical_url
 from lp.snappy.interfaces.snap import (
     SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
+    SNAP_USE_FETCH_SERVICE_FEATURE_FLAG,
     SnapBuildArchiveOwnerMismatch,
 )
 from lp.snappy.model.snapbuildbehaviour import (
@@ -235,7 +239,163 @@ class TestSnapBuildBehaviour(TestSnapBuildBehaviourBase):
         self.assertIn("Missing chroot", str(e))
 
 
-class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
+class TestAsyncSnapBuildBehaviourFetchService(
+    StatsMixin, TestSnapBuildBehaviourBase
+):
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=30
+    )
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        super().setUp()
+        self.session = {
+            "id": "1",
+            "token": uuid.uuid4().hex,
+        }
+        self.fetch_service_url = (
+            "http://{session_id}:{token}@{host}:{port}".format(
+                session_id=self.session["id"],
+                token=self.session["token"],
+                host=config.builddmaster.fetch_service_host,
+                port=config.builddmaster.fetch_service_port,
+            )
+        )
+        self.fetch_service_api = self.useFixture(
+            InProcessFetchServiceAuthAPIFixture()
+        )
+        yield self.fetch_service_api.start()
+        self.now = time.time()
+        self.useFixture(fixtures.MockPatch("time.time", return_value=self.now))
+        self.addCleanup(shut_down_default_process_pool)
+        self.setUpStats()
+
+    def makeJob(self, **kwargs):
+        # We need a builder worker in these tests, in order that requesting
+        # a proxy token can piggyback on its reactor and pool.
+        job = super().makeJob(**kwargs)
+        builder = MockBuilder()
+        builder.processor = job.build.processor
+        worker = self.useFixture(WorkerTestHelpers()).getClientWorker()
+        job.setBuilder(builder, worker)
+        self.addCleanup(worker.pool.closeCachedConnections)
+        return job
+
+    @defer.inlineCallbacks
+    def test_requestFetchServiceSession_unconfigured(self):
+        """Create a snap build request with an incomplete fetch service
+        configuration.
+
+        If `fetch_service_host` is not provided the function will return
+        without populating `proxy_url` and `revocation_endpoint`.
+        """
+        self.pushConfig("builddmaster", fetch_service_host=None)
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+        )
+        snap = self.factory.makeSnap(use_fetch_service=True)
+        request = self.factory.makeSnapBuildRequest(snap=snap)
+        job = self.makeJob(snap=snap, build_request=request)
+        with dbuser(config.builddmaster.dbuser):
+            args = yield job.extraBuildArgs()
+        self.assertEqual([], self.fetch_service_api.sessions.requests)
+        self.assertNotIn("proxy_url", args)
+        self.assertNotIn("revocation_endpoint", args)
+
+    @defer.inlineCallbacks
+    def test_requestFetchServiceSession_no_secret(self):
+        """Create a snap build request with an incomplete fetch service
+        configuration.
+
+        If `fetch_service_control_admin_secret` is not provided
+        the function raises a `CannotBuild` error.
+        """
+        self.pushConfig(
+            "builddmaster", fetch_service_control_admin_secret=None
+        )
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+        )
+        snap = self.factory.makeSnap(use_fetch_service=True)
+        request = self.factory.makeSnapBuildRequest(snap=snap)
+        job = self.makeJob(snap=snap, build_request=request)
+        expected_exception_msg = (
+            "fetch_service_control_admin_secret is not configured."
+        )
+        with ExpectedException(CannotBuild, expected_exception_msg):
+            yield job.extraBuildArgs()
+
+    @defer.inlineCallbacks
+    def test_requestFetchServiceSession(self):
+        """Create a snap build request with a successful fetch service
+        configuration.
+
+        `proxy_url` and `revocation_endpoint` are correctly populated.
+        """
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+        )
+        snap = self.factory.makeSnap(use_fetch_service=True)
+        request = self.factory.makeSnapBuildRequest(snap=snap)
+        job = self.makeJob(snap=snap, build_request=request)
+        args = yield job.extraBuildArgs()
+        expected_uri = urlsplit(
+            config.builddmaster.fetch_service_control_endpoint
+        ).path.encode("UTF-8")
+        request_matcher = MatchesDict(
+            {
+                "method": Equals(b"POST"),
+                "uri": Equals(expected_uri),
+                "headers": ContainsDict(
+                    {
+                        b"Authorization": MatchesListwise(
+                            [
+                                Equals(
+                                    b"Basic "
+                                    + base64.b64encode(
+                                        b"admin-launchpad.test:admin-secret"
+                                    )
+                                )
+                            ]
+                        ),
+                        b"Content-Type": MatchesListwise(
+                            [
+                                Equals(b"application/json"),
+                            ]
+                        ),
+                    }
+                ),
+                "json": MatchesDict(
+                    {
+                        "username": StartsWith(job.build.build_cookie + "-"),
+                        "policy": Equals("permissive"),
+                    }
+                ),
+            }
+        )
+        self.assertThat(
+            self.fetch_service_api.sessions.requests,
+            MatchesListwise([request_matcher]),
+        )
+        self.assertIn("proxy_url", args)
+        self.assertIn("revocation_endpoint", args)
+
+    @defer.inlineCallbacks
+    def test_requestFetchServiceSession_flag_off(self):
+        """Create a snap build request turning off the feature flag."""
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: None})
+        )
+        snap = self.factory.makeSnap(use_fetch_service=True)
+        request = self.factory.makeSnapBuildRequest(snap=snap)
+        job = self.makeJob(snap=snap, build_request=request)
+        yield job.extraBuildArgs()
+        self.assertEqual([], self.fetch_service_api.sessions.requests)
+
+
+class TestAsyncSnapBuildBehaviourBuilderProxy(
+    StatsMixin, TestSnapBuildBehaviourBase
+):
     run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
         timeout=30
     )