launchpad-reviewers team mailing list archive

[Ines Almeida <mp+464684@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master.

Commit message:
Refactor buildd-manager: remove unneeded variables, rename and add comments 

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464684
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master.

diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index a2b8f71..304c258 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -58,7 +58,7 @@ class BuilderProxyMixin:
         use_fetch_service: bool = False,
     ) -> Generator[None, Dict[str, str], None]:
 
-        self.proxy_service = None
+        self._proxy_service = None
 
         if not allow_internet:
             return
@@ -80,10 +80,10 @@ class BuilderProxyMixin:
             # non-production environments.
             return
 
-        self.proxy_service = proxy_service(
+        self._proxy_service = proxy_service(
             build_id=self.build.build_cookie, worker=self._worker
         )
-        new_session = yield self.proxy_service.startSession()
+        new_session = yield self._proxy_service.startSession()
         args["proxy_url"] = new_session["proxy_url"]
         args["revocation_endpoint"] = new_session["revocation_endpoint"]
 
@@ -181,7 +181,7 @@ class FetchService(IProxyService):
     """
 
     PROXY_URL = "http://{session_id}:{token}@{proxy_endpoint}";
-    START_SESSION_ENDPOINT = "{control_endpoint}"
+    START_SESSION_ENDPOINT = "{control_endpoint}/session"
     TOKEN_REVOCATION = "{control_endpoint}/session/{session_id}/token"
     RETRIEVE_METADATA_ENDPOINT = "{control_endpoint}/session/{session_id}"
     END_SESSION_ENDPOINT = "{control_endpoint}/session/{session_id}"
@@ -217,18 +217,12 @@ class FetchService(IProxyService):
 
         See IProxyService.
         """
-        timestamp = int(time.time())
-        proxy_username = "{build_id}-{timestamp}".format(
-            build_id=self.build_id, timestamp=timestamp
-        )
-
         session_data = yield self.worker.process_pool.doWork(
             RequestFetchServiceSessionCommand,
             url=self.START_SESSION_ENDPOINT.format(
                 control_endpoint=self.control_endpoint
             ),
             auth_header=self.auth_header,
-            proxy_username=proxy_username,
         )
 
         self.session_id = session_data["id"]
diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
index 7cf1c9b..ba3eaf2 100644
--- a/lib/lp/buildmaster/downloader.py
+++ b/lib/lp/buildmaster/downloader.py
@@ -49,7 +49,6 @@ class RequestFetchServiceSessionCommand(amp.Command):
     arguments = [
         (b"url", amp.Unicode()),
         (b"auth_header", amp.String()),
-        (b"proxy_username", amp.Unicode()),
     ]
     response = [
         (b"id", amp.Unicode()),
@@ -119,9 +118,7 @@ class RequestProcess(AMPChild):
             return response.json()
 
     @RequestFetchServiceSessionCommand.responder
-    def requestFetchServiceSessionCommand(
-        self, url, auth_header, proxy_username
-    ):
+    def requestFetchServiceSessionCommand(self, url, auth_header):
         with Session() as session:
             session.trust_env = False
             # XXX pelpsi: from ST108 and from what Claudio
@@ -134,7 +131,7 @@ class RequestProcess(AMPChild):
             response = session.post(
                 url,
                 headers={"Authorization": auth_header},
-                json={"username": proxy_username, "policy": "permissive"},
+                json={"policy": "permissive"},
             )
             response.raise_for_status()
             return response.json()
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index 1ba8b7b..dfcc86b 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -3,6 +3,7 @@
 
 __all__ = [
     "BuilderInteractor",
+    "BuilderWorker",
     "extract_vitals_from_db",
 ]
 
diff --git a/lib/lp/buildmaster/tests/fetchservice.py b/lib/lp/buildmaster/tests/fetchservice.py
index 51f73e3..16fabcb 100644
--- a/lib/lp/buildmaster/tests/fetchservice.py
+++ b/lib/lp/buildmaster/tests/fetchservice.py
@@ -17,16 +17,18 @@ 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."""
+class FetchServiceControlEndpoint(resource.Resource):
+    """A fake fetch service control endpoints API."""
 
     isLeaf = True
 
     def __init__(self):
         resource.Resource.__init__(self)
         self.requests = []
+        self.session_id = "2ea9c9f759044f9b9aff469fe90429a5"
 
     def render_POST(self, request):
+        """We make a POST request to start a fetch service session"""
         content = json.loads(request.content.read())
         self.requests.append(
             {
@@ -68,7 +70,7 @@ class InProcessFetchServiceAuthAPIFixture(fixtures.Fixture):
     @defer.inlineCallbacks
     def start(self):
         root = resource.Resource()
-        self.sessions = FetchServiceAuthAPITokensResource()
+        self.sessions = FetchServiceControlEndpoint()
         root.putChild(b"sessions", self.sessions)
         endpoint = endpoints.serverFromString(reactor, nativeString("tcp:0"))
         site = server.Site(self.sessions)
@@ -80,7 +82,7 @@ class InProcessFetchServiceAuthAPIFixture(fixtures.Fixture):
             [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_control_endpoint: http://{host}:{port}
             fetch_service_host: {host}
             fetch_service_port: {port}
             fetch_service_mitm_certificate: fake-cert
@@ -116,7 +118,7 @@ class RevocationEndpointMatcher(Equals):
 
     def __init__(self, session_id):
         super().__init__(
-            "{endpoint}/{session_id}".format(
+            "{endpoint}/session/{session_id}/token".format(
                 endpoint=config.builddmaster.fetch_service_control_endpoint,
                 session_id=session_id,
             )
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index d20bf33..73d4843 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -362,7 +362,7 @@ class TestAsyncSnapBuildBehaviourFetchService(
         job = self.makeJob(snap=snap, build_request=request)
         args = yield job.extraBuildArgs()
         expected_uri = urlsplit(
-            config.builddmaster.fetch_service_control_endpoint
+            f"{config.builddmaster.fetch_service_control_endpoint}/session"
         ).path.encode("UTF-8")
         request_matcher = MatchesDict(
             {
@@ -389,7 +389,6 @@ class TestAsyncSnapBuildBehaviourFetchService(
                 ),
                 "json": MatchesDict(
                     {
-                        "username": StartsWith(job.build.build_cookie + "-"),
                         "policy": Equals("permissive"),
                     }
                 ),