Handle <version>-<release> strings that aren't valid container tags

bodhi-server/bodhi/server/buildsys.py

@@ -18,6 +18,7 @@
 """Define tools for interacting with the build system and a fake build system for development."""
 
 from functools import wraps
+import hashlib
 from threading import Lock
 import logging
 import os
@@ -235,7 +236,9 @@ def getBuild(self, build='TurboGears-1.0.2.2-2.fc17', other=False, testing=False
 
                 format_data = {
                     'registry': 'candidate-registry.fedoraproject.org',
-                    'hash': 'sha256:2bd64a888...',
+                    # We make up a fake digest for the image manifest using a
+                    # digest of the version-release string
+                    'hash': hashlib.sha256(f"{version}-{release}".encode("UTF-8")).hexdigest(),
                     'version': version,
                     'release': release
                 }

bodhi-server/bodhi/server/util.py

@@ -1137,13 +1137,12 @@ def __exit__(self, *args, **kwargs):
         self.session.autoflush = self.autoflush
 
 
-def _get_build_repository(build):
+def _get_build_repository_and_digest(build):
     """
-    Return the registry repository name for the given Build from the container's pull string.
+    Return the registry repository and container digest from the container's pull string.
 
-    Examples -
-    'candidate-registry.fedoraproject.org/f29/cockpit:176-5.fc28' => 'f29/cockpit'.
-    'candidate-registry.fedoraproject.org/myrepo@sha256:<hash>' => 'myrepo'.
+    Example -
+    'candidate-registry.fedoraproject.org/myrepo@sha256:<hash>' => 'myrepo', 'sha256:<hash>.
 
     Args:
         build (bodhi.server.models.Build): A Build representing a container or flatpak.
@@ -1154,11 +1153,40 @@ def _get_build_repository(build):
     koji_build = koji.getBuild(build.nvr)
 
     pull_specs = koji_build['extra']['typeinfo']['image']['index']['pull']
-    # All the pull specs should have the same repository, so which one we use is arbitrary
-    base, tag = re.compile(r'[:@]').split(pull_specs[0], 1)
+    digest_pull_spec = [spec for spec in pull_specs if '@' in spec][0]
+
+    base, digest = digest_pull_spec.split('@', 1)
     server, repository = base.split('/', 1)
 
-    return repository
+    return repository, digest
+
+
+def make_valid_container_tag(original_tag):
+    """
+    Create a valid container tag from an arbitrary string.
+
+    Turns an arbitrary string (typically a koji 'version' or 'version-release' string),
+    into a valid OCI registry tag. This operation isn't reversable - multiple
+    tags might end up as the same thing. For example, 1.0_a5', '1.0^a5', and '1.0~a5'
+    all end up as '1.0_a5'.
+
+    Args:
+        original_tag: str: an arbitrary string
+    Returns:
+        str: a valid OCI registry tag based on original tag
+    """
+    # https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests
+    #
+    #    Throughout this document, <reference> as a tag MUST be at most 128 characters in length
+    #    and MUST match the following regular expression
+    #
+    #  [a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}
+
+    # Replace invalid characters with _
+    new_tag = re.sub(r'^[^a-zA-Z0-9_]|[^a-zA-Z0-9._-]', '_', original_tag)
+
+    # Truncate to 128 characters
+    return new_tag[0:128]
 
 
 def copy_container(build, destination_registry=None, destination_tag=None):
@@ -1177,17 +1205,17 @@ def copy_container(build, destination_registry=None, destination_tag=None):
         RuntimeError: If skopeo returns a non-0 exit code.
     """
     source_registry = config['container.source_registry']
-    source_tag = '{}-{}'.format(build.nvr_version, build.nvr_release)
 
     if destination_tag is None:
-        destination_tag = source_tag
+        destination_tag = '{}-{}'.format(build.nvr_version, build.nvr_release)
     if destination_registry is None:
         destination_registry = config['container.destination_registry']
 
-    repository = _get_build_repository(build)
+    repository, digest = _get_build_repository_and_digest(build)
 
-    source_url = _container_image_url(source_registry, repository, source_tag)
-    destination_url = _container_image_url(destination_registry, repository, destination_tag)
+    source_url = _container_image_url(source_registry, repository, digest=digest)
+    destination_url = _container_image_url(destination_registry, repository,
+                                           tag=make_valid_container_tag(destination_tag))
 
     skopeo_cmd = [
         config.get('skopeo.cmd'), 'copy', source_url, destination_url]
@@ -1197,7 +1225,7 @@ def copy_container(build, destination_registry=None, destination_tag=None):
     cmd(skopeo_cmd, raise_on_error=True)
 
 
-def _container_image_url(registry, repository, tag=None):
+def _container_image_url(registry, repository, *, tag=None, digest=None):
     """
     Return a URL suitable for use in Skopeo for copying or deleting container images.
 
@@ -1212,7 +1240,10 @@ def _container_image_url(registry, repository, tag=None):
     Returns:
         str: A URL referencing the given build and tag in the given registry.
     """
-    return 'docker://{}/{}:{}'.format(registry, repository, tag)
+    if tag:
+        return 'docker://{}/{}:{}'.format(registry, repository, tag)
+    else:
+        return 'docker://{}/{}@{}'.format(registry, repository, digest)
 
 
 def get_absolute_path(location):

bodhi-server/tests/tasks/test_composer.py

@@ -15,6 +15,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+import hashlib
 from http.client import IncompleteRead
 from unittest import mock
 from urllib.error import HTTPError, URLError
@@ -104,6 +105,24 @@
 }
 
 
