launchpad-reviewers team mailing list archive

[Ines Almeida <mp+462875@xxxxxxxxxxxxxxxxxx>]

Thanks you for the comments!

I don't think we need feature flag here simply because the feature flag in the launchpad code base should already do the same. The fetch service code in buildd will only be reached if the snap object has a `use_fetch_service` bool True - which will only be possible if the launchpad feature flag is ON.

As mentioned, I am only planning on merging this once the auth for the token revocation is defined in the meeting about the auth.

Diff comments:

> diff --git a/lpbuildd/proxy.py b/lpbuildd/proxy.py
> index ea33dda..1c8b532 100644
> --- a/lpbuildd/proxy.py
> +++ b/lpbuildd/proxy.py
> @@ -214,8 +214,18 @@ class BuilderProxyFactory(http.HTTPFactory):
>  
>  
>  class BuildManagerProxyMixin:
> +    @property
> +    def _use_fetch_service(self):
> +        return hasattr(self, "use_fetch_service") and getattr(
> +            self, "use_fetch_service"
> +        )
> +
>      def startProxy(self):
> -        """Start the local builder proxy, if necessary."""
> +        """Start the local builder proxy, if necessary.
> +
> +        This starts an internal proxy that stands before the Builder
> +        Proxy/Fetch Service. It is important for certain build types.

Sounds great, updated, thanks!

> +        """
>          if not self.proxy_url:
>              return []
>          proxy_port = self._builder._config.get("builder", "proxyport")
> diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
> index d9052f2..d649fad 100644
> --- a/lpbuildd/target/build_snap.py
> +++ b/lpbuildd/target/build_snap.py
> @@ -102,6 +102,12 @@ class BuildSnap(
>              action="store_true",
>              help="disable proxy access after the pull phase has finished",
>          )
> +        parser.add_argument(
> +            "--use_fetch_service",
> +            default=False,
> +            action="store_true",
> +            help="use the fetch service instead of the old builder proxy",

The issue with naming here is that the fetch service is a builder proxy as well, so this can be confusing. Happy to remove the 'old' though

> +        )
>          parser.add_argument("name", help="name of snap to build")
>  
>      def install_svn_servers(self):
> diff --git a/lpbuildd/tests/test_snap.py b/lpbuildd/tests/test_snap.py
> index 9bef60a..94b6706 100644
> --- a/lpbuildd/tests/test_snap.py
> +++ b/lpbuildd/tests/test_snap.py
> @@ -283,7 +283,8 @@ class TestSnapBuildManagerIteration(TestCase):
>              "/build/test-snap/test-snap_0_all.snap", b"I am a snap package."
>          )
>          self.buildmanager.backend.add_file(
> -            "/build/test-snap/test-snap+somecomponent_0.comp", b"I am a component."
> +            "/build/test-snap/test-snap+somecomponent_0.comp",
> +            b"I am a component.",

Hmmm, this was added automatically with the `pre-commit`. I'll separate it into a new commit before merging

>          )
>  
>          # After building the package, reap processes.
> @@ -754,6 +755,13 @@ class TestSnapBuildManagerIteration(TestCase):
>          yield self.startBuild(args, expected_options)
>  
>      @defer.inlineCallbacks
> +    def test_iterate_use_fetch_service(self):
> +        # The build manager can be told to use the fetch service as its proxy.
> +        args = {"use_fetch_service": True}
> +        expected_options = ["--use_fetch_service"]
> +        yield self.startBuild(args, expected_options)

Agree

> +
> +    @defer.inlineCallbacks
>      def test_iterate_disable_proxy_after_pull(self):
>          self.builder._config.set("builder", "proxyport", "8222")
>          args = {
> @@ -874,3 +882,25 @@ class TestSnapBuildManagerIteration(TestCase):
>          # XXX cjwatson 2023-02-07: Ideally we'd check the timeout as well,
>          # but the version of responses in Ubuntu 20.04 doesn't store it
>          # anywhere we can get at it.
> +
> +    @responses.activate
> +    def test_revokeProxyToken_fetch_service(self):
> +        session_id = "123"
> +
> +        responses.add(
> +            "DELETE", f"http://fetch-service.example/{session_id}/token";
> +        )
> +
> +        self.buildmanager.use_fetch_service = True
> +        self.buildmanager.revocation_endpoint = (
> +            f"http://fetch-service.example/{session_id}/token";
> +        )
> +        self.buildmanager.proxy_url = "http://session_id:test@proxy.example";
> +
> +        self.buildmanager.revokeProxyToken()
> +
> +        self.assertEqual(1, len(responses.calls))
> +        request = responses.calls[0].request
> +        self.assertEqual(
> +            f"http://fetch-service.example/{session_id}/token";, request.url
> +        )

There are a lot of places within this file and the rest of the repo that use the `.example`, so I think I might be more consistent to keep it IMO. I updated the URLs though.

I called it `control.fetch-service` instead of `orchestration` because I think that's been the most common name from the specs.

> diff --git a/lpbuildd/tests/test_util.py b/lpbuildd/tests/test_util.py
> index 005d42b..15c861c 100644
> --- a/lpbuildd/tests/test_util.py
> +++ b/lpbuildd/tests/test_util.py
> @@ -59,3 +67,43 @@ class TestSetPersonality(TestCase):
>              ["linux64", "sbuild"],
>              set_personality(["sbuild"], "amd64", series="trusty"),
>          )
> +
> +
> +class TestRevokeToken(TestCase):
> +    @responses.activate
> +    def test_revoke_proxy_token(self):
> +        """Proxy token revocation uses the right authentication"""
> +
> +        proxy_url = "http://username:password@host:port";
> +        revocation_endpoint = "http://builder.api.endpoint.token";

