launchpad-reviewers team mailing list archive

[Ines Almeida <mp+463145@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-add-get-data-endpoint into launchpad:master.

Commit message:
Send end_session_endpoint to build manager when using fetch service
    
The fetch service will require 2 endpoints: one for revoking the token (as the old builder proxy did) and one for ending the session and gathering the data - that we are adding in this MP

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/463145
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-add-get-data-endpoint into launchpad:master.

diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index 3375042..9309246 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -77,10 +77,10 @@ class BuilderProxyMixin:
                     port=_get_proxy_config("fetch_service_port"),
                 )
             )
-            args["revocation_endpoint"] = "{endpoint}/{session_id}".format(
-                endpoint=_get_proxy_config("fetch_service_control_endpoint"),
-                session_id=session["id"],
-            )
+            endpoint = _get_proxy_config("fetch_service_control_endpoint")
+            session_id = session["id"]
+            args["revocation_endpoint"] = f"{endpoint}/{session_id}/token"
+            args["end_session_endpoint"] = f"{endpoint}/{session_id}"
 
     @defer.inlineCallbacks
     def _requestProxyToken(self):
diff --git a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
index 6a808b3..2afb876 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
@@ -128,6 +128,10 @@ class BuildArgs(TypedDict, total=False):
     # The URL for revoking proxy authorization tokens [charm, ci, oci,
     # snap].
     revocation_endpoint: str
+    # The URL for ending a fetch service session (separate from the
+    # 'revocation_endpoint, as is certain cases want to revoke the token before
+    # closing the session)
+    end_session_endpoint: str
     # If True, scan job output for malware [ci].
     scan_malware: bool
     # A dictionary of secrets to pass to the CI build runner [ci].
diff --git a/lib/lp/buildmaster/tests/builderproxy.py b/lib/lp/buildmaster/tests/builderproxy.py
index fece785..1f47c71 100644
--- a/lib/lp/buildmaster/tests/builderproxy.py
+++ b/lib/lp/buildmaster/tests/builderproxy.py
@@ -10,7 +10,12 @@ from textwrap import dedent
 from urllib.parse import urlsplit
 
 import fixtures
-from testtools.matchers import Equals, HasLength, MatchesStructure
+from testtools.matchers import (
+    Equals,
+    HasLength,
+    MatchesRegex,
+    MatchesStructure,
+)
 from twisted.internet import defer, endpoints, reactor
 from twisted.python.compat import nativeString
 from twisted.web import resource, server
@@ -120,3 +125,26 @@ class RevocationEndpointMatcher(Equals):
                 int(now),
             )
         )
+
+
+class FetchServiceRevocationEndpointMatcher(MatchesRegex):
+    """Check that a string is a valid endpoint for proxy token revocation.
+
+    Expected type: {endpoint}/{session_id:int}/token
+    """
+
+    def __init__(self):
+        endpoint = config.builddmaster.fetch_service_control_endpoint
+        super().__init__(r"%s/\d/token" % (endpoint))
+
+
+class FetchServiceEndSessionEndpointMatcher(MatchesRegex):
+    """Check that a string is a valid endpoint for the fetch service session
+    closing.
+
+    Expected type: {endpoint}/{session_id:int}
+    """
+
+    def __init__(self):
+        endpoint = config.builddmaster.fetch_service_control_endpoint
+        super().__init__(r"%s/\d" % (endpoint))
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 2ccc18d..efab726 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -47,6 +47,8 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
 )
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.tests.builderproxy import (
+    FetchServiceEndSessionEndpointMatcher,
+    FetchServiceRevocationEndpointMatcher,
     InProcessProxyAuthAPIFixture,
     ProxyURLMatcher,
     RevocationEndpointMatcher,
@@ -287,7 +289,8 @@ class TestAsyncSnapBuildBehaviourFetchService(
         configuration.
 
         If `fetch_service_host` is not provided the function will return
-        without populating `proxy_url` and `revocation_endpoint`.
+        without populating `proxy_url`, `revocation_endpoint` and
+        `end_session_endpoint`.
         """
         self.pushConfig("builddmaster", fetch_service_host=None)
         self.useFixture(
@@ -301,6 +304,7 @@ class TestAsyncSnapBuildBehaviourFetchService(
         self.assertEqual([], self.fetch_service_api.sessions.requests)
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("end_session_endpoint", args)
 
     @defer.inlineCallbacks
     def test_requestFetchServiceSession_no_secret(self):
@@ -330,7 +334,8 @@ class TestAsyncSnapBuildBehaviourFetchService(
         """Create a snap build request with a successful fetch service
         configuration.
 
-        `proxy_url` and `revocation_endpoint` are correctly populated.
+        `proxy_url`, `revocation_endpoint` and `end_session_endpoint` are
+        correctly populated.
         """
         self.useFixture(
             FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
@@ -379,6 +384,15 @@ class TestAsyncSnapBuildBehaviourFetchService(
         )
         self.assertIn("proxy_url", args)
         self.assertIn("revocation_endpoint", args)
+        self.assertThat(
+            args["revocation_endpoint"],
+            FetchServiceRevocationEndpointMatcher(),
+        )
+        self.assertIn("end_session_endpoint", args)
+        self.assertThat(
+            args["end_session_endpoint"],
+            FetchServiceEndSessionEndpointMatcher(),
+        )
 
     @defer.inlineCallbacks
     def test_requestFetchServiceSession_flag_off(self):
@@ -465,6 +479,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
         self.assertEqual([], self.proxy_api.tokens.requests)
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("end_session_endpoint", args)
 
     @defer.inlineCallbacks
     def test_requestProxyToken_no_secret(self):
@@ -1321,6 +1336,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
             args = yield job.extraBuildArgs()
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("revocation_endpoint", args)
 
     @defer.inlineCallbacks
     def test_extraBuildArgs_build_source_tarball(self):