launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32500
[Merge] ~alvarocs/launchpad:charms-use-fetch-service into launchpad:master
Alvaro Crespo Serrano has proposed merging ~alvarocs/launchpad:charms-use-fetch-service into launchpad:master.
Commit message:
Add support for fetch service configuration for Charm builds
Integrate 'use_fetch_service' flag to activate or deactivate fetch service for a given charm recipe.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485867
Introduced the use_fetch_service attribute to CharmRecipe interface and model.
Updated CharmRecipeBuildBehaviour to pass use_fetch_service to startProxySession.
Added unit tests:
-Support use_fetch_service flag in factory method makeCharmRecipe.
-test_charmrecipe: test fetch service configuration for a charm recipe build,
and only admins can modify it.
-test_charmrecipebuildbehaviour: test correct fetch service session behaviour.
Used as reference the update done for rocks: https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/473393
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:charms-use-fetch-service into launchpad:master.
diff --git a/lib/lp/charms/interfaces/charmrecipe.py b/lib/lp/charms/interfaces/charmrecipe.py
index 9e5c06e..c23be55 100644
--- a/lib/lp/charms/interfaces/charmrecipe.py
+++ b/lib/lp/charms/interfaces/charmrecipe.py
@@ -771,6 +771,18 @@ class ICharmRecipeAdminAttributes(Interface):
)
)
+ use_fetch_service = exported(
+ Bool(
+ title=_("Use fetch service"),
+ required=True,
+ readonly=False,
+ description=_(
+ "If set, Rock builds will use the fetch-service instead "
+ "of the builder-proxy to access external resources."
+ ),
+ )
+ )
+
# XXX cjwatson 2021-09-15 bug=760849: "beta" is a lie to get WADL
# generation working. Individual attributes must set their version to
@@ -830,6 +842,7 @@ class ICharmRecipeSet(Interface):
store_secrets=None,
store_channels=None,
date_created=None,
+ use_fetch_service=False,
):
"""Create an `ICharmRecipe`."""
diff --git a/lib/lp/charms/model/charmrecipe.py b/lib/lp/charms/model/charmrecipe.py
index 965206e..04bc448 100644
--- a/lib/lp/charms/model/charmrecipe.py
+++ b/lib/lp/charms/model/charmrecipe.py
@@ -318,6 +318,8 @@ class CharmRecipe(StormBase, WebhookTargetMixin):
_store_channels = JSON("store_channels", allow_none=True)
+ use_fetch_service = Bool(name="use_fetch_service", allow_none=False)
+
def __init__(
self,
registrant,
@@ -336,6 +338,7 @@ class CharmRecipe(StormBase, WebhookTargetMixin):
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""Construct a `CharmRecipe`."""
if not getFeatureFlag(CHARM_RECIPE_ALLOW_CREATE):
@@ -361,6 +364,7 @@ class CharmRecipe(StormBase, WebhookTargetMixin):
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 "<CharmRecipe ~%s/%s/+charm/%s>" % (
@@ -957,6 +961,7 @@ class CharmRecipeSet:
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""See `ICharmRecipeSet`."""
if not registrant.inTeam(owner):
@@ -1002,6 +1007,7 @@ class CharmRecipeSet:
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/charms/model/charmrecipebuildbehaviour.py b/lib/lp/charms/model/charmrecipebuildbehaviour.py
index 4405065..6e1b0bd 100644
--- a/lib/lp/charms/model/charmrecipebuildbehaviour.py
+++ b/lib/lp/charms/model/charmrecipebuildbehaviour.py
@@ -79,7 +79,10 @@ class CharmRecipeBuildBehaviour(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
diff --git a/lib/lp/charms/tests/test_charmrecipe.py b/lib/lp/charms/tests/test_charmrecipe.py
index f6bde77..0150803 100644
--- a/lib/lp/charms/tests/test_charmrecipe.py
+++ b/lib/lp/charms/tests/test_charmrecipe.py
@@ -467,6 +467,18 @@ class TestCharmRecipe(TestCaseWithFactory):
build = recipe.requestBuild(build_request, das)
self.assertEqual(build_virt, build.virtualized)
+ def test_requestBuild_fetch_service(self):
+ # Activate fetch service for a charm recipe.
+ recipe = self.factory.makeCharmRecipe(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.makeCharmRecipeBuildRequest(recipe=recipe)
+ build = recipe.requestBuild(build_request, das)
+ self.assertEqual(True, build.recipe.use_fetch_service)
+
def test_requestBuild_nonvirtualized(self):
# A non-virtualized processor can build a charm recipe iff the
# recipe has require_virtualized set to False.
@@ -1685,6 +1697,7 @@ class TestCharmRecipeSet(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 charm recipe without a Git repository
@@ -2128,6 +2141,48 @@ class TestCharmRecipeSet(TestCaseWithFactory):
recipe, "date_last_modified", UTC_NOW
)
+ def test_admins_can_update_admin_only_fields(self):
+ # Test the admin fields can be updated by an admin
+
+ [ref] = self.factory.makeGitRefs()
+ charm = self.factory.makeCharmRecipe(
+ git_ref=ref, use_fetch_service=True
+ )
+
+ admin_fields = [
+ "require_virtualized",
+ "use_fetch_service",
+ ]
+
+ for field_name in admin_fields:
+ # exception is not raised when an admin does the same
+ with admin_logged_in():
+ setattr(charm, 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()
+ charm = self.factory.makeCharmRecipe(
+ 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,
+ charm,
+ field_name,
+ True,
+ )
+
class TestCharmRecipeWebservice(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
diff --git a/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py b/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
index 37b4a8b..a2f186a 100644
--- a/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
+++ b/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
@@ -4,13 +4,15 @@
"""Test charm 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,
@@ -561,6 +566,273 @@ class TestAsyncCharmRecipeBuildBehaviour(
)
+class TestAsyncCharmRecipeBuildBehaviourFetchService(
+ StatsMixin,
+ TestCharmRecipeBuildBehaviourBase,
+):
+ 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 charm 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 charm 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 charm 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 charm 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("strict"),
+ }
+ ),
+ }
+ )
+ 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 and
+ removing resources are 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 4 calls made to the fetch service API, in this order
+ self.assertEqual(4, 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"]
+ )
+
+ # Request removal of resources
+ remove_resources_request = self.fetch_service_api.sessions.requests[3]
+ self.assertEqual(b"DELETE", remove_resources_request["method"])
+ self.assertEqual(
+ f"/resources/{session_id}".encode(),
+ remove_resources_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 MakeCharmRecipeBuildMixin:
"""Provide the common makeBuild method returning a queued build."""
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 02e0e80..31bd3ed 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6751,6 +6751,7 @@ class LaunchpadObjectFactory(ObjectFactory):
store_secrets=None,
store_channels=None,
date_created=DEFAULT,
+ use_fetch_service=False,
):
"""Make a new charm recipe."""
if registrant is None:
@@ -6799,6 +6800,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
