From 23942bae0a96cae756e094ba217632f3c9ab5747 Mon Sep 17 00:00:00 2001 From: Tomer Date: Sun, 5 Jul 2026 10:53:23 +0300 Subject: [PATCH] Fix #193 skipper rmi -r and images -r remote registry operations Some registries (e.g. Harbor) run a token service that only accepts GET and require every repository to live under a namespace, breaking three remote-registry operations that worked against the previous registry. rmi -r: the manifest DELETE was routed through HttpBearerAuth, which fetches the bearer token by copying the original request method, sending DELETE to the token endpoint (405). Fetch the token with an explicit GET against the challenge realm, then send the DELETE with the resulting Authorization: Bearer header. rmi -r: image validation only consulted Dockerfile.* names, which cannot contain a slash, so a namespaced / could never be deleted. Honor the containers config as well, matching images and build. rmi -r: the manifest lookup accepted only the Docker v2 media type, so digest resolution 404'd for images pushed as OCI (the default for recent docker/buildkit). Accept the OCI manifest/index and Docker manifest-list media types too, and raise on a failed lookup instead of KeyError. images -r: a non-namespaced name is rejected with BAD_REQUEST, which aborted the whole listing. Treat BAD_REQUEST like NAME_UNKNOWN/NOT_FOUND and skip the offending image with a warning instead of raising. Also drop the deprecated pkg_resources (removed from recent setuptools and absent on Python 3.14): use importlib.metadata for the version and locate packaged data via __file__, keeping the existing compatibility floor (importlib_metadata backport for Python < 3.8). The build job installs setuptools<81 so the released bootstrap skipper still imports. --- .github/workflows/build.yml | 2 +- pyproject.toml | 3 +- skipper/cli.py | 13 +++++---- skipper/utils.py | 34 +++++++++++++++++++---- tests/test_cli.py | 27 ++++++++++++------ tests/test_runner.py | 18 ++++++------ tests/test_runner_podman.py | 10 +++---- tests/test_utils.py | 55 +++++++++++++++++++++++++++++++++++++ 8 files changed, 128 insertions(+), 34 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f727abcb..5e0423e5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -23,7 +23,7 @@ jobs: python-version: 3.x - name: Install dependencies run: | - pip install --user strato-skipper + pip install --user "setuptools<81" strato-skipper mkdir -p ~/.docker echo "{}" > ~/.docker/config.json touch ${HOME}/.gitconfig diff --git a/pyproject.toml b/pyproject.toml index 73cfc743..166f4a87 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,7 +34,8 @@ dependencies = [ "requests-bearer==0.5.1", "retry", "setuptools", - "pbr" + "pbr", + 'importlib_metadata; python_version < "3.8"' ] [project.optional-dependencies] diff --git a/skipper/cli.py b/skipper/cli.py index 0fe4fe04..ed01b46c 100644 --- a/skipper/cli.py +++ b/skipper/cli.py @@ -9,8 +9,11 @@ import click import six import tabulate +try: + from importlib.metadata import version as package_version +except ImportError: # Python < 3.8 + from importlib_metadata import version as package_version from pbr import packaging -from pkg_resources import get_distribution from skipper import builder, git, runner, utils from skipper.builder import BuildOptions, Image @@ -227,7 +230,7 @@ def rmi(ctx, remote, image, tag): Delete an image from local docker or from registry """ utils.logger.debug("Executing rmi command") - _validate_project_image(image) + _validate_project_image(image, ctx.obj.get("containers")) if remote: _validate_global_params(ctx, "registry") utils.delete_image_from_registry( @@ -360,7 +363,7 @@ def version(): output skipper version """ utils.logger.debug("printing skipper version") - click.echo(get_distribution("strato-skipper").version) # pylint: disable=no-member + click.echo(package_version("strato-skipper")) @cli.command() @@ -439,8 +442,8 @@ def _validate_global_params(ctx, *params): raise click.BadParameter(str(ctx.obj[param]), param_hint=param) -def _validate_project_image(image): - project_images = utils.get_images_from_dockerfiles() +def _validate_project_image(image, containers=None): + project_images = containers or utils.get_images_from_dockerfiles() if image not in project_images: raise click.BadParameter(f"'{image}' is not an image of this project, try {project_images}", param_hint="image") diff --git a/skipper/utils.py b/skipper/utils.py index 45d14075..9c0afd9f 100644 --- a/skipper/utils.py +++ b/skipper/utils.py @@ -3,18 +3,24 @@ import json import logging import os +import re import subprocess from shutil import which from six.moves import http_client import requests from requests_bearer import HttpBearerAuth import urllib3 -import pkg_resources REGISTRY_BASE_URL = 'https://%(registry)s/v2/' IMAGE_TAGS_URL = REGISTRY_BASE_URL + '%(image)s/tags/list' MANIFEST_URL = REGISTRY_BASE_URL + '%(image)s/manifests/%(reference)s' +MANIFEST_ACCEPT = ', '.join([ + 'application/vnd.docker.distribution.manifest.v2+json', + 'application/vnd.docker.distribution.manifest.list.v2+json', + 'application/vnd.oci.image.manifest.v1+json', + 'application/vnd.oci.image.index.v1+json', +]) DOCKER = "docker" PODMAN = "podman" @@ -103,8 +109,11 @@ def get_remote_image_info(image, registry, username, password): if info['tags']: image_info += [[registry, image, tag] for tag in info['tags']] else: - if info['errors'][0]['code'] in ['NAME_UNKNOWN', 'NOT_FOUND']: + error_code = info['errors'][0]['code'] + if error_code in ['NAME_UNKNOWN', 'NOT_FOUND']: pass + elif error_code == 'BAD_REQUEST': + logger.warning("Skipping image %s: registry rejected the name (%s)", image, info['errors'][0].get('message')) else: raise RuntimeError(info) @@ -114,15 +123,30 @@ def get_remote_image_info(image, registry, username, password): def get_image_digest(registry, image, tag, username, password): urllib3.disable_warnings() url = MANIFEST_URL % {"registry": registry, "image": image, "reference": tag} - headers = {"Accept": "application/vnd.docker.distribution.manifest.v2+json"} + headers = {"Accept": MANIFEST_ACCEPT} response = requests.get(url=url, headers=headers, verify=False, auth=HttpBearerAuth(username, password)) + response.raise_for_status() return response.headers['Docker-Content-Digest'] +def get_registry_token(www_authenticate, username, password): + params = dict(re.findall(r'(\w+)="([^"]+)"', www_authenticate[len("Bearer "):])) + realm = params.pop("realm") + response = requests.get(url=realm, params=params, verify=False, auth=(username, password)) + response.raise_for_status() + return response.json()["token"] + + def delete_image_from_registry(registry, image, tag, username, password): + urllib3.disable_warnings() digest = get_image_digest(registry, image, tag, username, password) url = MANIFEST_URL % {"registry": registry, "image": image, "reference": digest} - response = requests.delete(url=url, verify=False, auth=HttpBearerAuth(username, password)) + response = requests.delete(url=url, verify=False) + if response.status_code == http_client.UNAUTHORIZED: + challenge = response.headers.get("WWW-Authenticate", "") + if challenge.startswith("Bearer "): + token = get_registry_token(challenge, username, password) + response = requests.delete(url=url, headers={"Authorization": f"Bearer {token}"}, verify=False) response.raise_for_status() @@ -178,7 +202,7 @@ def get_runtime_command(): def get_extra_file(filename): - return pkg_resources.resource_filename("skipper", f"data/{filename}") + return os.path.join(os.path.dirname(os.path.abspath(__file__)), "data", filename) def run_container_command(args): diff --git a/tests/test_cli.py b/tests/test_cli.py index 85bf0fa5..ab56a368 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1050,14 +1050,14 @@ def test_rmi_remote(self, requests_get_mock, requests_delete_mock, requests_bear url = "https://%(registry)s/v2/%(image)s/manifests/%(reference)s" % dict( registry=REGISTRY, image=IMAGE, reference=TAG ) - headers = {"Accept": "application/vnd.docker.distribution.manifest.v2+json"} + headers = {"Accept": utils.MANIFEST_ACCEPT} requests_get_mock.assert_called_once_with( url=url, headers=headers, verify=False, auth=requests_bearer_auth_mock() ) url = "https://%(registry)s/v2/%(image)s/manifests/%(reference)s" % dict( registry=REGISTRY, image=IMAGE, reference="digest" ) - requests_delete_mock.assert_called_once_with(url=url, verify=False, auth=requests_bearer_auth_mock()) + requests_delete_mock.assert_called_once_with(url=url, verify=False) @mock.patch("skipper.utils.HttpBearerAuth", autospec=True) @mock.patch("glob.glob", mock.MagicMock(autospec=True, return_value=["Dockerfile." + IMAGE])) @@ -1072,14 +1072,14 @@ def test_rmi_remote_fail(self, requests_get_mock, requests_delete_mock, requests url = "https://%(registry)s/v2/%(image)s/manifests/%(reference)s" % dict( registry=REGISTRY, image=IMAGE, reference=TAG ) - headers = {"Accept": "application/vnd.docker.distribution.manifest.v2+json"} + headers = {"Accept": utils.MANIFEST_ACCEPT} requests_get_mock.assert_called_once_with( url=url, headers=headers, verify=False, auth=requests_bearer_auth_mock() ) url = "https://%(registry)s/v2/%(image)s/manifests/%(reference)s" % dict( registry=REGISTRY, image=IMAGE, reference="digest" ) - requests_delete_mock.assert_called_once_with(url=url, verify=False, auth=requests_bearer_auth_mock()) + requests_delete_mock.assert_called_once_with(url=url, verify=False) @mock.patch("glob.glob", mock.MagicMock(autospec=True, return_value=["Dockerfile." + IMAGE])) def test_validate_project_image(self): @@ -1088,6 +1088,18 @@ def test_validate_project_image(self): ) self.assertIsInstance(result.exception, click.BadParameter) + @mock.patch("skipper.utils.delete_image_from_registry", autospec=True) + @mock.patch("glob.glob", mock.MagicMock(autospec=True, return_value=["Dockerfile." + IMAGE])) + def test_rmi_remote_accepts_namespaced_image_from_config(self, delete_image_from_registry_mock): + result = self._invoke_cli( + defaults={"containers": {"foo/bar": "Dockerfile.foo"}}, + global_params=self.global_params, + subcmd="rmi", + subcmd_params=["-r", "foo/bar", TAG], + ) + self.assertIsNone(result.exception) + delete_image_from_registry_mock.assert_called_once() + @mock.patch("subprocess.check_output", mock.MagicMock(autospec=True, return_value="1234567\n")) @mock.patch("skipper.runner.run", autospec=True) def test_run_with_existing_local_build_container(self, skipper_runner_run_mock): @@ -1944,11 +1956,10 @@ def test_shell(self, skipper_runner_run_mock): ) @mock.patch("click.echo", autospec=True) - @mock.patch("skipper.cli.get_distribution", autospec=True) - def test_version(self, get_dist_mock, echo_mock): + @mock.patch("skipper.cli.package_version", autospec=True) + def test_version(self, package_version_mock, echo_mock): expected_version = "1.2.3" - get_dist_mock.return_value = mock.MagicMock() - get_dist_mock.return_value.version = expected_version + package_version_mock.return_value = expected_version self._invoke_cli( subcmd="version", diff --git a/tests/test_runner.py b/tests/test_runner.py index 707d101e..fc22460f 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -75,7 +75,7 @@ def test_run_complex_command_not_nested(self, popen_mock): @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_network_exist( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -145,7 +145,7 @@ def test_run_simple_command_nested_network_exist( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_network_not_exist( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -215,7 +215,7 @@ def test_run_simple_command_nested_network_not_exist( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_with_env( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -289,7 +289,7 @@ def test_run_simple_command_nested_with_env( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_with_env_file( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -361,7 +361,7 @@ def test_run_simple_command_nested_with_env_file( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_with_multiple_env_files( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -435,7 +435,7 @@ def test_run_simple_command_nested_with_multiple_env_files( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_interactive( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -512,7 +512,7 @@ def test_run_simple_command_nested_interactive( ) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_complex_command_nested( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -582,7 +582,7 @@ def test_run_complex_command_nested( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_complex_command_nested_with_env( self, resource_filename_mock, check_output_mock, popen_mock, grp_getgrnam_mock, os_getuid_mock ): @@ -659,7 +659,7 @@ def test_run_complex_command_nested_with_env( @mock.patch("grp.getgrnam", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_complex_command_nested_with_special_case_verification( self, resource_filename_mock, diff --git a/tests/test_runner_podman.py b/tests/test_runner_podman.py index e93e0cc4..e0c7c4b8 100644 --- a/tests/test_runner_podman.py +++ b/tests/test_runner_podman.py @@ -62,7 +62,7 @@ def test_run_complex_command_not_nested(self, popen_mock): @mock.patch("os.getuid", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_simple_command_nested_network_exist( self, resource_filename_mock, check_output_mock, popen_mock, os_getuid_mock ): @@ -125,7 +125,7 @@ def test_run_simple_command_nested_network_exist( @mock.patch("os.getuid", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=True) + @mock.patch("skipper.utils.get_extra_file", autospec=True) def test_run_simple_command_nested_network_not_exist( self, resource_filename_mock, check_output_mock, popen_mock, os_getuid_mock ): @@ -188,7 +188,7 @@ def test_run_simple_command_nested_network_not_exist( @mock.patch("os.getuid", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_complex_command_nested(self, resource_filename_mock, check_output_mock, popen_mock, os_getuid_mock): resource_filename_mock.return_value = "entrypoint.sh" popen_mock.return_value.stdout.readline.side_effect = ["aaa", "bbb", "ccc", ""] @@ -249,7 +249,7 @@ def test_run_complex_command_nested(self, resource_filename_mock, check_output_m @mock.patch("os.getuid", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) @mock.patch("os.makedirs", mock.MagicMock(autospec=True, side_effect=mock_makedirs)) def test_run_non_existent_unauthorized_volume( self, resource_filename_mock, check_output_mock, popen_mock, os_getuid_mock @@ -315,7 +315,7 @@ def test_run_non_existent_unauthorized_volume( @mock.patch("os.getuid", autospec=True) @mock.patch("subprocess.Popen", autospec=False) @mock.patch("subprocess.check_output", autospec=False) - @mock.patch("pkg_resources.resource_filename", autospec=False) + @mock.patch("skipper.utils.get_extra_file", autospec=False) def test_run_complex_command_nested_with_env( self, resource_filename_mock, check_output_mock, popen_mock, os_getuid_mock ): diff --git a/tests/test_utils.py b/tests/test_utils.py index b002f5f7..b704aefc 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,9 +1,12 @@ +import logging import os import unittest from unittest import mock from skipper import utils +utils.configure_logging("skipper-test", logging.WARNING) + class TestUtils(unittest.TestCase): @mock.patch("skipper.utils.which", autospec=False) @@ -47,3 +50,55 @@ def test_create_path_and_add_data(self, path_exists_mock, makedir_mock, open_moc utils.create_path_and_add_data(test_file, "", True) makedir_mock.assert_not_called() open_mock.assert_called_once_with(test_file, "w") + + @mock.patch("skipper.utils.get_image_digest", autospec=True, return_value="sha256:abc") + @mock.patch("skipper.utils.requests", autospec=True) + def test_delete_image_from_registry_fetches_token_with_get(self, requests_mock, _digest_mock): + challenge = ( + 'Bearer realm="https://registry/service/token",' + 'service="harbor-registry",scope="repository:foo/bar:pull,delete"' + ) + unauthorized = mock.Mock(status_code=401, headers={"WWW-Authenticate": challenge}) + accepted = mock.Mock(status_code=202) + requests_mock.delete.side_effect = [unauthorized, accepted] + token_response = mock.Mock(status_code=200) + token_response.json.return_value = {"token": "TKN"} + requests_mock.get.return_value = token_response + + utils.delete_image_from_registry("registry", "foo/bar", "t1", "user", "password") + + # Token fetched via GET against the realm, never via DELETE. + token_call = requests_mock.get.call_args + self.assertEqual(token_call.kwargs["url"], "https://registry/service/token") + # Final manifest DELETE carries the bearer token obtained above. + final_delete = requests_mock.delete.call_args + self.assertEqual(final_delete.kwargs["headers"]["Authorization"], "Bearer TKN") + + @mock.patch("skipper.utils.requests", autospec=True) + def test_get_image_digest_accepts_oci_manifests(self, requests_mock): + response = mock.Mock(status_code=200, headers={"Docker-Content-Digest": "sha256:abc"}) + requests_mock.get.return_value = response + + digest = utils.get_image_digest("registry", "foo/bar", "t1", "user", "password") + + self.assertEqual(digest, "sha256:abc") + accept = requests_mock.get.call_args.kwargs["headers"]["Accept"] + self.assertIn("application/vnd.oci.image.manifest.v1+json", accept) + self.assertIn("application/vnd.docker.distribution.manifest.v2+json", accept) + + @mock.patch("skipper.utils.requests", autospec=True) + def test_get_remote_image_info_tolerates_harbor_bad_request(self, requests_mock): + response = mock.Mock(ok=False) + response.json.return_value = {"errors": [{"code": "BAD_REQUEST", "message": "invalid repository name"}]} + requests_mock.get.return_value = response + + self.assertEqual(utils.get_remote_image_info("bare-name", "registry", "user", "password"), []) + + @mock.patch("skipper.utils.requests", autospec=True) + def test_get_remote_image_info_raises_on_unknown_error(self, requests_mock): + response = mock.Mock(ok=False) + response.json.return_value = {"errors": [{"code": "INTERNAL", "message": "boom"}]} + requests_mock.get.return_value = response + + with self.assertRaises(RuntimeError): + utils.get_remote_image_info("foo/bar", "registry", "user", "password")