Don't perform index image verification when overwrite_from_index is F…

…alse

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

@@ -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',

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.',

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,15 +106,23 @@ 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()
     update_request_payload = mock_ur.call_args[0][1]
     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,15 +1013,17 @@ 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()
     mock_oir.assert_called_once()
     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()

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