launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #31656
[Merge] ~ruinedyourlife/launchpad:fetch-service-configuration-for-craft-builds into launchpad:master
Quentin Debhi has proposed merging ~ruinedyourlife/launchpad:fetch-service-configuration-for-craft-builds into launchpad:master with ~ruinedyourlife/launchpad:requesting-subset-of-arch-craft-builds as a prerequisite.
Commit message:
Fetch service configuration for craft builds
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/474384
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:fetch-service-configuration-for-craft-builds into launchpad:master.
diff --git a/lib/lp/crafts/interfaces/craftrecipe.py b/lib/lp/crafts/interfaces/craftrecipe.py
index 1290e2e..ef8e752 100644
--- a/lib/lp/crafts/interfaces/craftrecipe.py
+++ b/lib/lp/crafts/interfaces/craftrecipe.py
@@ -711,6 +711,18 @@ class ICraftRecipeAdminAttributes(Interface):
)
)
+ use_fetch_service = exported(
+ Bool(
+ title=_("Use fetch service"),
+ required=True,
+ readonly=False,
+ description=_(
+ "If set, Craft builds will use the fetch-service instead "
+ "of the builder-proxy to access external resources."
+ ),
+ )
+ )
+
# XXX ruinedyourlife 2024-10-02
# https://bugs.launchpad.net/lazr.restful/+bug/760849:
@@ -775,6 +787,7 @@ class ICraftRecipeSet(Interface):
store_secrets=None,
store_channels=None,
date_created=None,
+ use_fetch_service=False,
):
"""Create an `ICraftRecipe`."""
diff --git a/lib/lp/crafts/model/craftrecipe.py b/lib/lp/crafts/model/craftrecipe.py
index 1368194..aea0593 100644
--- a/lib/lp/crafts/model/craftrecipe.py
+++ b/lib/lp/crafts/model/craftrecipe.py
@@ -205,6 +205,8 @@ class CraftRecipe(StormBase):
_store_channels = JSON("store_channels", allow_none=True)
+ use_fetch_service = Bool(name="use_fetch_service", allow_none=False)
+
def __init__(
self,
registrant,
@@ -223,6 +225,7 @@ class CraftRecipe(StormBase):
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""Construct a `CraftRecipe`."""
if not getFeatureFlag(CRAFT_RECIPE_ALLOW_CREATE):
@@ -248,6 +251,7 @@ class CraftRecipe(StormBase):
self.store_name = store_name
self.store_secrets = store_secrets
self.store_channels = store_channels
+ self.use_fetch_service = use_fetch_service
def __repr__(self):
return "<CraftRecipe ~%s/%s/+craft/%s>" % (
@@ -670,6 +674,7 @@ class CraftRecipeSet:
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""See `ICraftRecipeSet`."""
if not registrant.inTeam(owner):
@@ -714,6 +719,7 @@ class CraftRecipeSet:
store_secrets=store_secrets,
store_channels=store_channels,
date_created=date_created,
+ use_fetch_service=use_fetch_service,
)
store.add(recipe)
diff --git a/lib/lp/crafts/model/craftrecipebuildbehaviour.py b/lib/lp/crafts/model/craftrecipebuildbehaviour.py
index 5ff7740..5a328ad 100644
--- a/lib/lp/crafts/model/craftrecipebuildbehaviour.py
+++ b/lib/lp/crafts/model/craftrecipebuildbehaviour.py
@@ -79,7 +79,9 @@ class CraftRecipeBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
"""
build = self.build
args: BuildArgs = yield super().extraBuildArgs(logger=logger)
- yield self.startProxySession(args)
+ yield self.startProxySession(
+ args, use_fetch_service=build.recipe.use_fetch_service
+ )
args["name"] = build.recipe.store_name or build.recipe.name
channels = build.channels or {}
# We have to remove the security proxy that Zope applies to this
@@ -120,3 +122,7 @@ class CraftRecipeBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
# that check does not make sense. We do, however, refuse to build
# for obsolete series.
assert self.build.distro_series.status != SeriesStatus.OBSOLETE
+
+ @defer.inlineCallbacks
+ def _saveBuildSpecificFiles(self, upload_path):
+ yield self.endProxySession(upload_path)
diff --git a/lib/lp/crafts/tests/test_craftrecipe.py b/lib/lp/crafts/tests/test_craftrecipe.py
index aaa2316..9005b60 100644
--- a/lib/lp/crafts/tests/test_craftrecipe.py
+++ b/lib/lp/crafts/tests/test_craftrecipe.py
@@ -23,6 +23,7 @@ from testtools.matchers import (
MatchesStructure,
)
from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import InformationType
@@ -557,6 +558,18 @@ class TestCraftRecipe(TestCaseWithFactory):
builds, job, "20.04", ["armhf", "riscv64"], job.channels
)
+ def test_requestBuild_fetch_service(self):
+ # Activate fetch service for a craft recipe.
+ recipe = self.factory.makeCraftRecipe(use_fetch_service=True)
+ self.assertEqual(True, recipe.use_fetch_service)
+ distro_series = self.factory.makeDistroSeries()
+ das = self.makeBuildableDistroArchSeries(
+ distroseries=distro_series,
+ )
+ build_request = self.factory.makeCraftRecipeBuildRequest(recipe=recipe)
+ build = recipe.requestBuild(build_request, das)
+ self.assertEqual(True, build.recipe.use_fetch_service)
+
class TestCraftRecipeSet(TestCaseWithFactory):
@@ -611,6 +624,7 @@ class TestCraftRecipeSet(TestCaseWithFactory):
self.assertIsNone(recipe.store_name)
self.assertIsNone(recipe.store_secrets)
self.assertEqual([], recipe.store_channels)
+ self.assertFalse(recipe.use_fetch_service)
def test_creation_no_source(self):
# Attempting to create a craft recipe without a Git repository
@@ -840,6 +854,46 @@ class TestCraftRecipeSet(TestCaseWithFactory):
self.factory.makeDistribution(),
)
+ def test_admins_can_update_admin_only_fields(self):
+ # The admin fields can be updated by an admin
+ [ref] = self.factory.makeGitRefs()
+ craft = self.factory.makeCraftRecipe(
+ git_ref=ref, use_fetch_service=True
+ )
+
+ admin_fields = [
+ "require_virtualized",
+ "use_fetch_service",
+ ]
+
+ for field_name in admin_fields:
+ # exception isn't raised when an admin does the same
+ with admin_logged_in():
+ setattr(craft, field_name, True)
+
+ def test_non_admins_cannot_update_admin_only_fields(self):
+ # The admin fields cannot be updated by a non admin
+ [ref] = self.factory.makeGitRefs()
+ craft = self.factory.makeCraftRecipe(
+ git_ref=ref, use_fetch_service=True
+ )
+ person = self.factory.makePerson()
+ admin_fields = [
+ "require_virtualized",
+ "use_fetch_service",
+ ]
+
+ for field_name in admin_fields:
+ # exception is raised when a non admin updates the fields
+ with person_logged_in(person):
+ self.assertRaises(
+ Unauthorized,
+ setattr,
+ craft,
+ field_name,
+ True,
+ )
+
class TestCraftRecipeDeleteWithBuilds(TestCaseWithFactory):
diff --git a/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py b/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
index b11d81c..62bdbfd 100644
--- a/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
+++ b/lib/lp/crafts/tests/test_craftrecipebuildbehaviour.py
@@ -4,13 +4,15 @@
"""Test craft recipe build behaviour."""
import base64
+import json
import os.path
import time
import uuid
from datetime import datetime
+from unittest.mock import MagicMock
from urllib.parse import urlsplit
-from fixtures import MockPatch
+from fixtures import MockPatch, TempDir
from pymacaroons import Macaroon
from testtools import ExpectedException
from testtools.matchers import (
@@ -46,6 +48,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,
@@ -534,6 +539,263 @@ class TestAsyncCraftRecipeBuildBehaviour(
)
+class TestAsyncCraftRecipeBuildBehaviourFetchService(
+ StatsMixin, TestCraftRecipeBuildBehaviourBase
+):
+ 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(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 craft recipe 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)
+ job = self.makeJob(use_fetch_service=True)
+ 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_certificate(self):
+ """Create a craft recipe build request with an incomplete fetch service
+ configuration.
+
+ If `fetch_service_mitm_certificate` is not provided
+ the function raises a `CannotBuild` error.
+ """
+ self.pushConfig("builddmaster", fetch_service_mitm_certificate=None)
+ job = self.makeJob(use_fetch_service=True)
+ expected_exception_msg = (
+ "fetch_service_mitm_certificate is not configured."
+ )
+ with ExpectedException(CannotBuild, expected_exception_msg):
+ yield job.extraBuildArgs()
+
+ @defer.inlineCallbacks
+ def test_requestFetchServiceSession_no_secret(self):
+ """Create a craft recipe 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
+ )
+ job = self.makeJob(use_fetch_service=True)
+ 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 craft recipe build request with a successful fetch service
+ configuration.
+
+ `proxy_url` and `revocation_endpoint` are correctly populated.
+ """
+ job = self.makeJob(use_fetch_service=True)
+ args = yield job.extraBuildArgs()
+ request_matcher = MatchesDict(
+ {
+ "method": Equals(b"POST"),
+ "uri": Equals(b"/session"),
+ "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(
+ {
+ "policy": Equals("permissive"),
+ }
+ ),
+ }
+ )
+ self.assertThat(
+ self.fetch_service_api.sessions.requests,
+ MatchesListwise([request_matcher]),
+ )
+ self.assertIn("proxy_url", args)
+ self.assertIn("revocation_endpoint", args)
+ self.assertTrue(args["use_fetch_service"])
+ self.assertIn("secrets", args)
+ self.assertIn("fetch_service_mitm_certificate", args["secrets"])
+ self.assertIn(
+ "fake-cert", args["secrets"]["fetch_service_mitm_certificate"]
+ )
+
+ @defer.inlineCallbacks
+ def test_requestFetchServiceSession_mitm_certficate_redacted(self):
+ """The `fetch_service_mitm_certificate` field in the build arguments
+ is redacted in the build logs."""
+
+ job = self.makeJob(use_fetch_service=True)
+ args = yield job.extraBuildArgs()
+
+ chroot_lfa = self.factory.makeLibraryFileAlias(db_only=True)
+ job.build.distro_arch_series.addOrUpdateChroot(
+ chroot_lfa, image_type=BuildBaseImageType.CHROOT
+ )
+ lxd_lfa = self.factory.makeLibraryFileAlias(db_only=True)
+ job.build.distro_arch_series.addOrUpdateChroot(
+ lxd_lfa, image_type=BuildBaseImageType.LXD
+ )
+ deferred = defer.Deferred()
+ deferred.callback(None)
+ job._worker.sendFileToWorker = MagicMock(return_value=deferred)
+ job._worker.build = MagicMock(return_value=(None, None))
+
+ logger = BufferLogger()
+ yield job.dispatchBuildToWorker(logger)
+
+ # Secrets exist within the arguments
+ self.assertIn(
+ "fake-cert", args["secrets"]["fetch_service_mitm_certificate"]
+ )
+ # But are redacted in the log output
+ self.assertIn(
+ "'fetch_service_mitm_certificate': '<redacted>'",
+ logger.getLogBuffer(),
+ )
+
+ @defer.inlineCallbacks
+ def test_endProxySession(self):
+ """By ending a fetch service session, metadata is retrieved from the
+ fetch service and saved to a file; and call to end the session is made.
+ """
+ tem_upload_path = self.useFixture(TempDir()).path
+
+ job = self.makeJob(use_fetch_service=True)
+
+ 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)
+
+ # We expect 3 calls made to the fetch service API, in this order
+ self.assertEqual(3, len(self.fetch_service_api.sessions.requests))
+
+ # Request start a session
+ 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"])
+
+ # Request retrieve metadata
+ retrieve_metadata_request = self.fetch_service_api.sessions.requests[1]
+ self.assertEqual(b"GET", retrieve_metadata_request["method"])
+ self.assertEqual(
+ f"/session/{session_id}".encode(), retrieve_metadata_request["uri"]
+ )
+
+ # Request end session
+ end_session_request = self.fetch_service_api.sessions.requests[2]
+ self.assertEqual(b"DELETE", end_session_request["method"])
+ self.assertEqual(
+ f"/session/{session_id}".encode(), end_session_request["uri"]
+ )
+
+ # The expected file is created in the `tem_upload_path`
+ expected_filename = f"{job.build.build_cookie}_metadata.json"
+ expected_file_path = os.path.join(tem_upload_path, expected_filename)
+ with open(expected_file_path) as f:
+ self.assertEqual(
+ json.dumps(self.fetch_service_api.sessions.responses[1]),
+ f.read(),
+ )
+
+ @defer.inlineCallbacks
+ def test_endProxySession_fetch_Service_false(self):
+ """When `use_fetch_service` is False, we don't make any calls to the
+ fetch service API."""
+
+ job = self.makeJob(use_fetch_service=False)
+
+ job._worker.proxy_info = MagicMock(
+ return_value={
+ "revocation_endpoint": "https://builder-proxy.test/revoke";,
+ "use_fetch_service": False,
+ }
+ )
+
+ 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))
+
+
class MakeCraftRecipeBuildMixin:
"""Provide the common makeBuild method returning a queued build."""
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 909b220..2d37dc8 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6914,6 +6914,7 @@ class LaunchpadObjectFactory(ObjectFactory):
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""Make a new craft recipe."""
if registrant is None:
@@ -6962,6 +6963,7 @@ class LaunchpadObjectFactory(ObjectFactory):
store_secrets=store_secrets,
store_channels=store_channels,
date_created=date_created,
+ use_fetch_service=use_fetch_service,
)
if is_stale is not None:
removeSecurityProxy(recipe).is_stale = is_stale
