launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30953
Re: [Merge] ~pelpsi/launchpad:fetch-service-session-API into launchpad:master
Good work - left a few comments!
I'll leave the approval to Jürgen who is more into the details of the whole project.
Diff comments:
> diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
> index be65e29..4f249a7 100644
> --- a/lib/lp/buildmaster/builderproxy.py
> +++ b/lib/lp/buildmaster/builderproxy.py
> @@ -52,6 +62,25 @@ class BuilderProxyMixin:
> endpoint=_get_proxy_config("builder_proxy_auth_api_endpoint"),
> token=token["username"],
> )
> + if (
This should be an `elif` so that it doesn't try to check the second `if statement if the first is true.
Also, how about doing:
```
if not allow_internet:
return
if fetch_service and _get_proxy_config("fetch_service_host"):
...
elif not fetch_service and _get_proxy_config("builder_proxy_host"):
...
```
Seems a bit easier to follow IMO
> + fetch_service
> + and _get_proxy_config("fetch_service_host")
> + and allow_internet
> + ):
> + # http://<session-id>:<secret>@<addr>:9988/
Can you be more clear with what you mean with the comment? Is this the expected proxy_url format? If so, I see the string format bellow already hints the format quite well, so perhaps this is not needed
> + token = yield self._requestFetchServiceSession()
> + 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(
> + endpoint=_get_proxy_config("fetch_service_auth_api_endpoint"),
> + id=token["id"],
> + )
>
> @defer.inlineCallbacks
> def _requestProxyToken(self):
> @@ -86,3 +115,39 @@ class BuilderProxyMixin:
> proxy_username=proxy_username,
> )
> return token
> +
> + @defer.inlineCallbacks
> + def _requestFetchServiceSession(self):
> + # This is a different function if we have the needs to
Not sure what you mean in this comment, can you clarify?
> + # differentiate more the behavior.
> + admin_username = _get_proxy_config(
> + "fetch_service_auth_api_admin_username"
> + )
> + if not admin_username:
Since this pattern is used so much in this class here and by the builder proxy route (checking a variable and raising), perhaps we could have a method to check values are in config and raise separately?
Something like:
```
def get_value_from_config(self, key: string):
value = _get_proxy_config(key)
if not value:
raise CannotBuild(f"'{key}' is not configured")
return value
```
Which could then be called as:
```
admin_username = self.get_value_from_config("fetch_service_auth_api_admin_username")
```
Just a thought, this is also fine as is.
> + raise CannotBuild(
> + "fetch_service_auth_api_admin_username is not configured."
> + )
> + secret = _get_proxy_config("fetch_service_auth_api_admin_secret")
> + if not secret:
> + raise CannotBuild(
> + "fetch_service_auth_api_admin_secret is not configured."
> + )
> + url = _get_proxy_config("fetch_service_auth_api_endpoint")
> + if not secret:
> + raise CannotBuild(
> + "fetch_service_auth_api_endpoint is not configured."
> + )
> + 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(
> + 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..cee308a 100644
> --- a/lib/lp/buildmaster/downloader.py
> +++ b/lib/lp/buildmaster/downloader.py
> @@ -94,3 +110,24 @@ 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 2024-02-28: should take the same
I'm not sure I understand what's missing by this comment, can you clarify?
> + # json body? From ST108 we should provide
> + # {
> + # "timeout": <int>, // session timeout in seconds
> + # "policy": <string> // "strict" or "permissive"
> + # }
> + response = session.post(
> + url,
> + headers={"Authorization": auth_header},
> + json={"username": proxy_username},
> + )
> + print(" CI SIAMO???? ")
Extra comment left here :)
> + 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..9a12933
> --- /dev/null
> +++ b/lib/lp/buildmaster/tests/fetchservice.py
> @@ -0,0 +1,119 @@
> +# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
s/2015-2019/2024
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Fixtures for dealing with the build time HTTP proxy."""
> +
> +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."""
> +
> + 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.
> +
> + 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):
> + """Check that a string is a valid url for a builder proxy."""
> +
> + 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."""
> +
> + def __init__(self, session_id):
> + super().__init__(
> + "{}/session/{}".format(
Add variable names to make it easier to read?
> + config.builddmaster.fetch_service_auth_api_endpoint, session_id
> + )
> + )
> diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> index 5ec5804..45ee8c8 100644
> --- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> +++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> @@ -296,6 +317,85 @@ class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
> )
>
> @defer.inlineCallbacks
> + def test_requestFetchServiceSession_unconfigured(self):
> + self.pushConfig("builddmaster", fetch_service_host=None)
> + self.useFixture(
> + FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
Is t worth it to test that if the feature flag is OFF, it should defaut to the builder proxy? When the feature flag is off, the `snap.use_fetch_service` should return None so it should use the builder proxy, is that right?
> + )
> + 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):
> + 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):
> + 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 + "-"),
> + }
> + ),
> + }
> + )
> + self.assertThat(
> + self.fetch_service_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
