launchpad-reviewers team mailing list archive

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

That is a much cleaner approach - I like it a lot.

It is a pity that we do not have a coverage report, as I would be absolutely happy with no further unit tests as the functionality "should" be already 100% covered.

mypy is not happy yet

Diff comments:

> diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
> index 3375042..e4da105 100644
> --- a/lib/lp/buildmaster/builderproxy.py
> +++ b/lib/lp/buildmaster/builderproxy.py
> @@ -49,79 +49,159 @@ class BuilderProxyMixin:
>          self,
>          args: BuildArgs,
>          allow_internet: bool = True,
> -        fetch_service: bool = False,
> +        use_fetch_service: bool = False,
>      ) -> Generator[None, Dict[str, str], None]:
>          if not allow_internet:
>              return
> -        if not fetch_service and _get_proxy_config("builder_proxy_host"):
> -            token = yield self._requestProxyToken()
> -            args["proxy_url"] = (
> -                "http://{username}:{password}@{host}:{port}".format(
> -                    username=token["username"],
> -                    password=token["secret"],
> -                    host=_get_proxy_config("builder_proxy_host"),
> -                    port=_get_proxy_config("builder_proxy_port"),
> -                )
> -            )
> -            args["revocation_endpoint"] = "{endpoint}/{token}".format(
> -                endpoint=_get_proxy_config("builder_proxy_auth_api_endpoint"),
> -                token=token["username"],
> -            )
> -        elif fetch_service and _get_proxy_config("fetch_service_host"):
> -            session = yield self._requestFetchServiceSession()
> -            args["proxy_url"] = (
> -                "http://{session_id}:{token}@{host}:{port}".format(
> -                    session_id=session["id"],
> -                    token=session["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_control_endpoint"),
> -                session_id=session["id"],
> -            )
> +        if not use_fetch_service and _get_proxy_config("builder_proxy_host"):
> +            proxy_service = BuilderProxy
> +        elif use_fetch_service and _get_proxy_config("fetch_service_host"):
> +            proxy_service = FetchService
> +        else:

Could you please add an inline comment that describes what case this is handling and when this can occur?

> +            return
> +
> +        self.proxy_service = proxy_service(
> +            build_id=self.build.build_cookie, worker=self._worker
> +        )
> +        new_session = yield self.proxy_service.startSession()
> +        args["proxy_url"] = new_session["proxy_url"]
> +        args["revocation_endpoint"] = new_session["revocation_endpoint"]
> +
> +
> +class BuilderProxy:
> +
> +    def __init__(self, build_id, worker):
> +        self.control_endpoint = _get_value_from_config(
> +            "builder_proxy_auth_api_endpoint"
> +        )
> +        self.proxy_endpoint = "{host}:{port}".format(
> +            host=_get_proxy_config("builder_proxy_host"),
> +            port=_get_proxy_config("builder_proxy_port"),
> +        )
> +        self.auth_header = self._getAuthHeader()
> +
> +        self.build_id = build_id
> +        self.worker = worker
> +
> +    @staticmethod
> +    def _getAuthHeader():
> +        """Helper method to generate authentication needed to call the
> +        builder proxy authentication endpoint."""
>  
> -    @defer.inlineCallbacks
> -    def _requestProxyToken(self):
>          admin_username = _get_value_from_config(
>              "builder_proxy_auth_api_admin_username"
>          )
> -        secret = _get_value_from_config("builder_proxy_auth_api_admin_secret")
> -        url = _get_value_from_config("builder_proxy_auth_api_endpoint")
> +        admin_secret = _get_value_from_config(
> +            "builder_proxy_auth_api_admin_secret"
> +        )
> +        auth_string = f"{admin_username}:{admin_secret}".strip()
> +        return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
> +
> +    @defer.inlineCallbacks
> +    def startSession(self):
> +        """Request a token from the builder proxy to be used by the builders.
> +
> +        :returns: dictionary with an authenticated `proxy_url` for the builder
> +        to use, and a `revocation_endpoint` to revoke the token when it's no
> +        longer required.
> +        """
>          timestamp = int(time.time())
>          proxy_username = "{build_id}-{timestamp}".format(
> -            build_id=self.build.build_cookie, timestamp=timestamp
> +            build_id=self.build_id, 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(
> +        token = yield self.worker.process_pool.doWork(
>              RequestProxyTokenCommand,
> -            url=url,
> -            auth_header=auth_header,
> +            url=self.control_endpoint,
> +            auth_header=self.auth_header,
>              proxy_username=proxy_username,
>          )
> -        return token
>  
> -    @defer.inlineCallbacks
> -    def _requestFetchServiceSession(self):
> +        proxy_url = "http://{username}:{password}@{proxy_endpoint}".format(
> +            username=token["username"],
> +            password=token["secret"],
> +            proxy_endpoint=self.proxy_endpoint,
> +        )
> +        revocation_endpoint = "{endpoint}/{token}".format(
> +            endpoint=self.control_endpoint,
> +            token=token["username"],
> +        )
> +
> +        return {
> +            "proxy_url": proxy_url,
> +            "revocation_endpoint": revocation_endpoint,
> +        }
> +
> +
> +class FetchService:
> +
> +    PROXY_URL = "http://{session_id}:{token}@{proxy_endpoint}";
> +    START_SESSION_ENDPOINT = "{control_endpoint}"
> +    TOKEN_REVOCATION = "{control_endpoint}/session/{session_id}/token"
> +    RETRIEVE_METADATA_ENDPOINT = "{control_endpoint}/session/{session_id}"
> +    END_SESSION_ENDPOINT = "{control_endpoint}/session/{session_id}"
> +
> +    def __init__(self, build_id, worker):
> +        self.control_endpoint = _get_value_from_config(
> +            "fetch_service_control_endpoint"
> +        )
> +        self.proxy_endpoint = "{host}:{port}".format(
> +            host=_get_proxy_config("fetch_service_host"),
> +            port=_get_proxy_config("fetch_service_port"),
> +        )
> +        self.auth_header = self._getAuthHeader()
> +
> +        self.build_id = build_id
> +        self.worker = worker
> +        self.session_id = None
> +
> +    @staticmethod
> +    def _getAuthHeader():
> +        """Helper method to generate authentication needed to call the
> +        fetch service control endpoint."""
>          admin_username = _get_value_from_config(
>              "fetch_service_control_admin_username"
>          )
>          secret = _get_value_from_config("fetch_service_control_admin_secret")
> -        url = _get_value_from_config("fetch_service_control_endpoint")
> +        auth_string = f"{admin_username}:{secret}".strip()
> +        return b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
> +
> +    @defer.inlineCallbacks
> +    def startSession(self):
> +        """Requests a fetch service session and returns session information.
> +
> +        :returns: dictionary with an authenticated `proxy_url` for the builder
> +        to use, and a `revocation_endpoint` to revoke the token when it's no
> +        longer required.
> +        """
>          timestamp = int(time.time())
>          proxy_username = "{build_id}-{timestamp}".format(
> -            build_id=self.build.build_cookie, timestamp=timestamp
> +            build_id=self.build_id, timestamp=timestamp
>          )
> -        auth_string = f"{admin_username}:{secret}".strip()
> -        auth_header = b"Basic " + base64.b64encode(auth_string.encode("ASCII"))
>  
> -        session = yield self._worker.process_pool.doWork(
> +        session_data = yield self.worker.process_pool.doWork(
>              RequestFetchServiceSessionCommand,
> -            url=url,
> -            auth_header=auth_header,
> +            url=self.START_SESSION_ENDPOINT.format(
> +                control_endpoint=self.control_endpoint
> +            ),
> +            auth_header=self.auth_header,
>              proxy_username=proxy_username,
>          )
> -        return session
> +
> +        self.session_id = session_data["id"]
> +        token = session_data["token"]
> +
> +        proxy_url = self.PROXY_URL.format(
> +            session_id=self.session_id,
> +            token=token,
> +            proxy_endpoint=self.proxy_endpoint,
> +        )
> +        revocation_endpoint = self.TOKEN_REVOCATION.format(
> +            control_endpoint=self.control_endpoint,
> +            session_id=self.session_id,
> +        )
> +
> +        return {
> +            "proxy_url": proxy_url,
> +            "revocation_endpoint": revocation_endpoint,
> +        }


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464303
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master.