Skip to content

Commit

Permalink
When copying containers, copy from the source registry by digest
Browse files Browse the repository at this point in the history 
Turning version/release pairs into valid container registry tags can
be tricky (the OCI rules for tags are strict); avoid relying on the
details by copying from the source registry by digest.

Signed-off-by: Owen W. Taylor <[email protected]>
  • Loading branch information
@owtaylor @mattiaverga
owtaylor authored and mattiaverga committed Oct 3, 2023
1 parent b32286e commit 1a9e8eb
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 74 deletions.
5 changes: 4 additions & 1 deletion bodhi-server/bodhi/server/buildsys.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
32 changes: 17 additions & 15 deletions bodhi-server/bodhi/server/util.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -1154,11 +1153,12 @@ 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 copy_container(build, destination_registry=None, destination_tag=None):
Expand All @@ -1177,17 +1177,16 @@ 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=destination_tag)

skopeo_cmd = [
config.get('skopeo.cmd'), 'copy', source_url, destination_url]
Expand All @@ -1197,7 +1196,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.
Expand All @@ -1212,7 +1211,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):
Expand Down
69 changes: 33 additions & 36 deletions bodhi-server/tests/tasks/test_composer.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
49 changes: 27 additions & 22 deletions bodhi-server/tests/test_util.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -1266,8 +1265,11 @@ def test_build_evr(self):


@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."""

Expand All @@ -1286,21 +1288,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):
Expand All @@ -1309,7 +1314,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)


Expand Down

0 comments on commit 1a9e8eb

Please sign in to comment.