From bd88a9dbc3ab2b76a169b98fe36c61793c0ba8e5 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 | 73 +++++++++++++++---- .../test_build_merge_index_image.py | 9 +-- 4 files changed, 84 insertions(+), 42 deletions(-) diff --git a/iib/workers/tasks/build.py b/iib/workers/tasks/build.py index c84987056..86b16a0ce 100644 --- a/iib/workers/tasks/build.py +++ b/iib/workers/tasks/build.py @@ -160,6 +160,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. @@ -176,11 +177,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( @@ -608,7 +616,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. @@ -617,10 +629,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) @@ -1085,11 +1101,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: @@ -1104,6 +1115,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', @@ -1176,10 +1188,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) @@ -1190,6 +1198,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 eedeab8e3..6fb0c5244 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 @@ -254,15 +253,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, @@ -271,6 +261,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 2c1802c04..adaa6450a 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( @@ -794,7 +835,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 @@ -972,7 +1013,9 @@ def test_handle_rm_request( 'ocp_version': 'v4.6', 'distribution_scope': 'PROD', } - build.handle_rm_request(['some-operator'], 'binary-image:latest', 3, 'from-index:latest') + build.handle_rm_request( + ['some-operator'], 'binary-image:latest', 3, 'from-index:latest', + ) mock_cleanup.assert_called_once() mock_prfb.assert_called_once() @@ -980,7 +1023,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 314c026b0..bbd0ff682 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 @@ -12,7 +12,7 @@ (('target-from-index:1.0', 'target-index@sha256:resolved'), (None, None)), ) @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') @@ -67,10 +67,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() @@ -82,7 +81,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') @@ -144,7 +143,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()