Fix skipper rmi -r and images -r remote registry operations

.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

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]

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")
 

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):

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",

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,

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
     ):

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")