[Launchpad-reviewers] [Merge] ~pelpsi/launchpad:pass-information-via-XML-RPC-API-to-the-builders into launchpad:master
Simone Pelosi has proposed merging ~pelpsi/launchpad:pass-information-via-XML-RPC-API-to-the-builders into launchpad:master. Commit message: Passing certificate to the builders Builders need certificate to configure correctly env to use fetch service. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/464337 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:pass-information-via-XML-RPC-API-to-the-builders into launchpad:master. diff --git a/charm/launchpad-buildd-manager/config.yaml b/charm/launchpad-buildd-manager/config.yaml index caac316..630ce00 100644 --- a/charm/launchpad-buildd-manager/config.yaml +++ b/charm/launchpad-buildd-manager/config.yaml @@ -71,6 +71,10 @@ options: Fetch service host, it could be either a single instance or a load balancer in front. default: "" + fetch_service_mitm_certificate: +type: string +description: Fetch service certificate. +default: "" fetch_service_port: type: int description: Fetch service port. diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py index 3375042..5f3cf5b 100644 --- a/lib/lp/buildmaster/builderproxy.py +++ b/lib/lp/buildmaster/builderproxy.py @@ -82,6 +82,13 @@ class BuilderProxyMixin: session_id=session["id"], ) +# Append the fetch-service certificate to BuildArgs secrets. +if "secrets" not in args: +args["secrets"] = {} +args["secrets"]["fetch_service_mitm_certificate"] = ( +_get_value_from_config("fetch_service_mitm_certificate") +) + @defer.inlineCallbacks def _requestProxyToken(self): admin_username = _get_value_from_config( diff --git a/lib/lp/buildmaster/tests/fetchservice.py b/lib/lp/buildmaster/tests/fetchservice.py index 3fd879c..51f73e3 100644 --- a/lib/lp/buildmaster/tests/fetchservice.py +++ b/lib/lp/buildmaster/tests/fetchservice.py @@ -75,19 +75,18 @@ class InProcessFetchServiceAuthAPIFixture(fixtures.Fixture): 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_control_admin_secret: admin-secret -fetch_service_control_admin_username: admin-launchpad.test -fetch_service_control_endpoint: http://{host}:{port}/session -fetch_service_host: {host} -fetch_service_port: {port} -""" -).format(host=port.getHost().host, port=port.getHost().port), -) +configs = dedent( +""" +[builddmaster] +fetch_service_control_admin_secret: admin-secret +fetch_service_control_admin_username: admin-launchpad.test +fetch_service_control_endpoint: http://{host}:{port}/session +fetch_service_host: {host} +fetch_service_port: {port} +fetch_service_mitm_certificate: fake-cert +""" +).format(host=port.getHost().host, port=port.getHost().port) +config.push("in-process-fetch-service-api-fixture", configs) self.addCleanup(config.pop, "in-process-fetch-service-api-fixture") diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf index 94ca842..07f04c6 100644 --- a/lib/lp/services/config/schema-lazr.conf +++ b/lib/lp/services/config/schema-lazr.conf @@ -178,6 +178,9 @@ fetch_service_control_admin_username: none # Endpoint for fetch service authentication service fetch_service_control_endpoint: none +# Fetch service certificate +fetch_service_mitm_certificate: none + # Fetch service host, it could be either a single instance # or a load balancer in front fetch_service_host: none @@ -1883,6 +1886,9 @@ fetch_service_control_admin_username: none # Endpoint for fetch service control service. fetch_service_control_endpoint: none +# Fetch service certificate +fetch_service_mitm_certificate: none + # Fetch service host, it could be either a single instance # or a load balancer in front. fetch_service_host: none diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py index 2ccc18d..d20bf33 100644 --- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py +++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py @@ -303,6 +303,28 @@ class TestAsyncSnapBuildBehaviourFetchService( self.assertNotIn("revocation_endpoint", args) @defer.inlineCallbacks +def test_requestFetchServiceSession_no_certificate(self): +"""Create a snap build request with an incomplete fetch service +configuration. +
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master
@Ines I would love to have this merged asap, as both my and Simone's work base on this, and having this pre-requisite MPs juggling around is pretty cumbersome. The code path for the fetch service cannot be triggered yet, so I do not see any issues with merging this - even without the auth being specced out fully. -- 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. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master
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"Basi
[Launchpad-reviewers] [Merge] ~lgp171188/launchpad:update-bing-custom-search-site-url into launchpad:master
Guruprasad has proposed merging ~lgp171188/launchpad:update-bing-custom-search-site-url into launchpad:master. Commit message: Update Bing custom search site URL Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/464314 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:update-bing-custom-search-site-url into launchpad:master. diff --git a/configs/development/launchpad-lazr.conf b/configs/development/launchpad-lazr.conf index 4e80454..7527832 100644 --- a/configs/development/launchpad-lazr.conf +++ b/configs/development/launchpad-lazr.conf @@ -71,7 +71,7 @@ error_exchange: none [bing] # Development and the testrunner should use the stub service by default. -site: http://launchpad.test:8093/bingcustomsearch/v7.0/search +site: http://launchpad.test:8093/v7.0/custom/search subscription_key: abcdef01234567890abcdef012345678 custom_config_id: 1234567890 diff --git a/configs/testrunner/launchpad-lazr.conf b/configs/testrunner/launchpad-lazr.conf index 5de6421..00503ce 100644 --- a/configs/testrunner/launchpad-lazr.conf +++ b/configs/testrunner/launchpad-lazr.conf @@ -90,7 +90,7 @@ source_only: True root: /tmp/gina_test_archive [bing] -site: http://launchpad.test:8093/bingcustomsearch/v7.0/search +site: http://launchpad.test:8093/v7.0/custom/search [gpghandler] upload_keys: True diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf index fed991a..94ca842 100644 --- a/lib/lp/services/config/schema-lazr.conf +++ b/lib/lp/services/config/schema-lazr.conf @@ -889,11 +889,11 @@ launch: False [bing] # site is the host and path that search requests are made to. -# eg. https://api.cognitive.microsoft.com/bingcustomsearch/v7.0/search +# eg. https://api.bing.microsoft.com/v7.0/custom/search # datatype: string, a url to a host -site: https://api.cognitive.microsoft.com/bingcustomsearch/v7.0/search +site: https://api.bing.microsoft.com/v7.0/custom/search -# subscription_key is the Cognitive Services subscription key for +# subscription_key is the Bing subscription key for # Bing Custom Search API. # datatype: string subscription_key: diff --git a/lib/lp/services/sitesearch/__init__.py b/lib/lp/services/sitesearch/__init__.py index 8e78f60..822bae6 100644 --- a/lib/lp/services/sitesearch/__init__.py +++ b/lib/lp/services/sitesearch/__init__.py @@ -182,7 +182,7 @@ class BingSearchService: """The URL to the Bing Custom Search service. The URL is probably -https://api.cognitive.microsoft.com/bingcustomsearch/v7.0/search. +https://api.bing.microsoft.com/v7.0/custom/search. """ return config.bing.site diff --git a/lib/lp/services/sitesearch/tests/data/bingsearchservice-bugs-1.json b/lib/lp/services/sitesearch/tests/data/bingsearchservice-bugs-1.json index 835c9dd..07d7471 100644 --- a/lib/lp/services/sitesearch/tests/data/bingsearchservice-bugs-1.json +++ b/lib/lp/services/sitesearch/tests/data/bingsearchservice-bugs-1.json @@ -13,7 +13,7 @@ "totalEstimatedMatches": 25, "value": [ { -"id": "https://api.cognitive.microsoft.com/api/v7/#WebPages.0";, +"id": "https://api.bing.microsoft.com/api/v7/#WebPages.0";, "name": "Launchpad Bugs", "url": "https://launchpad.net/~ubuntu-bugs";, "urlPingSuffix": "DevEx,5080.1", @@ -24,7 +24,7 @@ "fixedPosition": false }, { -"id": "https://api.cognitive.microsoft.com/api/v7/#WebPages.1";, +"id": "https://api.bing.microsoft.com/api/v7/#WebPages.1";, "name": "Bugs in Ubuntu Linux", "url": "https://launchpad.net/gcleaner";, "urlPingSuffix": "DevEx,5095.1", @@ -35,7 +35,7 @@ "fixedPosition": false }, { -"id": "https://api.cognitive.microsoft.com/api/v7/#WebPages.2";, +"id": "https://api.bing.microsoft.com/api/v7/#WebPages.2";, "name": "Bugs related to Sample Person", "url": "https://help.launchpad.net/BugExpiry";, "urlPingSuffix": "DevEx,5110.1", @@ -46,7 +46,7 @@ "fixedPosition": false }, { -"id": "https://api.cognitive.microsoft.com/api/v7/#WebPages.3";, +"id": "https://api.bing.microsoft.com/api/v7/#WebPages.3";, "name": "Bug #1 in Mozilla Firefox: Firefox does not support SVG", "url": "https://launchpad.net/longomatch";, "urlPingSuffix": "DevEx,5125.1", @@ -57,7 +57,7 @@ "fixedPosition": false }, { -"id": "https://api.cognitive.microsoft.com/api/v7/#WebPages.4";, +"id": "https://api.bing.microsoft.com/api/v7/#WebPages.4";, "name": "Bugs in Source Package \"thunderbird\" in Ubuntu Linux", "url": "https://answers.launchpad.net/heat/+question/232632";, "urlPingSuffix": "DevEx,5140.1", @@ -69,7 +69,
[Launchpad-reviewers] [Merge] ~lgp171188/launchpad:allow-setting-bing-custom-search-endpoint-launchpad-appserver-charm into launchpad:master
Guruprasad has proposed merging ~lgp171188/launchpad:allow-setting-bing-custom-search-endpoint-launchpad-appserver-charm into launchpad:master. Commit message: charm/launchpad-appserver: Make the bing custom search endpoint configurable This allows us to update it without requiring any code changes to Launchpad itself. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/464308 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:allow-setting-bing-custom-search-endpoint-launchpad-appserver-charm into launchpad:master. diff --git a/charm/launchpad-appserver/config.yaml b/charm/launchpad-appserver/config.yaml index 7db0b26..b82a3d1 100644 --- a/charm/launchpad-appserver/config.yaml +++ b/charm/launchpad-appserver/config.yaml @@ -3,6 +3,10 @@ options: type: string description: Identifier for the Bing Custom Search instance. default: + bing_custom_search_endpoint: +type: string +description: The endpoint to send the Bing custom search requests to. +default: "https://api.bing.microsoft.com/v7.0/custom/search"; bing_subscription_key: type: string description: > diff --git a/charm/launchpad-appserver/templates/launchpad-appserver-lazr.conf b/charm/launchpad-appserver/templates/launchpad-appserver-lazr.conf index 642d52e..5d10f6d 100644 --- a/charm/launchpad-appserver/templates/launchpad-appserver-lazr.conf +++ b/charm/launchpad-appserver/templates/launchpad-appserver-lazr.conf @@ -12,6 +12,7 @@ extends: ../launchpad-db-lazr.conf [bing] +site: {{ bing_custom_search_endpoint }} {{- opt("custom_config_id", bing_custom_config_id) }} [launchpad] ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master. Commit message: Refactor buildd-manager builder proxy logic This separates the logic for making calls to the builder proxy and the fetch service out of the BuilderProxyMixin and into their own handlers. This will make it easier to later add the logic to end sessions or, retreieve metadata. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464303 All tests in `lp.snappy.tests` have be run and passed successfully. -- Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master. 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: +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}
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:fetch-service-buildd-manager-refactor into launchpad:master
The code itself should be tested from the existing unit tests, since the functionality was maintained. But I still need to have a look at adding new tests for the new classes. -- 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. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master
The proposal to merge ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master has been updated. Status: Needs review => Work in progress For more details, see: https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/464293 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master
Simone Pelosi has proposed merging ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master with ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service as a prerequisite. Commit message: Pass information via XML-RPC API to the builders Pass fetch-service proxy url containing token and session information. Set env variables to use proxy globally and pass certificate. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/464293 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master. diff --git a/lpbuildd/proxy.py b/lpbuildd/proxy.py index 1a05ff3..4645091 100644 --- a/lpbuildd/proxy.py +++ b/lpbuildd/proxy.py @@ -224,18 +224,25 @@ class BuildManagerProxyMixin: """Start the local builder proxy, if necessary. 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. +Proxy/Fetch Service. It is important for certain build types. """ if not self.proxy_url: return [] -proxy_port = self._builder._config.get("builder", "proxyport") proxy_factory = BuilderProxyFactory(self, self.proxy_url, timeout=60) +proxy_port = self._builder._config.get("builder", "proxyport") + +if self._use_fetch_service: +proxy_url = self.proxy_url.split("@") +proxy_port = proxy_url.split(":")[-1] + self.proxy_service = strports.service( "tcp:%s" % proxy_port, proxy_factory ) self.proxy_service.setServiceParent(self._builder.service) + +if self._use_fetch_service: +return ["--proxy-url", f"{self.proxy_url}"] + if hasattr(self.backend, "ipv4_network"): proxy_host = self.backend.ipv4_network.ip else: diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py index 82470d5..d649fad 100644 --- a/lpbuildd/target/build_snap.py +++ b/lpbuildd/target/build_snap.py @@ -106,7 +106,7 @@ class BuildSnap( "--use_fetch_service", default=False, action="store_true", -help="use the fetch service instead of the builder proxy", +help="use the fetch service instead of the old builder proxy", ) parser.add_argument("name", help="name of snap to build") diff --git a/lpbuildd/target/tests/test_build_snap.py b/lpbuildd/target/tests/test_build_snap.py index 8719e57..30f0c8f 100644 --- a/lpbuildd/target/tests/test_build_snap.py +++ b/lpbuildd/target/tests/test_build_snap.py @@ -194,6 +194,55 @@ class TestBuildSnap(TestCase): build_snap.backend.backend_fs["/root/.subversion/servers"], ) +def test_install_fetch_service(self): +args = [ +"buildsnap", +"--backend=fake", +"--series=xenial", +"--arch=amd64", +"1", +"--git-repository", +"lp:foo", +"--proxy-url", +"http://session:token@fetch-service.proxy:9988";, +"test-snap", +"--use_fetch_service" +] +build_snap = parse_args(args=args).operation +build_snap.bin = "/builderbin" +self.useFixture(FakeFilesystem()).add("/builderbin") +os.mkdir("/builderbin") +with open("/builderbin/lpbuildd-git-proxy", "w") as proxy_script: +proxy_script.write("proxy script\n") +os.fchmod(proxy_script.fileno(), 0o755) +build_snap.install() +self.assertThat( +build_snap.backend.run.calls, +MatchesListwise( +[ +RanAptGet( +"install", "python3", "socat", "git", "snapcraft" +), +RanCommand(["mkdir", "-p", "/root/.subversion"]), +] +), +) +self.assertEqual( +(b"proxy script\n", stat.S_IFREG | 0o755), +build_snap.backend.backend_fs["/usr/local/bin/lpbuildd-git-proxy"], +) +self.assertEqual( +( +b"[global]\n" +b"http-proxy-host = fetch-service.proxy\n" +b"http-proxy-port = 9988\n" +b"http-proxy-username = session\n" +b"http-proxy-password = token\n", +stat.S_IFREG | 0o644, +), +build_snap.backend.backend_fs["/root/.subversion/servers"], +) + def test_install_channels(self): args = [ "buildsnap", @@ -479,6 +528,68 @@ class TestBuildSnap
[Launchpad-reviewers] [Merge] ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master
The proposal to merge ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master has been updated. Status: Work in progress => Superseded For more details, see: https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/464292 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:pass-information-via-XML-RPC-API-to-the-builders into launchpad-buildd:master. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master
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/lpb
Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad-buildd:update-close-session-for-fetch-service into launchpad-buildd:master
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(