I updated the URLs to use the exact same ones as the previous tests. Note that this particular one is for the builder proxy (not the fetch service)

> +        token = base64.b64encode(b"username:password").decode()
> +
> +        responses.add(responses.DELETE, revocation_endpoint)
> +
> +        revoke_proxy_token(proxy_url, revocation_endpoint)
> +        self.assertEqual(1, len(responses.calls))
> +        request = responses.calls[0].request
> +        self.assertEqual("http://builder.api.endpoint.token/";, request.url)
> +        self.assertEqual(f"Basic {token}", request.headers["Authorization"])
> +
> +    @responses.activate
> +    def test_revoke_fetch_service_token(self):
> +        """Proxy token revocation for the fetch service"""
> +
> +        token = "test-token"
> +        proxy_url = f"http://session_id:{token}@host:port";
> +        revocation_endpoint = "http://fetch-service.example/session_id/token";

As mentioned, the rest of this repo has a lot of instances of `.example`... I think renaming these would only make sense by renaming all others, and I think that should go into a separate issue. I updated this to be the same as in the tests above though

> +
> +        responses.add(responses.DELETE, revocation_endpoint)
> +
> +        revoke_proxy_token(
> +            proxy_url,
> +            revocation_endpoint,
> +            use_fetch_service=True,
> +        )
> +
> +        self.assertEqual(1, len(responses.calls))
> +        request = responses.calls[0].request
> +        self.assertEqual(
> +            "http://fetch-service.example/session_id/token";, request.url
> +        )
> diff --git a/lpbuildd/util.py b/lpbuildd/util.py
> index eb35f01..0171e1f 100644
> --- a/lpbuildd/util.py
> +++ b/lpbuildd/util.py
> @@ -68,15 +68,34 @@ class RevokeProxyTokenError(Exception):
>          return f"Unable to revoke token for {self.username}: {self.exception}"
>  
>  
> -def revoke_proxy_token(proxy_url, revocation_endpoint):
> +def revoke_proxy_token(
> +    proxy_url, revocation_endpoint, use_fetch_service=False
> +):
>      """Revoke builder proxy token.
>  
> +    If not using the fetch service:
> +        The proxy_url for the current Builder Proxy has the following format:
> +        http://{username}:{password}@{host}:{port}
> +
> +        We use the username-password combo from the proxy_url for
> +        authentication to revoke its token.
> +
> +    If using thefetch service:

Updated

> +        The call to revoke a token does not require authentication.
> +
> +        TODO this might change depending on conversations about fetch service

IDEs should highlight the `TODO` comments as much as the `XXX` ones since it's a generally used format as well (https://peps.python.org/pep-0350/)  - mine does at least.

Regardless, IMO this should only get merged after that gets defined - this TODO was a note to the reviewer and to myself. I updated it since it's not a lot of work

> +        authentication. We might decide to instead use the token itself as the
> +        authentication.
> +
>      :raises RevokeProxyTokenError: if attempting to revoke the token failed.
>      """
>      url = urlparse(proxy_url)
> +
> +    auth = None
> +    if not use_fetch_service:
> +        auth = (url.username, url.password)
> +
>      try:
> -        requests.delete(
> -            revocation_endpoint, auth=(url.username, url.password), timeout=15
> -        )
> +        requests.delete(revocation_endpoint, auth=auth, timeout=15)
>      except requests.RequestException as e:
>          raise RevokeProxyTokenError(url.username, e)


-- 
https://code.launchpad.net/~ines-almeida/launchpad-buildd/+git/launchpad-buildd/+merge/462875
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master.