diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f727abc..5e0423e 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 73cfc74..166f4a8 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 0fe4fe0..ed01b46 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 45d1407..f049781 100644 --- a/skipper/utils.py +++ b/skipper/utils.py @@ -3,13 +3,13 @@ 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/' @@ -103,8 +103,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) @@ -119,10 +122,24 @@ def get_image_digest(registry, image, tag, username, password): 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 +195,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 85bf0fa..104b3f7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1057,7 +1057,7 @@ 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="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])) @@ -1079,7 +1079,7 @@ 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="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 707d101..fc22460 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 e93e0cc..e0c7c4b 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 b002f5f..88c5a48 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,43 @@ 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_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")