+def _expected_skopeo_args(config, source, dtag, extra_args=[]):
+    source_repository, source_version_release = source.split(':')
+    # Match how buildsys.py creates a fake digest
+    source_digest = hashlib.sha256(source_version_release.encode("UTF-8")).hexdigest()
+
+    return [config['skopeo.cmd'], 'copy'] + extra_args + [
+        'docker://{}/{}@sha256:{}'.format(config['container.source_registry'],
+                                          source_repository, source_digest),
+        'docker://{}/{}:{}'.format(config['container.destination_registry'],
+                                   source_repository, dtag)
+    ]
+
+
+def _expected_skopeo_call(config, source, dtag, extra_args=[]):
+    return mock.call(_expected_skopeo_args(config, source, dtag, extra_args=extra_args),
+                     shell=False, stderr=-1, stdout=-1, cwd=None)
+
+
 class TestTask:
     @mock.patch("bodhi.server.tasks.bugs")
     @mock.patch("bodhi.server.tasks.buildsys")
@@ -2371,15 +2390,10 @@ def test_request_not_stable(self, Popen):
         # Popen should have been called three times per build, once for each of the destination
         # tags. With two builds that is a total of 6 calls to Popen.
         expected_mock_calls = []
-        for source in ('testcontainer1:2.0.1-71.fc28container',
-                       'testcontainer2:1.0.1-1.fc28container'):
+        for source in ('f28/testcontainer1:2.0.1-71.fc28container',
+                       'f28/testcontainer2:1.0.1-1.fc28container'):
             for dtag in [source.split(':')[1], source.split(':')[1].split('-')[0], 'testing']:
-                mock_call = mock.call(
-                    [config['skopeo.cmd'], 'copy',
-                     'docker://{}/f28/{}'.format(config['container.source_registry'], source),
-                     'docker://{}/f28/{}:{}'.format(config['container.destination_registry'],
-                                                    source.split(':')[0], dtag)],
-                    shell=False, stderr=-1, stdout=-1, cwd=None)
+                mock_call = _expected_skopeo_call(config, source, dtag)
                 expected_mock_calls.append(mock_call)
                 expected_mock_calls.append(mock.call().communicate())
         assert Popen.mock_calls == expected_mock_calls
@@ -2400,15 +2414,10 @@ def test_request_stable(self, Popen):
         # Popen should have been called three times per build, once for each of the destination
         # tags. With two builds that is a total of 6 calls to Popen.
         expected_mock_calls = []
-        for source in ('testcontainer1:2.0.1-71.fc28container',
-                       'testcontainer2:1.0.1-1.fc28container'):
+        for source in ('f28/testcontainer1:2.0.1-71.fc28container',
+                       'f28/testcontainer2:1.0.1-1.fc28container'):
             for dtag in [source.split(':')[1], source.split(':')[1].split('-')[0], 'latest']:
