launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30963
Re: [Merge] ~pelpsi/launchpad:fetch-service-session-API into launchpad:master
Thanks a lot for the MP! We are on the right track!
As we went with the approach to handle both cases inside a single mixin, we really, really want to have a good class docstring to explain that there are two proxies involved.
Also we should add a comment or a paragraph in the docstring which describes the way forward, that at one time hopefully the fetch service will replace the builder proxy.
Also, we need to be careful with naming - do not copy paste naming from the builder proxy. We should align this with what the starcraft folks are using.
Diff comments:
> diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
> index be65e29..b30239f 100644
> --- a/lib/lp/buildmaster/builderproxy.py
> +++ b/lib/lp/buildmaster/builderproxy.py
> @@ -52,26 +67,28 @@ class BuilderProxyMixin:
> endpoint=_get_proxy_config("builder_proxy_auth_api_endpoint"),
> token=token["username"],
> )
> + elif fetch_service and _get_proxy_config("fetch_service_host"):
> + token = yield self._requestFetchServiceSession()
Why do you call it token when it is a response containing more than the token? ie session id and token?
> + args["proxy_url"] = (
> + "http://{session_id}:{token}@{host}:{port}".format(
> + session_id=token["id"],
> + token=token["token"],
> + host=_get_proxy_config("fetch_service_host"),
> + port=_get_proxy_config("fetch_service_port"),
> + )
> + )
> + args["revocation_endpoint"] = "{endpoint}/session/{id}".format(
Where is the term revocation coming from?
In the spec ST108 it is described as "end an active session" - or in LP 136 it is called "finalize session". I added a comment to the ST108 spec whether we could call it finalize.
> + endpoint=_get_proxy_config("fetch_service_auth_api_endpoint"),
> + id=token["id"],
> + )
>
> @defer.inlineCallbacks
> def _requestProxyToken(self):
> - admin_username = _get_proxy_config(
> + admin_username = _get_value_from_config(
> "builder_proxy_auth_api_admin_username"
> )
> - if not admin_username:
> - raise CannotBuild(
> - "builder_proxy_auth_api_admin_username is not configured."
> - )
> - secret = _get_proxy_config("builder_proxy_auth_api_admin_secret")
> - if not secret:
> - raise CannotBuild(
> - "builder_proxy_auth_api_admin_secret is not configured."
> - )
> - url = _get_proxy_config("builder_proxy_auth_api_endpoint")
> - if not secret:
> - raise CannotBuild(
> - "builder_proxy_auth_api_endpoint is not configured."
> - )
> + secret = _get_value_from_config("builder_proxy_auth_api_admin_secret")
> + url = _get_value_from_config("builder_proxy_auth_api_endpoint")
> timestamp = int(time.time())
> proxy_username = "{build_id}-{timestamp}".format(
> build_id=self.build.build_cookie, timestamp=timestamp
> @@ -86,3 +103,25 @@ class BuilderProxyMixin:
> proxy_username=proxy_username,
> )
> return token
> +
> + @defer.inlineCallbacks
> + def _requestFetchServiceSession(self):
> + admin_username = _get_value_from_config(
> + "fetch_service_auth_api_admin_username"
> + )
> + secret = _get_value_from_config("fetch_service_auth_api_admin_secret")
> + url = _get_value_from_config("fetch_service_auth_api_endpoint")
> + timestamp = int(time.time())
> + proxy_username = "{build_id}-{timestamp}".format(
> + build_id=self.build.build_cookie, timestamp=timestamp
> + )
> + auth_string = f"{admin_username}:{secret}".strip()
> + auth_header = b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
> +
> + token = yield self._worker.process_pool.doWork(
Is this really a token or both token and session id?
> + RequestFetchServiceSessionCommand,
> + url=url,
> + auth_header=auth_header,
> + proxy_username=proxy_username,
> + )
> + return token
> diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
> index fc16852..6f6c2be 100644
> --- a/lib/lp/buildmaster/downloader.py
> +++ b/lib/lp/buildmaster/downloader.py
> @@ -37,6 +38,21 @@ class DownloadCommand(amp.Command):
> }
>
>
> +class RequestFetchServiceSessionCommand(amp.Command):
Could you please add a descriptive docstring?
> + arguments = [
> + (b"url", amp.Unicode()),
> + (b"auth_header", amp.String()),
> + (b"proxy_username", amp.Unicode()),
> + ]
> + response = [
> + (b"id", amp.Unicode()),
> + (b"token", amp.Unicode()),
> + ]
> + errors = {
> + RequestException: b"REQUEST_ERROR",
> + }
> +
> +
> class RequestProxyTokenCommand(amp.Command):
> arguments = [
> (b"url", amp.Unicode()),
> @@ -94,3 +110,25 @@ class RequestProcess(AMPChild):
> )
> response.raise_for_status()
> return response.json()
> +
> + @RequestFetchServiceSessionCommand.responder
> + def requestFetchServiceSessionCommand(
> + self, url, auth_header, proxy_username
> + ):
> + with Session() as session:
> + session.trust_env = False
> + # XXX pelpsi: from ST108 and from what Claudio
> + # said `timeout` and `policy` are not mandatory
> + # now, they haven't decided if policy should be
> + # mandatory or optional (assuming "strict" as default),
> + # so for now it's better to pass it explicitly and
> + # permissive will never be the default, so until we're
> + # able to run in strict mode passing it as "permissive"
> + # will be required anyway.
This comment is a bit hard to understand. Can we rephrase it? Also, could you please split the comment into two sections, one for `timeout` and one for `policy`, as you handle both differently in the code, ie you pass one but not the other.
> + response = session.post(
> + url,
> + headers={"Authorization": auth_header},
> + json={"username": proxy_username, "policy": "permissive"},
> + )
> + response.raise_for_status()
> + return response.json()
> diff --git a/lib/lp/buildmaster/tests/fetchservice.py b/lib/lp/buildmaster/tests/fetchservice.py
> new file mode 100644
> index 0000000..6614f0e
> --- /dev/null
> +++ b/lib/lp/buildmaster/tests/fetchservice.py
> @@ -0,0 +1,120 @@
> +# Copyright 2015-2024 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Fixtures for dealing with the build time HTTP proxy."""
Did you copy/paste the module docstring?
Could you at very least mention the fetch service somehow?
> +
> +import json
> +import uuid
> +from textwrap import dedent
> +from urllib.parse import urlsplit
> +
> +import fixtures
> +from testtools.matchers import Equals, HasLength, MatchesStructure
> +from twisted.internet import defer, endpoints, reactor
> +from twisted.python.compat import nativeString
> +from twisted.web import resource, server
> +
> +from lp.services.config import config
> +
> +
> +class FetchServiceAuthAPITokensResource(resource.Resource):
> + """A test tokens resource for the proxy authentication API."""
Could you please update the docstring? From having a quick look I do not know what this class is used for.
Oh, so this is a fake implementation for the token API endpoint of the fetch service, right?
> +
> + isLeaf = True
> +
> + def __init__(self):
> + resource.Resource.__init__(self)
> + self.requests = []
> +
> + def render_POST(self, request):
> + content = json.loads(request.content.read().decode("UTF-8"))
> + self.requests.append(
> + {
> + "method": request.method,
> + "uri": request.uri,
> + "headers": dict(request.requestHeaders.getAllRawHeaders()),
> + "json": content,
> + }
> + )
> + return json.dumps(
> + {
> + "id": "1",
> + "token": uuid.uuid4().hex,
> + }
> + ).encode("UTF-8")
> +
> +
> +class InProcessFetchServiceAuthAPIFixture(fixtures.Fixture):
> + """A fixture that pretends to be the proxy authentication API.
Is it "pretends" or "replaces"?
> +
> + Users of this fixture must call the `start` method, which returns a
> + `Deferred`, and arrange for that to get back to the reactor. This is
> + necessary because the basic fixture API does not allow `setUp` to return
> + anything. For example:
> +
> + class TestSomething(TestCase):
> +
> + run_tests_with = AsynchronousDeferredRunTest.make_factory(
> + timeout=30)
> +
> + @defer.inlineCallbacks
> + def setUp(self):
> + super().setUp()
> + yield self.useFixture(
> + InProcessFetchServiceAuthAPIFixture()
> + ).start()
> + """
> +
> + @defer.inlineCallbacks
> + def start(self):
> + root = resource.Resource()
> + self.tokens = FetchServiceAuthAPITokensResource()
> + root.putChild(b"tokens", self.tokens)
> + endpoint = endpoints.serverFromString(reactor, nativeString("tcp:0"))
> + site = server.Site(self.tokens)
> + self.addCleanup(site.stopFactory)
> + port = yield endpoint.listen(site)
> + self.addCleanup(port.stopListening)
> + config.push(
> + "in-process-fetch-service-api-fixture",
> + dedent(
> + """
> + [builddmaster]
> + fetch_service_auth_api_admin_secret: admin-secret
> + fetch_service_auth_api_admin_username: admin-launchpad.test
> + fetch_service_auth_api_endpoint: http://{host}:{port}/tokens
> + fetch_service_host: {host}
> + fetch_service_port: {port}
> + """
> + ).format(host=port.getHost().host, port=port.getHost().port),
> + )
> + self.addCleanup(config.pop, "in-process-fetch-service-api-fixture")
> +
> +
> +class FetchServiceURLMatcher(MatchesStructure):
Where is this used and why is it in a fixture file? Oh, I see, this is used as an assertion.
> + """Check that a string is a valid url for a builder proxy."""
Please update the docstring.
> +
> + def __init__(self):
> + super().__init__(
> + scheme=Equals("http"),
> + id=Equals(1),
> + token=HasLength(32),
> + hostname=Equals(config.builddmaster.fetch_service_host),
> + port=Equals(config.builddmaster.fetch_service_port),
> + path=Equals(""),
> + )
> +
> + def match(self, matchee):
> + super().match(urlsplit(matchee))
> +
> +
> +class RevocationEndpointMatcher(Equals):
> + """Check that a string is a valid endpoint for proxy token revocation."""
could you please update the docstring? Please use fetch service.
> +
> + def __init__(self, session_id):
> + super().__init__(
> + "{endpoint}/session/{session_id}".format(
> + endpoint=config.builddmaster.fetch_service_auth_api_endpoint,
> + session_id=session_id,
> + )
> + )
> diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
> index ac68bd4..9e2a463 100644
> --- a/lib/lp/services/config/schema-lazr.conf
> +++ b/lib/lp/services/config/schema-lazr.conf
> @@ -167,6 +167,23 @@ builder_proxy_host: none
> # Builder http proxy port
> builder_proxy_port: none
>
> +# Admin secret for requesting sessions from the fetch service.
> +# For a mojo deployed proxy service, the secret is generated and will reside in
> +# /srv/mojo/${MOJO_PROJECT}/${SERIES}/${MOJO_WORKSPACE}/local/admin-api-secret
> +fetch_service_auth_api_admin_secret: none
> +
> +# Admin username for fetch service.
> +fetch_service_auth_api_admin_username: none
Where is "auth" coming from?
In https://docs.google.com/document/d/1Ta0THOsHLwbOA6H7ewHa-6s2GtZRWxvvtiMKFk5jiq8/edit#heading=h.145ztybhclll the terminology "control API" is used. We should try hard to align the terminology, so we do not have two labels for the one thing, and always need to translate in our heads.
> +
> +# Endpoint for fetch service authentication service
> +fetch_service_auth_api_endpoint: none
> +
> +# Fetch service http proxy host
> +fetch_service_host: none
We will very likely use a proxy in front of several fetch services. Could we come up with a single name which accommodates both scenarios, with and without a proxy in front?
> +
> +# Fetch service http proxy port
> +fetch_service_port: none
> +
> [canonical]
> # datatype: boolean
> show_tracebacks: False
> @@ -1854,6 +1871,23 @@ builder_proxy_host: none
> # Deprecated in favour of the same key in the builddmaster section.
> builder_proxy_port: none
>
> +# Admin secret for requesting sessions from the fetch service.
> +# For a mojo deployed proxy service, the secret is generated and will reside in
> +# /srv/mojo/${MOJO_PROJECT}/${SERIES}/${MOJO_WORKSPACE}/local/admin-api-secret
> +fetch_service_auth_api_admin_secret: none
> +
> +# Admin username for fetch service.
> +fetch_service_auth_api_admin_username: none
> +
> +# Endpoint for fetch service authentication service
> +fetch_service_auth_api_endpoint: none
> +
> +# Fetch service http proxy host
> +fetch_service_host: none
> +
> +# Fetch service http proxy port
> +fetch_service_port: none
Why do we define all configuration twice?
> +
> # Optional sources.list entry to send to build workers when doing snap
> # package builds. Use this form so that the series is set:
> # deb http://foo %(series)s main
> diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
> index f2b8008..c3dc03c 100644
> --- a/lib/lp/snappy/tests/test_snap.py
> +++ b/lib/lp/snappy/tests/test_snap.py
> @@ -2373,14 +2372,10 @@ class TestSnapSet(TestCaseWithFactory):
> ]
>
> for field_name in admin_fields:
> - # exception is raised when user tried to update admin-only fields
> - with person_logged_in(non_admin):
> - self.assertRaises(
> - Unauthorized, setattr, snap, field_name, True
> - )
> - # exception isn't raised when an admin does the same
> - with admin_logged_in():
> - setattr(snap, field_name, True)
> + login(ANONYMOUS)
> + self.assertRaises(Unauthorized, setattr, snap, field_name, True)
> + login_admin()
> + setattr(snap, field_name, True)
>
You need to rebase onto Inês latest version for her MP.
> def test_snap_use_fetch_service_feature_flag(self):
> # The snap.use_fetch_service API only works when feature flag is set
> diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> index 5ec5804..36802cf 100644
> --- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> +++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> @@ -296,6 +317,132 @@ class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
> )
>
> @defer.inlineCallbacks
> + def test_requestFetchServiceSession_unconfigured(self):
What do you think of having fetch service related tests in a separate class?
Also, from reading the name of the test I do not know what this test is doing.
Could you come up with a more descriptive naming? In case it is hard to put in the test name, please add a docstring.
> + self.pushConfig("builddmaster", fetch_service_host=None)
> + self.useFixture(
> + FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
> + )
> + snap = self.factory.makeSnap(use_fetch_service=True)
> + request = self.factory.makeSnapBuildRequest(snap=snap)
> + job = self.makeJob(snap=snap, build_request=request)
> + with dbuser(config.builddmaster.dbuser):
> + args = yield job.extraBuildArgs()
> + self.assertEqual([], self.fetch_service_api.tokens.requests)
> + self.assertNotIn("proxy_url", args)
> + self.assertNotIn("revocation_endpoint", args)
> +
> + @defer.inlineCallbacks
> + def test_requestFetchServiceSession_no_secret(self):
>From reading the name of the test I do not know what this test is doing.
Could you come up with a more descriptive naming? In case it is hard to put in the test name, please add a docstring.
> + self.pushConfig(
> + "builddmaster", fetch_service_auth_api_admin_secret=None
> + )
> + self.useFixture(
> + FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
> + )
> + snap = self.factory.makeSnap(use_fetch_service=True)
> + request = self.factory.makeSnapBuildRequest(snap=snap)
> + job = self.makeJob(snap=snap, build_request=request)
> + expected_exception_msg = (
> + "fetch_service_auth_api_admin_secret is not configured."
> + )
> + with ExpectedException(CannotBuild, expected_exception_msg):
> + yield job.extraBuildArgs()
> +
> + @defer.inlineCallbacks
> + def test_requestFetchServiceSession(self):
>From reading the name of the test I do not know what this test is doing.
Could you come up with a more descriptive naming? In case it is hard to put in the test name, please add a docstring.
> + self.useFixture(
> + FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
> + )
> + 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()
> + expected_uri = urlsplit(
> + config.builddmaster.fetch_service_auth_api_endpoint
> + ).path.encode("UTF-8")
> + request_matcher = MatchesDict(
> + {
> + "method": Equals(b"POST"),
> + "uri": Equals(expected_uri),
> + "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(
> + {
> + "username": StartsWith(job.build.build_cookie + "-"),
> + "policy": Equals("permissive"),
> + }
> + ),
> + }
> + )
> + self.assertThat(
> + self.fetch_service_api.tokens.requests,
> + MatchesListwise([request_matcher]),
> + )
> +
> + @defer.inlineCallbacks
> + def test_requestFetchServiceSession_flag_off(self):
>From reading the name of the test I do not know what this test is doing.
Could you come up with a more descriptive naming? In case it is hard to put in the test name, please add a docstring.
> + self.useFixture(
> + FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: 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()
> + expected_uri = urlsplit(
> + config.builddmaster.builder_proxy_auth_api_endpoint
> + ).path.encode("UTF-8")
> + request_matcher = MatchesDict(
> + {
> + "method": Equals(b"POST"),
> + "uri": Equals(expected_uri),
> + "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(
> + {
> + "username": StartsWith(job.build.build_cookie + "-"),
> + }
> + ),
> + }
> + )
> + self.assertThat(
> + self.proxy_api.tokens.requests, MatchesListwise([request_matcher])
> + )
> +
> + @defer.inlineCallbacks
> def test_requestProxyToken_unconfigured(self):
> self.pushConfig("builddmaster", builder_proxy_host=None)
> branch = self.factory.makeBranch()
--
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/461721
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:fetch-service-session-API into launchpad:master.
References
