Launchpad itself

Merge ~ines-almeida/launchpad:fetch-service-update-end-session into launchpad:master

  1. Git
  2. lp:~ines-almeida/launchpad
  3. fetch-service-update-end-session
  4. Merge into master
Proposed by Ines Almeida
Status: Merged
Approved by: Ines Almeida
Approved revision: c55da58c037c6cb89c77787ae1c8b59c60eb3b68
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ines-almeida/launchpad:fetch-service-update-end-session
Merge into: launchpad:master
Diff against target: 413 lines (+131/-51)
4 files modified
lib/lp/buildmaster/builderproxy.py (+74/-39)
lib/lp/buildmaster/interactor.py (+4/-0)
lib/lp/buildmaster/tests/mock_workers.py (+12/-0)
lib/lp/snappy/tests/test_snapbuildbehaviour.py (+41/-12)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+465152@code.launchpad.net

Commit message

Update how to keep track of session_id within a builder session

To post a comment you must log in.
Revision history for this message
Ines Almeida (ines-almeida) wrote :

This has not been cleaned up (and mypy issues not fixed). Opening it up for an early review.

Revision history for this message
Jürgen Gmach (jugmac00) :
Revision history for this message
Jürgen Gmach (jugmac00) :
Revision history for this message
Ines Almeida (ines-almeida) wrote :

Fixed the mypy issues and updated the logic to use the `use_fetch_service` flag directly from the builder proxy info. I still need to re-work a test, but this is now ready for review.

