From 1e1784de55ca687fe80adbf386c4234373da185f Mon Sep 17 00:00:00 2001 From: evonetix_matthall Date: Mon, 23 Jan 2023 11:40:58 +0000 Subject: [PATCH 1/5] Proposed fix for ACR caching while providing guidance for GitLab users. --- binderhub/registry.py | 1 - .../zero-to-binderhub/setup-binderhub.rst | 23 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/binderhub/registry.py b/binderhub/registry.py index f915a78a6..2bdf43493 100644 --- a/binderhub/registry.py +++ b/binderhub/registry.py @@ -264,7 +264,6 @@ async def get_image_manifest(self, image, tag): client, self.token_url, scope=f"repository:{image}:pull", - service="container_registry", ) req = httpclient.HTTPRequest( url, diff --git a/docs/source/zero-to-binderhub/setup-binderhub.rst b/docs/source/zero-to-binderhub/setup-binderhub.rst index e0cefd5bf..8b06fae5c 100644 --- a/docs/source/zero-to-binderhub/setup-binderhub.rst +++ b/docs/source/zero-to-binderhub/setup-binderhub.rst @@ -174,6 +174,29 @@ where: If this is not provided, you may find BinderHub rebuilds images every launch instead of pulling them from the ACR. Suggestions for `` could be `ACR_NAME` or the name you give your BinderHub. +If you are using GitLab Container Registry +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If you want your BinderHub to push and pull images from a GitLab Container Registry, then your `config.yaml` file will look as follows:: + + config: + BinderHub: + use_registry: true + image_prefix: registry.gitlab.com///- + DockerRegistry: + token_url: "https://gitlab.com/jwt/auth?service=container_registry" + url: "https://registry.gitlab.com" + registry: + url: https://registry.gitlab.com + +where: + +* `` is your gitlab username, +* `` is an arbitrary name that is required due to BinderHub assuming that `image_prefix` contains an extra level for the project name. + See `this issue `_ for futher discussion. + If this is not provided, you may find BinderHub rebuilds images every launch instead of pulling them from the GitLab container registry. + Suggestions for `` could be the name you give your BinderHub. + If you are using OVH Container Registry ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From 3924ef9e9620766fca213ec02b0ee376ccb895c1 Mon Sep 17 00:00:00 2001 From: evonetix_matthall Date: Mon, 23 Jan 2023 11:50:33 +0000 Subject: [PATCH 2/5] Updated documentation regarding private GitLab CRs to be explicitly for private / authenticated ones (not applicable to public CRs). --- docs/source/zero-to-binderhub/setup-binderhub.rst | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/source/zero-to-binderhub/setup-binderhub.rst b/docs/source/zero-to-binderhub/setup-binderhub.rst index 8b06fae5c..28809e422 100644 --- a/docs/source/zero-to-binderhub/setup-binderhub.rst +++ b/docs/source/zero-to-binderhub/setup-binderhub.rst @@ -174,10 +174,10 @@ where: If this is not provided, you may find BinderHub rebuilds images every launch instead of pulling them from the ACR. Suggestions for `` could be `ACR_NAME` or the name you give your BinderHub. -If you are using GitLab Container Registry -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +If you are using an authenticated (private) GitLab Container Registry +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -If you want your BinderHub to push and pull images from a GitLab Container Registry, then your `config.yaml` file will look as follows:: +If you want your BinderHub to push and pull images from an authenticated (private) GitLab Container Registry, then your `config.yaml` file will look as follows:: config: BinderHub: @@ -186,12 +186,17 @@ If you want your BinderHub to push and pull images from a GitLab Container Regis DockerRegistry: token_url: "https://gitlab.com/jwt/auth?service=container_registry" url: "https://registry.gitlab.com" + username: + password: registry: url: https://registry.gitlab.com + username: + password: where: * `` is your gitlab username, +* `` and `` are valid GitLab credentials. (Passwords may be moved to `secret.yaml`.) * `` is an arbitrary name that is required due to BinderHub assuming that `image_prefix` contains an extra level for the project name. See `this issue `_ for futher discussion. If this is not provided, you may find BinderHub rebuilds images every launch instead of pulling them from the GitLab container registry. From 8d7649d02dd3a46312193f82a346e043df3a5b7a Mon Sep 17 00:00:00 2001 From: evonetix_matthall Date: Mon, 23 Jan 2023 12:42:12 +0000 Subject: [PATCH 3/5] Make scope query string parameter optional for registry `_get_token`. --- binderhub/registry.py | 17 ++++++++++----- binderhub/tests/test_registry.py | 37 +++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/binderhub/registry.py b/binderhub/registry.py index 2bdf43493..eee4b8473 100644 --- a/binderhub/registry.py +++ b/binderhub/registry.py @@ -204,13 +204,19 @@ def _parse_www_authenticate_header(self, header): ) from None async def _get_token(self, client, token_url, service, scope): + # specify required query parameters + query_params = { + "scope": scope, + } + + # add optional query parameters + if service is not None: + query_params["service"] = service + auth_req = httpclient.HTTPRequest( url_concat( token_url, - { - "scope": scope, - "service": service, - }, + query_params, ), auth_username=self.username, auth_password=self.password, @@ -263,7 +269,8 @@ async def get_image_manifest(self, image, tag): token = await self._get_token( client, self.token_url, - scope=f"repository:{image}:pull", + None, # service may be pre-set in self.token_url as per Helm chart docs for specific CRs + f"repository:{image}:pull", ) req = httpclient.HTTPRequest( url, diff --git a/binderhub/tests/test_registry.py b/binderhub/tests/test_registry.py index 1ad1485c6..a12d82a21 100644 --- a/binderhub/tests/test_registry.py +++ b/binderhub/tests/test_registry.py @@ -169,7 +169,7 @@ async def test_get_token(): test_handle = {"username": username, "password": password} app = Application( [ - (r"/token", MockTokenHandler, {"test_handle": test_handle}), + (r"/token", MockTokenHandler, {"test_handle": test_handle, "service": "service.1", "scope": "scope.2"}), ] ) ip = "127.0.0.1" @@ -196,6 +196,41 @@ async def test_get_token(): assert token == test_handle["token"] +async def test_get_token_with_config_supplied_service(): + # regression coverage for https://github.com/jupyterhub/binderhub/issues/1620 + + username = "user" + password = "pass" + test_handle = {"username": username, "password": password} + app = Application( + [ + (r"/token", MockTokenHandler, {"test_handle": test_handle, "service": "service.1", "scope": "scope.2"}), + ] + ) + ip = "127.0.0.1" + port = randint(10000, 65535) + app.listen(port, ip) + + registry = DockerRegistry( + url="https://example.org", username=username, password=password + ) + + assert registry.url == "https://example.org" + assert registry.auth_config_url == "https://example.org" + # token_url should be unset, since it should be determined by the caller from a + # WWW-Authenticate header + assert registry.token_url == "" + assert registry.username == username + assert registry.password == password + token = await registry._get_token( + httpclient.AsyncHTTPClient(), + f"http://{ip}:{port}/token?service=service.1", + None, + "scope.2", + ) + assert token == test_handle["token"] + + @pytest.mark.parametrize("token_url_known", [True, False]) async def test_get_image_manifest(tmpdir, token_url_known): username = "asdf" From e22eec1d305d733d0c1d2221cde8516e4f5c9667 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 23 Jan 2023 12:44:46 +0000 Subject: [PATCH 4/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/tests/test_registry.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/binderhub/tests/test_registry.py b/binderhub/tests/test_registry.py index a12d82a21..639ff5795 100644 --- a/binderhub/tests/test_registry.py +++ b/binderhub/tests/test_registry.py @@ -169,7 +169,15 @@ async def test_get_token(): test_handle = {"username": username, "password": password} app = Application( [ - (r"/token", MockTokenHandler, {"test_handle": test_handle, "service": "service.1", "scope": "scope.2"}), + ( + r"/token", + MockTokenHandler, + { + "test_handle": test_handle, + "service": "service.1", + "scope": "scope.2", + }, + ), ] ) ip = "127.0.0.1" @@ -204,7 +212,15 @@ async def test_get_token_with_config_supplied_service(): test_handle = {"username": username, "password": password} app = Application( [ - (r"/token", MockTokenHandler, {"test_handle": test_handle, "service": "service.1", "scope": "scope.2"}), + ( + r"/token", + MockTokenHandler, + { + "test_handle": test_handle, + "service": "service.1", + "scope": "scope.2", + }, + ), ] ) ip = "127.0.0.1" From 409ed4c8890c0d52c7ca38f5c6456160b4dc6c89 Mon Sep 17 00:00:00 2001 From: evonetix_matthall Date: Mon, 23 Jan 2023 13:24:13 +0000 Subject: [PATCH 5/5] Fix `test_get_image_manifest` to include `service` query string parameter if "token URL is known". --- binderhub/tests/test_registry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/tests/test_registry.py b/binderhub/tests/test_registry.py index a12d82a21..f5b3b9f0c 100644 --- a/binderhub/tests/test_registry.py +++ b/binderhub/tests/test_registry.py @@ -265,7 +265,7 @@ async def test_get_image_manifest(tmpdir, token_url_known): f, ) if token_url_known: - token_url = url + "/token" + token_url = url + "/token?service=container_registry" else: token_url = "" registry = DockerRegistry(