-                mock_call = mock.call(
-                    [config['skopeo.cmd'], 'copy',
-                     'docker://{}/f28/{}'.format(config['container.source_registry'], source),
-                     'docker://{}/f28/{}:{}'.format(config['container.destination_registry'],
-                                                    source.split(':')[0], dtag)],
-                    shell=False, stderr=-1, stdout=-1, cwd=None)
+                mock_call = _expected_skopeo_call(config, source, dtag)
                 expected_mock_calls.append(mock_call)
                 expected_mock_calls.append(mock.call().communicate())
         assert Popen.mock_calls == expected_mock_calls
@@ -2428,12 +2437,9 @@ def test_skopeo_error_code(self, Popen):
             t._compose_updates()
 
         # Popen should have been called once.
-        skopeo_cmd = [
-            config['skopeo.cmd'], 'copy',
-            'docker://{}/f28/testcontainer1:2.0.1-71.fc28container'.format(
-                config['container.source_registry']),
-            'docker://{}/f28/testcontainer1:2.0.1-71.fc28container'.format(
-                config['container.destination_registry'])]
+        skopeo_cmd = _expected_skopeo_args(config,
+                                           "f28/testcontainer1:2.0.1-71.fc28container",
+                                           "2.0.1-71.fc28container")
         Popen.assert_called_once_with(skopeo_cmd, shell=False, stderr=-1, stdout=-1, cwd=None)
         assert f"{' '.join(skopeo_cmd)} returned a non-0 exit code: 1" in str(exc.value)
 
@@ -2453,15 +2459,11 @@ def test_skopeo_extra_copy_flags(self, Popen):
         # Popen should have been called three times per build, once for each of the destination
         # tags. With two builds that is a total of 6 calls to Popen.
         expected_mock_calls = []
-        for source in ('testcontainer1:2.0.1-71.fc28container',
-                       'testcontainer2:1.0.1-1.fc28container'):
+        for source in ('f28/testcontainer1:2.0.1-71.fc28container',
+                       'f28/testcontainer2:1.0.1-1.fc28container'):
             for dtag in [source.split(':')[1], source.split(':')[1].split('-')[0], 'testing']:
-                mock_call = mock.call(
-                    [config['skopeo.cmd'], 'copy', '--dest-tls-verify=false',
-                     'docker://{}/f28/{}'.format(config['container.source_registry'], source),
-                     'docker://{}/f28/{}:{}'.format(config['container.destination_registry'],
-                                                    source.split(':')[0], dtag)],
-                    shell=False, stderr=-1, stdout=-1, cwd=None)
+                mock_call = _expected_skopeo_call(config, source, dtag,
+                                                  extra_args=['--dest-tls-verify=false'])
                 expected_mock_calls.append(mock_call)
                 expected_mock_calls.append(mock.call().communicate())
         assert Popen.mock_calls == expected_mock_calls
@@ -2547,12 +2549,7 @@ def test_flatpak_compose(self, Popen):
         expected_mock_calls = []
         for source in ('testflatpak1:2.0.1-71.fc28flatpak', 'testflatpak2:1.0.1-1.fc28flatpak'):
             for dtag in [source.split(':')[1], source.split(':')[1].split('-')[0], 'testing']:
-                mock_call = mock.call(
-                    [config['skopeo.cmd'], 'copy',
-                     'docker://{}/{}'.format(config['container.source_registry'], source),
-                     'docker://{}/{}:{}'.format(config['container.destination_registry'],
-                                                source.split(':')[0], dtag)],
-                    shell=False, stderr=-1, stdout=-1, cwd=None)
+                mock_call = _expected_skopeo_call(config, source, dtag)
                 expected_mock_calls.append(mock_call)
                 expected_mock_calls.append(mock.call().communicate())
         assert Popen.mock_calls == expected_mock_calls

bodhi-server/tests/test_util.py

@@ -1230,29 +1230,28 @@ def test_err_zero_return_code(self, mock_popen, mock_error, mock_debug):
         mock_debug.assert_called_with('subprocess output: output\nerror')
 
     @mock.patch('bodhi.server.buildsys.get_session')
-    def test__get_build_repository(self, session):
+    def test__get_build_repository_and_digest(self, session):
+        pull_specs = [
+            'candidate-registry.fedoraproject.org/f29/cockpit:176-5.fc28',
+            'candidate-registry.fedoraproject.org/myrepo@sha256:abcdefg123456'
+        ]
+
         session.return_value = mock.Mock()