Revision history for this message
Jürgen Gmach (jugmac00) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
2index 1fc0d9b..a52ce06 100644
3--- a/lib/lp/buildmaster/builderproxy.py
4+++ b/lib/lp/buildmaster/builderproxy.py
5@@ -17,8 +17,9 @@ __all__ = [
6
7 import base64
8 import os
9+import re
10 import time
11-from typing import Dict, Generator, Type
12+from typing import Optional
13
14 from twisted.internet import defer
15
16@@ -37,6 +38,10 @@ from lp.services.config import config
17 BUILD_METADATA_FILENAME_FORMAT = "{build_id}_metadata.json"
18
19
20+class ProxyServiceException(Exception):
21+ pass
22+
23+
24 def _get_proxy_config(name):
25 """Get a config item from builddmaster (current) or snappy (deprecated)."""
26 return getattr(config.builddmaster, name) or getattr(config.snappy, name)
27@@ -56,28 +61,23 @@ class BuilderProxyMixin:
28 build: BuildFarmJob
29 _worker: BuilderWorker
30
31- def __init__(self, *args, **kwargs):
32- super().__init__(*args, **kwargs)
33- self._use_fetch_service = False
34- self._proxy_service = None
35-
36 @defer.inlineCallbacks
37 def startProxySession(
38 self,
39 args: BuildArgs,
40 allow_internet: bool = True,
41 use_fetch_service: bool = False,
42- ) -> Generator[None, Dict[str, str], None]:
43-
44- self._use_fetch_service = use_fetch_service
45+ ):
46
47 if not allow_internet:
48 return
49
50+ proxy_service: IProxyService
51+
52 if not use_fetch_service and _get_proxy_config("builder_proxy_host"):
53- proxy_service: Type[IProxyService] = BuilderProxy
54+ proxy_service = BuilderProxy(worker=self._worker)
55 elif use_fetch_service and _get_proxy_config("fetch_service_host"):
56- proxy_service = FetchService
57+ proxy_service = FetchService(worker=self._worker)
58
59 # Append the fetch-service certificate to BuildArgs secrets.
60 if "secrets" not in args:
61@@ -91,10 +91,9 @@ class BuilderProxyMixin:
62 # non-production environments.
63 return
64
65- self._proxy_service = proxy_service(
66- build_id=self.build.build_cookie, worker=self._worker
67+ session_data = yield proxy_service.startSession(
68+ build_id=self.build.build_cookie
69 )
70- session_data = yield self._proxy_service.startSession()
71
72 args["proxy_url"] = session_data["proxy_url"]
73 args["revocation_endpoint"] = session_data["revocation_endpoint"]
74@@ -115,34 +114,55 @@ class BuilderProxyMixin:
75 Sessions will be closed automatically within the Fetch Service after
76 a certain amount of time configured by its charm (default 6 hours).
77 """
78- if not self._use_fetch_service:
79- # No cleanup needed for the builder proxy
80- return
81
82- if self._proxy_service is None:
83- # A session was never started. This can happen if the proxy configs
84- # are not set (see `startProxySession`)
85+ proxy_info = yield self._worker.proxy_info()
86+ use_fetch_service = proxy_info.get("use_fetch_service")
87+
88+ if not use_fetch_service:
89+ # No cleanup needed when not using the fetch service
90+ # This is true both when we use the builder proxy and when we don't
91+ # allow internet access to the builds.
92 return
93
94+ proxy_service = FetchService(worker=self._worker)
95+
96+ # XXX ines-almeida 2024-04-30: Getting the session_id from the
97+ # revocation_endpoint feels a little like going back and forwards
98+ # given the revocation_endpoint is created on `startProxySession()`.
99+ # Ideally, we would update `buildd` and `buildd-manager` to senf and
100+ # retrieve the session ID directly (instead of having to parse it).
101+ revocation_endpoint = proxy_info.get("revocation_endpoint")
102+ session_id = proxy_service.extractSessionIDFromRevocationEndpoint(
103+ revocation_endpoint
104+ )
105+
106+ if session_id is None:
107+ raise ProxyServiceException(
108+ "Could not parse the revocation endpoint fetched from buildd "
109+ f"('{revocation_endpoint}') to get the fetch service "
110+ "`session_id` used within the build."
111+ )
112+
113 metadata_file_name = BUILD_METADATA_FILENAME_FORMAT.format(
114 build_id=self.build.build_cookie
115 )
116 file_path = os.path.join(upload_path, metadata_file_name)
117- yield self._proxy_service.retrieveMetadataFromSession(
118- save_content_to=file_path
119+ yield proxy_service.retrieveMetadataFromSession(
120+ session_id=session_id,
121+ save_content_to=file_path,
122 )
123
124- yield self._proxy_service.endSession()
125+ yield proxy_service.endSession(session_id=session_id)
126
127
128 class IProxyService:
129 """Interface for Proxy Services - either FetchService or BuilderProxy."""
130
131- def __init__(self, build_id: str, worker: BuilderWorker):
132+ def __init__(self, worker: BuilderWorker):
133 pass
134
135 @defer.inlineCallbacks
136- def startSession(self):
137+ def startSession(self, build_id: str):
138 """Start a proxy session and request required
139
140 :returns: dictionary with an authenticated `proxy_url` for the builder
141@@ -159,7 +179,7 @@ class BuilderProxy(IProxyService):
142 making API requests directly to the builder proxy control endpoint.
143 """
144
145- def __init__(self, build_id: str, worker: BuilderWorker):
146+ def __init__(self, worker: BuilderWorker):
147 self.control_endpoint = _get_value_from_config(
148 "builder_proxy_auth_api_endpoint"
149 )
150@@ -168,8 +188,6 @@ class BuilderProxy(IProxyService):
151 port=_get_value_from_config("builder_proxy_port"),
152 )
153 self.auth_header = self._getAuthHeader()
154-
155- self.build_id = build_id
156 self.worker = worker
157
158 @staticmethod
159@@ -187,14 +205,14 @@ class BuilderProxy(IProxyService):
160 return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
161
162 @defer.inlineCallbacks
163- def startSession(self):
164+ def startSession(self, build_id: str):
165 """Request a token from the builder proxy to be used by the builders.
166
167 See IProxyService.
168 """
169 timestamp = int(time.time())
170 proxy_username = "{build_id}-{timestamp}".format(
171- build_id=self.build_id, timestamp=timestamp
172+ build_id=build_id, timestamp=timestamp
173 )
174
175 token = yield self.worker.process_pool.doWork(
176@@ -233,7 +251,7 @@ class FetchService(IProxyService):
177 RETRIEVE_METADATA_ENDPOINT = "{control_endpoint}/session/{session_id}"
178 END_SESSION_ENDPOINT = "{control_endpoint}/session/{session_id}"
179
180- def __init__(self, build_id: str, worker: BuilderWorker):
181+ def __init__(self, worker: BuilderWorker):
182 self.control_endpoint = _get_value_from_config(
183 "fetch_service_control_endpoint"
184 )
185@@ -242,10 +260,7 @@ class FetchService(IProxyService):
186 port=_get_value_from_config("fetch_service_port"),
187 )
188 self.auth_header = self._getAuthHeader()
189-
190- self.build_id = build_id
191 self.worker = worker
192- self.session_id = None
193
194 @staticmethod
195 def _getAuthHeader():
196@@ -259,7 +274,7 @@ class FetchService(IProxyService):
197 return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
198
199 @defer.inlineCallbacks
200- def startSession(self):
201+ def startSession(self, build_id: str):
202 """Requests a fetch service session and returns session information.
203
204 See IProxyService.
205@@ -290,8 +305,28 @@ class FetchService(IProxyService):
206 "revocation_endpoint": revocation_endpoint,
207 }
208
209+ def extractSessionIDFromRevocationEndpoint(
210+ self, revocation_endpoint: str
211+ ) -> Optional[str]:
212+ """Helper method to get the session_id out of the revocation
213+ endpoint.
214+
215+ Example `revocation_endpoint`:
216+ http://fetch-service.net:9999/session/913890402914ce3be9ffd4d0/token
217+
218+ Would return: 913890402914ce3be9ffd4d0
219+ """
220+ re_pattern = self.TOKEN_REVOCATION.format(
221+ control_endpoint=self.control_endpoint,
222+ session_id="(?P<session_id>.*)",
223+ )
224+ match = re.match(re_pattern, revocation_endpoint)
225+ return match["session_id"] if match else None
226+
227 @defer.inlineCallbacks
228- def retrieveMetadataFromSession(self, save_content_to: str):
229+ def retrieveMetadataFromSession(
230+ self, session_id: str, save_content_to: str
231+ ):
232 """Make request to retrieve metadata from the current session.
233
234 Data is stored directly into a file whose path is `save_content_to`
235@@ -300,7 +335,7 @@ class FetchService(IProxyService):
236 """
237 url = self.RETRIEVE_METADATA_ENDPOINT.format(
238 control_endpoint=self.control_endpoint,
239- session_id=self.session_id,
240+ session_id=session_id,
241 )
242 yield self.worker.process_pool.doWork(
243 RetrieveFetchServiceSessionCommand,
244@@ -310,14 +345,14 @@ class FetchService(IProxyService):
245 )
246
247 @defer.inlineCallbacks
248- def endSession(self):
249+ def endSession(self, session_id: str):
250 """End the proxy session and do any cleanup needed.
251
252 :raises: RequestException if request to Fetch Service fails
253 """
254 url = self.END_SESSION_ENDPOINT.format(
255 control_endpoint=self.control_endpoint,
256- session_id=self.session_id,
257+ session_id=session_id,
258 )
259 yield self.worker.process_pool.doWork(
260 EndFetchServiceSessionCommand,
261diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
262index dfcc86b..9bf1a71 100644
263--- a/lib/lp/buildmaster/interactor.py
264+++ b/lib/lp/buildmaster/interactor.py
265@@ -209,6 +209,10 @@ class BuilderWorker:
266 """Echo the arguments back."""
267 return self._with_timeout(self._server.callRemote("echo", *args))
268
269+ def proxy_info(self):
270+ """Return the details for the proxy used by the manager."""
271+ return self._with_timeout(self._server.callRemote("proxy_info"))
272+
273 def info(self):
274 """Return the protocol version and the builder methods supported."""
275 return self._with_timeout(self._server.callRemote("info"))
276diff --git a/lib/lp/buildmaster/tests/mock_workers.py b/lib/lp/buildmaster/tests/mock_workers.py
277index a880a9a..5f5563b 100644
278--- a/lib/lp/buildmaster/tests/mock_workers.py
279+++ b/lib/lp/buildmaster/tests/mock_workers.py
280@@ -151,6 +151,18 @@ class OkWorker:
281 self.call_log.append("info")
282 return defer.succeed(("1.0", self.arch_tag, "binarypackage"))
283
284+ def proxy_info(self):
285+ self.call_log.append("proxy_info")
286+ return defer.succeed(
287+ {
288+ "revocation_endpoint": (
289+ "http://fetch-service.test:9999/session/"
290+ "9138904ce3be9ffd4d0/token"
291+ ),
292+ "use_fetch_service": False,
293+ }
294+ )
295+
296 def resume(self):
297 self.call_log.append("resume")
298 return defer.succeed(("", "", 0))
299diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
300index 81d6926..cb2269b 100644
301--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
302+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
303@@ -10,6 +10,7 @@ import time
304 import uuid
305 from datetime import datetime
306 from textwrap import dedent
307+from unittest.mock import MagicMock
308 from urllib.parse import urlsplit
309
310 import fixtures
311@@ -40,7 +41,6 @@ from lp.app.enums import InformationType
312 from lp.archivepublisher.interfaces.archivegpgsigningkey import (
313 IArchiveGPGSigningKey,
314 )
315-from lp.buildmaster.builderproxy import BuilderProxy
316 from lp.buildmaster.enums import BuildBaseImageType, BuildStatus
317 from lp.buildmaster.interactor import shut_down_default_process_pool
318 from lp.buildmaster.interfaces.builder import CannotBuild
319@@ -431,6 +431,20 @@ class TestAsyncSnapBuildBehaviourFetchService(
320 snap = self.factory.makeSnap(use_fetch_service=True)
321 request = self.factory.makeSnapBuildRequest(snap=snap)
322 job = self.makeJob(snap=snap, build_request=request)
323+
324+ host = config.builddmaster.fetch_service_host
325+ port = config.builddmaster.fetch_service_port
326+ session_id = self.fetch_service_api.sessions.session_id
327+ revocation_endpoint = (
328+ f"http://{host}:{port}/session/{session_id}/token"
329+ )
330+
331+ job._worker.proxy_info = MagicMock(
332+ return_value={
333+ "revocation_endpoint": revocation_endpoint,
334+ "use_fetch_service": True,
335+ }
336+ )
337 yield job.extraBuildArgs()
338
339 # End the session
340@@ -443,7 +457,6 @@ class TestAsyncSnapBuildBehaviourFetchService(
341 start_session_request = self.fetch_service_api.sessions.requests[0]
342 self.assertEqual(b"POST", start_session_request["method"])
343 self.assertEqual(b"/session", start_session_request["uri"])
344- session_id = self.fetch_service_api.sessions.responses[0]["id"]
345
346 # Request retrieve metadata
347 retrieve_metadata_request = self.fetch_service_api.sessions.requests[1]
348@@ -478,6 +491,14 @@ class TestAsyncSnapBuildBehaviourFetchService(
349 snap = self.factory.makeSnap(use_fetch_service=False)
350 request = self.factory.makeSnapBuildRequest(snap=snap)
351 job = self.makeJob(snap=snap, build_request=request)
352+
353+ job._worker.proxy_info = MagicMock(
354+ return_value={
355+ "revocation_endpoint": "https://builder-proxy.test/revoke",
356+ "use_fetch_service": False,
357+ }
358+ )
359+
360 yield job.extraBuildArgs()
361 yield job.endProxySession(upload_path="test_path")
362
363@@ -485,23 +506,33 @@ class TestAsyncSnapBuildBehaviourFetchService(
364 self.assertEqual(0, len(self.fetch_service_api.sessions.requests))
365
366 @defer.inlineCallbacks
367- def test_endProxySession_no_proxy_service(self):
368- """When the `fetch_service_host` is not set, the calls to the fetch
369- service don't go through."""
370+ def test_endProxySession_fetch_Service_allow_internet_false(self):
371+ """When `allow_internet` is False, we don't send proxy variables to the
372+ buildd, and ending the session does not make calls to the fetch
373+ service."""
374 self.useFixture(
375 FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
376 )
377- self.useFixture(FeatureFixture({"fetch_service_host": None}))
378-
379- snap = self.factory.makeSnap(use_fetch_service=True)
380+ snap = self.factory.makeSnap(allow_internet=False)
381 request = self.factory.makeSnapBuildRequest(snap=snap)
382 job = self.makeJob(snap=snap, build_request=request)
383- yield job.extraBuildArgs()
384+ args = yield job.extraBuildArgs()
385+
386+ # Scenario when we don't allow internet
387+ job._worker.proxy_info = MagicMock(
388+ return_value={
389+ "revocation_endpoint": None,
390+ "use_fetch_service": None,
391+ }
392+ )
393+
394 yield job.endProxySession(upload_path="test_path")
395
396+ # No proxy config sent to buildd
397+ self.assertIsNone(args.get("use_fetch_service"))
398+ self.assertIsNone(args.get("revocation_endpoint"))
399 # No calls go through to the fetch service
400 self.assertEqual(0, len(self.fetch_service_api.sessions.requests))
401- self.assertEqual(None, job._proxy_service)
402
403
404 class TestAsyncSnapBuildBehaviourBuilderProxy(
405@@ -1470,8 +1501,6 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
406
407 # End the session
408 yield job.endProxySession(upload_path="test_path")
409- self.assertFalse(job.use_fetch_service)
410- self.assertTrue(isinstance(job.proxy_service, BuilderProxy))
411
412 @defer.inlineCallbacks
413 def test_composeBuildRequest_proxy_url_set(self):

Subscribers

People subscribed via source and target branches

to status/vote changes: