launchpad-reviewers team mailing list archive

[Ines Almeida <mp+461721@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 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

Will you ask before merging this MP? If so that's fine, but if we want to merge this MP first, then I'd clarify the comment itself so that a person that reads it understands what you just said above

> +            # 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???? ")
> +            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

You updated this to 2015-2024 but it should be 
```
 Copyright 2024 Canonical Ltd.
```

This is a new file (starting in 2024) and we no longer want to add the end date to these.

> +# 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(
> +                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"})

The goal of that test would be IMO to verify that things work whether the feature flag is ON or OFF, which I think is worth a small test IMO

> +        )
> +        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.