-        session.return_value.getBuild.side_effect = [
-            {
-                'extra': {
-                    'typeinfo': {
-                        'image': {
-                            'index': {
-                                'pull': [pullspec]
-                            }
+        session.return_value.getBuild.return_value = {
+            'extra': {
+                'typeinfo': {
+                    'image': {
+                        'index': {
+                            'pull': pull_specs
                         }
                     }
                 }
             }
-            for pullspec in [
-                'candidate-registry.fedoraproject.org/f29/cockpit:176-5.fc28',
-                'candidate-registry.fedoraproject.org/myrepo@sha256:abcdefg123456'
-            ]
-        ]
+        }
+
         build = mock.Mock()
         build.nvr = 'cockpit-167-5'
-        assert util._get_build_repository(build) == 'f29/cockpit'
-        assert util._get_build_repository(build) == 'myrepo'
+        assert util._get_build_repository_and_digest(build) == ('myrepo', 'sha256:abcdefg123456')
         session.return_value.getBuild.assert_called_with('cockpit-167-5')
 
     def test_build_evr(self):
@@ -1265,9 +1264,27 @@ def test_build_evr(self):
         assert util.build_evr(build) == ('2', '1', '2.fc30')
 
 
+class TestMakeValidContainerTag(base.BasePyTestCase):
+    """Test the make_valid_container_tag() function."""
+
+    def test_valid_tag(self):
+        def E(input, result):
+            assert util.make_valid_container_tag(input) == result
+
+        # Invalid characters turned to _
+        E("%{foo}-%{bar}", "__foo_-__bar_")
+        # . is only invalid when leading
+        E(".f-.g", "_f-.g")
+        # truncate to 128 characters
+        E("x" * 129, "x" * 128)
+
+
 @mock.patch('bodhi.server.util.cmd', autospec=True)
-@mock.patch('bodhi.server.util._container_image_url', new=lambda sr, r, st: f'{sr}:{r}:{st}')
-@mock.patch('bodhi.server.util._get_build_repository', new=lambda b: 'testrepo')
+@mock.patch('bodhi.server.util._container_image_url',
+            new=lambda sr, r, tag=None, digest=None:
+                f'{sr}:{r}:{tag}' if tag else f'{sr}:{r}@{digest}')
+@mock.patch('bodhi.server.util._get_build_repository_and_digest',
+            new=lambda b: ('testrepo', 'sha256:f00d00'))
 class TestCopyContainer(base.BasePyTestCase):
     """Test the copy_container() function."""
 
@@ -1286,21 +1303,24 @@ def test_default(self, cmd):
         """Test the default code path."""
         util.copy_container(self.build)
 
-        cmd.assert_called_once_with(['skopeo', 'copy', 'src:testrepo:1-1', 'dest:testrepo:1-1'],
+        cmd.assert_called_once_with(['skopeo', 'copy',
+                                     'src:testrepo@sha256:f00d00', 'dest:testrepo:1-1'],
                                     raise_on_error=True)
 
     def test_with_destination_registry(self, cmd):
         """Test with specified destination_registry."""
         util.copy_container(self.build, destination_registry='boo')
 
-        cmd.assert_called_once_with(['skopeo', 'copy', 'src:testrepo:1-1', 'boo:testrepo:1-1'],
+        cmd.assert_called_once_with(['skopeo', 'copy',
+                                     'src:testrepo@sha256:f00d00', 'boo:testrepo:1-1'],
                                     raise_on_error=True)
 
     def test_with_destination_tag(self, cmd):
         """Test with specified destination_tag."""
         util.copy_container(self.build, destination_tag='2-2')
 
-        cmd.assert_called_once_with(['skopeo', 'copy', 'src:testrepo:1-1', 'dest:testrepo:2-2'],
+        cmd.assert_called_once_with(['skopeo', 'copy',
+                                     'src:testrepo@sha256:f00d00', 'dest:testrepo:2-2'],
                                     raise_on_error=True)
 
     def test_with_extra_copy_flags(self, cmd):
@@ -1309,7 +1329,7 @@ def test_with_extra_copy_flags(self, cmd):
         util.copy_container(self.build)
 
         cmd.assert_called_once_with(['skopeo', 'copy', '--quiet', '--remove-signatures',
-                                     'src:testrepo:1-1', 'dest:testrepo:1-1'],
+                                     'src:testrepo@sha256:f00d00', 'dest:testrepo:1-1'],
                                     raise_on_error=True)
 
 

news/PR5497.bug

@@ -0,0 +1 @@
+Fix handling container tags which aren't valid OCI tags