launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #31093
[Merge] ~ines-almeida/launchpad:fetch-service-update-end-session into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-update-end-session into launchpad:master.
Commit message:
Update how to keep track of session_id within a builder session
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/465152
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-update-end-session into launchpad:master.
diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index 1fc0d9b..88f1792 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -17,6 +17,7 @@ __all__ = [
import base64
import os
+import re
import time
from typing import Dict, Generator, Type
@@ -56,11 +57,6 @@ class BuilderProxyMixin:
build: BuildFarmJob
_worker: BuilderWorker
- def __init__(self, *args, **kwargs):
- super().__init__(*args, **kwargs)
- self._use_fetch_service = False
- self._proxy_service = None
-
@defer.inlineCallbacks
def startProxySession(
self,
@@ -69,15 +65,15 @@ class BuilderProxyMixin:
use_fetch_service: bool = False,
) -> Generator[None, Dict[str, str], None]:
- self._use_fetch_service = use_fetch_service
-
if not allow_internet:
return
if not use_fetch_service and _get_proxy_config("builder_proxy_host"):
- proxy_service: Type[IProxyService] = BuilderProxy
+ proxy_service: Type[IProxyService] = BuilderProxy(
+ worker=self._worker
+ )
elif use_fetch_service and _get_proxy_config("fetch_service_host"):
- proxy_service = FetchService
+ proxy_service = FetchService(worker=self._worker)
# Append the fetch-service certificate to BuildArgs secrets.
if "secrets" not in args:
@@ -91,17 +87,16 @@ class BuilderProxyMixin:
# non-production environments.
return
- self._proxy_service = proxy_service(
- build_id=self.build.build_cookie, worker=self._worker
+ session_data = yield proxy_service.startSession(
+ build_id=self.build.build_cookie
)
- session_data = yield self._proxy_service.startSession()
args["proxy_url"] = session_data["proxy_url"]
args["revocation_endpoint"] = session_data["revocation_endpoint"]
args["use_fetch_service"] = use_fetch_service
@defer.inlineCallbacks
- def endProxySession(self, upload_path: str):
+ def endProxySession(self, upload_path: str, use_fetch_service: bool):
"""Handles all the necessary cleanup to be done at the end of a build.
For the fetch service case, this means:
@@ -115,30 +110,32 @@ class BuilderProxyMixin:
Sessions will be closed automatically within the Fetch Service after
a certain amount of time configured by its charm (default 6 hours).
"""
- if not self._use_fetch_service:
+ if not use_fetch_service:
# No cleanup needed for the builder proxy
return
- if self._proxy_service is None:
- # A session was never started. This can happen if the proxy configs
- # are not set (see `startProxySession`)
- return
+ proxy_service = FetchService(worker=self._worker)
+ proxy_info = yield self._worker.proxy_info()
+ session_id = proxy_service.parseRevocationEndpoint(
+ proxy_info["revocation_endpoint"]
+ )
metadata_file_name = BUILD_METADATA_FILENAME_FORMAT.format(
build_id=self.build.build_cookie
)
file_path = os.path.join(upload_path, metadata_file_name)
- yield self._proxy_service.retrieveMetadataFromSession(
- save_content_to=file_path
+ yield proxy_service.retrieveMetadataFromSession(
+ session_id=session_id,
+ save_content_to=file_path,
)
- yield self._proxy_service.endSession()
+ yield proxy_service.endSession(session_id=session_id)
class IProxyService:
"""Interface for Proxy Services - either FetchService or BuilderProxy."""
- def __init__(self, build_id: str, worker: BuilderWorker):
+ def __init__(self, worker: BuilderWorker):
pass
@defer.inlineCallbacks
@@ -159,7 +156,7 @@ class BuilderProxy(IProxyService):
making API requests directly to the builder proxy control endpoint.
"""
- def __init__(self, build_id: str, worker: BuilderWorker):
+ def __init__(self, worker: BuilderWorker):
self.control_endpoint = _get_value_from_config(
"builder_proxy_auth_api_endpoint"
)
@@ -168,8 +165,6 @@ class BuilderProxy(IProxyService):
port=_get_value_from_config("builder_proxy_port"),
)
self.auth_header = self._getAuthHeader()
-
- self.build_id = build_id
self.worker = worker
@staticmethod
@@ -187,14 +182,14 @@ class BuilderProxy(IProxyService):
return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
@defer.inlineCallbacks
- def startSession(self):
+ def startSession(self, build_id: str):
"""Request a token from the builder proxy to be used by the builders.
See IProxyService.
"""
timestamp = int(time.time())
proxy_username = "{build_id}-{timestamp}".format(
- build_id=self.build_id, timestamp=timestamp
+ build_id=build_id, timestamp=timestamp
)
token = yield self.worker.process_pool.doWork(
@@ -233,7 +228,7 @@ class FetchService(IProxyService):
RETRIEVE_METADATA_ENDPOINT = "{control_endpoint}/session/{session_id}"
END_SESSION_ENDPOINT = "{control_endpoint}/session/{session_id}"
- def __init__(self, build_id: str, worker: BuilderWorker):
+ def __init__(self, worker: BuilderWorker):
self.control_endpoint = _get_value_from_config(
"fetch_service_control_endpoint"
)
@@ -242,10 +237,7 @@ class FetchService(IProxyService):
port=_get_value_from_config("fetch_service_port"),
)
self.auth_header = self._getAuthHeader()
-
- self.build_id = build_id
self.worker = worker
- self.session_id = None
@staticmethod
def _getAuthHeader():
@@ -259,7 +251,7 @@ class FetchService(IProxyService):
return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
@defer.inlineCallbacks
- def startSession(self):
+ def startSession(self, build_id: str):
"""Requests a fetch service session and returns session information.
See IProxyService.
@@ -290,8 +282,20 @@ class FetchService(IProxyService):
"revocation_endpoint": revocation_endpoint,
}
+ def parseRevocationEndpoint(self, revocation_endpoint: str) -> str:
+ """Helper method to get the session_id out of the revocation
+ endpoint."""
+ re_pattern = self.TOKEN_REVOCATION.format(
+ control_endpoint=self.control_endpoint,
+ session_id="(?P<session_id>.*)",
+ )
+ match = re.match(re_pattern, revocation_endpoint)
+ return match["session_id"] if match else None
+
@defer.inlineCallbacks
- def retrieveMetadataFromSession(self, save_content_to: str):
+ def retrieveMetadataFromSession(
+ self, session_id: str, save_content_to: str
+ ):
"""Make request to retrieve metadata from the current session.
Data is stored directly into a file whose path is `save_content_to`
@@ -300,7 +304,7 @@ class FetchService(IProxyService):
"""
url = self.RETRIEVE_METADATA_ENDPOINT.format(
control_endpoint=self.control_endpoint,
- session_id=self.session_id,
+ session_id=session_id,
)
yield self.worker.process_pool.doWork(
RetrieveFetchServiceSessionCommand,
@@ -310,14 +314,14 @@ class FetchService(IProxyService):
)
@defer.inlineCallbacks
- def endSession(self):
+ def endSession(self, session_id: str):
"""End the proxy session and do any cleanup needed.
:raises: RequestException if request to Fetch Service fails
"""
url = self.END_SESSION_ENDPOINT.format(
control_endpoint=self.control_endpoint,
- session_id=self.session_id,
+ session_id=session_id,
)
yield self.worker.process_pool.doWork(
EndFetchServiceSessionCommand,
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index dfcc86b..9bf1a71 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -209,6 +209,10 @@ class BuilderWorker:
"""Echo the arguments back."""
return self._with_timeout(self._server.callRemote("echo", *args))
+ def proxy_info(self):
+ """Return the details for the proxy used by the manager."""
+ return self._with_timeout(self._server.callRemote("proxy_info"))
+
def info(self):
"""Return the protocol version and the builder methods supported."""
return self._with_timeout(self._server.callRemote("info"))
diff --git a/lib/lp/buildmaster/tests/mock_workers.py b/lib/lp/buildmaster/tests/mock_workers.py
index a880a9a..0a4a21f 100644
--- a/lib/lp/buildmaster/tests/mock_workers.py
+++ b/lib/lp/buildmaster/tests/mock_workers.py
@@ -151,6 +151,15 @@ class OkWorker:
self.call_log.append("info")
return defer.succeed(("1.0", self.arch_tag, "binarypackage"))
+ def proxy_info(self):
+ self.call_log.append("proxy_info")
+ return defer.succeed(
+ {
+ "revocation_endpoint": "https://proxy.test.net/revoke_token";,
+ "use_fetch_service": False,
+ }
+ )
+
def resume(self):
self.call_log.append("resume")
return defer.succeed(("", "", 0))
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index 7f0cf6d..3303517 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -212,4 +212,6 @@ class SnapBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
@defer.inlineCallbacks
def _saveBuildSpecificFiles(self, upload_path):
- yield self.endProxySession(upload_path)
+ yield self.endProxySession(
+ upload_path, self.build.snap.use_fetch_service
+ )
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 81d6926..3f7b1d7 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -10,6 +10,7 @@ import time
import uuid
from datetime import datetime
from textwrap import dedent
+from unittest.mock import MagicMock
from urllib.parse import urlsplit
import fixtures
@@ -40,7 +41,6 @@ from lp.app.enums import InformationType
from lp.archivepublisher.interfaces.archivegpgsigningkey import (
IArchiveGPGSigningKey,
)
-from lp.buildmaster.builderproxy import BuilderProxy
from lp.buildmaster.enums import BuildBaseImageType, BuildStatus
from lp.buildmaster.interactor import shut_down_default_process_pool
from lp.buildmaster.interfaces.builder import CannotBuild
@@ -431,10 +431,26 @@ class TestAsyncSnapBuildBehaviourFetchService(
snap = self.factory.makeSnap(use_fetch_service=True)
request = self.factory.makeSnapBuildRequest(snap=snap)
job = self.makeJob(snap=snap, build_request=request)
+
+ host = config.builddmaster.fetch_service_host
+ port = config.builddmaster.fetch_service_port
+ session_id = self.fetch_service_api.sessions.session_id
+ revocation_endpoint = (
+ f"http://{host}:{port}/session/{session_id}/token";
+ )
+
+ job._worker.proxy_info = MagicMock(
+ return_value={
+ "revocation_endpoint": revocation_endpoint,
+ "use_fetch_service": True,
+ }
+ )
yield job.extraBuildArgs()
# End the session
- yield job.endProxySession(upload_path=tem_upload_path)
+ yield job.endProxySession(
+ upload_path=tem_upload_path, use_fetch_service=True
+ )
# We expect 3 calls made to the fetch service API, in this order
self.assertEqual(3, len(self.fetch_service_api.sessions.requests))
@@ -443,7 +459,6 @@ class TestAsyncSnapBuildBehaviourFetchService(
start_session_request = self.fetch_service_api.sessions.requests[0]
self.assertEqual(b"POST", start_session_request["method"])
self.assertEqual(b"/session", start_session_request["uri"])
- session_id = self.fetch_service_api.sessions.responses[0]["id"]
# Request retrieve metadata
retrieve_metadata_request = self.fetch_service_api.sessions.requests[1]
@@ -479,29 +494,12 @@ class TestAsyncSnapBuildBehaviourFetchService(
request = self.factory.makeSnapBuildRequest(snap=snap)
job = self.makeJob(snap=snap, build_request=request)
yield job.extraBuildArgs()
- yield job.endProxySession(upload_path="test_path")
-
- # No calls go through to the fetch service
- self.assertEqual(0, len(self.fetch_service_api.sessions.requests))
-
- @defer.inlineCallbacks
- def test_endProxySession_no_proxy_service(self):
- """When the `fetch_service_host` is not set, the calls to the fetch
- service don't go through."""
- self.useFixture(
- FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+ yield job.endProxySession(
+ upload_path="test_path", use_fetch_service=False
)
- self.useFixture(FeatureFixture({"fetch_service_host": 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()
- yield job.endProxySession(upload_path="test_path")
# No calls go through to the fetch service
self.assertEqual(0, len(self.fetch_service_api.sessions.requests))
- self.assertEqual(None, job._proxy_service)
class TestAsyncSnapBuildBehaviourBuilderProxy(
@@ -1469,9 +1467,9 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
yield job.extraBuildArgs()
# End the session
- yield job.endProxySession(upload_path="test_path")
- self.assertFalse(job.use_fetch_service)
- self.assertTrue(isinstance(job.proxy_service, BuilderProxy))
+ yield job.endProxySession(
+ upload_path="test_path", use_fetch_service=False
+ )
@defer.inlineCallbacks
def test_composeBuildRequest_proxy_url_set(self):
Follow ups
