launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+462875@xxxxxxxxxxxxxxxxxx>]

Thanks! I added some comments. This is almost done, but let's try to use consistent, descriptive and RFC compliant naming for the hosts.

Do you think we need a feature flag for this? I do not think so, but you might have more insight.

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.

It would be awesome to know which ones these are. I do not know them from the top of the head, but we could rephrase it to make it more clear (feel free to update wording):

```
This starts an internal proxy that stands before the Builder
Proxy/Fetch Service, so build systems, which do not comply
with standard `http(s)_proxy` environment variables, would
still work with the builder proxy.
```

> +        """
>          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",

I'd remove the "old" - this adds no value, and might confuse future developers.

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

Usually, it is a good idea not to mix new feature development with clean up tasks like this. This makes review harder (not in this case), and it changes the git history for these lines to a completely unrelated commit message.

You can keep this in this MP, but please create a separate commit for it.

>          )
>  
>          # 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)

These kind of tests without an assert statement are pretty confusing - and I am aware that you did not create the `startBuild` method, but you are just following the current practice.

Maybe we can come up with a better name (`assertExpectedCommandsArePresent` or similar - but not now.

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

Could you please update the URLs, see other comment about the RFC which offers standardized URLs for testing?

I am a bit confused about the two URLs. The one is for the orchestration endpoints, the other one is for the proxy, right?

What about `orchestration.fetch-service.example.net` and `proxy.fetch-service.example.net`?

> 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";

We should try to use consistent host names for tests.

Usually, example.net is used for tests, see https://www.iana.org/help/example-domains.

Another option could be to use the future name for the endpoint and just append example.net at the end? Though I like the ones I came up earlier.

> +        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";

at least "example" -> "example.net"

> +
> +        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:

thefetch -> the fetch

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

Could you please use the common TODO format? See https://documentation.ubuntu.com/launchpad/en/latest/explanation/xxx-policy/ - I am not 100% sure whether I really like that format, but at least we have something consistent. And also IDEs are able to highlight (consistent) todo comments.

> +        authentication. We might decide to instead use the token itself as the
> +        authentication.

Indeed, we have agreed with Claudio that the token is enough to revoke an endpoint. This might be subject to change due to the upcoming security meeting on Tuesday, 16th of April.

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