[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 =
[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,7 @@
[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 =
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
[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
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()
