From 883faa6f1931cc20f8f97e5fd88b2c91ab3fe00e Mon Sep 17 00:00:00 2001 From: Lubomir Gallovic Date: Wed, 30 Sep 2020 13:52:09 +0200 Subject: [PATCH] Don't perform index image verification when overwrite_from_index is False There is no risk of data loss when IIB doesn't overwrite the original index image. Thus, to prevent unnecessary failures, this check is disabled when overwrite_from_index is False. The check has been moved to overwrite_from_index function, as it is now only performed before overwrite_from_index is attempted. --- iib/workers/tasks/build.py | 33 +++++---- iib/workers/tasks/build_merge_index_image.py | 11 +-- tests/test_workers/test_tasks/test_build.py | 69 +++++++++++++++---- .../test_build_merge_index_image.py | 9 ++- 4 files changed, 81 insertions(+), 41 deletions(-) diff --git a/iib/workers/tasks/build.py b/iib/workers/tasks/build.py index fb8a0b08..77b95264 100644 --- a/iib/workers/tasks/build.py +++ b/iib/workers/tasks/build.py @@ -165,6 +165,7 @@ def _update_index_image_pull_spec( from_index=None, overwrite_from_index=False, overwrite_from_index_token=None, + resolved_prebuild_from_index=None, ): """ Update the request with the modified index image. @@ -181,11 +182,18 @@ def _update_index_image_pull_spec( index image. :param str overwrite_from_index_token: the token used for overwriting the input ``from_index`` image. + :param str resolved_prebuild_from_index: resolved index image before starting the build. :raises IIBError: if the manifest list couldn't be created and pushed """ conf = get_worker_config() if from_index and overwrite_from_index: - _overwrite_from_index(request_id, output_pull_spec, from_index, overwrite_from_index_token) + _overwrite_from_index( + request_id, + output_pull_spec, + from_index, + resolved_prebuild_from_index, + overwrite_from_index_token, + ) index_image = from_index elif conf['iib_index_image_output_registry']: index_image = output_pull_spec.replace( @@ -613,7 +621,11 @@ def _opm_index_rm(base_dir, operators, binary_image, from_index, overwrite_from_ def _overwrite_from_index( - request_id, output_pull_spec, from_index, overwrite_from_index_token=None + request_id, + output_pull_spec, + from_index, + resolved_prebuild_from_index, + overwrite_from_index_token=None, ): """ Overwrite the ``from_index`` image. @@ -622,10 +634,14 @@ def _overwrite_from_index( :param str output_pull_spec: the pull specification of the manifest list for the index image that IIB built. :param str from_index: the pull specification of the image to overwrite. + :param str resolved_prebuild_from_index: resolved index image before starting the build. :param str overwrite_from_index_token: the user supplied token to use when overwriting the ``from_index`` image. If this is not set, IIB's configured credentials will be used. - :raises IIBError: if one of the skopeo commands fails. + :raises IIBError: if one of the skopeo commands fails or if the index image has changed + since IIB build started. """ + _verify_index_image(resolved_prebuild_from_index, from_index, overwrite_from_index_token) + state_reason = f'Overwriting the index image {from_index} with {output_pull_spec}' log.info(state_reason) set_request_state(request_id, 'in_progress', state_reason) @@ -1131,11 +1147,6 @@ def handle_add_request( _build_image(temp_dir, 'index.Dockerfile', request_id, arch) _push_image(request_id, arch) - if from_index: - _verify_index_image( - prebuild_info['from_index_resolved'], from_index, overwrite_from_index_token - ) - set_request_state(request_id, 'in_progress', 'Creating the manifest list') output_pull_spec = _create_and_push_manifest_list(request_id, arches) if legacy_support_packages: @@ -1150,6 +1161,7 @@ def handle_add_request( from_index, overwrite_from_index, overwrite_from_index_token, + prebuild_info['from_index_resolved'], ) set_request_state( request_id, 'complete', 'The operator bundle(s) were successfully added to the index image', @@ -1232,10 +1244,6 @@ def handle_rm_request( _build_image(temp_dir, 'index.Dockerfile', request_id, arch) _push_image(request_id, arch) - _verify_index_image( - prebuild_info['from_index_resolved'], from_index, overwrite_from_index_token - ) - set_request_state(request_id, 'in_progress', 'Creating the manifest list') output_pull_spec = _create_and_push_manifest_list(request_id, arches) @@ -1246,6 +1254,7 @@ def handle_rm_request( from_index, overwrite_from_index, overwrite_from_index_token, + prebuild_info['from_index_resolved'], ) set_request_state( request_id, 'complete', 'The operator(s) were successfully removed from the index image', diff --git a/iib/workers/tasks/build_merge_index_image.py b/iib/workers/tasks/build_merge_index_image.py index 21dc6eac..7847276d 100644 --- a/iib/workers/tasks/build_merge_index_image.py +++ b/iib/workers/tasks/build_merge_index_image.py @@ -17,7 +17,6 @@ _push_image, _update_index_image_build_state, _update_index_image_pull_spec, - _verify_index_image, ) from iib.workers.tasks.celery import app from iib.workers.tasks.utils import request_logger, run_cmd, set_registry_token @@ -266,15 +265,6 @@ def handle_merge_request( _build_image(temp_dir, 'index.Dockerfile', request_id, arch) _push_image(request_id, arch) - _verify_index_image( - prebuild_info['source_from_index_resolved'], source_from_index, overwrite_target_index_token - ) - - if target_index: - _verify_index_image( - prebuild_info['target_index_resolved'], target_index, overwrite_target_index_token - ) - output_pull_spec = _create_and_push_manifest_list(request_id, prebuild_info['arches']) _update_index_image_pull_spec( output_pull_spec, @@ -283,6 +273,7 @@ def handle_merge_request( target_index, overwrite_target_index, overwrite_target_index_token, + prebuild_info['target_index_resolved'], ) set_request_state( request_id, 'complete', 'The index image was successfully cleaned and updated.' diff --git a/tests/test_workers/test_tasks/test_build.py b/tests/test_workers/test_tasks/test_build.py index 693d5d11..bf661429 100644 --- a/tests/test_workers/test_tasks/test_build.py +++ b/tests/test_workers/test_tasks/test_build.py @@ -68,24 +68,32 @@ def test_create_and_push_manifest_list(mock_run_cmd, mock_td, tmp_path): @pytest.mark.parametrize( - 'iib_index_image_output_registry, from_index, overwrite, expected', + 'iib_index_image_output_registry, from_index, overwrite, expected, resolved_from_index', ( - (None, None, False, '{default}'), + (None, None, False, '{default}', None), ( 'registry-proxy.domain.local', None, False, 'registry-proxy.domain.local/{default_no_registry}', + None, ), - (None, 'quay.io/ns/iib:v4.5', True, 'quay.io/ns/iib:v4.5'), - (None, 'quay.io/ns/iib:v5.4', True, 'quay.io/ns/iib:v5.4'), + (None, 'quay.io/ns/iib:v4.5', True, 'quay.io/ns/iib:v4.5', 'quay.io/ns/iib:abcdef'), + (None, 'quay.io/ns/iib:v5.4', True, 'quay.io/ns/iib:v5.4', 'quay.io/ns/iib:fedcba'), ), ) @mock.patch('iib.workers.tasks.build.get_worker_config') @mock.patch('iib.workers.tasks.build._overwrite_from_index') @mock.patch('iib.workers.tasks.build.update_request') def test_update_index_image_pull_spec( - mock_ur, mock_ofi, mock_gwc, iib_index_image_output_registry, from_index, overwrite, expected + mock_ur, + mock_ofi, + mock_gwc, + iib_index_image_output_registry, + from_index, + overwrite, + expected, + resolved_from_index, ): default_no_registry = 'namespace/some-image:3' default = f'quay.io/{default_no_registry}' @@ -98,7 +106,13 @@ def test_update_index_image_pull_spec( 'iib_registry': 'quay.io', } build._update_index_image_pull_spec( - default, request_id, arches, from_index, overwrite, overwrite_token + default, + request_id, + arches, + from_index, + overwrite, + overwrite_token, + resolved_prebuild_from_index=resolved_from_index, ) mock_ur.assert_called_once() @@ -106,7 +120,9 @@ def test_update_index_image_pull_spec( assert update_request_payload.keys() == {'arches', 'index_image'} assert update_request_payload['index_image'] == expected_pull_spec if overwrite: - mock_ofi.assert_called_once_with(request_id, default, from_index, overwrite_token) + mock_ofi.assert_called_once_with( + request_id, default, from_index, resolved_from_index, overwrite_token + ) else: mock_ofi.assert_not_called() @@ -412,29 +428,53 @@ def test_opm_index_rm(mock_run_cmd, mock_srt): @pytest.mark.parametrize( - 'output_pull_spec, from_index, overwrite_from_index_token, oci_export_expected', + 'output_pull_spec, from_index, resolved_from_index,' + 'overwrite_from_index_token, oci_export_expected', ( - ('quay.io/ns/repo:1', 'quay.io/user_ns/repo:v1', 'user:pass', True), - ('quay.io/ns/repo:1', 'docker.io/user_ns/repo:v1', 'user:pass', False), - ('quay.io/ns/repo:1', 'quay.io/user_ns/repo:v1', None, False), + ( + 'quay.io/ns/repo:1', + 'quay.io/user_ns/repo:v1', + 'quay.io/user_ns/repo:abcdef', + 'user:pass', + True, + ), + ( + 'quay.io/ns/repo:1', + 'docker.io/user_ns/repo:v1', + 'quay.io/user_ns/repo:abcdef', + 'user:pass', + False, + ), + ( + 'quay.io/ns/repo:1', + 'quay.io/user_ns/repo:v1', + 'quay.io/user_ns/repo:abcdef', + None, + False, + ), ), ) @mock.patch('iib.workers.tasks.build.set_request_state') @mock.patch('iib.workers.tasks.build.tempfile.TemporaryDirectory') @mock.patch('iib.workers.tasks.build._skopeo_copy') @mock.patch('iib.workers.tasks.build.set_registry_token') +@mock.patch('iib.workers.tasks.build._verify_index_image') def test_overwrite_from_index( + mock_vii, mock_srt, mock_sc, mock_td, mock_srs, output_pull_spec, from_index, + resolved_from_index, overwrite_from_index_token, oci_export_expected, ): mock_td.return_value.name = '/tmp/iib-12345' - build._overwrite_from_index(1, output_pull_spec, from_index, overwrite_from_index_token) + build._overwrite_from_index( + 1, output_pull_spec, from_index, resolved_from_index, overwrite_from_index_token + ) if oci_export_expected: oci_pull_spec = f'oci:{mock_td.return_value.name}' @@ -457,6 +497,7 @@ def test_overwrite_from_index( mock_td.return_value.cleanup.assert_not_called() mock_srt.assert_called_once() + mock_vii.assert_called_once_with(resolved_from_index, from_index, overwrite_from_index_token) @pytest.mark.parametrize( @@ -844,7 +885,7 @@ def test_handle_add_request( assert organization in export_args mock_uiips.assert_called_once() - mock_vii.assert_called_once() + mock_vii.assert_not_called() mock_capml.assert_called_once() assert mock_srs.call_count == 4 @@ -1048,7 +1089,7 @@ def test_handle_rm_request( assert mock_alti.call_count == 2 assert mock_bi.call_count == len(arches) assert mock_pi.call_count == len(arches) - mock_vii.assert_called_once() + mock_vii.assert_not_called() assert mock_srs.call_count == 2 mock_capml.assert_called_once() mock_uiips.assert_called_once() diff --git a/tests/test_workers/test_tasks/test_build_merge_index_image.py b/tests/test_workers/test_tasks/test_build_merge_index_image.py index ce4caab1..ac43ebf6 100644 --- a/tests/test_workers/test_tasks/test_build_merge_index_image.py +++ b/tests/test_workers/test_tasks/test_build_merge_index_image.py @@ -15,7 +15,7 @@ ), ) @mock.patch('iib.workers.tasks.build_merge_index_image._update_index_image_pull_spec') -@mock.patch('iib.workers.tasks.build_merge_index_image._verify_index_image') +@mock.patch('iib.workers.tasks.build._verify_index_image') @mock.patch('iib.workers.tasks.build_merge_index_image._create_and_push_manifest_list') @mock.patch('iib.workers.tasks.build_merge_index_image._push_image') @mock.patch('iib.workers.tasks.build_merge_index_image._build_image') @@ -84,10 +84,9 @@ def test_handle_merge_request( mock_uiibs.assert_called_once_with(1, prebuild_info) if target_index: assert mock_gpb.call_count == 2 - assert mock_vii.call_count == 2 else: assert mock_gpb.call_count == 1 - assert mock_vii.call_count == 1 + mock_vii.assert_not_called() mock_abmis.assert_called_once() mock_gbfdl.assert_called_once() mock_geaps.assert_called_once() @@ -99,7 +98,7 @@ def test_handle_merge_request( @mock.patch('iib.workers.tasks.build_merge_index_image._update_index_image_pull_spec') -@mock.patch('iib.workers.tasks.build_merge_index_image._verify_index_image') +@mock.patch('iib.workers.tasks.build._verify_index_image') @mock.patch('iib.workers.tasks.build_merge_index_image._create_and_push_manifest_list') @mock.patch('iib.workers.tasks.build_merge_index_image._push_image') @mock.patch('iib.workers.tasks.build_merge_index_image._build_image') @@ -168,7 +167,7 @@ def test_handle_merge_request_no_deprecate( assert mock_dep_b.call_count == 0 assert mock_bi.call_count == 2 assert mock_pi.call_count == 2 - assert mock_vii.call_count == 2 + mock_vii.assert_not_called() mock_capml.assert_called_once() mock_uiips.assert_called_once()