From a4ed53b4fb71b471848361e6ae43fc14504be0d1 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 00:04:58 -0700 Subject: [PATCH 01/18] fix: key index.db artifact/ImageStream identity on image content digest Derive the index.db artifact and ImageStream tag from the index image's manifest digest (idb-) instead of the pullspec string. Different content never collides; identical content addressed by different pullspecs (e.g. a released mirror) shares one artifact. Co-Authored-By: Claude Opus 4.8 --- iib/workers/config.py | 2 +- .../build_containerized_create_empty_index.py | 3 +- iib/workers/tasks/containerized_utils.py | 4 +- iib/workers/tasks/oras_utils.py | 52 ++--- ..._build_containerized_create_empty_index.py | 9 - .../test_tasks/test_containerized_utils.py | 2 +- .../test_tasks/test_oras_utils.py | 196 ++++++++++-------- 7 files changed, 144 insertions(+), 124 deletions(-) diff --git a/iib/workers/config.py b/iib/workers/config.py index 2b8bdd3db..7ffec7435 100644 --- a/iib/workers/config.py +++ b/iib/workers/config.py @@ -71,7 +71,7 @@ class Config(object): iib_index_db_oras_auth_path: str = os.path.join( os.path.expanduser('~'), '.docker', 'oras', 'config.json' ) - iib_index_db_artifact_tag_template: str = '{image_name}-{tag}' + iib_index_db_artifact_tag_template: str = 'idb-{digest}' iib_index_db_artifact_template: str = '{registry}/index-db:{tag}' # Whether to use OpenShift ImageStream cache for index.db artifacts # Requires OpenShift cluster with ImageStream configured diff --git a/iib/workers/tasks/build_containerized_create_empty_index.py b/iib/workers/tasks/build_containerized_create_empty_index.py index ed6bfc348..17e68622a 100644 --- a/iib/workers/tasks/build_containerized_create_empty_index.py +++ b/iib/workers/tasks/build_containerized_create_empty_index.py @@ -34,7 +34,6 @@ opm_validate, ) from iib.workers.tasks.oras_utils import ( - _get_artifact_combined_tag, _get_name_and_tag_from_pullspec, get_oras_artifact, ) @@ -203,7 +202,7 @@ def handle_containerized_create_empty_index_request( image_name, _ = _get_name_and_tag_from_pullspec(from_index) empty_artifact_ref = conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], - tag=_get_artifact_combined_tag(image_name, empty_tag), + tag=f"{image_name}-{empty_tag}", ) log.info('Fetching empty index.db from %s', empty_artifact_ref) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index 160f491c7..c2b085c16 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -32,7 +32,6 @@ ) from iib.workers.tasks.oras_utils import ( _get_artifact_combined_tag, - _get_name_and_tag_from_pullspec, get_image_digest, get_indexdb_artifact_pullspec, get_imagestream_artifact_pullspec, @@ -380,11 +379,10 @@ def push_index_db_artifact( # Push with request_id tag irrespective of overwrite_from_index set_request_state(request_id, 'in_progress', 'Pushing updated index database') - image_name, tag = _get_name_and_tag_from_pullspec(from_index) conf = get_worker_config() request_artifact_ref = conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], - tag=f"{_get_artifact_combined_tag(image_name, tag)}-{request_id}", + tag=f"{_get_artifact_combined_tag(from_index)}-{request_id}", ) artifact_refs = [request_artifact_ref] if overwrite_from_index: diff --git a/iib/workers/tasks/oras_utils.py b/iib/workers/tasks/oras_utils.py index 358aefc4b..9912a0fdd 100644 --- a/iib/workers/tasks/oras_utils.py +++ b/iib/workers/tasks/oras_utils.py @@ -58,21 +58,31 @@ def _get_name_and_tag_from_pullspec(image_pullspec: str) -> Tuple[str, str]: return index_name, tag -def _get_artifact_combined_tag(image_name: str, tag: str) -> str: +def _get_index_digest(pullspec: str) -> str: """ - Generate a combined artifact tag for the given image name and tag. + Resolve a pullspec to its bare hex manifest digest (sha256), for artifact tags. - This function generates a unique combined tag for an image by using a template - string defined in the worker configuration and replacing placeholders with the - provided image name and tag. + :param str pullspec: The full index image pullspec. + :return: The 64-char hex digest without the ``sha256:`` prefix. + :rtype: str + """ + return get_image_digest(pullspec).split(':', 1)[-1] - :param str image_name: The name of the image. - :param str tag: The version or identifier tag to be combined. - :return: A formatted string representing the combined artifact tag. + +def _get_artifact_combined_tag(from_index: str) -> str: + """ + Generate the content-addressed artifact/ImageStream tag for an index image. + + Keyed on the image's manifest digest ALONE (not name/tag/pullspec) so that + identical content addressed by different pullspecs (e.g. a released mirror) + shares one artifact, while different content never collides. + + :param str from_index: The full index image pullspec (registry/namespace/repo:tag). + :return: Combined tag, e.g. "idb-<64-hex-sha256>". :rtype: str """ return get_worker_config()['iib_index_db_artifact_tag_template'].format( - image_name=image_name, tag=tag + digest=_get_index_digest(from_index) ) @@ -87,11 +97,10 @@ def get_indexdb_artifact_pullspec(from_index: str) -> str: :rtype: str """ conf = get_worker_config() - image_name, tag = _get_name_and_tag_from_pullspec(from_index) return conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], - tag=_get_artifact_combined_tag(image_name, tag), + tag=_get_artifact_combined_tag(from_index), ) @@ -285,16 +294,15 @@ def verify_indexdb_cache_for_image(index_image_pullspec: str) -> bool: """ Verify the synchronization state of the index database cache for a given container image. - This function extracts the image name and tag from the specified image - pullspec, generates an artifact combined tag, and verifies whether the - database cache for the image is synchronized. + This function generates the digest-based artifact combined tag for the + specified image pullspec and verifies whether the database cache for the + image is synchronized. :param str index_image_pullspec: The pull specification string of the container image. :return: The result of the cache synchronization verification process. :rtype: str """ - index_name, tag = _get_name_and_tag_from_pullspec(index_image_pullspec) - return verify_indexdb_cache_sync(_get_artifact_combined_tag(index_name, tag)) + return verify_indexdb_cache_sync(_get_artifact_combined_tag(index_image_pullspec)) def refresh_indexdb_cache( @@ -335,15 +343,12 @@ def refresh_indexdb_cache_for_image(index_image_pullspec: str) -> None: """ Refresh the cached data for an index database, associating it with the given image pullspec. - This function extracts the name and tag from the specified image pullspec, - and refreshes the associated index database cache. + This function generates the digest-based artifact combined tag for the + specified image pullspec, and refreshes the associated index database cache. :param str index_image_pullspec: The pull specification of the index image to cache. - :return: A formatted string combining the index name and tag. - :rtype: str """ - index_name, tag = _get_name_and_tag_from_pullspec(index_image_pullspec) - refresh_indexdb_cache(_get_artifact_combined_tag(index_name, tag)) + refresh_indexdb_cache(_get_artifact_combined_tag(index_image_pullspec)) def get_imagestream_artifact_pullspec(from_index: str) -> str: @@ -358,8 +363,7 @@ def get_imagestream_artifact_pullspec(from_index: str) -> str: :rtype: str """ conf = get_worker_config() - image_name, tag = _get_name_and_tag_from_pullspec(from_index) - combined_tag = _get_artifact_combined_tag(image_name, tag) + combined_tag = _get_artifact_combined_tag(from_index) # ImageStream pullspec format: # image-registry.openshift-image-registry.svc:5000/{namespace}/index-db:{combined_tag} diff --git a/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py b/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py index 7f5424871..9394dd3bc 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py +++ b/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py @@ -21,7 +21,6 @@ @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -69,7 +68,6 @@ def test_handle_containerized_create_empty_index_primary_path( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -136,7 +134,6 @@ def test_handle_containerized_create_empty_index_primary_path( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' @@ -239,7 +236,6 @@ def test_handle_containerized_create_empty_index_primary_path( @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -295,7 +291,6 @@ def test_handle_containerized_create_empty_index_fallback( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -382,7 +377,6 @@ def test_handle_containerized_create_empty_index_fallback( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:fallback' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' @@ -602,7 +596,6 @@ def test_handle_containerized_create_empty_index_missing_git_mapping( @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -658,7 +651,6 @@ def test_handle_containerized_create_empty_index_unexpected_opm_error( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -720,7 +712,6 @@ def test_handle_containerized_create_empty_index_unexpected_opm_error( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:abc' diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index bb3908599..5b1426b05 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -517,7 +517,7 @@ def test_cleanup_on_failure_restores_index_db_artifact( @patch('iib.workers.tasks.containerized_utils.run_cmd') -@patch('iib.workers.tasks.oras_utils.get_indexdb_artifact_pullspec') +@patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @patch('iib.workers.tasks.containerized_utils.log') def test_cleanup_on_failure_restore_failure_is_logged( mock_log, mock_get_indexdb_artifact_pullspec, mock_run_cmd diff --git a/tests/test_workers/test_tasks/test_oras_utils.py b/tests/test_workers/test_tasks/test_oras_utils.py index 88337c391..55630d330 100644 --- a/tests/test_workers/test_tasks/test_oras_utils.py +++ b/tests/test_workers/test_tasks/test_oras_utils.py @@ -13,9 +13,49 @@ verify_indexdb_cache_sync, get_image_stream_digest, refresh_indexdb_cache, + _get_index_digest, + _get_artifact_combined_tag, + get_indexdb_artifact_pullspec, ) +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_index_digest_strips_algo_prefix(mock_digest): + mock_digest.return_value = 'sha256:' + 'a' * 64 + assert _get_index_digest('quay.io/ns/repo:v4.17') == 'a' * 64 + mock_digest.assert_called_once_with('quay.io/ns/repo:v4.17') + + +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_combined_tag_is_digest_only(mock_digest, mock_gwc): + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = 'sha256:' + 'b' * 64 + assert _get_artifact_combined_tag('quay.io/my-ns/iib-pub:v4.17') == 'idb-' + 'b' * 64 + + +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_same_content_different_pullspec_same_tag(mock_digest, mock_gwc): + """Problem 1b: promotion — identical digest under two pullspecs => identical tag.""" + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = 'sha256:' + 'c' * 64 + staging = _get_artifact_combined_tag('quay.io/my-ns/iib-pub:v4.17') + released = _get_artifact_combined_tag('registry.access.redhat.com/some-ns/operator-index:v4.17') + assert staging == released == 'idb-' + 'c' * 64 + + +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_different_content_different_tag(mock_digest, mock_gwc): + """Problem 1: different images (different digests) => different tags.""" + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.side_effect = ['sha256:' + 'd' * 64, 'sha256:' + 'e' * 64] + a = _get_artifact_combined_tag('quay.io/redhat/foo:v4.17') + b = _get_artifact_combined_tag('quay.io/redhat-pending/foo:v4.17') + assert a != b + + @pytest.fixture() def registry_auths(): return {'auths': {'quay.io': {'auth': 'dXNlcjpwYXNz'}}} # base64 encoded user:pass @@ -653,170 +693,158 @@ def test_get_name_and_tag_from_pullspec_invalid(invalid_pullspec, expected_error @pytest.mark.parametrize( - "image_name,tag,expected_tag", + "pullspec,digest,expected_tag", [ - ("iib-pub-pending", "v4.17", "iib-pub-pending-v4.17"), - ("my-image", "latest", "my-image-latest"), - ("test-index", "v1.0.0", "test-index-v1.0.0"), + ("quay.io/ns/iib-pub-pending:v4.17", "a" * 64, "idb-" + "a" * 64), + ("quay.io/ns/my-image:latest", "b" * 64, "idb-" + "b" * 64), + ("quay.io/ns/test-index:v1.0.0", "c" * 64, "idb-" + "c" * 64), ], ) @mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_artifact_combined_tag(mock_gwc, image_name, tag, expected_tag): +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_artifact_combined_tag(mock_digest, mock_gwc, pullspec, digest, expected_tag): """Test generating combined artifact tags.""" - from iib.workers.tasks.oras_utils import _get_artifact_combined_tag + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = f'sha256:{digest}' - mock_gwc.return_value = {'iib_index_db_artifact_tag_template': '{image_name}-{tag}'} - - result = _get_artifact_combined_tag(image_name, tag) + result = _get_artifact_combined_tag(pullspec) assert result == expected_tag @pytest.mark.parametrize( - "from_index,expected_pullspec", + "from_index,digest", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "test-artifact-registry/index-db:iib-pub-pending-v4.17", - ), - ( - "quay.io/namespace/my-image:latest", - "test-artifact-registry/index-db:my-image-latest", - ), - ( - "registry.io/org/repo/index-image:v1.0.0", - "test-artifact-registry/index-db:index-image-v1.0.0", - ), - ( - "registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", - "test-artifact-registry/index-db:iib-pub-pending-v4.17", - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64), + ("quay.io/namespace/my-image:latest", "b" * 64), + ("registry.io/org/repo/index-image:v1.0.0", "c" * 64), + ("registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", "d" * 64), ], ) @mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_indexdb_artifact_pullspec(mock_gwc, from_index, expected_pullspec): +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_indexdb_artifact_pullspec(mock_digest, mock_gwc, from_index, digest): """Test constructing index DB artifact pullspecs.""" - from iib.workers.tasks.oras_utils import get_indexdb_artifact_pullspec - mock_gwc.return_value = { 'iib_index_db_artifact_registry': 'test-artifact-registry', 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', + 'iib_index_db_artifact_tag_template': 'idb-{digest}', } + mock_digest.return_value = f'sha256:{digest}' result = get_indexdb_artifact_pullspec(from_index) - assert result == expected_pullspec + assert result == f'test-artifact-registry/index-db:idb-{digest}' + mock_digest.assert_called_once_with(from_index) @mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_indexdb_artifact_pullspec_invalid(mock_gwc): - """Test _get_indexdb_artifact_pullspec with invalid pullspec.""" - from iib.workers.tasks.oras_utils import get_indexdb_artifact_pullspec - +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_indexdb_artifact_pullspec_digest_resolution_failure(mock_digest, mock_gwc): + """Test get_indexdb_artifact_pullspec propagates digest resolution failures.""" mock_gwc.return_value = { 'iib_index_db_artifact_registry': 'test-artifact-registry', 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', + 'iib_index_db_artifact_tag_template': 'idb-{digest}', } + mock_digest.side_effect = IIBError('Failed to inspect image') - with pytest.raises(IIBError, match="Missing tag"): - get_indexdb_artifact_pullspec("registry.example.com/namespace/image") + with pytest.raises(IIBError, match="Failed to inspect image"): + get_indexdb_artifact_pullspec("registry.example.com/namespace/image:v1.0.0") @mock.patch('iib.workers.tasks.oras_utils.verify_indexdb_cache_sync') +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') @pytest.mark.parametrize( - "pullspec,expected_combined_tag,sync_result", + "pullspec,digest,sync_result", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "iib-pub-pending-v4.17", - True, - ), - ( - "quay.io/namespace/my-image:latest", - "my-image-latest", - False, - ), - ( - "registry.io/org/repo/index-image:v1.0.0@sha256:abc123", - "index-image-v1.0.0", - True, - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64, True), + ("quay.io/namespace/my-image:latest", "b" * 64, False), + ("registry.io/org/repo/index-image:v1.0.0@sha256:abc123", "c" * 64, True), ], ) def test_verify_indexdb_cache_for_image( - mock_verify_sync, pullspec, expected_combined_tag, sync_result + mock_digest, mock_gwc, mock_verify_sync, pullspec, digest, sync_result ): """Test verify_indexdb_cache_for_image with various pullspecs.""" from iib.workers.tasks.oras_utils import verify_indexdb_cache_for_image + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = f'sha256:{digest}' mock_verify_sync.return_value = sync_result result = verify_indexdb_cache_for_image(pullspec) assert result == sync_result - mock_verify_sync.assert_called_once_with(expected_combined_tag) + mock_verify_sync.assert_called_once_with(f'idb-{digest}') @mock.patch('iib.workers.tasks.oras_utils.verify_indexdb_cache_sync') -def test_verify_indexdb_cache_for_image_invalid_pullspec(mock_verify_sync): - """Test verify_indexdb_cache_for_image with invalid pullspec.""" +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_verify_indexdb_cache_for_image_digest_resolution_failure(mock_digest, mock_verify_sync): + """Test verify_indexdb_cache_for_image propagates digest resolution failures.""" from iib.workers.tasks.oras_utils import verify_indexdb_cache_for_image - with pytest.raises(IIBError, match="Missing tag"): - verify_indexdb_cache_for_image("registry.example.com/namespace/image") + mock_digest.side_effect = IIBError('Failed to inspect image') + + with pytest.raises(IIBError, match="Failed to inspect image"): + verify_indexdb_cache_for_image("registry.example.com/namespace/image:v1.0.0") mock_verify_sync.assert_not_called() @mock.patch('iib.workers.tasks.oras_utils.refresh_indexdb_cache') +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') @pytest.mark.parametrize( - "pullspec,expected_combined_tag", + "pullspec,digest", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "iib-pub-pending-v4.17", - ), - ( - "quay.io/namespace/my-image:latest", - "my-image-latest", - ), - ( - "registry.io/org/repo/index-image:v1.0.0", - "index-image-v1.0.0", - ), - ( - "registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", - "iib-pub-pending-v4.17", - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64), + ("quay.io/namespace/my-image:latest", "b" * 64), + ("registry.io/org/repo/index-image:v1.0.0", "c" * 64), + ("registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", "d" * 64), ], ) -def test_refresh_indexdb_cache_for_image(mock_refresh_cache, pullspec, expected_combined_tag): +def test_refresh_indexdb_cache_for_image( + mock_digest, mock_gwc, mock_refresh_cache, pullspec, digest +): """Test refresh_indexdb_cache_for_image with various pullspecs.""" from iib.workers.tasks.oras_utils import refresh_indexdb_cache_for_image + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = f'sha256:{digest}' + refresh_indexdb_cache_for_image(pullspec) - mock_refresh_cache.assert_called_once_with(expected_combined_tag) + mock_refresh_cache.assert_called_once_with(f'idb-{digest}') @mock.patch('iib.workers.tasks.oras_utils.refresh_indexdb_cache') -def test_refresh_indexdb_cache_for_image_invalid_pullspec(mock_refresh_cache): - """Test refresh_indexdb_cache_for_image with invalid pullspec.""" +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_refresh_indexdb_cache_for_image_digest_resolution_failure(mock_digest, mock_refresh_cache): + """Test refresh_indexdb_cache_for_image propagates digest resolution failures.""" from iib.workers.tasks.oras_utils import refresh_indexdb_cache_for_image - with pytest.raises(IIBError, match="Missing tag"): - refresh_indexdb_cache_for_image("registry.example.com/namespace/image") + mock_digest.side_effect = IIBError('Failed to inspect image') + + with pytest.raises(IIBError, match="Failed to inspect image"): + refresh_indexdb_cache_for_image("registry.example.com/namespace/image:v1.0.0") mock_refresh_cache.assert_not_called() @mock.patch('iib.workers.tasks.oras_utils.refresh_indexdb_cache') -def test_refresh_indexdb_cache_for_image_propagates_exception(mock_refresh_cache): +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_refresh_indexdb_cache_for_image_propagates_exception( + mock_digest, mock_gwc, mock_refresh_cache +): """Test if refresh_indexdb_cache_for_image propagates exceptions from refresh_indexdb_cache.""" from iib.workers.tasks.oras_utils import refresh_indexdb_cache_for_image + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = 'sha256:' + 'e' * 64 mock_refresh_cache.side_effect = IIBError('Refresh failed') with pytest.raises(IIBError, match='Refresh failed'): From fffa76eb44171f0b3157dd01867dc6ceddd4587f Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 01:41:22 -0700 Subject: [PATCH 02/18] feat: add remote_branch_exists and get_index_tag helpers Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/git_utils.py | 12 ++++++++++++ iib/workers/tasks/oras_utils.py | 13 +++++++++++++ tests/test_workers/test_tasks/test_git_utils.py | 12 ++++++++++++ tests/test_workers/test_tasks/test_oras_utils.py | 5 +++++ 4 files changed, 42 insertions(+) diff --git a/iib/workers/tasks/git_utils.py b/iib/workers/tasks/git_utils.py index cd98ac50b..8bbb4ee36 100644 --- a/iib/workers/tasks/git_utils.py +++ b/iib/workers/tasks/git_utils.py @@ -165,6 +165,18 @@ def validate_git_remote_branch(repo_url: str, branch: str) -> None: raise IIBError(f"Remote branch '{branch}' not found for repo {repo_url}") +def remote_branch_exists(repo_url: str, branch: str) -> bool: + """ + Return True if the given branch exists on the remote, without raising. + + :param str repo_url: The git repository URL. + :param str branch: The branch name to check. + :rtype: bool + """ + remote_branch_status = run_cmd(["git", "ls-remote", "--heads", repo_url, branch], strict=False) + return bool(remote_branch_status.strip()) + + def commit_and_push( request_id: int, local_repo_path: str, diff --git a/iib/workers/tasks/oras_utils.py b/iib/workers/tasks/oras_utils.py index 9912a0fdd..2674f5d97 100644 --- a/iib/workers/tasks/oras_utils.py +++ b/iib/workers/tasks/oras_utils.py @@ -69,6 +69,19 @@ def _get_index_digest(pullspec: str) -> str: return get_image_digest(pullspec).split(':', 1)[-1] +def get_index_tag(from_index: str) -> str: + """ + Return only the tag portion of a full index image pullspec. + + :param str from_index: The full index image pullspec (registry/namespace/repo:tag). + :return: The tag portion of the pullspec. + :rtype: str + :raises IIBError: If the pullspec parsing fails within the helper function. + """ + _, tag = _get_name_and_tag_from_pullspec(from_index) + return tag + + def _get_artifact_combined_tag(from_index: str) -> str: """ Generate the content-addressed artifact/ImageStream tag for an index image. diff --git a/tests/test_workers/test_tasks/test_git_utils.py b/tests/test_workers/test_tasks/test_git_utils.py index f2e6fc939..3bf3aa343 100644 --- a/tests/test_workers/test_tasks/test_git_utils.py +++ b/tests/test_workers/test_tasks/test_git_utils.py @@ -1273,3 +1273,15 @@ def test_merge_gitlab_mr_retries_on_transient_failure(mock_extract, mock_session assert result == 'abc123' assert mock_session.put.call_count == 2 mock_sleep.assert_called_once() + + +@mock.patch('iib.workers.tasks.git_utils.run_cmd') +def test_remote_branch_exists_true(mock_run): + mock_run.return_value = 'abc123\trefs/heads/v4.14\n' + assert git_utils.remote_branch_exists('https://gitlab/x.git', 'v4.14') is True + + +@mock.patch('iib.workers.tasks.git_utils.run_cmd') +def test_remote_branch_exists_false(mock_run): + mock_run.return_value = '' + assert git_utils.remote_branch_exists('https://gitlab/x.git', 'test') is False diff --git a/tests/test_workers/test_tasks/test_oras_utils.py b/tests/test_workers/test_tasks/test_oras_utils.py index 55630d330..5fac8c09a 100644 --- a/tests/test_workers/test_tasks/test_oras_utils.py +++ b/tests/test_workers/test_tasks/test_oras_utils.py @@ -16,6 +16,7 @@ _get_index_digest, _get_artifact_combined_tag, get_indexdb_artifact_pullspec, + get_index_tag, ) @@ -849,3 +850,7 @@ def test_refresh_indexdb_cache_for_image_propagates_exception( with pytest.raises(IIBError, match='Refresh failed'): refresh_indexdb_cache_for_image("registry.example.com/namespace/image:v1.0.0") + + +def test_get_index_tag(): + assert get_index_tag('quay.io/redhat/my-index:v4.17') == 'v4.17' From 42ce2d961c6c6730981aa4e53c4433460db98dde Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 01:58:49 -0700 Subject: [PATCH 03/18] feat: unprivileged extraction of configs+index.db from index image Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 56 +++++++++++++- .../test_tasks/test_containerized_utils.py | 74 +++++++++++++++++++ 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index c2b085c16..118f956bc 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -40,7 +40,7 @@ refresh_indexdb_cache_for_image, verify_indexdb_cache_for_image, ) -from iib.workers.tasks.utils import run_cmd, skopeo_inspect +from iib.workers.tasks.utils import get_image_label, run_cmd, skopeo_inspect log = logging.getLogger(__name__) @@ -143,6 +143,60 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) +def extract_catalog_and_db_from_image(from_index: str, temp_dir: str) -> Tuple[str, str]: + """ + Extract FBC configs and index.db from an index image, unprivileged. + + Used on the divergent-tag path where no git branch / ORAS artifact exists yet. + The from_index image is the source of truth for its own content. + + index.db precedence: hidden db path -> configs database label -> empty db + (pure-FBC images may carry no db at all; there is no primitive to build a + SQLite index.db back from FBC configs, so an empty db is created and the + FBC configs remain authoritative). + + :param str from_index: The from_index image pullspec. + :param str temp_dir: Base temp directory for extraction. + :return: Tuple of (configs_dir_path, index_db_path). + :rtype: Tuple[str, str] + :raises IIBError: If the image has no FBC configs label. + """ + configs_label = get_image_label(from_index, 'operators.operatorframework.io.index.configs.v1') + if not configs_label: + raise IIBError(f"Index image {from_index} does not contain a file-based catalog.") + + configs_dir = str(Path(temp_dir) / 'extracted_configs') + extract_files_from_image_non_privileged(from_index, configs_label, configs_dir) + + index_db_path = str(Path(temp_dir) / 'extracted_index.db') + conf = get_worker_config() + hidden_db_path = conf['hidden_index_db_path'] + + try: + # Prefer the hidden db (carries deprecated/hidden bundle state). + extract_files_from_image_non_privileged(from_index, hidden_db_path, index_db_path) + return configs_dir, index_db_path + except IIBError: + log.info("No hidden index.db in %s; trying labeled db.", from_index) + + db_label = get_image_label(from_index, 'operators.operatorframework.io.index.database.v1') + if db_label: + extract_files_from_image_non_privileged(from_index, db_label, index_db_path) + return configs_dir, index_db_path + + # Pure FBC image: no embedded index.db (hidden or labeled). There is no opm + # primitive that builds a SQLite index.db from FBC configs (opm migrate only + # goes db -> configs), so create an empty index.db. The FBC configs are + # authoritative for this image; the empty db is populated by the handler's + # subsequent add/rm operations. + log.info("No embedded index.db found in %s; creating empty index.db.", from_index) + Path(index_db_path).parent.mkdir(parents=True, exist_ok=True) + with open(index_db_path, 'w'): + pass + + return configs_dir, index_db_path + + class ValidateBundlesThread(threading.Thread): """Thread to validate whether the bundle pullspecs are present in the registry.""" diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index 5b1426b05..3c460aa04 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -8,6 +8,7 @@ from iib.exceptions import IIBError from iib.workers.tasks.containerized_utils import ( + extract_catalog_and_db_from_image, extract_files_from_image_non_privileged, pull_index_db_artifact, write_build_metadata, @@ -1413,3 +1414,76 @@ def test_merge_mr_after_build_failure_closes_mr(mock_merge_mr, mock_close_mr): merge_mr_after_build(mr_details, 'https://gitlab.example.com/project') mock_close_mr.assert_called_once_with(mr_details, 'https://gitlab.example.com/project') + + +@patch('iib.workers.tasks.containerized_utils.get_image_label') +@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') +def test_extract_catalog_and_db_prefers_hidden_db(mock_extract, mock_label, tmp_path): + """When a hidden index.db exists, it is preferred over the labeled db.""" + + def label_side_effect(image, label): + return { + 'operators.operatorframework.io.index.configs.v1': '/configs', + 'operators.operatorframework.io.index.database.v1': '/database/index.db', + }[label] + + mock_label.side_effect = label_side_effect + + configs_dir, index_db = extract_catalog_and_db_from_image( + 'quay.io/redhat/my-index:test', str(tmp_path) + ) + + assert configs_dir.endswith('configs') + assert index_db.endswith('index.db') + # Two extractions: configs dir and the hidden db file. + assert mock_extract.call_count == 2 + + +@patch('iib.workers.tasks.containerized_utils.get_image_label') +@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') +def test_extract_catalog_and_db_falls_back_to_labeled_db(mock_extract, mock_label, tmp_path): + """When the hidden db is missing, fall back to the labeled database.v1 path.""" + mock_label.side_effect = lambda image, label: { + 'operators.operatorframework.io.index.configs.v1': '/configs', + 'operators.operatorframework.io.index.database.v1': '/database/index.db', + }[label] + # First call (configs) ok; second call (hidden db) raises -> fall back to labeled db + mock_extract.side_effect = [None, IIBError('no hidden db'), None] + + _, index_db = extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', str(tmp_path)) + + assert index_db.endswith('index.db') + assert mock_extract.call_count == 3 + + +@patch('iib.workers.tasks.containerized_utils.get_image_label') +@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') +def test_extract_catalog_and_db_pure_fbc_creates_empty_db(mock_extract, mock_label, tmp_path): + """Pure-FBC image (no hidden or labeled db) results in an empty index.db.""" + mock_label.side_effect = lambda image, label: { + 'operators.operatorframework.io.index.configs.v1': '/configs', + 'operators.operatorframework.io.index.database.v1': '', + }[label] + # First call (configs) ok; second call (hidden db) raises -> no labeled db either + mock_extract.side_effect = [None, IIBError('no hidden db')] + + configs_dir, index_db = extract_catalog_and_db_from_image( + 'quay.io/redhat/my-index:test', str(tmp_path) + ) + + assert configs_dir.endswith('configs') + assert index_db.endswith('index.db') + assert os.path.exists(index_db) + assert os.path.getsize(index_db) == 0 + # Only two extraction attempts: configs dir and the failed hidden db lookup. + # No opm_migrate / privileged call is ever invoked for the pure-FBC fallback. + assert mock_extract.call_count == 2 + + +@patch('iib.workers.tasks.containerized_utils.get_image_label') +def test_extract_catalog_and_db_raises_without_configs_label(mock_label): + """If the image has no FBC configs label, an IIBError is raised.""" + mock_label.return_value = '' + + with pytest.raises(IIBError, match='does not contain a file-based catalog'): + extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', '/tmp/does-not-matter') From b0db932d059cb030542ef7499754a697ff52b02f Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 02:11:30 -0700 Subject: [PATCH 04/18] feat: prepare_build_sources orchestrates normal vs divergent build paths Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 108 ++++++++++++++++++ .../test_tasks/test_containerized_utils.py | 78 +++++++++++++ 2 files changed, 186 insertions(+) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index 118f956bc..ca28559e2 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -7,6 +7,7 @@ import tarfile import tempfile import threading +from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple, Union @@ -22,6 +23,7 @@ get_git_token, get_last_commit_sha, merge_mr, + remote_branch_exists, resolve_git_url, revert_last_commit, ) @@ -33,6 +35,7 @@ from iib.workers.tasks.oras_utils import ( _get_artifact_combined_tag, get_image_digest, + get_index_tag, get_indexdb_artifact_pullspec, get_imagestream_artifact_pullspec, get_oras_artifact, @@ -548,6 +551,111 @@ def cleanup_on_failure( log.error("Failed to restore index.db artifact: %s", restore_error) +@dataclass +class BuildSources: + """Resolved inputs for a containerized build.""" + + index_git_repo: str + local_git_repo_path: str + localized_git_catalog_path: str + index_db_path: Optional[str] # None => pull from ORAS; set => use extracted db + target_branch: str + is_divergent: bool + + +def prepare_build_sources( + request_id: int, + from_index: str, + temp_dir: str, + ocp_version: str, + index_to_gitlab_push_map: Dict[str, str], + overwrite_from_index: bool, +) -> BuildSources: + """ + Resolve git repo + branch and decide the normal vs divergent build path. + + Normal path: a branch named after the image tag exists -> build against it + (overwrite allowed). Divergent path: no branch for the tag -> reject + overwrite, reuse the base OCP branch's Konflux Component, and seed content + by extracting configs+index.db from the image. + + :param int request_id: The IIB request ID + :param str from_index: The from_index pullspec + :param str temp_dir: Temporary directory to clone into / extract to + :param str ocp_version: Base OCP version branch, e.g. "v4.19" + :param Dict[str, str] index_to_gitlab_push_map: Mapping of index images to Git repositories + :param bool overwrite_from_index: Whether the request wants to overwrite from_index + :return: The resolved build sources + :rtype: BuildSources + :raises IIBError: if the git mapping is missing, overwrite is requested on + the divergent path, or the base OCP branch is not onboarded. + """ + index_git_repo = resolve_git_url(from_index=from_index, index_repo_map=index_to_gitlab_push_map) + if not index_git_repo: + raise IIBError( + f"Git repository mapping not found for from_index: {from_index}. " + "index_to_gitlab_push_map is required." + ) + token_name, git_token = get_git_token(index_git_repo) + + tag = get_index_tag(from_index) + set_request_state(request_id, 'in_progress', 'Cloning Git repository') + + if remote_branch_exists(index_git_repo, tag): + # Normal path — build against the tag branch, pull index.db from ORAS. + target_branch = tag + local_git_repo_path = Path(temp_dir) / 'git' / target_branch + local_git_repo_path.mkdir(parents=True, exist_ok=True) + clone_git_repo( + index_git_repo, target_branch, token_name, git_token, str(local_git_repo_path) + ) + catalog_path = local_git_repo_path / 'configs' + if not catalog_path.exists(): + raise IIBError(f"Catalogs directory not found in {local_git_repo_path}") + return BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=str(local_git_repo_path), + localized_git_catalog_path=str(catalog_path), + index_db_path=None, + target_branch=target_branch, + is_divergent=False, + ) + + # Divergent path. + if overwrite_from_index: + raise IIBError( + f"Cannot overwrite tag '{tag}' of {from_index}: no onboarded branch/Component " + f"exists for it. Onboard a '{tag}' branch to enable overwrite." + ) + + target_branch = ocp_version + if not remote_branch_exists(index_git_repo, target_branch): + raise IIBError( + f"Base OCP branch '{target_branch}' is not onboarded for {index_git_repo}. " + "Onboard the index before building divergent tags." + ) + + local_git_repo_path = Path(temp_dir) / 'git' / target_branch + local_git_repo_path.mkdir(parents=True, exist_ok=True) + clone_git_repo(index_git_repo, target_branch, token_name, git_token, str(local_git_repo_path)) + + # Content from the image (source of truth), scaffolding from the OCP branch. + extracted_configs, extracted_db = extract_catalog_and_db_from_image(from_index, temp_dir) + catalog_path = local_git_repo_path / 'configs' + if catalog_path.exists(): + shutil.rmtree(catalog_path) + shutil.copytree(extracted_configs, catalog_path) + + return BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=str(local_git_repo_path), + localized_git_catalog_path=str(catalog_path), + index_db_path=extracted_db, + target_branch=target_branch, + is_divergent=True, + ) + + def prepare_git_repository_for_build( request_id: int, from_index: str, diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index 3c460aa04..75b7d6f89 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -2,11 +2,13 @@ import json import os import tarfile +from unittest import mock from unittest.mock import patch import pytest from iib.exceptions import IIBError +from iib.workers.tasks import containerized_utils as cu from iib.workers.tasks.containerized_utils import ( extract_catalog_and_db_from_image, extract_files_from_image_non_privileged, @@ -1487,3 +1489,79 @@ def test_extract_catalog_and_db_raises_without_configs_label(mock_label): with pytest.raises(IIBError, match='does not contain a file-based catalog'): extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', '/tmp/does-not-matter') + + +@mock.patch('iib.workers.tasks.containerized_utils.set_request_state') +@mock.patch('iib.workers.tasks.containerized_utils.clone_git_repo') +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists', return_value=True) +@mock.patch('iib.workers.tasks.containerized_utils.get_git_token', return_value=('n', 't')) +@mock.patch( + 'iib.workers.tasks.containerized_utils.resolve_git_url', return_value='https://gitlab/x.git' +) +def test_prepare_build_sources_normal( + mock_url, mock_tok, mock_exists, mock_clone, mock_state, tmp_path +): + (tmp_path / 'git' / 'v4.14' / 'configs').mkdir(parents=True) + src = cu.prepare_build_sources( + request_id=1, + from_index='quay.io/redhat/my-index:v4.14', + temp_dir=str(tmp_path), + ocp_version='v4.14', + index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, + overwrite_from_index=True, + ) + assert src.is_divergent is False + assert src.target_branch == 'v4.14' + assert src.index_db_path is None # normal path pulls from ORAS + + +@mock.patch('iib.workers.tasks.containerized_utils.set_request_state') +@mock.patch('iib.workers.tasks.containerized_utils.get_git_token', return_value=('n', 't')) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists', return_value=False) +@mock.patch( + 'iib.workers.tasks.containerized_utils.resolve_git_url', return_value='https://gitlab/x.git' +) +def test_prepare_build_sources_divergent_rejects_overwrite( + mock_url, mock_exists, mock_tok, mock_state, tmp_path +): + with pytest.raises(IIBError, match='overwrite'): + cu.prepare_build_sources( + request_id=1, + from_index='quay.io/redhat/my-index:test', + temp_dir=str(tmp_path), + ocp_version='v4.14', + index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, + overwrite_from_index=True, + ) + + +@mock.patch('iib.workers.tasks.containerized_utils.set_request_state') +@mock.patch('iib.workers.tasks.containerized_utils.extract_catalog_and_db_from_image') +@mock.patch('iib.workers.tasks.containerized_utils.clone_git_repo') +@mock.patch('iib.workers.tasks.containerized_utils.get_git_token', return_value=('n', 't')) +@mock.patch( + 'iib.workers.tasks.containerized_utils.resolve_git_url', return_value='https://gitlab/x.git' +) +def test_prepare_build_sources_divergent_extracts( + mock_url, mock_tok, mock_clone, mock_extract, mock_state, tmp_path +): + # tag branch missing, ocp branch present + with mock.patch( + 'iib.workers.tasks.containerized_utils.remote_branch_exists', + side_effect=[False, True], + ): + cfg = tmp_path / 'ex_configs' + cfg.mkdir() + (cfg / 'op').mkdir() + mock_extract.return_value = (str(cfg), str(tmp_path / 'ex.db')) + src = cu.prepare_build_sources( + request_id=1, + from_index='quay.io/redhat/my-index:test', + temp_dir=str(tmp_path), + ocp_version='v4.14', + index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, + overwrite_from_index=False, + ) + assert src.is_divergent is True + assert src.target_branch == 'v4.14' + assert src.index_db_path == str(tmp_path / 'ex.db') From de380a406ce1fe9c547424373d216ee278e495d4 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 02:20:19 -0700 Subject: [PATCH 05/18] feat: route add requests through normal/divergent build sources Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/build_containerized_add.py | 37 ++-- .../test_build_containerized_add.py | 203 ++++++++++++++++-- 2 files changed, 205 insertions(+), 35 deletions(-) diff --git a/iib/workers/tasks/build_containerized_add.py b/iib/workers/tasks/build_containerized_add.py index cc614c9ec..ad93d027b 100644 --- a/iib/workers/tasks/build_containerized_add.py +++ b/iib/workers/tasks/build_containerized_add.py @@ -20,7 +20,7 @@ ) from iib.workers.tasks.celery import app from iib.workers.tasks.containerized_utils import ( - prepare_git_repository_for_build, + prepare_build_sources, fetch_and_verify_index_db_artifact, write_build_metadata, git_commit_and_create_mr, @@ -160,26 +160,29 @@ def handle_containerized_add_request( present_bundles: List[BundleImage] = [] present_bundles_pull_spec: List[str] = [] with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: - branch = prebuild_info['ocp_version'] - - # Set up and clone Git repository - ( - index_git_repo, - local_git_repo_path, - localized_git_catalog_path, - ) = prepare_git_repository_for_build( + sources = prepare_build_sources( request_id=request_id, from_index=str(from_index), temp_dir=temp_dir, - branch=branch, + ocp_version=prebuild_info['ocp_version'], index_to_gitlab_push_map=index_to_gitlab_push_map, + overwrite_from_index=overwrite_from_index, ) - - # Pull index.db artifact (uses ImageStream cache if configured, otherwise pulls directly) - artifact_index_db_file = fetch_and_verify_index_db_artifact( - from_index=str(from_index), - temp_dir=temp_dir, - ) + index_git_repo = sources.index_git_repo + local_git_repo_path = sources.local_git_repo_path + localized_git_catalog_path = sources.localized_git_catalog_path + branch = sources.target_branch + + # Divergent path already has index.db extracted from the image; the normal + # path pulls it from ORAS. NEVER fall back to ORAS on the divergent path — + # that would read the base OCP branch's index.db. + if sources.index_db_path is not None: + artifact_index_db_file = sources.index_db_path + else: + artifact_index_db_file = fetch_and_verify_index_db_artifact( + from_index=str(from_index), + temp_dir=temp_dir, + ) msg = 'Checking if bundles are already present in index image' log.info(msg) @@ -343,7 +346,7 @@ def handle_containerized_add_request( # Merge or close the MR as the final step so that all side effects # (replication, metadata, index.db push) have succeeded before git # is advanced. This prevents git/index.db divergence on partial failure. - if overwrite_from_index: + if overwrite_from_index and not sources.is_divergent: merge_mr_after_build(mr_details, index_git_repo) # Prevent cleanup_on_failure from trying to close an already-merged MR mr_details = None diff --git a/tests/test_workers/test_tasks/test_build_containerized_add.py b/tests/test_workers/test_tasks/test_build_containerized_add.py index 9ba81a39f..a357ea957 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_add.py +++ b/tests/test_workers/test_tasks/test_build_containerized_add.py @@ -5,7 +5,7 @@ import pytest from iib.exceptions import IIBError -from iib.workers.tasks import build_containerized_add +from iib.workers.tasks import build_containerized_add, containerized_utils @pytest.mark.parametrize('check_related_images', (True, False)) @@ -51,7 +51,7 @@ @mock.patch('iib.workers.tasks.build_containerized_add._get_missing_bundles') @mock.patch('iib.workers.tasks.build_containerized_add._get_present_bundles') @mock.patch('iib.workers.tasks.build_containerized_add.fetch_and_verify_index_db_artifact') -@mock.patch('iib.workers.tasks.build_containerized_add.prepare_git_repository_for_build') +@mock.patch('iib.workers.tasks.build_containerized_add.prepare_build_sources') @mock.patch('iib.workers.tasks.build_containerized_add.tempfile.TemporaryDirectory') @mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_build_state') @mock.patch('iib.workers.tasks.build_containerized_add.Opm') @@ -71,7 +71,7 @@ def test_handle_containerized_add_request( mock_opm, mock_update_build_state, mock_td, - mock_prepare_git, + mock_prepare_sources, mock_fetch_index_db, mock_get_present, mock_get_missing, @@ -128,10 +128,13 @@ def test_handle_containerized_add_request( local_git_repo_path = Path(tmpdir) / 'git_repo' localized_git_catalog_path = Path(local_git_repo_path) / "configs" local_git_repo_path.mkdir(parents=True) - mock_prepare_git.return_value = ( - index_git_repo, - local_git_repo_path, - localized_git_catalog_path, + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=local_git_repo_path, + localized_git_catalog_path=localized_git_catalog_path, + index_db_path=None, + target_branch='v4.12', + is_divergent=False, ) mock_fetch_index_db.return_value = index_db_path @@ -207,12 +210,13 @@ def test_handle_containerized_add_request( mock_prepare_req.assert_called_once() # Verify git preparation - mock_prepare_git.assert_called_once_with( + mock_prepare_sources.assert_called_once_with( request_id=request_id, from_index=str(from_index), temp_dir=temp_dir_path, - branch='v4.12', + ocp_version='v4.12', index_to_gitlab_push_map={}, + overwrite_from_index=False, ) # Verify bundle checks @@ -307,7 +311,7 @@ def test_handle_containerized_add_request( @mock.patch('iib.workers.tasks.build_containerized_add._get_missing_bundles') @mock.patch('iib.workers.tasks.build_containerized_add._get_present_bundles') @mock.patch('iib.workers.tasks.build_containerized_add.fetch_and_verify_index_db_artifact') -@mock.patch('iib.workers.tasks.build_containerized_add.prepare_git_repository_for_build') +@mock.patch('iib.workers.tasks.build_containerized_add.prepare_build_sources') @mock.patch('iib.workers.tasks.build_containerized_add.tempfile.TemporaryDirectory') @mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_build_state') @mock.patch('iib.workers.tasks.build_containerized_add.Opm') @@ -327,7 +331,7 @@ def test_handle_containerized_add_request_failure( mock_opm, mock_update_build_state, mock_td, - mock_prepare_git, + mock_prepare_sources, mock_fetch_index_db, mock_get_present, mock_get_missing, @@ -371,7 +375,14 @@ def test_handle_containerized_add_request_failure( mock_prepare_req.return_value = prebuild_info # Mock git repo preparation - mock_prepare_git.return_value = (mock.Mock(), '/tmp/repo', '/tmp/repo/catalog') + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=mock.Mock(), + local_git_repo_path='/tmp/repo', + localized_git_catalog_path='/tmp/repo/catalog', + index_db_path=None, + target_branch='v4.12', + is_divergent=False, + ) # Mock TD mock_td.return_value.__enter__.return_value = '/tmp/iib-test' @@ -428,7 +439,7 @@ def test_handle_containerized_add_request_failure( @mock.patch('iib.workers.tasks.build_containerized_add._get_missing_bundles') @mock.patch('iib.workers.tasks.build_containerized_add._get_present_bundles') @mock.patch('iib.workers.tasks.build_containerized_add.fetch_and_verify_index_db_artifact') -@mock.patch('iib.workers.tasks.build_containerized_add.prepare_git_repository_for_build') +@mock.patch('iib.workers.tasks.build_containerized_add.prepare_build_sources') @mock.patch('iib.workers.tasks.build_containerized_add.tempfile.TemporaryDirectory') @mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_build_state') @mock.patch('iib.workers.tasks.build_containerized_add.Opm') @@ -450,7 +461,7 @@ def test_handle_containerized_add_request_overwrite( mock_opm, mock_update_build_state, mock_td, - mock_prepare_git, + mock_prepare_sources, mock_fetch_index_db, mock_get_present, mock_get_missing, @@ -502,10 +513,13 @@ def test_handle_containerized_add_request_overwrite( local_git_repo_path = Path(tmpdir) / 'git_repo' localized_git_catalog_path = Path(local_git_repo_path) / "configs" local_git_repo_path.mkdir(parents=True) - mock_prepare_git.return_value = ( - index_git_repo, - local_git_repo_path, - localized_git_catalog_path, + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=local_git_repo_path, + localized_git_catalog_path=localized_git_catalog_path, + index_db_path=None, + target_branch='v4.12', + is_divergent=False, ) mock_fetch_index_db.return_value = index_db_path @@ -547,3 +561,156 @@ def test_handle_containerized_add_request_overwrite( # Verify the handler completed successfully mock_push_index_db.assert_called_once() mock_cleanup_failure.assert_not_called() + + +@mock.patch('iib.workers.tasks.build_containerized_add.shutil.copytree') +@mock.patch('iib.workers.tasks.build_containerized_add.Path.mkdir') +@mock.patch('iib.workers.tasks.build_containerized_add.cleanup_on_failure') +@mock.patch('iib.workers.tasks.build_containerized_add.set_request_state') +@mock.patch('iib.workers.tasks.build_containerized_add.cleanup_merge_request_if_exists') +@mock.patch('iib.workers.tasks.build_containerized_add.push_index_db_artifact') +@mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_pull_spec') +@mock.patch('iib.workers.tasks.build_containerized_add.replicate_image_to_tagged_destinations') +@mock.patch('iib.workers.tasks.build_containerized_add.monitor_pipeline_and_extract_image') +@mock.patch('iib.workers.tasks.build_containerized_add.git_commit_and_create_mr') +@mock.patch('iib.workers.tasks.build_containerized_add.write_build_metadata') +@mock.patch('iib.workers.tasks.build_containerized_add.chmod_recursively') +@mock.patch('iib.workers.tasks.build_containerized_add.merge_catalogs_dirs') +@mock.patch( + 'iib.workers.tasks.build_containerized_add.remove_deprecated_operators_from_git_catalog' +) +@mock.patch('iib.workers.tasks.build_containerized_add.Path.is_dir') +@mock.patch('iib.workers.tasks.build_containerized_add.opm_migrate') +@mock.patch('iib.workers.tasks.build_containerized_add.deprecate_bundles_db') +@mock.patch('iib.workers.tasks.build_containerized_add.get_bundles_from_deprecation_list') +@mock.patch('iib.workers.tasks.build_containerized_add._opm_registry_add') +@mock.patch('iib.workers.tasks.build_containerized_add._get_missing_bundles') +@mock.patch('iib.workers.tasks.build_containerized_add._get_present_bundles') +@mock.patch('iib.workers.tasks.build_containerized_add.fetch_and_verify_index_db_artifact') +@mock.patch('iib.workers.tasks.build_containerized_add.prepare_build_sources') +@mock.patch('iib.workers.tasks.build_containerized_add.tempfile.TemporaryDirectory') +@mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_build_state') +@mock.patch('iib.workers.tasks.build_containerized_add.Opm') +@mock.patch('iib.workers.tasks.build_containerized_add.prepare_request_for_build') +@mock.patch('iib.workers.tasks.build_containerized_add.inspect_related_images') +@mock.patch('iib.workers.tasks.build_containerized_add.verify_labels') +@mock.patch('iib.workers.tasks.build_containerized_add.get_resolved_bundles') +@mock.patch('iib.workers.tasks.build_containerized_add.set_registry_token') +@mock.patch('iib.workers.tasks.build_containerized_add.reset_docker_config') +@mock.patch('iib.workers.tasks.build_containerized_add.merge_mr_after_build') +def test_add_divergent_never_merges( + mock_merge_mr, + mock_reset_docker, + mock_set_token, + mock_get_resolved, + mock_verify_labels, + mock_inspect, + mock_prepare_req, + mock_opm, + mock_update_build_state, + mock_td, + mock_prepare_sources, + mock_fetch_index_db, + mock_get_present, + mock_get_missing, + mock_opm_add, + mock_get_deprecations, + mock_deprecate, + mock_opm_migrate, + mock_path_isdir, + mock_remove_deprecated, + mock_merge, + mock_chmod, + mock_write_meta, + mock_git_commit, + mock_monitor, + mock_replicate, + mock_update_pull_spec, + mock_push_index_db, + mock_cleanup_mr, + mock_set_state, + mock_cleanup_failure, + mock_makedirs, + mock_copytree, + tmpdir, +): + """Divergent path must never fall back to ORAS and must never merge the MR.""" + bundles = ['some-bundle:latest'] + request_id = 789 + binary_image = 'binary-image:latest' + resolved_bundles = ['some-bundle@sha256:123456'] + index_db_path = '/tmp/divergent/index.db' + temp_dir_path = '/tmp/iib-789-temp' + from_index = 'index:latest' + + mock_get_resolved.return_value = resolved_bundles + mock_td.return_value.__enter__.return_value = temp_dir_path + + prebuild_info = { + 'from_index_resolved': 'from-index@sha256:abcdef', + 'binary_image_resolved': 'binary-image@sha256:fedcba', + 'arches': {'amd64'}, + 'bundle_mapping': {'some-operator': resolved_bundles}, + 'ocp_version': 'v4.12', + 'distribution_scope': 'prod', + 'binary_image': binary_image, + } + mock_prepare_req.return_value = prebuild_info + + index_git_repo = mock.Mock() + local_git_repo_path = Path(tmpdir) / 'git_repo' + localized_git_catalog_path = Path(local_git_repo_path) / "configs" + local_git_repo_path.mkdir(parents=True) + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=local_git_repo_path, + localized_git_catalog_path=localized_git_catalog_path, + index_db_path=index_db_path, + target_branch='v4.14', + is_divergent=True, + ) + + mock_path_isdir.return_value = True + mock_get_present.return_value = ([], []) + mock_get_missing.return_value = resolved_bundles + mock_get_deprecations.return_value = [] + + catalog_from_db = '/tmp/from_db' + mock_opm_migrate.return_value = (catalog_from_db, None) + + mr_details = {'mr_id': 1, 'mr_url': 'https://gitlab.com/mr/1', 'source_branch': 'iib-789-v4.14'} + mock_git_commit.return_value = (mr_details, 'commit_sha_789') + + image_url = 'registry.example.com/output-image:tag' + mock_monitor.return_value = image_url + + output_pull_specs = ['registry.example.com/final-image:789'] + mock_replicate.return_value = output_pull_specs + + mock_push_index_db.return_value = 'sha256:index_db_digest' + + build_containerized_add.handle_containerized_add_request( + bundles=bundles, + request_id=request_id, + binary_image=binary_image, + from_index=from_index, + overwrite_from_index=False, + overwrite_from_index_token="user:pass", + ) + + # Divergent path uses the extracted index.db, never ORAS. + mock_fetch_index_db.assert_not_called() + + # Divergent MR must never be merged, even though overwrite_from_index is False here + # (the guard also protects the True case, but this exercises the non-merge branch). + mock_merge_mr.assert_not_called() + mock_cleanup_mr.assert_called_once() + + mock_opm_add.assert_called_once_with( + base_dir=temp_dir_path, + index_db=index_db_path, + bundles=resolved_bundles, + overwrite_csv=False, + graph_update_mode=None, + ) + mock_cleanup_failure.assert_not_called() From 31d072afe1347fbdd4c38eb63ea9fc3ca6360e8d Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 02:30:53 -0700 Subject: [PATCH 06/18] feat: route rm requests through normal/divergent build sources Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/build_containerized_rm.py | 37 +++-- .../test_tasks/test_build_containerized_rm.py | 146 +++++++++++++++++- 2 files changed, 165 insertions(+), 18 deletions(-) diff --git a/iib/workers/tasks/build_containerized_rm.py b/iib/workers/tasks/build_containerized_rm.py index 1d5185953..5dec5f80b 100644 --- a/iib/workers/tasks/build_containerized_rm.py +++ b/iib/workers/tasks/build_containerized_rm.py @@ -20,7 +20,7 @@ fetch_and_verify_index_db_artifact, git_commit_and_create_mr, monitor_pipeline_and_extract_image, - prepare_git_repository_for_build, + prepare_build_sources, push_index_db_artifact, replicate_image_to_tagged_destinations, write_build_metadata, @@ -134,26 +134,29 @@ def handle_containerized_rm_request( original_index_db_digest: Optional[str] = None with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: - branch = ocp_version - - # Set up and clone Git repository - ( - index_git_repo, - local_git_repo_path, - localized_git_catalog_path, - ) = prepare_git_repository_for_build( + sources = prepare_build_sources( request_id=request_id, from_index=from_index, temp_dir=temp_dir, - branch=branch, + ocp_version=ocp_version, index_to_gitlab_push_map=index_to_gitlab_push_map or {}, + overwrite_from_index=overwrite_from_index, ) - - # Pull index.db artifact (uses ImageStream cache if configured, otherwise pulls directly) - index_db_path = fetch_and_verify_index_db_artifact( - from_index=from_index, - temp_dir=temp_dir, - ) + index_git_repo = sources.index_git_repo + local_git_repo_path = sources.local_git_repo_path + localized_git_catalog_path = sources.localized_git_catalog_path + branch = sources.target_branch + + # Divergent path already has index.db extracted from the image; the normal + # path pulls it from ORAS. NEVER fall back to ORAS on the divergent path — + # that would read the base OCP branch's index.db. + if sources.index_db_path is not None: + index_db_path = sources.index_db_path + else: + index_db_path = fetch_and_verify_index_db_artifact( + from_index=from_index, + temp_dir=temp_dir, + ) # Remove operators from /configs set_request_state(request_id, 'in_progress', 'Removing operators from catalog') @@ -299,7 +302,7 @@ def handle_containerized_rm_request( # Merge or close the MR as the final step so that all side effects # (replication, metadata, index.db push) have succeeded before git # is advanced. This prevents git/index.db divergence on partial failure. - if overwrite_from_index: + if overwrite_from_index and not sources.is_divergent: merge_mr_after_build(mr_details, index_git_repo) # Prevent cleanup_on_failure from trying to close an already-merged MR mr_details = None diff --git a/tests/test_workers/test_tasks/test_build_containerized_rm.py b/tests/test_workers/test_tasks/test_build_containerized_rm.py index 76eef493a..94b43e2b3 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_rm.py +++ b/tests/test_workers/test_tasks/test_build_containerized_rm.py @@ -4,10 +4,11 @@ from unittest import mock from iib.exceptions import IIBError -from iib.workers.tasks import build_containerized_rm +from iib.workers.tasks import build_containerized_rm, containerized_utils from iib.workers.tasks.utils import RequestConfigAddRm +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -83,6 +84,7 @@ def test_handle_containerized_rm_request_success_with_overwrite( mock_uiips, mock_cof, mock_rdc, + mock_rbe, ): """Test successful operator removal with overwrite_from_index.""" # Setup @@ -220,6 +222,7 @@ def test_handle_containerized_rm_request_success_with_overwrite( assert mock_rdc.call_count >= 1 +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -291,6 +294,7 @@ def test_handle_containerized_rm_request_with_mr( mock_uiips, mock_cof, mock_rdc, + mock_rbe, ): """Test operator removal without overwrite creates and closes MR.""" # Setup @@ -383,6 +387,7 @@ def test_handle_containerized_rm_request_with_mr( ({'op1', 'op2'}, True), # Multiple operators in DB ], ) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -456,6 +461,7 @@ def test_handle_containerized_rm_conditional_opm_rm( mock_uiips, mock_cof, mock_rdc, + mock_rbe, operators_in_db, should_call_opm_rm, ): @@ -589,6 +595,7 @@ def test_handle_containerized_rm_missing_git_mapping( ) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.containerized_utils.clone_git_repo') @@ -614,6 +621,7 @@ def test_handle_containerized_rm_missing_configs_dir( mock_cgr, mock_cof, mock_rdc, + mock_rbe, ): """Test that missing configs directory raises error.""" request_id = 5 @@ -650,6 +658,7 @@ def test_handle_containerized_rm_missing_configs_dir( mock_cof.assert_not_called() +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.containerized_utils.pull_index_db_artifact') @@ -681,6 +690,7 @@ def test_handle_containerized_rm_missing_index_db( mock_pida, mock_cof, mock_rdc, + mock_rbe, ): """Test that missing index.db file raises error.""" request_id = 6 @@ -717,6 +727,7 @@ def test_handle_containerized_rm_missing_index_db( mock_cof.assert_not_called() +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @@ -772,6 +783,7 @@ def test_handle_containerized_rm_pipeline_failure( mock_gpiu, mock_cof, mock_rdc, + mock_rbe, ): """Test that pipeline failure triggers cleanup.""" request_id = 7 @@ -828,6 +840,7 @@ def test_handle_containerized_rm_pipeline_failure( assert 'Pipeline not found' in cleanup_call[1]['reason'] +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -901,6 +914,7 @@ def test_handle_containerized_rm_with_index_db_push( mock_uiips, mock_cof, mock_rdc, + mock_rbe, ): """Test that index.db is pushed when operators exist in DB and overwrite token is provided.""" request_id = 8 @@ -990,6 +1004,7 @@ def test_handle_containerized_rm_with_index_db_push( (['latest', 'v4.14'], 3), # request_id + latest + v4.14 ], ) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -1061,6 +1076,7 @@ def test_handle_containerized_rm_with_build_tags( mock_uiips, mock_cof, mock_rdc, + mock_rbe, build_tags, expected_tag_count, ): @@ -1131,6 +1147,7 @@ def test_handle_containerized_rm_with_build_tags( assert mock_poa.call_count == 2 # request_id tag + v4.x tag (overwrite_from_index=True) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') @@ -1202,6 +1219,7 @@ def test_handle_containerized_rm_close_mr_failure_logged( mock_uiips, mock_cof, mock_rdc, + mock_rbe, ): """Test that MR close failure is logged but doesn't fail the request.""" request_id = 10 @@ -1268,6 +1286,7 @@ def test_handle_containerized_rm_close_mr_failure_logged( assert final_call[0][1] == 'complete' +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -1319,6 +1338,7 @@ def test_handle_containerized_rm_pipelinerun_missing_name( mock_fpr, mock_cof, mock_rdc, + mock_rbe, ): """Test error when pipelinerun metadata doesn't contain name.""" request_id = 11 @@ -1372,6 +1392,7 @@ def test_handle_containerized_rm_pipelinerun_missing_name( mock_cof.assert_called_once() +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') @mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') @mock.patch('iib.workers.tasks.containerized_utils._skopeo_copy') @@ -1433,6 +1454,7 @@ def test_handle_containerized_rm_missing_output_pull_spec( mock_sc, mock_cof, mock_rdc, + mock_rbe, ): """Test error when output_pull_spec is not set (defensive check).""" request_id = 12 @@ -1490,3 +1512,125 @@ def test_handle_containerized_rm_missing_output_pull_spec( # Verify cleanup was called mock_cof.assert_called_once() + + +@mock.patch('iib.workers.tasks.build_containerized_rm.reset_docker_config') +@mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_on_failure') +@mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_pull_spec') +@mock.patch('iib.workers.tasks.build_containerized_rm.cleanup_merge_request_if_exists') +@mock.patch('iib.workers.tasks.build_containerized_rm.merge_mr_after_build') +@mock.patch('iib.workers.tasks.build_containerized_rm.push_index_db_artifact') +@mock.patch('iib.workers.tasks.build_containerized_rm.replicate_image_to_tagged_destinations') +@mock.patch('iib.workers.tasks.build_containerized_rm.monitor_pipeline_and_extract_image') +@mock.patch('iib.workers.tasks.build_containerized_rm.git_commit_and_create_mr') +@mock.patch('iib.workers.tasks.build_containerized_rm.write_build_metadata') +@mock.patch('iib.workers.tasks.build_containerized_rm.opm_validate') +@mock.patch('iib.workers.tasks.build_containerized_rm.merge_catalogs_dirs') +@mock.patch('iib.workers.tasks.build_containerized_rm.opm_registry_rm_fbc') +@mock.patch('iib.workers.tasks.build_containerized_rm.verify_operators_exists') +@mock.patch('iib.workers.tasks.build_containerized_rm.fetch_and_verify_index_db_artifact') +@mock.patch('iib.workers.tasks.build_containerized_rm.prepare_build_sources') +@mock.patch('iib.workers.tasks.build_containerized_rm.remove_operator_deprecations') +@mock.patch('iib.workers.tasks.build_containerized_rm.os.path.exists') +@mock.patch('iib.workers.tasks.build_containerized_rm.os.rename') +@mock.patch('iib.workers.tasks.build_containerized_rm.shutil.rmtree') +@mock.patch('iib.workers.tasks.build_containerized_rm.shutil.copytree') +@mock.patch('iib.workers.tasks.build_containerized_rm.tempfile.TemporaryDirectory') +@mock.patch('iib.workers.tasks.build_containerized_rm._update_index_image_build_state') +@mock.patch('iib.workers.tasks.build_containerized_rm.Opm') +@mock.patch('iib.workers.tasks.build_containerized_rm.prepare_request_for_build') +@mock.patch('iib.workers.tasks.build_containerized_rm.set_request_state') +def test_rm_divergent_never_merges( + mock_srs, + mock_prfb, + mock_opm, + mock_uiibs, + mock_tempdir, + mock_copytree, + mock_rmtree, + mock_rename, + mock_os_exists, + mock_remove_deprecations, + mock_prepare_sources, + mock_fetch_index_db, + mock_voe, + mock_orrf, + mock_mcd, + mock_ov, + mock_wbm, + mock_git_commit, + mock_monitor, + mock_replicate, + mock_push_index_db, + mock_merge_mr, + mock_cleanup_mr, + mock_update_pull_spec, + mock_cof, + mock_rdc, + tmp_path, +): + """Divergent path must never fall back to ORAS and must never merge the MR.""" + request_id = 789 + operators = ['operator1'] + from_index = 'quay.io/namespace/index-image:v4.99' + binary_image = 'registry.io/binary:latest' + + temp_dir = '/tmp/iib-789-test' + mock_tempdir.return_value.__enter__.return_value = temp_dir + + mock_prfb.return_value = { + 'arches': {'amd64'}, + 'binary_image_resolved': 'registry.io/binary@sha256:abc', + 'from_index_resolved': 'quay.io/namespace/index-image@sha256:def', + 'ocp_version': 'v4.14', + 'distribution_scope': 'prod', + } + mock_opm.opm_version = 'v1.28.0' + mock_os_exists.return_value = True + + index_db_path = str(tmp_path / 'index.db') + index_git_repo = 'https://gitlab.com/repo/x.git' + local_git_repo_path = str(tmp_path / 'git_repo') + localized_git_catalog_path = str(tmp_path / 'git_repo' / 'configs') + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=local_git_repo_path, + localized_git_catalog_path=localized_git_catalog_path, + index_db_path=index_db_path, + target_branch='v4.14', + is_divergent=True, + ) + + mock_voe.return_value = ({'operator1'}, index_db_path) + fbc_dir = os.path.join(temp_dir, 'fbc') + mock_orrf.return_value = (fbc_dir, None) + + mr_details = { + 'mr_id': '1', + 'mr_url': 'https://gitlab.com/mr/1', + 'source_branch': 'iib-request-789-v4.14', + } + mock_git_commit.return_value = (mr_details, 'commit_sha_789') + mock_monitor.return_value = 'quay.io/konflux/built-image@sha256:xyz789' + mock_replicate.return_value = ['registry.example.com/final-image:789'] + mock_push_index_db.return_value = 'sha256:index_db_digest' + + build_containerized_rm.handle_containerized_rm_request( + operators=operators, + request_id=request_id, + from_index=from_index, + binary_image=binary_image, + overwrite_from_index=False, + index_to_gitlab_push_map={'quay.io/namespace/index-image': index_git_repo}, + ) + + # Divergent path uses the extracted index.db, never ORAS. + mock_fetch_index_db.assert_not_called() + + # Divergent MR must never be merged. + mock_merge_mr.assert_not_called() + mock_cleanup_mr.assert_called_once() + + mock_orrf.assert_called_once() + assert mock_orrf.call_args.kwargs['index_db_path'] == index_db_path + mock_cof.assert_not_called() From abcb41abe53bcf30ea4c419f960cee9e20e8c955 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 02:43:41 -0700 Subject: [PATCH 07/18] feat: route fbc-operations through normal/divergent build sources Co-Authored-By: Claude Opus 4.8 --- .../build_containerized_fbc_operations.py | 35 ++--- ...test_build_containerized_fbc_operations.py | 128 +++++++++++++++++- 2 files changed, 144 insertions(+), 19 deletions(-) diff --git a/iib/workers/tasks/build_containerized_fbc_operations.py b/iib/workers/tasks/build_containerized_fbc_operations.py index d28e1118d..910dadffa 100644 --- a/iib/workers/tasks/build_containerized_fbc_operations.py +++ b/iib/workers/tasks/build_containerized_fbc_operations.py @@ -18,7 +18,7 @@ fetch_and_verify_index_db_artifact, git_commit_and_create_mr, monitor_pipeline_and_extract_image, - prepare_git_repository_for_build, + prepare_build_sources, push_index_db_artifact, replicate_image_to_tagged_destinations, write_build_metadata, @@ -131,26 +131,29 @@ def handle_containerized_fbc_operation_request( _update_index_image_build_state(request_id, prebuild_info) with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: - branch = prebuild_info['ocp_version'] - - # Set up and clone Git repository - ( - index_git_repo, - local_git_repo_path, - localized_git_catalog_path, - ) = prepare_git_repository_for_build( + sources = prepare_build_sources( request_id=request_id, from_index=from_index, temp_dir=temp_dir, - branch=branch, + ocp_version=prebuild_info['ocp_version'], index_to_gitlab_push_map=index_to_gitlab_push_map, + overwrite_from_index=overwrite_from_index, ) + index_git_repo = sources.index_git_repo + local_git_repo_path = sources.local_git_repo_path + localized_git_catalog_path = sources.localized_git_catalog_path + branch = sources.target_branch - # Pull index.db artifact (uses ImageStream cache if configured, otherwise pulls directly) - artifact_index_db_file = fetch_and_verify_index_db_artifact( - from_index=from_index, - temp_dir=temp_dir, - ) + # Divergent path already has index.db extracted from the image; the normal + # path pulls it from ORAS. NEVER fall back to ORAS on the divergent path — + # that would read the base OCP branch's index.db. + if sources.index_db_path is not None: + artifact_index_db_file = sources.index_db_path + else: + artifact_index_db_file = fetch_and_verify_index_db_artifact( + from_index=from_index, + temp_dir=temp_dir, + ) set_request_state(request_id, 'in_progress', 'Adding fbc fragment') ( @@ -245,7 +248,7 @@ def handle_containerized_fbc_operation_request( # Merge or close the MR as the final step so that all side effects # (replication, metadata, index.db push) have succeeded before git # is advanced. This prevents git/index.db divergence on partial failure. - if overwrite_from_index: + if overwrite_from_index and not sources.is_divergent: merge_mr_after_build(mr_details, index_git_repo) # Prevent cleanup_on_failure from trying to close an already-merged MR mr_details = None diff --git a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py index dccb7feb9..999fe91fc 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py +++ b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py @@ -3,10 +3,11 @@ import pytest from iib.exceptions import IIBError -from iib.workers.tasks import build_containerized_fbc_operations +from iib.workers.tasks import build_containerized_fbc_operations, containerized_utils from iib.workers.tasks.utils import RequestConfigFBCOperation +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_pull_spec') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.push_index_db_artifact') @@ -60,10 +61,11 @@ def test_handle_containerized_fbc_operation_request( mock_pida_push, mock_cof, mock_uiips, + mock_rbe, ): """Test containerized FBC operation with single fragment.""" request_id = 10 - from_index = 'from-index:latest' + from_index = 'quay.io/iib/from-index:latest' binary_image = 'binary-image:latest' binary_image_config = {'prod': {'v4.5': 'some_image'}} fbc_fragments = ['fbc-fragment:latest'] @@ -176,6 +178,7 @@ def test_handle_containerized_fbc_operation_request( assert mock_srs.call_args[0][1] == 'complete' +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_pull_spec') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.push_index_db_artifact') @@ -229,10 +232,11 @@ def test_handle_containerized_fbc_operation_request_multiple_fragments( mock_pida_push, mock_cof, mock_uiips, + mock_rbe, ): """Test containerized FBC operation with multiple fragments.""" request_id = 10 - from_index = 'from-index:latest' + from_index = 'quay.io/iib/from-index:latest' binary_image = 'binary-image:latest' binary_image_config = {'prod': {'v4.5': 'some_image'}} fbc_fragments = ['fbc-fragment1:latest', 'fbc-fragment2:latest'] @@ -291,6 +295,7 @@ def test_handle_containerized_fbc_operation_request_multiple_fragments( ] +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_pull_spec') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.push_index_db_artifact') @@ -346,6 +351,7 @@ def test_handle_containerized_fbc_operation_request_with_overwrite( mock_pida_push, mock_cof, mock_uiips, + mock_rbe, ): """Test containerized FBC operation with overwrite_from_index=True.""" request_id = 10 @@ -418,6 +424,7 @@ def test_handle_containerized_fbc_operation_request_with_overwrite( ) +@mock.patch('iib.workers.tasks.containerized_utils.remote_branch_exists') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_pull_spec') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_on_failure') @mock.patch('iib.workers.tasks.build_containerized_fbc_operations.push_index_db_artifact') @@ -469,6 +476,7 @@ def test_handle_containerized_fbc_operation_request_failure( mock_pida_push, mock_cof, mock_uiips, + mock_rbe, ): """Test containerized FBC operation failure handling.""" request_id = 10 @@ -514,3 +522,117 @@ def test_handle_containerized_fbc_operation_request_failure( args, kwargs = mock_cof.call_args assert kwargs['request_id'] == request_id assert "error: Download failed" in kwargs['reason'] + + +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_pull_spec') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_on_failure') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.cleanup_merge_request_if_exists') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.merge_mr_after_build') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.push_index_db_artifact') +@mock.patch( + 'iib.workers.tasks.build_containerized_fbc_operations.replicate_image_to_tagged_destinations' +) +@mock.patch( + 'iib.workers.tasks.build_containerized_fbc_operations.monitor_pipeline_and_extract_image' +) +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.git_commit_and_create_mr') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.write_build_metadata') +@mock.patch( + 'iib.workers.tasks.build_containerized_fbc_operations.opm_registry_add_fbc_fragment_containerized' +) +@mock.patch( + 'iib.workers.tasks.build_containerized_fbc_operations.fetch_and_verify_index_db_artifact' +) +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.prepare_build_sources') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations._update_index_image_build_state') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.Opm.set_opm_version') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.prepare_request_for_build') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.get_resolved_image') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.set_request_state') +@mock.patch('iib.workers.tasks.build_containerized_fbc_operations.reset_docker_config') +def test_fbc_operations_divergent_never_merges( + mock_rdc, + mock_srs, + mock_gri, + mock_prfb, + mock_sov, + mock_uiibs, + mock_prepare_sources, + mock_fetch_index_db, + mock_oraff, + mock_wbm, + mock_git_commit, + mock_monitor, + mock_replicate, + mock_push_index_db, + mock_merge_mr, + mock_cleanup_mr, + mock_cof, + mock_uiips, + tmp_path, +): + """Divergent path must never fall back to ORAS and must never merge the MR.""" + request_id = 999 + from_index = 'quay.io/iib/from-index:v4.99' + binary_image = 'binary-image:latest' + fbc_fragments = ['quay.io/iib/fbc-fragment1:latest'] + + mock_prfb.return_value = { + 'arches': {'amd64'}, + 'binary_image_resolved': 'binary@sha256:123', + 'from_index_resolved': 'index@sha256:456', + 'ocp_version': 'v4.14', + 'distribution_scope': 'prod', + } + mock_gri.return_value = 'fbc@sha256:789' + + index_db_path = str(tmp_path / 'index.db') + index_git_repo = 'https://gitlab.com/repo/x.git' + local_git_repo_path = str(tmp_path / 'git_repo') + localized_git_catalog_path = str(tmp_path / 'git_repo' / 'configs') + mock_prepare_sources.return_value = containerized_utils.BuildSources( + index_git_repo=index_git_repo, + local_git_repo_path=local_git_repo_path, + localized_git_catalog_path=localized_git_catalog_path, + index_db_path=index_db_path, + target_branch='v4.14', + is_divergent=True, + ) + + mock_oraff.return_value = ('/tmp/updated_catalog_path', index_db_path, ['op1']) + + mr_details = { + 'mr_id': '1', + 'mr_url': 'https://gitlab.com/mr/1', + 'source_branch': 'iib-request-999-v4.14', + } + mock_git_commit.return_value = (mr_details, 'commit_sha_999') + mock_monitor.return_value = 'registry/output-image:sha256-12345' + mock_replicate.return_value = ['registry.example.com/final-image:999'] + mock_push_index_db.return_value = 'sha256:index_db_digest' + + build_containerized_fbc_operations.handle_containerized_fbc_operation_request( + request_id=request_id, + fbc_fragments=fbc_fragments, + from_index=from_index, + binary_image=binary_image, + overwrite_from_index=False, + index_to_gitlab_push_map={'quay.io/iib/from-index': index_git_repo}, + ) + + # Divergent path uses the extracted index.db, never ORAS. + mock_fetch_index_db.assert_not_called() + + # Divergent MR must never be merged. + mock_merge_mr.assert_not_called() + mock_cleanup_mr.assert_called_once() + + mock_oraff.assert_called_once_with( + request_id=request_id, + temp_dir=mock.ANY, + from_index_configs_dir=localized_git_catalog_path, + fbc_fragments=['fbc@sha256:789'], + overwrite_from_index_token=None, + index_db_path=index_db_path, + ) + mock_cof.assert_not_called() From d60faf9e5238b649e22b0235da1196756fd1bfb6 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 03:34:49 -0700 Subject: [PATCH 08/18] test: prove divergent never-merge guard with overwrite=True Co-Authored-By: Claude Opus 4.8 --- .../test_tasks/test_build_containerized_add.py | 8 +++++--- .../test_build_containerized_fbc_operations.py | 11 ++++++++--- .../test_tasks/test_build_containerized_rm.py | 8 ++++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/tests/test_workers/test_tasks/test_build_containerized_add.py b/tests/test_workers/test_tasks/test_build_containerized_add.py index a357ea957..8c2d28e8f 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_add.py +++ b/tests/test_workers/test_tasks/test_build_containerized_add.py @@ -694,15 +694,17 @@ def test_add_divergent_never_merges( request_id=request_id, binary_image=binary_image, from_index=from_index, - overwrite_from_index=False, + overwrite_from_index=True, overwrite_from_index_token="user:pass", ) # Divergent path uses the extracted index.db, never ORAS. mock_fetch_index_db.assert_not_called() - # Divergent MR must never be merged, even though overwrite_from_index is False here - # (the guard also protects the True case, but this exercises the non-merge branch). + # overwrite_from_index=True here: the divergent BuildSources bypasses Task 4's + # entry-point overwrite rejection (mocked directly), so the ONLY thing that can + # prevent a merge is the handler-level `and not sources.is_divergent` guard. If + # that guard were removed, this MR would be merged and this assertion would fail. mock_merge_mr.assert_not_called() mock_cleanup_mr.assert_called_once() diff --git a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py index 999fe91fc..33ccd0c79 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py +++ b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py @@ -611,19 +611,24 @@ def test_fbc_operations_divergent_never_merges( mock_replicate.return_value = ['registry.example.com/final-image:999'] mock_push_index_db.return_value = 'sha256:index_db_digest' + overwrite_token = 'user:token' build_containerized_fbc_operations.handle_containerized_fbc_operation_request( request_id=request_id, fbc_fragments=fbc_fragments, from_index=from_index, binary_image=binary_image, - overwrite_from_index=False, + overwrite_from_index=True, + overwrite_from_index_token=overwrite_token, index_to_gitlab_push_map={'quay.io/iib/from-index': index_git_repo}, ) # Divergent path uses the extracted index.db, never ORAS. mock_fetch_index_db.assert_not_called() - # Divergent MR must never be merged. + # overwrite_from_index=True here: the divergent BuildSources bypasses Task 4's + # entry-point overwrite rejection (mocked directly), so the ONLY thing that can + # prevent a merge is the handler-level `and not sources.is_divergent` guard. If + # that guard were removed, this MR would be merged and this assertion would fail. mock_merge_mr.assert_not_called() mock_cleanup_mr.assert_called_once() @@ -632,7 +637,7 @@ def test_fbc_operations_divergent_never_merges( temp_dir=mock.ANY, from_index_configs_dir=localized_git_catalog_path, fbc_fragments=['fbc@sha256:789'], - overwrite_from_index_token=None, + overwrite_from_index_token=overwrite_token, index_db_path=index_db_path, ) mock_cof.assert_not_called() diff --git a/tests/test_workers/test_tasks/test_build_containerized_rm.py b/tests/test_workers/test_tasks/test_build_containerized_rm.py index 94b43e2b3..39f826c7f 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_rm.py +++ b/tests/test_workers/test_tasks/test_build_containerized_rm.py @@ -1620,14 +1620,18 @@ def test_rm_divergent_never_merges( request_id=request_id, from_index=from_index, binary_image=binary_image, - overwrite_from_index=False, + overwrite_from_index=True, + overwrite_from_index_token='user:token', index_to_gitlab_push_map={'quay.io/namespace/index-image': index_git_repo}, ) # Divergent path uses the extracted index.db, never ORAS. mock_fetch_index_db.assert_not_called() - # Divergent MR must never be merged. + # overwrite_from_index=True here: the divergent BuildSources bypasses Task 4's + # entry-point overwrite rejection (mocked directly), so the ONLY thing that can + # prevent a merge is the handler-level `and not sources.is_divergent` guard. If + # that guard were removed, this MR would be merged and this assertion would fail. mock_merge_mr.assert_not_called() mock_cleanup_mr.assert_called_once() From 66d231bc536bcafe1b78be0c3c566876ea681a63 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sun, 30 Aug 2026 03:41:53 -0700 Subject: [PATCH 09/18] docs: document branch=tag convention and divergent-tag builds Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 1 + docker/containerized/README.md | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index a410b1006..609fed7f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,3 +57,4 @@ Key modules: - **Never edit existing Alembic migrations** — generate a new revision instead. - **API ↔ Worker task signatures must match** — renaming args silently breaks in-flight requests. - **Avoid making changes to the old worker architecture** - Always ask before making changes to the following functions - `handle_add_request`, `handle_rm_request`, `handle_merge_request`, `handle_create_empty_index_request`, `handle_fbc_operation_request`, `handle_regenerate_bundle_request` +- **Divergent-tag requests never merge their MR** — they reuse the base OCP branch's Konflux Component and must stay throw-away; the guard is `overwrite_from_index and not sources.is_divergent`. Never source their index.db from ORAS. diff --git a/docker/containerized/README.md b/docker/containerized/README.md index 375a1465e..b2284f6cf 100644 --- a/docker/containerized/README.md +++ b/docker/containerized/README.md @@ -253,6 +253,36 @@ The worker configuration is in `docker/containerized/worker_config.py`. This fil - Includes the containerized task modules - Validates required configuration on startup +## Git Branch and Tag Semantics + +The Git branch used for a request's catalog is keyed on the **image tag**, not a fixed OCP-version mapping. For the existing fleet this is a no-op, since prod index tags already equal the OCP version (e.g. `v4.19` → branch `v4.19`). + +### Onboarding a Non-OCP Tag + +To onboard a tag that isn't an OCP version (e.g. a custom or pre-release tag), a maintainer must: + +1. Create a Git branch in the catalog repository named exactly after the tag. +2. Provision a Konflux Component for that branch, so pushes to it trigger a PipelineRun. + +Once both exist, requests against that tag build and push normally, and `overwrite_from_index` is honored like any other branch. + +### Divergent Tags (No Matching Branch) + +If a request targets a tag with no corresponding Git branch — for example a timestamped, point-in-time tag cut from an existing index — IIB treats it as a **divergent tag**: + +- Configs and `index.db` are extracted directly from the index image (unprivileged), not sourced from ORAS. +- The build reuses the base OCP branch's existing Konflux Component via a throw-away merge request. +- That MR is **never merged** — it is always closed after the pipeline completes (or on failure), regardless of outcome. +- `overwrite_from_index` is **rejected** for divergent-tag requests; there is no direct-push path. + +This lets IIB build and validate a one-off tag without requiring per-tag branch/Component provisioning. + +## Index DB Artifact and ImageStream Tag Naming + +Cached `index.db` artifact tags (ORAS) and ImageStream tags now include a short hash of the index image pullspec, making them namespace-safe: two images with the same repository name in different registry namespaces (e.g. `quay.io/redhat/my-index:v4.17` vs `quay.io/redhat-pending/my-index:v4.17`) no longer collide on the same cache tag. + +Cache entries written under the old (pre-hash) naming scheme are orphaned by this change — they are not migrated in place. They are cleaned up by the existing cache-pruning process rather than any code path in this workflow. + ## Differences from Traditional Workflow | Aspect | Traditional Workflow | Containerized Workflow | From 8861631b42b0c5dc5ad1ae6108211caaf5db376e Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 00:35:19 -0700 Subject: [PATCH 10/18] fix: address code review findings on divergent-tag build path Resolve three correctness issues raised by automated reviewers on the index.db naming / divergent-tag work: 1. remote_branch_exists no longer runs git ls-remote with strict=False, which swallowed network/auth failures as "branch absent" and could misroute a request onto the divergent build path. It now raises on command failure and accepts optional token args so the check works against private repos (auth injected into the URL, kept out of errors). 2. Divergent extraction now sources index.db/configs from the digest-resolved pullspec (from_index_resolved) instead of the mutable tag, so the extracted content matches the image the request already inspected during prebuild and cannot drift if the tag moves. 3. extract_catalog_and_db_from_image only falls back to the labeled db / empty db when the hidden-db path is genuinely absent. A new FileNotFoundInImageError (subclass of IIBError) distinguishes an absent path from a real registry/OCI/layer/tar failure, which now propagates instead of silently degrading to a wrong index.db. Also add clarifying comments: the empty index.db artifact tag is intentionally namespace-agnostic, test_get_artifact_combined_tag intentionally omits the pullspec hash, and drop a misleading :raises IIBError: from get_index_tag's docstring. Additionally, update stale test expectations uncovered by rebasing this work onto the digest-identity base (a4ed53b): test_utils.py carried duplicate copies of the oras_utils pullspec tests still asserting the old {image_name}-{tag} naming without mocking get_image_digest, and test_build_containerized_rm.py mocked containerized_utils._get_name_and_tag_from_pullspec, a symbol the digest-identity refactor removed from that module. Both are test-expectation fixes only; no production code changed. Co-Authored-By: Claude Opus 4.8 --- iib/exceptions.py | 9 ++ iib/workers/tasks/build_containerized_add.py | 1 + .../build_containerized_create_empty_index.py | 7 +- .../build_containerized_fbc_operations.py | 1 + iib/workers/tasks/build_containerized_rm.py | 1 + iib/workers/tasks/containerized_utils.py | 70 +++++++---- iib/workers/tasks/git_utils.py | 36 +++++- iib/workers/tasks/oras_utils.py | 1 - .../test_build_containerized_add.py | 1 + .../test_tasks/test_build_containerized_rm.py | 18 --- .../test_tasks/test_containerized_utils.py | 41 ++++++- .../test_workers/test_tasks/test_git_utils.py | 29 +++++ tests/test_workers/test_tasks/test_utils.py | 116 ++++++------------ 13 files changed, 205 insertions(+), 126 deletions(-) diff --git a/iib/exceptions.py b/iib/exceptions.py index 72cdd28b4..c52454aaf 100644 --- a/iib/exceptions.py +++ b/iib/exceptions.py @@ -13,6 +13,15 @@ class IIBError(BaseException): """An error was encountered in IIB.""" +class FileNotFoundInImageError(IIBError): + """A requested path was not present in a container image. + + Subclasses IIBError so existing ``except IIBError`` handlers still catch it, + while letting callers distinguish a genuinely absent path from a real + extraction failure (registry, OCI parsing, layer, or tar error). + """ + + class ValidationError(BaseException): """Denote invalid input.""" diff --git a/iib/workers/tasks/build_containerized_add.py b/iib/workers/tasks/build_containerized_add.py index ad93d027b..99d92494d 100644 --- a/iib/workers/tasks/build_containerized_add.py +++ b/iib/workers/tasks/build_containerized_add.py @@ -163,6 +163,7 @@ def handle_containerized_add_request( sources = prepare_build_sources( request_id=request_id, from_index=str(from_index), + from_index_resolved=from_index_resolved, temp_dir=temp_dir, ocp_version=prebuild_info['ocp_version'], index_to_gitlab_push_map=index_to_gitlab_push_map, diff --git a/iib/workers/tasks/build_containerized_create_empty_index.py b/iib/workers/tasks/build_containerized_create_empty_index.py index 17e68622a..7bf4d94c6 100644 --- a/iib/workers/tasks/build_containerized_create_empty_index.py +++ b/iib/workers/tasks/build_containerized_create_empty_index.py @@ -198,7 +198,12 @@ def handle_containerized_create_empty_index_request( conf = get_worker_config() empty_tag = conf.get('iib_empty_index_db_tag', 'empty') - # Construct the pullspec for the empty index.db artifact + # Construct the pullspec for the empty index.db artifact. + # This tag intentionally omits the pullspec hash used by + # _get_artifact_combined_tag: the empty index.db is a shared, content-free + # seed artifact keyed only by image name + "empty", so it is deliberately + # namespace-agnostic and does not risk the cross-namespace collisions that + # the hash guards against for real per-index artifacts. image_name, _ = _get_name_and_tag_from_pullspec(from_index) empty_artifact_ref = conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], diff --git a/iib/workers/tasks/build_containerized_fbc_operations.py b/iib/workers/tasks/build_containerized_fbc_operations.py index 910dadffa..803985073 100644 --- a/iib/workers/tasks/build_containerized_fbc_operations.py +++ b/iib/workers/tasks/build_containerized_fbc_operations.py @@ -134,6 +134,7 @@ def handle_containerized_fbc_operation_request( sources = prepare_build_sources( request_id=request_id, from_index=from_index, + from_index_resolved=from_index_resolved, temp_dir=temp_dir, ocp_version=prebuild_info['ocp_version'], index_to_gitlab_push_map=index_to_gitlab_push_map, diff --git a/iib/workers/tasks/build_containerized_rm.py b/iib/workers/tasks/build_containerized_rm.py index 5dec5f80b..b74da2f86 100644 --- a/iib/workers/tasks/build_containerized_rm.py +++ b/iib/workers/tasks/build_containerized_rm.py @@ -137,6 +137,7 @@ def handle_containerized_rm_request( sources = prepare_build_sources( request_id=request_id, from_index=from_index, + from_index_resolved=from_index_resolved, temp_dir=temp_dir, ocp_version=ocp_version, index_to_gitlab_push_map=index_to_gitlab_push_map or {}, diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index ca28559e2..c6900e45b 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -11,7 +11,7 @@ from pathlib import Path from typing import Dict, List, Optional, Tuple, Union -from iib.exceptions import IIBError +from iib.exceptions import IIBError, FileNotFoundInImageError from iib.workers.api_utils import set_request_state from iib.workers.config import get_worker_config from iib.workers.tasks.iib_static_types import BundleImage @@ -125,9 +125,13 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path normalized_src = src_path.lstrip('/') source_full_path = extract_dir / normalized_src - # Verify the requested path exists in the extracted filesystem + # Verify the requested path exists in the extracted filesystem. + # Raise the specific FileNotFoundInImageError (a subclass of IIBError) so + # callers can distinguish a genuinely absent path from a real extraction + # failure (registry, OCI parsing, layer, or tar error), which all raise + # plain IIBError above. if not source_full_path.exists(): - raise IIBError( + raise FileNotFoundInImageError( f'Path {src_path} not found in image {image}. ' f'Looked for {source_full_path} in extracted filesystem.' ) @@ -146,30 +150,39 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) -def extract_catalog_and_db_from_image(from_index: str, temp_dir: str) -> Tuple[str, str]: +def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) -> Tuple[str, str]: """ Extract FBC configs and index.db from an index image, unprivileged. Used on the divergent-tag path where no git branch / ORAS artifact exists yet. - The from_index image is the source of truth for its own content. + The image is the source of truth for its own content. + + The caller must pass the digest-resolved pullspec (``from_index_resolved``), + not the mutable tag, so the extracted content matches the image the request + already inspected during prebuild (OPM version, build metadata) and so the + repeated image reads here cannot disagree with each other. index.db precedence: hidden db path -> configs database label -> empty db (pure-FBC images may carry no db at all; there is no primitive to build a SQLite index.db back from FBC configs, so an empty db is created and the FBC configs remain authoritative). - :param str from_index: The from_index image pullspec. + :param str from_index_resolved: The digest-resolved from_index image pullspec. :param str temp_dir: Base temp directory for extraction. :return: Tuple of (configs_dir_path, index_db_path). :rtype: Tuple[str, str] - :raises IIBError: If the image has no FBC configs label. + :raises IIBError: If the image has no FBC configs label, or if a hidden-db + extraction attempt fails for a reason other than the path being absent + (e.g. a registry, OCI parsing, layer, or tar error). """ - configs_label = get_image_label(from_index, 'operators.operatorframework.io.index.configs.v1') + configs_label = get_image_label( + from_index_resolved, 'operators.operatorframework.io.index.configs.v1' + ) if not configs_label: - raise IIBError(f"Index image {from_index} does not contain a file-based catalog.") + raise IIBError(f"Index image {from_index_resolved} does not contain a file-based catalog.") configs_dir = str(Path(temp_dir) / 'extracted_configs') - extract_files_from_image_non_privileged(from_index, configs_label, configs_dir) + extract_files_from_image_non_privileged(from_index_resolved, configs_label, configs_dir) index_db_path = str(Path(temp_dir) / 'extracted_index.db') conf = get_worker_config() @@ -177,14 +190,19 @@ def extract_catalog_and_db_from_image(from_index: str, temp_dir: str) -> Tuple[s try: # Prefer the hidden db (carries deprecated/hidden bundle state). - extract_files_from_image_non_privileged(from_index, hidden_db_path, index_db_path) + extract_files_from_image_non_privileged(from_index_resolved, hidden_db_path, index_db_path) return configs_dir, index_db_path - except IIBError: - log.info("No hidden index.db in %s; trying labeled db.", from_index) - - db_label = get_image_label(from_index, 'operators.operatorframework.io.index.database.v1') + except FileNotFoundInImageError: + # Only a genuinely absent hidden-db path falls through to the next source; + # real extraction failures (registry/OCI/layer/tar) raise plain IIBError + # and propagate, so we never silently degrade to a wrong index.db. + log.info("No hidden index.db in %s; trying labeled db.", from_index_resolved) + + db_label = get_image_label( + from_index_resolved, 'operators.operatorframework.io.index.database.v1' + ) if db_label: - extract_files_from_image_non_privileged(from_index, db_label, index_db_path) + extract_files_from_image_non_privileged(from_index_resolved, db_label, index_db_path) return configs_dir, index_db_path # Pure FBC image: no embedded index.db (hidden or labeled). There is no opm @@ -192,7 +210,7 @@ def extract_catalog_and_db_from_image(from_index: str, temp_dir: str) -> Tuple[s # goes db -> configs), so create an empty index.db. The FBC configs are # authoritative for this image; the empty db is populated by the handler's # subsequent add/rm operations. - log.info("No embedded index.db found in %s; creating empty index.db.", from_index) + log.info("No embedded index.db found in %s; creating empty index.db.", from_index_resolved) Path(index_db_path).parent.mkdir(parents=True, exist_ok=True) with open(index_db_path, 'w'): pass @@ -566,6 +584,7 @@ class BuildSources: def prepare_build_sources( request_id: int, from_index: str, + from_index_resolved: str, temp_dir: str, ocp_version: str, index_to_gitlab_push_map: Dict[str, str], @@ -579,8 +598,14 @@ def prepare_build_sources( overwrite, reuse the base OCP branch's Konflux Component, and seed content by extracting configs+index.db from the image. + Branch selection keys on the mutable tag (``from_index``), but divergent + content is extracted from the digest-resolved pullspec + (``from_index_resolved``) so it matches the image the request already + inspected during prebuild and cannot drift if the tag moves mid-request. + :param int request_id: The IIB request ID - :param str from_index: The from_index pullspec + :param str from_index: The from_index pullspec (tag form, used for branch selection) + :param str from_index_resolved: The digest-resolved from_index pullspec (content source) :param str temp_dir: Temporary directory to clone into / extract to :param str ocp_version: Base OCP version branch, e.g. "v4.19" :param Dict[str, str] index_to_gitlab_push_map: Mapping of index images to Git repositories @@ -601,7 +626,7 @@ def prepare_build_sources( tag = get_index_tag(from_index) set_request_state(request_id, 'in_progress', 'Cloning Git repository') - if remote_branch_exists(index_git_repo, tag): + if remote_branch_exists(index_git_repo, tag, token_name, git_token): # Normal path — build against the tag branch, pull index.db from ORAS. target_branch = tag local_git_repo_path = Path(temp_dir) / 'git' / target_branch @@ -629,7 +654,7 @@ def prepare_build_sources( ) target_branch = ocp_version - if not remote_branch_exists(index_git_repo, target_branch): + if not remote_branch_exists(index_git_repo, target_branch, token_name, git_token): raise IIBError( f"Base OCP branch '{target_branch}' is not onboarded for {index_git_repo}. " "Onboard the index before building divergent tags." @@ -640,7 +665,10 @@ def prepare_build_sources( clone_git_repo(index_git_repo, target_branch, token_name, git_token, str(local_git_repo_path)) # Content from the image (source of truth), scaffolding from the OCP branch. - extracted_configs, extracted_db = extract_catalog_and_db_from_image(from_index, temp_dir) + # Extract from the digest-resolved pullspec, not the mutable tag. + extracted_configs, extracted_db = extract_catalog_and_db_from_image( + from_index_resolved, temp_dir + ) catalog_path = local_git_repo_path / 'configs' if catalog_path.exists(): shutil.rmtree(catalog_path) diff --git a/iib/workers/tasks/git_utils.py b/iib/workers/tasks/git_utils.py index 8bbb4ee36..fa71b6112 100644 --- a/iib/workers/tasks/git_utils.py +++ b/iib/workers/tasks/git_utils.py @@ -165,15 +165,43 @@ def validate_git_remote_branch(repo_url: str, branch: str) -> None: raise IIBError(f"Remote branch '{branch}' not found for repo {repo_url}") -def remote_branch_exists(repo_url: str, branch: str) -> bool: +def remote_branch_exists( + repo_url: str, + branch: str, + token_name: Optional[str] = None, + token: Optional[str] = None, +) -> bool: """ - Return True if the given branch exists on the remote, without raising. + Return True if the given branch exists on the remote. + + This distinguishes a reachable remote with no matching branch (returns + False) from a failed command such as a network or auth error (raises), so a + transient failure is never silently interpreted as a missing branch. A + silent false negative would misroute a request onto the divergent build path. + + When ``token_name`` and ``token`` are provided, authentication is injected + into the remote URL the same way :func:`clone_git_repo` does, so the check + works against private repositories. :param str repo_url: The git repository URL. :param str branch: The branch name to check. + :param str token_name: Optional name of the Git repository token. + :param str token: Optional value of the Git repository token. :rtype: bool - """ - remote_branch_status = run_cmd(["git", "ls-remote", "--heads", repo_url, branch], strict=False) + :raises IIBError: If the ``git ls-remote`` command fails (e.g. network or auth error). + """ + ls_remote_url = repo_url + if token_name and token: + base_url = repo_url.replace('https://', '') + ls_remote_url = f"https://{token_name}:{token}@{base_url}" + + # strict=True (default) so a command failure raises IIBError instead of + # returning empty output that would be misread as "branch absent". The + # exc_msg deliberately references repo_url, not the token-injected URL. + remote_branch_status = run_cmd( + ["git", "ls-remote", "--heads", ls_remote_url, branch], + exc_msg=f"Error checking for remote branch '{branch}' in repo {repo_url}", + ) return bool(remote_branch_status.strip()) diff --git a/iib/workers/tasks/oras_utils.py b/iib/workers/tasks/oras_utils.py index 2674f5d97..d3d4c11d9 100644 --- a/iib/workers/tasks/oras_utils.py +++ b/iib/workers/tasks/oras_utils.py @@ -76,7 +76,6 @@ def get_index_tag(from_index: str) -> str: :param str from_index: The full index image pullspec (registry/namespace/repo:tag). :return: The tag portion of the pullspec. :rtype: str - :raises IIBError: If the pullspec parsing fails within the helper function. """ _, tag = _get_name_and_tag_from_pullspec(from_index) return tag diff --git a/tests/test_workers/test_tasks/test_build_containerized_add.py b/tests/test_workers/test_tasks/test_build_containerized_add.py index 8c2d28e8f..f66285e1b 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_add.py +++ b/tests/test_workers/test_tasks/test_build_containerized_add.py @@ -213,6 +213,7 @@ def test_handle_containerized_add_request( mock_prepare_sources.assert_called_once_with( request_id=request_id, from_index=str(from_index), + from_index_resolved='from-index@sha256:abcdef', temp_dir=temp_dir_path, ocp_version='v4.12', index_to_gitlab_push_map={}, diff --git a/tests/test_workers/test_tasks/test_build_containerized_rm.py b/tests/test_workers/test_tasks/test_build_containerized_rm.py index 39f826c7f..142164072 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_rm.py +++ b/tests/test_workers/test_tasks/test_build_containerized_rm.py @@ -19,7 +19,6 @@ @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -73,7 +72,6 @@ def test_handle_containerized_rm_request_success_with_overwrite( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -142,7 +140,6 @@ def test_handle_containerized_rm_request_success_with_overwrite( mock_gpiu.return_value = 'quay.io/konflux/built-image@sha256:xyz789' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' @@ -232,7 +229,6 @@ def test_handle_containerized_rm_request_success_with_overwrite( @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -284,7 +280,6 @@ def test_handle_containerized_rm_request_with_mr( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_poa, @@ -345,7 +340,6 @@ def test_handle_containerized_rm_request_with_mr( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions (only request_id tag, no overwrite) - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' @@ -398,7 +392,6 @@ def test_handle_containerized_rm_request_with_mr( @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -450,7 +443,6 @@ def test_handle_containerized_rm_conditional_opm_rm( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -510,7 +502,6 @@ def test_handle_containerized_rm_conditional_opm_rm( mock_gpiu.return_value = 'image@sha' # Mock ORAS push related functions (conditionally used based on operators_in_db) - mock_gntfp.return_value = ('index', 'v4.14') mock_gact.return_value = 'index-v4.14' mock_giap.return_value = 'reg/index-db:v4.14' mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' @@ -851,7 +842,6 @@ def test_handle_containerized_rm_pipeline_failure( @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -903,7 +893,6 @@ def test_handle_containerized_rm_with_index_db_push( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -958,7 +947,6 @@ def test_handle_containerized_rm_with_index_db_push( mock_gpiu.return_value = 'image@sha' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789ab' @@ -1014,7 +1002,6 @@ def test_handle_containerized_rm_with_index_db_push( @mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -1066,7 +1053,6 @@ def test_handle_containerized_rm_with_build_tags( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_gid, @@ -1116,7 +1102,6 @@ def test_handle_containerized_rm_with_build_tags( # Mock ORAS-related functions # (needed because push_index_db_artifact now called even with empty operators) - mock_gntfp.return_value = ('index', 'v4.14') mock_gact.return_value = 'index-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' mock_gid.return_value = 'sha256:abcdef' @@ -1157,7 +1142,6 @@ def test_handle_containerized_rm_with_build_tags( @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') -@mock.patch('iib.workers.tasks.containerized_utils._get_name_and_tag_from_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -1209,7 +1193,6 @@ def test_handle_containerized_rm_close_mr_failure_logged( mock_fpr, mock_wfpc, mock_gpiu, - mock_gntfp, mock_gact, mock_giap, mock_poa, @@ -1256,7 +1239,6 @@ def test_handle_containerized_rm_close_mr_failure_logged( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gntfp.return_value = ('index-image', 'v4.14') mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index 75b7d6f89..b0ad9c31f 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -7,7 +7,7 @@ import pytest -from iib.exceptions import IIBError +from iib.exceptions import IIBError, FileNotFoundInImageError from iib.workers.tasks import containerized_utils as cu from iib.workers.tasks.containerized_utils import ( extract_catalog_and_db_from_image, @@ -1449,8 +1449,9 @@ def test_extract_catalog_and_db_falls_back_to_labeled_db(mock_extract, mock_labe 'operators.operatorframework.io.index.configs.v1': '/configs', 'operators.operatorframework.io.index.database.v1': '/database/index.db', }[label] - # First call (configs) ok; second call (hidden db) raises -> fall back to labeled db - mock_extract.side_effect = [None, IIBError('no hidden db'), None] + # First call (configs) ok; second call (hidden db) raises FileNotFoundInImageError + # (path genuinely absent) -> fall back to labeled db + mock_extract.side_effect = [None, FileNotFoundInImageError('no hidden db'), None] _, index_db = extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', str(tmp_path)) @@ -1466,8 +1467,9 @@ def test_extract_catalog_and_db_pure_fbc_creates_empty_db(mock_extract, mock_lab 'operators.operatorframework.io.index.configs.v1': '/configs', 'operators.operatorframework.io.index.database.v1': '', }[label] - # First call (configs) ok; second call (hidden db) raises -> no labeled db either - mock_extract.side_effect = [None, IIBError('no hidden db')] + # First call (configs) ok; second call (hidden db) raises FileNotFoundInImageError + # (path genuinely absent) -> no labeled db either + mock_extract.side_effect = [None, FileNotFoundInImageError('no hidden db')] configs_dir, index_db = extract_catalog_and_db_from_image( 'quay.io/redhat/my-index:test', str(tmp_path) @@ -1482,6 +1484,28 @@ def test_extract_catalog_and_db_pure_fbc_creates_empty_db(mock_extract, mock_lab assert mock_extract.call_count == 2 +@patch('iib.workers.tasks.containerized_utils.get_image_label') +@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') +def test_extract_catalog_and_db_propagates_real_extraction_error( + mock_extract, mock_label, tmp_path +): + """A genuine extraction failure (not a missing path) must propagate, not degrade. + + Only FileNotFoundInImageError signals an absent hidden db; a plain IIBError + (registry/OCI/layer/tar failure) must not be silently swallowed as "no hidden + db", which would build an image from an incomplete index.db. + """ + mock_label.side_effect = lambda image, label: { + 'operators.operatorframework.io.index.configs.v1': '/configs', + 'operators.operatorframework.io.index.database.v1': '/database/index.db', + }[label] + # First call (configs) ok; second call (hidden db) raises a real error. + mock_extract.side_effect = [None, IIBError('registry unreachable')] + + with pytest.raises(IIBError, match='registry unreachable'): + extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', str(tmp_path)) + + @patch('iib.workers.tasks.containerized_utils.get_image_label') def test_extract_catalog_and_db_raises_without_configs_label(mock_label): """If the image has no FBC configs label, an IIBError is raised.""" @@ -1505,6 +1529,7 @@ def test_prepare_build_sources_normal( src = cu.prepare_build_sources( request_id=1, from_index='quay.io/redhat/my-index:v4.14', + from_index_resolved='quay.io/redhat/my-index@sha256:deadbeef', temp_dir=str(tmp_path), ocp_version='v4.14', index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, @@ -1528,6 +1553,7 @@ def test_prepare_build_sources_divergent_rejects_overwrite( cu.prepare_build_sources( request_id=1, from_index='quay.io/redhat/my-index:test', + from_index_resolved='quay.io/redhat/my-index@sha256:deadbeef', temp_dir=str(tmp_path), ocp_version='v4.14', index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, @@ -1557,6 +1583,7 @@ def test_prepare_build_sources_divergent_extracts( src = cu.prepare_build_sources( request_id=1, from_index='quay.io/redhat/my-index:test', + from_index_resolved='quay.io/redhat/my-index@sha256:deadbeef', temp_dir=str(tmp_path), ocp_version='v4.14', index_to_gitlab_push_map={'quay.io/redhat/my-index': 'https://gitlab/x.git'}, @@ -1565,3 +1592,7 @@ def test_prepare_build_sources_divergent_extracts( assert src.is_divergent is True assert src.target_branch == 'v4.14' assert src.index_db_path == str(tmp_path / 'ex.db') + # Divergent extraction must read the resolved digest, not the mutable tag, so + # the index.db matches the exact image the request resolved to. + mock_extract.assert_called_once() + assert mock_extract.call_args.args[0] == 'quay.io/redhat/my-index@sha256:deadbeef' diff --git a/tests/test_workers/test_tasks/test_git_utils.py b/tests/test_workers/test_tasks/test_git_utils.py index 3bf3aa343..ee04b347a 100644 --- a/tests/test_workers/test_tasks/test_git_utils.py +++ b/tests/test_workers/test_tasks/test_git_utils.py @@ -1285,3 +1285,32 @@ def test_remote_branch_exists_true(mock_run): def test_remote_branch_exists_false(mock_run): mock_run.return_value = '' assert git_utils.remote_branch_exists('https://gitlab/x.git', 'test') is False + + +@mock.patch('iib.workers.tasks.git_utils.run_cmd') +def test_remote_branch_exists_injects_token(mock_run): + mock_run.return_value = 'abc123\trefs/heads/v4.14\n' + + assert ( + git_utils.remote_branch_exists( + 'https://gitlab/x.git', 'v4.14', token_name='user', token='secret' + ) + is True + ) + + args, kwargs = mock_run.call_args + ls_remote_cmd = args[0] + # Token is injected into the URL passed to git, but the user-facing exc_msg + # references the token-free repo_url so the secret is not leaked in errors. + assert ls_remote_cmd[-2] == 'https://user:secret@gitlab/x.git' + assert 'secret' not in kwargs['exc_msg'] + + +@mock.patch('iib.workers.tasks.git_utils.run_cmd') +def test_remote_branch_exists_propagates_command_failure(mock_run): + # A command failure (network/auth error) must raise, not be silently read as + # "branch absent", which would misroute the request onto the divergent path. + mock_run.side_effect = IIBError('git ls-remote failed') + + with pytest.raises(IIBError, match='git ls-remote failed'): + git_utils.remote_branch_exists('https://gitlab/x.git', 'v4.14') diff --git a/tests/test_workers/test_tasks/test_utils.py b/tests/test_workers/test_tasks/test_utils.py index cad086d23..9b97bf8fb 100644 --- a/tests/test_workers/test_tasks/test_utils.py +++ b/tests/test_workers/test_tasks/test_utils.py @@ -1874,135 +1874,99 @@ def test_change_dir_invalid_directory_does_not_change_cwd(tmp_path): @pytest.mark.parametrize( - "from_index,expected_pullspec", + "from_index,digest", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "test-artifact-registry/index-db:iib-pub-pending-v4.17", - ), - ( - "quay.io/namespace/my-image:latest", - "test-artifact-registry/index-db:my-image-latest", - ), - ( - "registry.io/org/repo/index-image:v1.0.0", - "test-artifact-registry/index-db:index-image-v1.0.0", - ), - ( - "registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", - "test-artifact-registry/index-db:iib-pub-pending-v4.17", - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64), + ("quay.io/namespace/my-image:latest", "b" * 64), + ("registry.io/org/repo/index-image:v1.0.0", "c" * 64), + ("registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", "d" * 64), ], ) @mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_indexdb_artifact_pullspec(mock_gwc, from_index, expected_pullspec): +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_indexdb_artifact_pullspec(mock_digest, mock_gwc, from_index, digest): """Test constructing index DB artifact pullspecs.""" from iib.workers.tasks.oras_utils import get_indexdb_artifact_pullspec mock_gwc.return_value = { 'iib_index_db_artifact_registry': 'test-artifact-registry', 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', + 'iib_index_db_artifact_tag_template': 'idb-{digest}', } + mock_digest.return_value = f'sha256:{digest}' result = get_indexdb_artifact_pullspec(from_index) - assert result == expected_pullspec + assert result == f'test-artifact-registry/index-db:idb-{digest}' -@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_indexdb_artifact_pullspec_invalid(mock_gwc): - """Test _get_indexdb_artifact_pullspec with invalid pullspec.""" +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_indexdb_artifact_pullspec_invalid(mock_digest): + """Test get_indexdb_artifact_pullspec propagates digest resolution failures.""" from iib.workers.tasks.oras_utils import get_indexdb_artifact_pullspec - mock_gwc.return_value = { - 'iib_index_db_artifact_registry': 'test-artifact-registry', - 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', - } + mock_digest.side_effect = IIBError('Failed to inspect image') - with pytest.raises(IIBError, match="Missing tag"): + with pytest.raises(IIBError, match="Failed to inspect image"): get_indexdb_artifact_pullspec("registry.example.com/namespace/image") @pytest.mark.parametrize( - "from_index,expected_pullspec", + "from_index,digest", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "test-imagestream-registry/index-db:iib-pub-pending-v4.17", - ), - ( - "quay.io/namespace/my-image:latest", - "test-imagestream-registry/index-db:my-image-latest", - ), - ( - "registry.io/org/repo/index-image:v1.0.0", - "test-imagestream-registry/index-db:index-image-v1.0.0", - ), - ( - "registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", - "test-imagestream-registry/index-db:iib-pub-pending-v4.17", - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64), + ("quay.io/namespace/my-image:latest", "b" * 64), + ("registry.io/org/repo/index-image:v1.0.0", "c" * 64), + ("registry.example.com/namespace/iib-pub-pending:v4.17@sha256:abc123", "d" * 64), ], ) @mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_imagestream_artifact_pullspec(mock_gwc, from_index, expected_pullspec): +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_imagestream_artifact_pullspec(mock_digest, mock_gwc, from_index, digest): """Test constructing ImageStream artifact pullspecs.""" mock_gwc.return_value = { 'iib_index_db_imagestream_registry': 'test-imagestream-registry', 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', + 'iib_index_db_artifact_tag_template': 'idb-{digest}', } + mock_digest.return_value = f'sha256:{digest}' result = get_imagestream_artifact_pullspec(from_index) - assert result == expected_pullspec + assert result == f'test-imagestream-registry/index-db:idb-{digest}' -@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') -def test_get_imagestream_artifact_pullspec_invalid(mock_gwc): - """Test get_imagestream_artifact_pullspec with invalid pullspec.""" - mock_gwc.return_value = { - 'iib_index_db_imagestream_registry': 'test-imagestream-registry', - 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', - 'iib_index_db_artifact_tag_template': '{image_name}-{tag}', - } +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') +def test_get_imagestream_artifact_pullspec_invalid(mock_digest): + """Test get_imagestream_artifact_pullspec propagates digest resolution failures.""" + mock_digest.side_effect = IIBError('Failed to inspect image') - with pytest.raises(IIBError, match="Missing tag"): + with pytest.raises(IIBError, match="Failed to inspect image"): get_imagestream_artifact_pullspec("registry.example.com/namespace/image") @mock.patch('iib.workers.tasks.oras_utils.verify_indexdb_cache_sync') +@mock.patch('iib.workers.tasks.oras_utils.get_worker_config') +@mock.patch('iib.workers.tasks.oras_utils.get_image_digest') @pytest.mark.parametrize( - "pullspec,expected_combined_tag,sync_result", + "pullspec,digest,sync_result", [ - ( - "registry.example.com/namespace/iib-pub-pending:v4.17", - "iib-pub-pending-v4.17", - True, - ), - ( - "quay.io/namespace/my-image:latest", - "my-image-latest", - False, - ), - ( - "registry.io/org/repo/index-image:v1.0.0@sha256:abc123", - "index-image-v1.0.0", - True, - ), + ("registry.example.com/namespace/iib-pub-pending:v4.17", "a" * 64, True), + ("quay.io/namespace/my-image:latest", "b" * 64, False), + ("registry.io/org/repo/index-image:v1.0.0@sha256:abc123", "c" * 64, True), ], ) def test_verify_indexdb_cache_for_image( - mock_verify_sync, pullspec, expected_combined_tag, sync_result + mock_digest, mock_gwc, mock_verify_sync, pullspec, digest, sync_result ): """Test verify_indexdb_cache_for_image with various pullspecs.""" from iib.workers.tasks.oras_utils import verify_indexdb_cache_for_image + mock_gwc.return_value = {'iib_index_db_artifact_tag_template': 'idb-{digest}'} + mock_digest.return_value = f'sha256:{digest}' mock_verify_sync.return_value = sync_result result = verify_indexdb_cache_for_image(pullspec) assert result == sync_result - mock_verify_sync.assert_called_once_with(expected_combined_tag) + mock_verify_sync.assert_called_once_with(f'idb-{digest}') From 0da753d2f5483ff271d025f5ba0dca74eb0e46b2 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 00:52:03 -0700 Subject: [PATCH 11/18] feat: key index.db write on output image digest with warm-push Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 102 +++++++++--------- .../test_tasks/test_containerized_utils.py | 61 +++++++++++ 2 files changed, 110 insertions(+), 53 deletions(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index c6900e45b..30513ef58 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -33,8 +33,7 @@ wait_for_pipeline_completion, ) from iib.workers.tasks.oras_utils import ( - _get_artifact_combined_tag, - get_image_digest, + _get_index_digest, get_index_tag, get_indexdb_artifact_pullspec, get_imagestream_artifact_pullspec, @@ -422,71 +421,68 @@ def push_index_db_artifact( from_index: str, index_db_path: str, operators: List[str], + output_image: str, overwrite_from_index: bool = False, request_type: str = 'rm', -) -> Optional[str]: +) -> None: """ - Push updated index.db artifact to registry with appropriate tags. + Push the updated index.db artifact, keyed on the built OUTPUT image digest. - This function pushes the index.db file to the artifact registry with a request-specific - tag and optionally to the v4.x tag if overwrite_from_index is True. It captures - the original digest of the v4.x tag before overwriting for potential rollback. + A per-request tag is always pushed for traceability. For overwrite requests + (which advance the from_index tag to the output image) the current artifact + ``idb-`` is also pushed (warm-push), so the next request that + resolves the tag to that digest finds it cached. Content keys are never + overwritten in place, so no original-digest rollback is required. :param int request_id: The IIB request ID :param str from_index: The from_index pullspec :param str index_db_path: Path to the index.db file to push :param List[str] operators: List of operators involved in the operation + :param str output_image: The built output image pullspec, used to derive the content key :param bool overwrite_from_index: Whether to overwrite the from_index :param str request_type: Type of request (e.g., 'rm', 'add') - :return: Original digest of v4.x tag if captured, None otherwise - :rtype: Optional[str] - """ - original_index_db_digest = None - - if index_db_path and Path(index_db_path).exists(): - # Get directory and filename separately to push only the filename - # This ensures ORAS extracts the file as just "index.db" without - # directory structure - index_db_file = Path(index_db_path) - index_db_dir = str(index_db_file.parent) - index_db_filename = index_db_file.name - log.info('Pushing from directory: %s, filename: %s', index_db_dir, index_db_filename) - - # Push with request_id tag irrespective of overwrite_from_index - set_request_state(request_id, 'in_progress', 'Pushing updated index database') - conf = get_worker_config() - request_artifact_ref = conf['iib_index_db_artifact_template'].format( + """ + if not (index_db_path and Path(index_db_path).exists()): + return + + index_db_file = Path(index_db_path) + index_db_dir = str(index_db_file.parent) + index_db_filename = index_db_file.name + log.info('Pushing from directory: %s, filename: %s', index_db_dir, index_db_filename) + + set_request_state(request_id, 'in_progress', 'Pushing updated index database') + conf = get_worker_config() + output_tag = f'idb-{_get_index_digest(output_image)}' + + request_artifact_ref = conf['iib_index_db_artifact_template'].format( + registry=conf['iib_index_db_artifact_registry'], + tag=f'{output_tag}-{request_id}', + ) + artifact_refs = [request_artifact_ref] + if overwrite_from_index: + current_artifact_ref = conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], - tag=f"{_get_artifact_combined_tag(from_index)}-{request_id}", + tag=output_tag, ) - artifact_refs = [request_artifact_ref] - if overwrite_from_index: - # Get the current digest of v4.x tag before overwriting it - # This allows us to restore it if anything fails after the push - v4x_artifact_ref = get_indexdb_artifact_pullspec(from_index) - log.info('Capturing original digest of %s for potential rollback', v4x_artifact_ref) - original_index_db_digest = get_image_digest(v4x_artifact_ref) - log.info('Original index.db digest: %s', original_index_db_digest) - artifact_refs.append(v4x_artifact_ref) - - # Build annotations - only include operators if not empty - annotations = { - 'request_id': str(request_id), - 'request_type': request_type, - } - if operators: - annotations['operators'] = ','.join(operators) - - for artifact_ref in artifact_refs: - push_oras_artifact( - artifact_ref=artifact_ref, - local_path=index_db_filename, - cwd=index_db_dir, - annotations=annotations.copy(), - ) - log.info('Pushed %s to registry', artifact_ref) + artifact_refs.append(current_artifact_ref) - return original_index_db_digest + annotations = { + 'request_id': str(request_id), + 'request_type': request_type, + 'from_index': from_index, + 'output_image': output_image, + } + if operators: + annotations['operators'] = ','.join(operators) + + for artifact_ref in artifact_refs: + push_oras_artifact( + artifact_ref=artifact_ref, + local_path=index_db_filename, + cwd=index_db_dir, + annotations=annotations.copy(), + ) + log.info('Pushed %s to registry', artifact_ref) def cleanup_on_failure( diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index b0ad9c31f..f7ddded72 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -13,6 +13,7 @@ extract_catalog_and_db_from_image, extract_files_from_image_non_privileged, pull_index_db_artifact, + push_index_db_artifact, write_build_metadata, cleanup_on_failure, validate_bundles_in_parallel, @@ -287,6 +288,66 @@ def test_pull_index_db_artifact_refresh_cache_fails_falls_back_to_quay( ) +@mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') +@mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') +@mock.patch('iib.workers.tasks.containerized_utils.set_request_state') +@mock.patch('pathlib.Path.exists', return_value=True) +def test_push_keys_current_artifact_on_output_digest( + m_exists, m_state, m_gwc, m_digest, m_push, tmp_path +): + m_gwc.return_value = { + 'iib_index_db_artifact_registry': 'quay.io/iib', + 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', + } + # digest resolved from the OUTPUT image, not from_index + m_digest.return_value = 'f' * 64 + db = tmp_path / 'index.db' + db.write_text('x') + result = push_index_db_artifact( + request_id=42, + from_index='quay.io/ns/foo:v4.17', + index_db_path=str(db), + operators=['op1'], + output_image='quay.io/ns/foo@sha256:' + 'f' * 64, + overwrite_from_index=True, + request_type='add', + ) + assert result is None + m_digest.assert_called_with('quay.io/ns/foo@sha256:' + 'f' * 64) + pushed_refs = {c.kwargs['artifact_ref'] for c in m_push.call_args_list} + assert 'quay.io/iib/index-db:idb-' + 'f' * 64 in pushed_refs # warm-push (overwrite) + assert 'quay.io/iib/index-db:idb-' + 'f' * 64 + '-42' in pushed_refs # per-request tag + + +@mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') +@mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') +@mock.patch('iib.workers.tasks.containerized_utils.set_request_state') +@mock.patch('pathlib.Path.exists', return_value=True) +def test_push_throwaway_skips_current_artifact( + m_exists, m_state, m_gwc, m_digest, m_push, tmp_path +): + m_gwc.return_value = { + 'iib_index_db_artifact_registry': 'quay.io/iib', + 'iib_index_db_artifact_template': '{registry}/index-db:{tag}', + } + m_digest.return_value = 'a' * 64 + db = tmp_path / 'index.db' + db.write_text('x') + push_index_db_artifact( + request_id=7, + from_index='quay.io/ns/foo:v4.17', + index_db_path=str(db), + operators=[], + output_image='quay.io/ns/foo@sha256:' + 'a' * 64, + overwrite_from_index=False, + request_type='add', + ) + pushed_refs = {c.kwargs['artifact_ref'] for c in m_push.call_args_list} + assert pushed_refs == {'quay.io/iib/index-db:idb-' + 'a' * 64 + '-7'} # only per-request tag + + @patch('iib.workers.tasks.containerized_utils.log') def test_write_build_metadata_creates_expected_json(mock_log, tmp_path): """write_build_metadata should create JSON file with expected content.""" From 4f4df4c1f9152bc4c28f50be7f5f3d0b6e816554 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 00:53:59 -0700 Subject: [PATCH 12/18] refactor: drop index.db rollback capture (content keys are immutable) Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 35 +------ .../test_tasks/test_containerized_utils.py | 99 +------------------ 2 files changed, 9 insertions(+), 125 deletions(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index 30513ef58..cf60483fe 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -42,7 +42,7 @@ refresh_indexdb_cache_for_image, verify_indexdb_cache_for_image, ) -from iib.workers.tasks.utils import get_image_label, run_cmd, skopeo_inspect +from iib.workers.tasks.utils import get_image_label, skopeo_inspect log = logging.getLogger(__name__) @@ -493,16 +493,15 @@ def cleanup_on_failure( request_id: int, from_index: str, index_repo_map: Dict[str, str], - original_index_db_digest: Optional[str] = None, reason: str = "error", ) -> None: """ - Clean up Git changes and index.db artifacts on failure. + Clean up Git changes on failure. If a merge request was created, it will be closed (since the commit is only in a feature branch). If changes were pushed directly to the main branch, the commit - will be reverted. If the index.db artifact was pushed to the v4.x tag, it will be - restored to the original digest. + will be reverted. Content-addressed index.db artifacts are immutable, so there is + nothing to roll back on the artifact side. :param Optional[Dict[str, str]] mr_details: Details of the merge request if one was created :param Optional[str] last_commit_sha: The SHA of the last commit @@ -511,7 +510,6 @@ def cleanup_on_failure( :param int request_id: The IIB request ID :param str from_index: The from_index pullspec :param Dict[str, str] index_repo_map: Mapping of index images to Git repositories - :param Optional[str] original_index_db_digest: Original digest of index.db before overwrite :param str reason: Reason for the cleanup (used in log messages) """ if mr_details and index_git_repo: @@ -539,31 +537,6 @@ def cleanup_on_failure( else: log.error("Neither MR nor commit to revert. No cleanup needed for %s", reason) - # Restore index.db artifact to original digest if it was overwritten - if original_index_db_digest: - log.info("Restoring index.db artifact to original digest due to %s", reason) - try: - # Get the v4.x artifact reference - v4x_artifact_ref = get_indexdb_artifact_pullspec(from_index) - - # Extract registry and repository from the pullspec - # Format: quay.io/namespace/repo:tag -> we need quay.io/namespace/repo - artifact_name = v4x_artifact_ref.rsplit(':', 1)[0] - - # Use oras copy to restore the old digest to v4.x tag - # This is a registry-to-registry copy, no download needed - source_ref = f'{artifact_name}@{original_index_db_digest}' - log.info("Restoring %s from %s", v4x_artifact_ref, source_ref) - - run_cmd( - ['oras', 'copy', source_ref, v4x_artifact_ref], - exc_msg=f'Failed to restore index.db artifact ' - f'from {source_ref} to {v4x_artifact_ref}', - ) - log.info("Successfully restored index.db artifact to original digest") - except Exception as restore_error: - log.error("Failed to restore index.db artifact: %s", restore_error) - @dataclass class BuildSources: diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index f7ddded72..dd1dbe19a 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-3.0-or-later +import inspect import json import os import tarfile @@ -533,100 +534,10 @@ def test_cleanup_on_failure_no_mr_no_commit(mock_log): ) -@patch('iib.workers.tasks.containerized_utils.run_cmd') -@patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@patch('iib.workers.tasks.containerized_utils.log') -def test_cleanup_on_failure_restores_index_db_artifact( - mock_log, mock_get_indexdb_artifact_pullspec, mock_run_cmd -): - """If original_index_db_digest is provided, oras copy should be invoked correctly.""" - mr_details = None - last_commit_sha = None - index_git_repo = None - overwrite_from_index = False - request_id = 1 - from_index = 'quay.io/ns/index:v4.19' - index_repo_map = {} - original_digest = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef' - - v4x_artifact_ref = 'quay.io/ns/index-indexdb:v4.19' - mock_get_indexdb_artifact_pullspec.return_value = v4x_artifact_ref - - cleanup_on_failure( - mr_details=mr_details, - last_commit_sha=last_commit_sha, - index_git_repo=index_git_repo, - overwrite_from_index=overwrite_from_index, - request_id=request_id, - from_index=from_index, - index_repo_map=index_repo_map, - original_index_db_digest=original_digest, - ) - - mock_log.info.assert_any_call( - "Restoring index.db artifact to original digest due to %s", "error" - ) - - artifact_name = v4x_artifact_ref.rsplit(':', 1)[0] - expected_source_ref = f'{artifact_name}@{original_digest}' - - mock_run_cmd.assert_called_once_with( - ['oras', 'copy', expected_source_ref, v4x_artifact_ref], - exc_msg=( - f'Failed to restore index.db artifact from {expected_source_ref} ' - f'to {v4x_artifact_ref}' - ), - ) - mock_log.info.assert_any_call("Successfully restored index.db artifact to original digest") - - -@patch('iib.workers.tasks.containerized_utils.run_cmd') -@patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@patch('iib.workers.tasks.containerized_utils.log') -def test_cleanup_on_failure_restore_failure_is_logged( - mock_log, mock_get_indexdb_artifact_pullspec, mock_run_cmd -): - """If restoring the artifact fails, error should be logged.""" - mock_get_indexdb_artifact_pullspec.return_value = 'quay.io/ns/index-indexdb:v4.19' - mock_run_cmd.side_effect = RuntimeError("oras copy failed") - - cleanup_on_failure( - mr_details=None, - last_commit_sha=None, - index_git_repo=None, - overwrite_from_index=False, - request_id=1, - from_index='quay.io/ns/index:v4.19', - index_repo_map={}, - original_index_db_digest='sha256:0123456789abcdef0123456789abcdef0123456789abcde', - ) - - mock_run_cmd.assert_called_once() - mock_log.error.assert_any_call( - "Failed to restore index.db artifact: %s", mock_run_cmd.side_effect - ) - - -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.oras_utils.get_indexdb_artifact_pullspec') -@patch('iib.workers.tasks.utils.run_cmd') -def test_cleanup_on_failure_no_restore_when_no_original_digest( - mock_run_cmd, mock_get_indexdb_artifact_pullspec, mock_log -): - """If original_index_db_digest is not provided, restoration must not be attempted.""" - cleanup_on_failure( - mr_details=None, - last_commit_sha=None, - index_git_repo=None, - overwrite_from_index=False, - request_id=1, - from_index='quay.io/ns/index:v4.19', - index_repo_map={}, - original_index_db_digest=None, - ) - - mock_get_indexdb_artifact_pullspec.assert_not_called() - mock_run_cmd.assert_not_called() +def test_cleanup_on_failure_has_no_rollback_param(): + """Content keys are immutable: cleanup_on_failure no longer restores artifacts.""" + params = inspect.signature(cleanup_on_failure).parameters + assert 'original_index_db_digest' not in params @patch('iib.workers.tasks.containerized_utils.skopeo_inspect') From 961cd7ed41005ce788664da33d8ce24d0f393c82 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 01:11:37 -0700 Subject: [PATCH 13/18] feat: route handlers' index.db push through the output image digest Also updates build_containerized_merge.py and build_containerized_regenerate_bundle.py, which call the same push_index_db_artifact/cleanup_on_failure interfaces changed in Task 5/6 but were not enumerated in the task-7 brief; mypy caught the now-invalid call signatures. Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/build_containerized_add.py | 11 ++-- .../build_containerized_create_empty_index.py | 7 ++- .../build_containerized_fbc_operations.py | 11 ++-- .../tasks/build_containerized_merge.py | 7 ++- .../build_containerized_regenerate_bundle.py | 1 - iib/workers/tasks/build_containerized_rm.py | 7 ++- .../test_build_containerized_add.py | 9 ++-- ..._build_containerized_create_empty_index.py | 21 +++----- ...test_build_containerized_fbc_operations.py | 3 +- .../test_tasks/test_build_containerized_rm.py | 54 +++++++++---------- .../test_tasks/test_containerized_utils.py | 6 +-- 11 files changed, 60 insertions(+), 77 deletions(-) diff --git a/iib/workers/tasks/build_containerized_add.py b/iib/workers/tasks/build_containerized_add.py index 99d92494d..e3dc7c2d8 100644 --- a/iib/workers/tasks/build_containerized_add.py +++ b/iib/workers/tasks/build_containerized_add.py @@ -148,11 +148,10 @@ def handle_containerized_add_request( distribution_scope = prebuild_info['distribution_scope'] index_to_gitlab_push_map = index_to_gitlab_push_map or {} - # Variables mr_details, last_commit_sha and original_index_db_digest - # needs to be assigned; otherwise cleanup_on_failure() fails when an exception is raised. + # Variables mr_details and last_commit_sha need to be assigned; otherwise + # cleanup_on_failure() fails when an exception is raised. mr_details: Optional[Dict[str, str]] = None last_commit_sha: Optional[str] = None - original_index_db_digest: Optional[str] = None Opm.set_opm_version(from_index_resolved) @@ -334,12 +333,13 @@ def handle_containerized_add_request( # Push updated index.db before merging the MR so that on failure both # git and the index.db artifact remain consistent (MR stays open, - # cleanup_on_failure closes it and rolls back the artifact). - original_index_db_digest = push_index_db_artifact( + # cleanup_on_failure closes it; the content-addressed artifact needs no rollback). + push_index_db_artifact( request_id=request_id, from_index=str(from_index), index_db_path=artifact_index_db_file, operators=operators, + output_image=image_url, overwrite_from_index=overwrite_from_index, request_type='add', ) @@ -368,7 +368,6 @@ def handle_containerized_add_request( request_id=request_id, from_index=str(from_index), index_repo_map=index_to_gitlab_push_map or {}, - original_index_db_digest=original_index_db_digest, reason=f"error: {e}", ) raise IIBError(f"Failed to add bundles: {e}") diff --git a/iib/workers/tasks/build_containerized_create_empty_index.py b/iib/workers/tasks/build_containerized_create_empty_index.py index 7bf4d94c6..603c44587 100644 --- a/iib/workers/tasks/build_containerized_create_empty_index.py +++ b/iib/workers/tasks/build_containerized_create_empty_index.py @@ -175,7 +175,6 @@ def handle_containerized_create_empty_index_request( index_git_repo: Optional[str] = None last_commit_sha: Optional[str] = None output_pull_spec: Optional[str] = None - original_index_db_digest: Optional[str] = None with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: branch = ocp_version @@ -348,12 +347,13 @@ def handle_containerized_create_empty_index_request( # Push the empty index.db with request ID tag # Since overwrite_from_index is False, this will only push with request_id tag - # and will not overwrite the v4.x tag - original_index_db_digest = push_index_db_artifact( + # and will not overwrite the current artifact + push_index_db_artifact( request_id=request_id, from_index=from_index, index_db_path=str(index_db_path), operators=[], # Empty list since we're creating an empty index + output_image=image_url, overwrite_from_index=False, # Always False for create_empty_index request_type='create_empty_index', ) @@ -376,7 +376,6 @@ def handle_containerized_create_empty_index_request( request_id=request_id, from_index=from_index, index_repo_map=index_to_gitlab_push_map or {}, - original_index_db_digest=original_index_db_digest, reason=f"error: {e}", ) raise IIBError(f"Failed to create empty index: {e}") diff --git a/iib/workers/tasks/build_containerized_fbc_operations.py b/iib/workers/tasks/build_containerized_fbc_operations.py index 803985073..66df83df3 100644 --- a/iib/workers/tasks/build_containerized_fbc_operations.py +++ b/iib/workers/tasks/build_containerized_fbc_operations.py @@ -112,11 +112,10 @@ def handle_containerized_fbc_operation_request( distribution_scope = prebuild_info['distribution_scope'] index_to_gitlab_push_map = index_to_gitlab_push_map or {} - # Variables mr_details, last_commit_sha and original_index_db_digest - # needs to be assigned; otherwise cleanup_on_failure() fails when an exception is raised. + # Variables mr_details and last_commit_sha need to be assigned; otherwise + # cleanup_on_failure() fails when an exception is raised. mr_details: Optional[Dict[str, str]] = None last_commit_sha: Optional[str] = None - original_index_db_digest: Optional[str] = None Opm.set_opm_version(from_index_resolved) @@ -236,12 +235,13 @@ def handle_containerized_fbc_operation_request( # Push updated index.db before merging the MR so that on failure both # git and the index.db artifact remain consistent (MR stays open, - # cleanup_on_failure closes it and rolls back the artifact). - original_index_db_digest = push_index_db_artifact( + # cleanup_on_failure closes it; the content-addressed artifact needs no rollback). + push_index_db_artifact( request_id=request_id, from_index=from_index, index_db_path=index_db_path, operators=operators_in_db, + output_image=image_url, overwrite_from_index=overwrite_from_index, request_type='fbc_operations', ) @@ -271,7 +271,6 @@ def handle_containerized_fbc_operation_request( request_id=request_id, from_index=from_index, index_repo_map=index_to_gitlab_push_map or {}, - original_index_db_digest=original_index_db_digest, reason=f"error: {e}", ) raise IIBError(f"Failed to add FBC fragment: {e}") diff --git a/iib/workers/tasks/build_containerized_merge.py b/iib/workers/tasks/build_containerized_merge.py index 299123b8e..d6a61a8e5 100644 --- a/iib/workers/tasks/build_containerized_merge.py +++ b/iib/workers/tasks/build_containerized_merge.py @@ -159,7 +159,6 @@ def handle_containerized_merge_request( index_git_repo: Optional[str] = None last_commit_sha: Optional[str] = None output_pull_spec: Optional[str] = None - original_index_db_digest: Optional[str] = None with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: # Setup and clone Git repository @@ -371,12 +370,13 @@ def handle_containerized_merge_request( # Push updated index.db before merging the MR so that on failure both # git and the index.db artifact remain consistent (MR stays open, - # cleanup_on_failure closes it and rolls back the artifact). - original_index_db_digest = push_index_db_artifact( + # cleanup_on_failure closes it; the content-addressed artifact needs no rollback). + push_index_db_artifact( request_id=request_id, from_index=effective_index_image, index_db_path=source_index_db_path, operators=operators_in_db, + output_image=image_url, overwrite_from_index=overwrite_target_index, request_type='merge', ) @@ -407,7 +407,6 @@ def handle_containerized_merge_request( request_id=request_id, from_index=effective_index_image, index_repo_map={}, - original_index_db_digest=original_index_db_digest, reason=f"error: {e}", ) # Reset Docker config for the next request. This is a fail safe. diff --git a/iib/workers/tasks/build_containerized_regenerate_bundle.py b/iib/workers/tasks/build_containerized_regenerate_bundle.py index 6db7d81cf..1f4b80dbd 100644 --- a/iib/workers/tasks/build_containerized_regenerate_bundle.py +++ b/iib/workers/tasks/build_containerized_regenerate_bundle.py @@ -274,7 +274,6 @@ def handle_containerized_regenerate_bundle_request( request_id=request_id, from_index='', # No from_index for bundle regeneration index_repo_map={}, - original_index_db_digest=None, # No index.db for bundle regeneration reason=f"error: {e}", ) raise IIBError(f"Failed to regenerate bundle: {e}") diff --git a/iib/workers/tasks/build_containerized_rm.py b/iib/workers/tasks/build_containerized_rm.py index b74da2f86..029dcec6d 100644 --- a/iib/workers/tasks/build_containerized_rm.py +++ b/iib/workers/tasks/build_containerized_rm.py @@ -131,7 +131,6 @@ def handle_containerized_rm_request( operators_in_db: Set[str] = set() last_commit_sha: Optional[str] = None output_pull_spec: Optional[str] = None - original_index_db_digest: Optional[str] = None with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir: sources = prepare_build_sources( @@ -290,12 +289,13 @@ def handle_containerized_rm_request( # Push updated index.db before merging the MR so that on failure both # git and the index.db artifact remain consistent (MR stays open, - # cleanup_on_failure closes it and rolls back the artifact). - original_index_db_digest = push_index_db_artifact( + # cleanup_on_failure closes it; the content-addressed artifact needs no rollback). + push_index_db_artifact( request_id=request_id, from_index=from_index, index_db_path=index_db_path, operators=operators, + output_image=image_url, overwrite_from_index=overwrite_from_index, request_type='rm', ) @@ -326,7 +326,6 @@ def handle_containerized_rm_request( request_id=request_id, from_index=from_index, index_repo_map=index_to_gitlab_push_map or {}, - original_index_db_digest=original_index_db_digest, reason=f"error: {e}", ) raise IIBError(f"Failed to remove operators: {e}") diff --git a/tests/test_workers/test_tasks/test_build_containerized_add.py b/tests/test_workers/test_tasks/test_build_containerized_add.py index f66285e1b..585441ada 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_add.py +++ b/tests/test_workers/test_tasks/test_build_containerized_add.py @@ -171,7 +171,7 @@ def test_handle_containerized_add_request( mock_replicate.return_value = output_pull_specs # Mock final artifact push - mock_push_index_db.return_value = 'sha256:index_db_digest' + mock_push_index_db.return_value = None # Call the function if with_deprecations: @@ -284,6 +284,7 @@ def test_handle_containerized_add_request( ) mock_push_index_db.assert_called_once() + assert mock_push_index_db.call_args.kwargs['output_image'] == image_url mock_cleanup_mr.assert_called_once() mock_cleanup_failure.assert_not_called() @@ -542,7 +543,7 @@ def test_handle_containerized_add_request_overwrite( output_pull_specs = ['registry.example.com/final-image:456'] mock_replicate.return_value = output_pull_specs - mock_push_index_db.return_value = 'sha256:index_db_digest' + mock_push_index_db.return_value = None build_containerized_add.handle_containerized_add_request( bundles=bundles, @@ -561,6 +562,7 @@ def test_handle_containerized_add_request_overwrite( # Verify the handler completed successfully mock_push_index_db.assert_called_once() + assert mock_push_index_db.call_args.kwargs['output_image'] == image_url mock_cleanup_failure.assert_not_called() @@ -688,7 +690,7 @@ def test_add_divergent_never_merges( output_pull_specs = ['registry.example.com/final-image:789'] mock_replicate.return_value = output_pull_specs - mock_push_index_db.return_value = 'sha256:index_db_digest' + mock_push_index_db.return_value = None build_containerized_add.handle_containerized_add_request( bundles=bundles, @@ -701,6 +703,7 @@ def test_add_divergent_never_merges( # Divergent path uses the extracted index.db, never ORAS. mock_fetch_index_db.assert_not_called() + assert mock_push_index_db.call_args.kwargs['output_image'] == image_url # overwrite_from_index=True here: the divergent BuildSources bypasses Task 4's # entry-point overwrite rejection (mocked directly), so the ONLY thing that can diff --git a/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py b/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py index 9394dd3bc..860282e25 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py +++ b/tests/test_workers/test_tasks/test_build_containerized_create_empty_index.py @@ -18,9 +18,8 @@ @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -68,7 +67,6 @@ def test_handle_containerized_create_empty_index_primary_path( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -134,9 +132,8 @@ def test_handle_containerized_create_empty_index_primary_path( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' + mock_gid.return_value = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc0' # Mock worker config for utils mock_gwc_utils.return_value = { @@ -233,9 +230,8 @@ def test_handle_containerized_create_empty_index_primary_path( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -291,7 +287,6 @@ def test_handle_containerized_create_empty_index_fallback( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -377,9 +372,8 @@ def test_handle_containerized_create_empty_index_fallback( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:fallback' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' + mock_gid.return_value = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc0' # Mock worker config for utils mock_gwc_utils.return_value = { @@ -593,9 +587,8 @@ def test_handle_containerized_create_empty_index_missing_git_mapping( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -651,7 +644,6 @@ def test_handle_containerized_create_empty_index_unexpected_opm_error( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -712,9 +704,8 @@ def test_handle_containerized_create_empty_index_unexpected_opm_error( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:abc' + mock_gid.return_value = 'abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234567' mock_ritd.return_value = ['registry.io/iib-build:5'] mock_gwc_utils.return_value = { diff --git a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py index 33ccd0c79..8e55a50f6 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py +++ b/tests/test_workers/test_tasks/test_build_containerized_fbc_operations.py @@ -405,6 +405,7 @@ def test_handle_containerized_fbc_operation_request_with_overwrite( from_index='quay.io/iib/from-index:latest', index_db_path='/tmp/d', operators=['op1'], + output_image='reg/img', overwrite_from_index=True, request_type='fbc_operations', ) @@ -609,7 +610,7 @@ def test_fbc_operations_divergent_never_merges( mock_git_commit.return_value = (mr_details, 'commit_sha_999') mock_monitor.return_value = 'registry/output-image:sha256-12345' mock_replicate.return_value = ['registry.example.com/final-image:999'] - mock_push_index_db.return_value = 'sha256:index_db_digest' + mock_push_index_db.return_value = None overwrite_token = 'user:token' build_containerized_fbc_operations.handle_containerized_fbc_operation_request( diff --git a/tests/test_workers/test_tasks/test_build_containerized_rm.py b/tests/test_workers/test_tasks/test_build_containerized_rm.py index 142164072..ea405686c 100644 --- a/tests/test_workers/test_tasks/test_build_containerized_rm.py +++ b/tests/test_workers/test_tasks/test_build_containerized_rm.py @@ -16,9 +16,8 @@ @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -72,7 +71,6 @@ def test_handle_containerized_rm_request_success_with_overwrite( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -140,9 +138,8 @@ def test_handle_containerized_rm_request_success_with_overwrite( mock_gpiu.return_value = 'quay.io/konflux/built-image@sha256:xyz789' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' + mock_gid.return_value = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc0' # Mock worker config mock_gwc.return_value = { @@ -206,9 +203,12 @@ def test_handle_containerized_rm_request_success_with_overwrite( # Verify image was copied assert mock_sc.call_count >= 1 - # Verify index.db was pushed (2 times: request_id tag + v4.x tag) + # Verify index.db was pushed (2 times: request_id tag + current-artifact tag) assert mock_poa.call_count == 2 + # Verify the content key was resolved from the built output image + mock_gid.assert_called_once_with('quay.io/konflux/built-image@sha256:xyz789') + # Verify final state final_call = mock_srs.call_args_list[-1] assert final_call[0][0] == request_id @@ -227,8 +227,8 @@ def test_handle_containerized_rm_request_success_with_overwrite( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -280,7 +280,7 @@ def test_handle_containerized_rm_request_with_mr( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, + mock_gid, mock_giap, mock_poa, mock_gwc, @@ -340,8 +340,8 @@ def test_handle_containerized_rm_request_with_mr( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions (only request_id tag, no overwrite) - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' + mock_gid.return_value = 'abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234567' # Mock config mock_gwc.return_value = { @@ -389,9 +389,8 @@ def test_handle_containerized_rm_request_with_mr( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -443,7 +442,6 @@ def test_handle_containerized_rm_conditional_opm_rm( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -502,9 +500,8 @@ def test_handle_containerized_rm_conditional_opm_rm( mock_gpiu.return_value = 'image@sha' # Mock ORAS push related functions (conditionally used based on operators_in_db) - mock_gact.return_value = 'index-v4.14' mock_giap.return_value = 'reg/index-db:v4.14' - mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc' + mock_gid.return_value = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abc0' mock_gwc.return_value = { 'iib_registry': 'reg', @@ -839,9 +836,8 @@ def test_handle_containerized_rm_pipeline_failure( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -893,7 +889,6 @@ def test_handle_containerized_rm_with_index_db_push( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -947,9 +942,8 @@ def test_handle_containerized_rm_with_index_db_push( mock_gpiu.return_value = 'image@sha' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789ab' + mock_gid.return_value = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789ab00' mock_gwc.return_value = { 'iib_registry': 'registry.io', @@ -971,10 +965,10 @@ def test_handle_containerized_rm_with_index_db_push( # Verify MR was merged after build (overwrite flow) mock_merge_mr.assert_called_once() - # Verify index.db was pushed (2 times: request_id tag + v4.x tag) + # Verify index.db was pushed (2 times: request_id tag + current-artifact tag) assert mock_poa.call_count == 2 - # Verify original digest was captured + # Verify the content key was resolved from the built output image mock_gid.assert_called_once() # Verify annotations were added @@ -999,9 +993,8 @@ def test_handle_containerized_rm_with_index_db_push( @mock.patch('iib.workers.tasks.containerized_utils._skopeo_copy') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_image_digest') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -1053,7 +1046,6 @@ def test_handle_containerized_rm_with_build_tags( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, mock_giap, mock_gid, mock_poa, @@ -1102,9 +1094,8 @@ def test_handle_containerized_rm_with_build_tags( # Mock ORAS-related functions # (needed because push_index_db_artifact now called even with empty operators) - mock_gact.return_value = 'index-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' - mock_gid.return_value = 'sha256:abcdef' + mock_gid.return_value = 'abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234567' mock_gwc.return_value = { 'iib_registry': 'registry.io', @@ -1140,8 +1131,8 @@ def test_handle_containerized_rm_with_build_tags( @mock.patch('iib.workers.tasks.containerized_utils.set_request_state') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils._get_artifact_combined_tag') @mock.patch('iib.workers.tasks.containerized_utils.get_pipelinerun_image_url') @mock.patch('iib.workers.tasks.containerized_utils.wait_for_pipeline_completion') @mock.patch('iib.workers.tasks.containerized_utils.find_pipelinerun') @@ -1193,7 +1184,7 @@ def test_handle_containerized_rm_close_mr_failure_logged( mock_fpr, mock_wfpc, mock_gpiu, - mock_gact, + mock_gid, mock_giap, mock_poa, mock_gwc, @@ -1239,8 +1230,8 @@ def test_handle_containerized_rm_close_mr_failure_logged( mock_gpiu.return_value = 'quay.io/konflux/image@sha256:built' # Mock ORAS push related functions - mock_gact.return_value = 'index-image-v4.14' mock_giap.return_value = 'registry.io/index-db:v4.14' + mock_gid.return_value = 'abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234568' mock_gwc.return_value = { 'iib_registry': 'registry.io', @@ -1595,7 +1586,7 @@ def test_rm_divergent_never_merges( mock_git_commit.return_value = (mr_details, 'commit_sha_789') mock_monitor.return_value = 'quay.io/konflux/built-image@sha256:xyz789' mock_replicate.return_value = ['registry.example.com/final-image:789'] - mock_push_index_db.return_value = 'sha256:index_db_digest' + mock_push_index_db.return_value = None build_containerized_rm.handle_containerized_rm_request( operators=operators, @@ -1620,3 +1611,6 @@ def test_rm_divergent_never_merges( mock_orrf.assert_called_once() assert mock_orrf.call_args.kwargs['index_db_path'] == index_db_path mock_cof.assert_not_called() + + expected_output_image = 'quay.io/konflux/built-image@sha256:xyz789' + assert mock_push_index_db.call_args.kwargs['output_image'] == expected_output_image diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index dd1dbe19a..945de0b6c 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -317,8 +317,8 @@ def test_push_keys_current_artifact_on_output_digest( assert result is None m_digest.assert_called_with('quay.io/ns/foo@sha256:' + 'f' * 64) pushed_refs = {c.kwargs['artifact_ref'] for c in m_push.call_args_list} - assert 'quay.io/iib/index-db:idb-' + 'f' * 64 in pushed_refs # warm-push (overwrite) - assert 'quay.io/iib/index-db:idb-' + 'f' * 64 + '-42' in pushed_refs # per-request tag + assert 'quay.io/iib/index-db:idb-' + 'f' * 64 in pushed_refs # warm-push (overwrite) + assert 'quay.io/iib/index-db:idb-' + 'f' * 64 + '-42' in pushed_refs # per-request tag @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') @@ -346,7 +346,7 @@ def test_push_throwaway_skips_current_artifact( request_type='add', ) pushed_refs = {c.kwargs['artifact_ref'] for c in m_push.call_args_list} - assert pushed_refs == {'quay.io/iib/index-db:idb-' + 'a' * 64 + '-7'} # only per-request tag + assert pushed_refs == {'quay.io/iib/index-db:idb-' + 'a' * 64 + '-7'} # only per-request tag @patch('iib.workers.tasks.containerized_utils.log') From ee6aa5978fbdc97e177e5c0513b5956a73d61bac Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 01:25:54 -0700 Subject: [PATCH 14/18] feat: bootstrap index.db from image on digest cache miss (read-through) Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 53 ++++++++++++++++--- .../test_tasks/test_containerized_utils.py | 50 +++++++++++++++++ 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index cf60483fe..dd84ea8ab 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -2,6 +2,7 @@ """This file contains utility functions for containerized IIB operations.""" import json import logging +import os import queue import shutil import tarfile @@ -42,7 +43,7 @@ refresh_indexdb_cache_for_image, verify_indexdb_cache_for_image, ) -from iib.workers.tasks.utils import get_image_label, skopeo_inspect +from iib.workers.tasks.utils import get_image_label, get_resolved_image, skopeo_inspect log = logging.getLogger(__name__) @@ -300,6 +301,42 @@ def validate_bundles_in_parallel( return None +def bootstrap_index_db_from_image(from_index: str, temp_dir: str) -> str: + """ + Extract index.db from the index image and populate the digest-keyed cache. + + Used on a read miss (a from_index digest never seen before — pre-cutover or + externally-built index). Safe to write because the artifact key is + content-addressed and uniquely identifies "the index.db for this image". + + Reuses Part B's ``extract_catalog_and_db_from_image``, which already + implements the hidden -> labeled -> empty index.db precedence, so bootstrap + works for pure-FBC and labeled-db images too, not just hidden-db ones. + + :param str from_index: The from_index pullspec. + :param str temp_dir: Temporary directory to extract into. + :return: Directory containing the extracted ``index.db``. + :rtype: str + :raises IIBError: If the image has no FBC configs label, or if extraction + fails for a reason other than the hidden-db path being absent. + """ + from_index_resolved = get_resolved_image(from_index) + _, extracted_db = extract_catalog_and_db_from_image(from_index_resolved, temp_dir) + + dest = os.path.join(temp_dir, 'index_db_bootstrap') + os.makedirs(dest, exist_ok=True) + shutil.copyfile(extracted_db, os.path.join(dest, 'index.db')) + + artifact_ref = get_indexdb_artifact_pullspec(from_index) + push_oras_artifact( + artifact_ref=artifact_ref, + local_path='index.db', + cwd=dest, + annotations={'from_index': from_index, 'bootstrap': 'true'}, + ) + return dest + + def pull_index_db_artifact(from_index: str, temp_dir: str) -> str: """ Pull index.db artifact from registry, using ImageStream cache if available. @@ -340,12 +377,14 @@ def pull_index_db_artifact(from_index: str, temp_dir: str) -> str: # Pull directly from Quay — either cache is disabled, stale, or unavailable artifact_ref = get_indexdb_artifact_pullspec(from_index) - artifact_dir = get_oras_artifact( - artifact_ref, - temp_dir, - ) - - return artifact_dir + try: + return get_oras_artifact( + artifact_ref, + temp_dir, + ) + except IIBError: + log.info('index.db artifact %s not found; bootstrapping from image', artifact_ref) + return bootstrap_index_db_from_image(from_index, temp_dir) def write_build_metadata( diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index 945de0b6c..bd8d9965b 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -289,6 +289,56 @@ def test_pull_index_db_artifact_refresh_cache_fails_falls_back_to_quay( ) +@mock.patch('iib.workers.tasks.containerized_utils.bootstrap_index_db_from_image') +@mock.patch('iib.workers.tasks.containerized_utils.get_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') +@mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') +def test_pull_falls_back_to_bootstrap_on_miss(m_gwc, m_ref, m_pull, m_boot): + """When the digest-keyed artifact is missing in Quay, bootstrap from the image.""" + m_gwc.return_value = {'iib_use_imagestream_cache': False} + m_ref.return_value = 'quay.io/iib/index-db:idb-x' + m_pull.side_effect = IIBError('not found') # Quay miss + m_boot.return_value = '/tmp/boot' + out = pull_index_db_artifact('quay.io/ns/foo:v4.17', '/tmp/req') + assert out == '/tmp/boot' + m_boot.assert_called_once_with('quay.io/ns/foo:v4.17', '/tmp/req') + + +@mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') +@mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') +@mock.patch('iib.workers.tasks.containerized_utils.get_resolved_image') +@mock.patch('iib.workers.tasks.containerized_utils.extract_catalog_and_db_from_image') +def test_bootstrap_index_db_from_image(m_extract, m_resolved, m_ref, m_push, tmp_path): + """Bootstrap extracts index.db via Part B's extractor and pushes it to the digest key.""" + from_index = 'quay.io/ns/foo:v4.17' + from_index_resolved = 'quay.io/ns/foo@sha256:deadbeef' + artifact_ref = 'quay.io/iib/index-db:idb-deadbeef' + + # Simulate the extractor writing its output file into temp_dir. + def extract_side_effect(resolved, temp_dir): + extracted_db = os.path.join(temp_dir, 'extracted_index.db') + with open(extracted_db, 'w') as f: + f.write('sqlite-bytes') + return os.path.join(temp_dir, 'extracted_configs'), extracted_db + + m_extract.side_effect = extract_side_effect + m_resolved.return_value = from_index_resolved + m_ref.return_value = artifact_ref + + result = cu.bootstrap_index_db_from_image(from_index, str(tmp_path)) + + assert os.path.isfile(os.path.join(result, 'index.db')) + m_resolved.assert_called_once_with(from_index) + m_extract.assert_called_once_with(from_index_resolved, str(tmp_path)) + m_ref.assert_called_once_with(from_index) + m_push.assert_called_once_with( + artifact_ref=artifact_ref, + local_path='index.db', + cwd=result, + annotations={'from_index': from_index, 'bootstrap': 'true'}, + ) + + @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') @mock.patch('iib.workers.tasks.containerized_utils._get_index_digest') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') From b73a69f054b27916da1f44afa2fde8a3fa48d835 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Wed, 2 Sep 2026 01:30:51 -0700 Subject: [PATCH 15/18] docs: describe content-digest index.db cache naming Update the containerized README's cache-naming section and an inline comment to reflect the content-digest identity scheme (replacing the earlier pullspec-hash description). Co-Authored-By: Claude Opus 4.8 --- docker/containerized/README.md | 7 +++++-- .../tasks/build_containerized_create_empty_index.py | 10 +++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docker/containerized/README.md b/docker/containerized/README.md index b2284f6cf..de6bc11fc 100644 --- a/docker/containerized/README.md +++ b/docker/containerized/README.md @@ -279,9 +279,12 @@ This lets IIB build and validate a one-off tag without requiring per-tag branch/ ## Index DB Artifact and ImageStream Tag Naming -Cached `index.db` artifact tags (ORAS) and ImageStream tags now include a short hash of the index image pullspec, making them namespace-safe: two images with the same repository name in different registry namespaces (e.g. `quay.io/redhat/my-index:v4.17` vs `quay.io/redhat-pending/my-index:v4.17`) no longer collide on the same cache tag. +Cached `index.db` artifact tags (ORAS) and ImageStream tags are keyed on the index image's content (manifest) digest — `idb-` — resolved via `skopeo inspect`, not on its pullspec. Because the key is the content itself, it is both namespace-safe and promotion-safe: -Cache entries written under the old (pre-hash) naming scheme are orphaned by this change — they are not migrated in place. They are cleaned up by the existing cache-pruning process rather than any code path in this workflow. +- **Namespace-safe:** two images that share a repository name in different registry namespaces (e.g. `quay.io/redhat/my-index:v4.17` vs `quay.io/redhat-pending/my-index:v4.17`) have different content and therefore different digests, so they never collide on the same cache tag. +- **Promotion-safe:** the same image content addressed by different pullspecs after a release or mirror (e.g. `quay.io/my-namespace/iib-pub:v4.17` → `registry.access.redhat.com/some-namespace/operator-index:v4.17`) preserves its manifest digest, so both pullspecs resolve to the *same* cache entry and share one `index.db`. + +Cache entries written under the previous pullspec-derived naming scheme are orphaned by this change — they are not migrated in place. The digest-keyed name simply misses on the first request after cutover and is repopulated via read-through bootstrap (the `index.db` is extracted from the image and pushed under the digest key). Orphaned entries are cleaned up by the existing cache-pruning process rather than any code path in this workflow. ## Differences from Traditional Workflow diff --git a/iib/workers/tasks/build_containerized_create_empty_index.py b/iib/workers/tasks/build_containerized_create_empty_index.py index 603c44587..203db2aa8 100644 --- a/iib/workers/tasks/build_containerized_create_empty_index.py +++ b/iib/workers/tasks/build_containerized_create_empty_index.py @@ -198,11 +198,11 @@ def handle_containerized_create_empty_index_request( empty_tag = conf.get('iib_empty_index_db_tag', 'empty') # Construct the pullspec for the empty index.db artifact. - # This tag intentionally omits the pullspec hash used by - # _get_artifact_combined_tag: the empty index.db is a shared, content-free - # seed artifact keyed only by image name + "empty", so it is deliberately - # namespace-agnostic and does not risk the cross-namespace collisions that - # the hash guards against for real per-index artifacts. + # This tag intentionally does NOT use the content-digest key that + # _get_artifact_combined_tag derives for real indexes: the empty index.db + # is a shared, content-free seed artifact keyed only by image name + + # "empty", so it is deliberately reusable across indexes rather than tied + # to any single image's manifest digest. image_name, _ = _get_name_and_tag_from_pullspec(from_index) empty_artifact_ref = conf['iib_index_db_artifact_template'].format( registry=conf['iib_index_db_artifact_registry'], From b3210b78acb71cbb7e3197b5969ece9db61b2eae Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Thu, 3 Sep 2026 17:05:49 -0700 Subject: [PATCH 16/18] fix: require hidden index.db, fail instead of falling back extract_catalog_and_db_from_image now extracts only the FBC configs and the hidden index.db. Drop the labeled-db and synthesised empty-db fallbacks: an image with no hidden index.db has not been onboarded to the containerized flow, so the request fails ("no index.db found, onboard the image to build"). pull_index_db_artifact no longer bootstraps index.db from the image on an ORAS miss. On the normal path a missing digest-keyed artifact fails the request ("no index.db found for the image, onboard the image to build") rather than silently sourcing content from the image. Remove the now-dead bootstrap_index_db_from_image helper and unused imports, add source/destination path logging to aid debugging, and update tests and the containerized README to match. Co-Authored-By: Claude Opus 4.8 --- docker/containerized/README.md | 4 +- iib/workers/tasks/containerized_utils.py | 103 ++++++------------ .../test_tasks/test_containerized_utils.py | 92 ++++------------ 3 files changed, 55 insertions(+), 144 deletions(-) diff --git a/docker/containerized/README.md b/docker/containerized/README.md index de6bc11fc..26b564960 100644 --- a/docker/containerized/README.md +++ b/docker/containerized/README.md @@ -270,7 +270,7 @@ Once both exist, requests against that tag build and push normally, and `overwri If a request targets a tag with no corresponding Git branch — for example a timestamped, point-in-time tag cut from an existing index — IIB treats it as a **divergent tag**: -- Configs and `index.db` are extracted directly from the index image (unprivileged), not sourced from ORAS. +- Configs and `index.db` are extracted directly from the index image (unprivileged), not sourced from ORAS. Only the FBC configs and the hidden `index.db` are extracted; if the image carries no hidden `index.db`, the request fails ("no index.db found, onboard the image to build") — there is no labeled-db or empty-db fallback. - The build reuses the base OCP branch's existing Konflux Component via a throw-away merge request. - That MR is **never merged** — it is always closed after the pipeline completes (or on failure), regardless of outcome. - `overwrite_from_index` is **rejected** for divergent-tag requests; there is no direct-push path. @@ -284,7 +284,7 @@ Cached `index.db` artifact tags (ORAS) and ImageStream tags are keyed on the ind - **Namespace-safe:** two images that share a repository name in different registry namespaces (e.g. `quay.io/redhat/my-index:v4.17` vs `quay.io/redhat-pending/my-index:v4.17`) have different content and therefore different digests, so they never collide on the same cache tag. - **Promotion-safe:** the same image content addressed by different pullspecs after a release or mirror (e.g. `quay.io/my-namespace/iib-pub:v4.17` → `registry.access.redhat.com/some-namespace/operator-index:v4.17`) preserves its manifest digest, so both pullspecs resolve to the *same* cache entry and share one `index.db`. -Cache entries written under the previous pullspec-derived naming scheme are orphaned by this change — they are not migrated in place. The digest-keyed name simply misses on the first request after cutover and is repopulated via read-through bootstrap (the `index.db` is extracted from the image and pushed under the digest key). Orphaned entries are cleaned up by the existing cache-pruning process rather than any code path in this workflow. +Cache entries written under the previous pullspec-derived naming scheme are orphaned by this change — they are not migrated in place. On the normal path IIB never falls back to extracting `index.db` from the image: if the digest-keyed artifact is missing, the request fails with a "no index.db found for the image, onboard the image to build" error, and the image must be onboarded (which populates the artifact) before it can be built. Orphaned entries are cleaned up by the existing cache-pruning process rather than any code path in this workflow. ## Differences from Traditional Workflow diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index dd84ea8ab..6ae1f9a27 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -2,7 +2,6 @@ """This file contains utility functions for containerized IIB operations.""" import json import logging -import os import queue import shutil import tarfile @@ -43,7 +42,7 @@ refresh_indexdb_cache_for_image, verify_indexdb_cache_for_image, ) -from iib.workers.tasks.utils import get_image_label, get_resolved_image, skopeo_inspect +from iib.workers.tasks.utils import get_image_label, skopeo_inspect log = logging.getLogger(__name__) @@ -162,18 +161,19 @@ def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) - already inspected during prebuild (OPM version, build metadata) and so the repeated image reads here cannot disagree with each other. - index.db precedence: hidden db path -> configs database label -> empty db - (pure-FBC images may carry no db at all; there is no primitive to build a - SQLite index.db back from FBC configs, so an empty db is created and the - FBC configs remain authoritative). + Only the FBC configs and the hidden index.db are extracted. The hidden db is + the sole source of truth for the SQLite index; there is no fallback to the + labeled database path and no synthesised empty db. An image that carries no + hidden index.db has not been onboarded to the containerized build flow and + the request is failed so the image can be onboarded first. :param str from_index_resolved: The digest-resolved from_index image pullspec. :param str temp_dir: Base temp directory for extraction. :return: Tuple of (configs_dir_path, index_db_path). :rtype: Tuple[str, str] - :raises IIBError: If the image has no FBC configs label, or if a hidden-db - extraction attempt fails for a reason other than the path being absent - (e.g. a registry, OCI parsing, layer, or tar error). + :raises IIBError: If the image has no FBC configs label, if it carries no + hidden index.db, or if the hidden-db extraction fails for any other + reason (e.g. a registry, OCI parsing, layer, or tar error). """ configs_label = get_image_label( from_index_resolved, 'operators.operatorframework.io.index.configs.v1' @@ -182,6 +182,12 @@ def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) - raise IIBError(f"Index image {from_index_resolved} does not contain a file-based catalog.") configs_dir = str(Path(temp_dir) / 'extracted_configs') + log.info( + 'Extracting FBC configs from %s (label path %s) to %s', + from_index_resolved, + configs_label, + configs_dir, + ) extract_files_from_image_non_privileged(from_index_resolved, configs_label, configs_dir) index_db_path = str(Path(temp_dir) / 'extracted_index.db') @@ -189,32 +195,21 @@ def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) - hidden_db_path = conf['hidden_index_db_path'] try: - # Prefer the hidden db (carries deprecated/hidden bundle state). + # The hidden db is the only source of truth for the SQLite index. + log.info( + 'Extracting hidden index.db from %s (image path %s) to %s', + from_index_resolved, + hidden_db_path, + index_db_path, + ) extract_files_from_image_non_privileged(from_index_resolved, hidden_db_path, index_db_path) - return configs_dir, index_db_path except FileNotFoundInImageError: - # Only a genuinely absent hidden-db path falls through to the next source; - # real extraction failures (registry/OCI/layer/tar) raise plain IIBError - # and propagate, so we never silently degrade to a wrong index.db. - log.info("No hidden index.db in %s; trying labeled db.", from_index_resolved) - - db_label = get_image_label( - from_index_resolved, 'operators.operatorframework.io.index.database.v1' - ) - if db_label: - extract_files_from_image_non_privileged(from_index_resolved, db_label, index_db_path) - return configs_dir, index_db_path - - # Pure FBC image: no embedded index.db (hidden or labeled). There is no opm - # primitive that builds a SQLite index.db from FBC configs (opm migrate only - # goes db -> configs), so create an empty index.db. The FBC configs are - # authoritative for this image; the empty db is populated by the handler's - # subsequent add/rm operations. - log.info("No embedded index.db found in %s; creating empty index.db.", from_index_resolved) - Path(index_db_path).parent.mkdir(parents=True, exist_ok=True) - with open(index_db_path, 'w'): - pass + raise IIBError( + f"No index.db found in image {from_index_resolved} at hidden path " + f"{hidden_db_path}. Onboard the image to build." + ) + log.info('Extracted FBC configs to %s and index.db to %s', configs_dir, index_db_path) return configs_dir, index_db_path @@ -301,42 +296,6 @@ def validate_bundles_in_parallel( return None -def bootstrap_index_db_from_image(from_index: str, temp_dir: str) -> str: - """ - Extract index.db from the index image and populate the digest-keyed cache. - - Used on a read miss (a from_index digest never seen before — pre-cutover or - externally-built index). Safe to write because the artifact key is - content-addressed and uniquely identifies "the index.db for this image". - - Reuses Part B's ``extract_catalog_and_db_from_image``, which already - implements the hidden -> labeled -> empty index.db precedence, so bootstrap - works for pure-FBC and labeled-db images too, not just hidden-db ones. - - :param str from_index: The from_index pullspec. - :param str temp_dir: Temporary directory to extract into. - :return: Directory containing the extracted ``index.db``. - :rtype: str - :raises IIBError: If the image has no FBC configs label, or if extraction - fails for a reason other than the hidden-db path being absent. - """ - from_index_resolved = get_resolved_image(from_index) - _, extracted_db = extract_catalog_and_db_from_image(from_index_resolved, temp_dir) - - dest = os.path.join(temp_dir, 'index_db_bootstrap') - os.makedirs(dest, exist_ok=True) - shutil.copyfile(extracted_db, os.path.join(dest, 'index.db')) - - artifact_ref = get_indexdb_artifact_pullspec(from_index) - push_oras_artifact( - artifact_ref=artifact_ref, - local_path='index.db', - cwd=dest, - annotations={'from_index': from_index, 'bootstrap': 'true'}, - ) - return dest - - def pull_index_db_artifact(from_index: str, temp_dir: str) -> str: """ Pull index.db artifact from registry, using ImageStream cache if available. @@ -377,14 +336,18 @@ def pull_index_db_artifact(from_index: str, temp_dir: str) -> str: # Pull directly from Quay — either cache is disabled, stale, or unavailable artifact_ref = get_indexdb_artifact_pullspec(from_index) + log.info('Pulling index.db artifact %s into %s', artifact_ref, temp_dir) try: return get_oras_artifact( artifact_ref, temp_dir, ) except IIBError: - log.info('index.db artifact %s not found; bootstrapping from image', artifact_ref) - return bootstrap_index_db_from_image(from_index, temp_dir) + log.error('index.db artifact %s not found for image %s', artifact_ref, from_index) + raise IIBError( + f"No index.db found for the image {from_index} (artifact {artifact_ref}). " + "Onboard the image to build." + ) def write_build_metadata( diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index bd8d9965b..bff7b6f42 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -289,54 +289,21 @@ def test_pull_index_db_artifact_refresh_cache_fails_falls_back_to_quay( ) -@mock.patch('iib.workers.tasks.containerized_utils.bootstrap_index_db_from_image') @mock.patch('iib.workers.tasks.containerized_utils.get_oras_artifact') @mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') @mock.patch('iib.workers.tasks.containerized_utils.get_worker_config') -def test_pull_falls_back_to_bootstrap_on_miss(m_gwc, m_ref, m_pull, m_boot): - """When the digest-keyed artifact is missing in Quay, bootstrap from the image.""" +def test_pull_raises_when_artifact_missing(m_gwc, m_ref, m_pull): + """When the digest-keyed artifact is missing in Quay, the request must fail. + + The normal path never falls back to extracting index.db from the image; an + un-onboarded image has no artifact and the request is failed so it can be + onboarded first. + """ m_gwc.return_value = {'iib_use_imagestream_cache': False} m_ref.return_value = 'quay.io/iib/index-db:idb-x' m_pull.side_effect = IIBError('not found') # Quay miss - m_boot.return_value = '/tmp/boot' - out = pull_index_db_artifact('quay.io/ns/foo:v4.17', '/tmp/req') - assert out == '/tmp/boot' - m_boot.assert_called_once_with('quay.io/ns/foo:v4.17', '/tmp/req') - - -@mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') -@mock.patch('iib.workers.tasks.containerized_utils.get_indexdb_artifact_pullspec') -@mock.patch('iib.workers.tasks.containerized_utils.get_resolved_image') -@mock.patch('iib.workers.tasks.containerized_utils.extract_catalog_and_db_from_image') -def test_bootstrap_index_db_from_image(m_extract, m_resolved, m_ref, m_push, tmp_path): - """Bootstrap extracts index.db via Part B's extractor and pushes it to the digest key.""" - from_index = 'quay.io/ns/foo:v4.17' - from_index_resolved = 'quay.io/ns/foo@sha256:deadbeef' - artifact_ref = 'quay.io/iib/index-db:idb-deadbeef' - - # Simulate the extractor writing its output file into temp_dir. - def extract_side_effect(resolved, temp_dir): - extracted_db = os.path.join(temp_dir, 'extracted_index.db') - with open(extracted_db, 'w') as f: - f.write('sqlite-bytes') - return os.path.join(temp_dir, 'extracted_configs'), extracted_db - - m_extract.side_effect = extract_side_effect - m_resolved.return_value = from_index_resolved - m_ref.return_value = artifact_ref - - result = cu.bootstrap_index_db_from_image(from_index, str(tmp_path)) - - assert os.path.isfile(os.path.join(result, 'index.db')) - m_resolved.assert_called_once_with(from_index) - m_extract.assert_called_once_with(from_index_resolved, str(tmp_path)) - m_ref.assert_called_once_with(from_index) - m_push.assert_called_once_with( - artifact_ref=artifact_ref, - local_path='index.db', - cwd=result, - annotations={'from_index': from_index, 'bootstrap': 'true'}, - ) + with pytest.raises(IIBError, match='No index.db found for the image'): + pull_index_db_artifact('quay.io/ns/foo:v4.17', '/tmp/req') @mock.patch('iib.workers.tasks.containerized_utils.push_oras_artifact') @@ -1465,44 +1432,25 @@ def label_side_effect(image, label): @patch('iib.workers.tasks.containerized_utils.get_image_label') @patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') -def test_extract_catalog_and_db_falls_back_to_labeled_db(mock_extract, mock_label, tmp_path): - """When the hidden db is missing, fall back to the labeled database.v1 path.""" - mock_label.side_effect = lambda image, label: { - 'operators.operatorframework.io.index.configs.v1': '/configs', - 'operators.operatorframework.io.index.database.v1': '/database/index.db', - }[label] - # First call (configs) ok; second call (hidden db) raises FileNotFoundInImageError - # (path genuinely absent) -> fall back to labeled db - mock_extract.side_effect = [None, FileNotFoundInImageError('no hidden db'), None] - - _, index_db = extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', str(tmp_path)) +def test_extract_catalog_and_db_raises_when_no_hidden_db(mock_extract, mock_label, tmp_path): + """When the hidden db is absent, the request fails; there is no labeled-db or empty-db fallback. - assert index_db.endswith('index.db') - assert mock_extract.call_count == 3 - - -@patch('iib.workers.tasks.containerized_utils.get_image_label') -@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') -def test_extract_catalog_and_db_pure_fbc_creates_empty_db(mock_extract, mock_label, tmp_path): - """Pure-FBC image (no hidden or labeled db) results in an empty index.db.""" + An image with no hidden index.db has not been onboarded to the containerized + build flow, so the request must fail rather than degrade to a labeled db or a + synthesised empty db. + """ mock_label.side_effect = lambda image, label: { 'operators.operatorframework.io.index.configs.v1': '/configs', - 'operators.operatorframework.io.index.database.v1': '', + 'operators.operatorframework.io.index.database.v1': '/database/index.db', }[label] - # First call (configs) ok; second call (hidden db) raises FileNotFoundInImageError - # (path genuinely absent) -> no labeled db either + # First call (configs) ok; second call (hidden db) raises FileNotFoundInImageError. mock_extract.side_effect = [None, FileNotFoundInImageError('no hidden db')] - configs_dir, index_db = extract_catalog_and_db_from_image( - 'quay.io/redhat/my-index:test', str(tmp_path) - ) + with pytest.raises(IIBError, match='No index.db found in image'): + extract_catalog_and_db_from_image('quay.io/redhat/my-index:test', str(tmp_path)) - assert configs_dir.endswith('configs') - assert index_db.endswith('index.db') - assert os.path.exists(index_db) - assert os.path.getsize(index_db) == 0 # Only two extraction attempts: configs dir and the failed hidden db lookup. - # No opm_migrate / privileged call is ever invoked for the pure-FBC fallback. + # The labeled database.v1 path is never read. assert mock_extract.call_count == 2 From 512411288ed95ef0540906a13c0e6c3286c371b9 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Fri, 4 Sep 2026 02:19:08 -0700 Subject: [PATCH 17/18] fix: extract image files via 'oc image extract', not tarfile extract_files_from_image_non_privileged reconstructed the entire root filesystem with tarfile.extractall to copy out one subpath. That fails on real UBI/RHEL images: the 'data' filter rejects absolute symlinks (/etc/alternatives, AbsoluteLinkError), 'tar' rejects escaping symlink targets (/etc/crypto-policies, OutsideDestinationError), and 'fully_trusted' trips on read-only files overwritten across layers (/etc/machine-id) -- plus tarfile ignores OCI whiteouts and hardlinks. Delegate to 'oc image extract' instead, which applies OCI layer/whiteout semantics correctly, runs unprivileged, and is already installed in the worker base image and used elsewhere in the repo. The function signature is unchanged, so callers are unaffected. 'oc' distinguishes files from directories by argument shape and exits 0 extracting nothing when a path is absent, so probe the directory form ('/*:') first, then the file form (':'), and treat "nothing extracted" as FileNotFoundInImageError. Tests rewritten to mock 'oc'/run_cmd instead of tarfile/skopeo internals. Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 176 +++++----- .../test_tasks/test_containerized_utils.py | 309 +++++------------- 2 files changed, 164 insertions(+), 321 deletions(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index 6ae1f9a27..c8f6bc99e 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -2,9 +2,9 @@ """This file contains utility functions for containerized IIB operations.""" import json import logging +import posixpath import queue import shutil -import tarfile import tempfile import threading from dataclasses import dataclass @@ -49,104 +49,100 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path: str) -> None: """ - Extract files from container image without podman/docker runtime. + Extract a file or directory from a container image, unprivileged. - This function uses skopeo to download the image as OCI layout, then extracts - the requested path from the image layers. This approach works in non-privileged - environments without container runtime access. + Delegates to ``oc image extract``, which correctly applies OCI layer and + whiteout semantics and handles the absolute symlinks (e.g. /etc/alternatives, + /etc/crypto-policies) and read-only cross-layer overwrites (e.g. /etc/machine-id) + that a hand-rolled ``tarfile`` reconstruction of the whole root filesystem + chokes on. It works without a container runtime, so it is safe in the + unprivileged API/worker containers. ``oc`` is already installed in the worker + base image (see docker/Dockerfile-base-image). + + ``oc image extract`` distinguishes files from directories by argument shape and + exits 0 while extracting nothing when the path is absent, so this function + probes both shapes and treats "nothing extracted" as a missing path: + + * directory: ``--path=/*:`` unpacks the directory's contents into + ``dest_path`` (matching the previous copytree-of-contents behaviour); + * file: ``--path=:`` places the file at ``/``, which + is then copied to ``dest_path`` (which may have a different basename). + + A glob on a file path matches nothing, so trying the directory form first is + safe; the file form is only reached (a second ``oc`` invocation) when the + directory form yields nothing. An existing-but-empty directory therefore also + reads as "not found" — acceptable for the FBC configs / manifests / metadata / + hidden index.db paths IIB extracts, none of which are ever legitimately empty. :param str image: the pull specification of the container image - :param str src_path: the full path within the container image to copy from - :param str dest_path: the full path on the local host to copy into - :raises IIBError: if the extraction fails or src_path is not found + :param str src_path: the absolute path within the container image to copy from + :param str dest_path: the path on the local host to copy into + :raises FileNotFoundInImageError: if src_path is absent in the image + :raises IIBError: if src_path is not absolute or ``oc image extract`` fails """ - # Create temporary directory for OCI layout + from iib.workers.tasks.utils import run_cmd + + if not src_path.startswith('/'): + raise IIBError(f'src_path must be an absolute image path, got {src_path!r}') + + normalized = src_path.rstrip('/') + if not normalized: + raise IIBError(f'src_path must name a path under /, got {src_path!r}') + with tempfile.TemporaryDirectory(prefix='iib-extract-') as temp_dir: temp_path = Path(temp_dir) - oci_dir = temp_path / 'oci' - oci_dir.mkdir(parents=True, exist_ok=True) - # Download image as OCI layout using skopeo - log.info('Downloading image %s as OCI layout', image) - _skopeo_copy( - source=f'docker://{image}', - destination=f'oci:{oci_dir}', - copy_all=False, - exc_msg=f'Failed to download image {image} as OCI layout', + # 1) Directory form: unpack the directory's children into a staging dir. + dir_staging = temp_path / 'dir' + dir_staging.mkdir() + log.info('Extracting %s from image %s to %s', src_path, image, dest_path) + run_cmd( + [ + 'oc', + 'image', + 'extract', + '--confirm', + f'--path={normalized}/*:{dir_staging.as_posix()}', + image, + ], + exc_msg=f'Failed to extract {src_path} from image {image}', ) - - # Read OCI index to find the manifest - index_path = oci_dir / 'index.json' - if not index_path.exists(): - raise IIBError(f'OCI index.json not found at {index_path}') - - with open(index_path, 'r') as f: - index = json.load(f) - - # Get the manifest digest from the index - manifests = index.get('manifests', []) - if not manifests: - raise IIBError(f'No manifests found in OCI index for image {image}') - - manifest_digest = manifests[0]['digest'].replace('sha256:', '') - manifest_path = oci_dir / 'blobs' / 'sha256' / manifest_digest - - # Read manifest to get layer information - with open(manifest_path, 'r') as f: - manifest = json.load(f) - - layers = manifest.get('layers', []) - if not layers: - raise IIBError(f'No layers found in manifest for image {image}') - - # Create extraction directory to build the filesystem - extract_dir = temp_path / 'rootfs' - extract_dir.mkdir(parents=True, exist_ok=True) - - # Extract each layer in order to build the complete filesystem - log.info('Extracting %d layers from image %s', len(layers), image) - for layer in layers: - layer_digest = layer['digest'].replace('sha256:', '') - layer_path = oci_dir / 'blobs' / 'sha256' / layer_digest - - if not layer_path.exists(): - raise IIBError(f'Layer blob not found at {layer_path}') - - # Extract layer tar.gz to build filesystem - try: - with tarfile.open(layer_path, 'r:gz') as tar: - # Extract all members safely with path traversal protection - tar.extractall(path=extract_dir, filter='data') - except Exception as e: - raise IIBError(f'Failed to extract layer {layer_digest}: {e}') - - # Normalize src_path (remove leading slash for filesystem access) - normalized_src = src_path.lstrip('/') - source_full_path = extract_dir / normalized_src - - # Verify the requested path exists in the extracted filesystem. - # Raise the specific FileNotFoundInImageError (a subclass of IIBError) so - # callers can distinguish a genuinely absent path from a real extraction - # failure (registry, OCI parsing, layer, or tar error), which all raise - # plain IIBError above. - if not source_full_path.exists(): - raise FileNotFoundInImageError( - f'Path {src_path} not found in image {image}. ' - f'Looked for {source_full_path} in extracted filesystem.' - ) - - # Copy the requested path to destination - dest = Path(dest_path) - log.info('Copying %s from image to %s', src_path, dest_path) - if source_full_path.is_dir(): - # If source is a directory, copy its contents - shutil.copytree(source_full_path, dest, dirs_exist_ok=True) - else: - # If source is a file, copy the file + if any(dir_staging.iterdir()): + dest = Path(dest_path) + dest.mkdir(parents=True, exist_ok=True) + # Copy the directory's contents into dest_path, preserving any nested + # symlinks verbatim (symlinks=True) rather than resolving them against + # the host root during the copy. + shutil.copytree(dir_staging, dest, dirs_exist_ok=True, symlinks=True) + log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) + return + + # 2) File form: the path is a single file, placed at /. + file_staging = temp_path / 'file' + file_staging.mkdir() + run_cmd( + [ + 'oc', + 'image', + 'extract', + '--confirm', + f'--path={normalized}:{file_staging.as_posix()}', + image, + ], + exc_msg=f'Failed to extract {src_path} from image {image}', + ) + extracted_file = file_staging / posixpath.basename(normalized) + if extracted_file.is_file(): + dest = Path(dest_path) dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(source_full_path, dest) - - log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) + shutil.copy2(extracted_file, dest) + log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) + return + + # 3) Neither shape produced output: the path is absent in the image. Raise + # the specific FileNotFoundInImageError (a subclass of IIBError) so callers + # can distinguish a genuinely absent path from an 'oc' failure above. + raise FileNotFoundInImageError(f'Path {src_path} not found in image {image}.') def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) -> Tuple[str, str]: diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index bff7b6f42..c8aa9eed6 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -2,7 +2,6 @@ import inspect import json import os -import tarfile from unittest import mock from unittest.mock import patch @@ -1079,267 +1078,115 @@ def test_wait_for_bundle_validation_threads_failure_raises_error_string( mock_log.error.assert_called() -# Tests for extract_files_from_image_non_privileged -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_success_directory( - mock_skopeo_copy, mock_log, tmpdir -): - """Test successful extraction of a directory from container image.""" - import os - import tarfile - - # Setup destination directory - dest_dir = tmpdir.join('dest') - - # Mock skopeo_copy to create proper OCI layout - def mock_copy(source, destination, copy_all, exc_msg): - # Extract OCI directory path from destination (format: oci:/path/to/oci) - oci_path = destination.replace('oci:', '') - - # Create OCI layout structure - os.makedirs(oci_path, exist_ok=True) - blobs_dir = os.path.join(oci_path, 'blobs', 'sha256') - os.makedirs(blobs_dir, exist_ok=True) - - # Create index.json - index_json = { - 'manifests': [ - { - 'digest': 'sha256:abc123', - 'mediaType': 'application/vnd.oci.image.manifest.v1+json', - } - ] - } - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) - - # Create manifest - manifest_json = { - 'layers': [ - { - 'digest': 'sha256:layer1', - 'mediaType': 'application/vnd.oci.image.layer.v1.tar+gzip', - } - ] - } - with open(os.path.join(blobs_dir, 'abc123'), 'w') as f: - json.dump(manifest_json, f) - - # Create layer tar.gz with /manifests directory - layer_path = os.path.join(blobs_dir, 'layer1') - with tarfile.open(layer_path, 'w:gz') as tar: - # Create a temporary test file - test_file = tmpdir.join('temp_test_manifest.yaml') - test_file.write('test: data') - # Add it to the tar with the path we expect in the image - tar.add(str(test_file), arcname='manifests/test_manifest.yaml') - - mock_skopeo_copy.side_effect = mock_copy - - # Call the function under test - extract_files_from_image_non_privileged('quay.io/ns/test:v1', '/manifests', str(dest_dir)) - - # Verify the extraction succeeded - assert dest_dir.check(dir=True) - extracted_file = dest_dir.join('test_manifest.yaml') - assert extracted_file.check(file=True) - assert extracted_file.read() == 'test: data' +# Tests for extract_files_from_image_non_privileged (backed by 'oc image extract') +def _parse_oc_extract_path(cmd): + """Return (src, dst, is_dir_form) parsed from an 'oc image extract' argv.""" + assert cmd[:4] == ['oc', 'image', 'extract', '--confirm'] + path_arg = next(a for a in cmd if a.startswith('--path=')).removeprefix('--path=') + src, dst = path_arg.rsplit(':', 1) + if src.endswith('/*'): + return src[: -len('/*')], dst, True + return src, dst, False - # Verify skopeo was called - mock_skopeo_copy.assert_called_once() - call_args = mock_skopeo_copy.call_args - assert call_args[1]['source'] == 'docker://quay.io/ns/test:v1' - assert 'oci:' in call_args[1]['destination'] - assert call_args[1]['copy_all'] is False +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_success_directory(mock_run_cmd, tmpdir): + """A directory is extracted via the '/*' glob form into dest_path.""" -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_missing_index(mock_skopeo_copy, mock_log, tmpdir): - """Test extraction fails when OCI index.json is missing.""" - # Mock skopeo_copy to create OCI dir without index.json - def mock_copy(source, destination, copy_all, exc_msg): - # Extract OCI directory path from destination - oci_path = destination.replace('oci:', '') - os.makedirs(oci_path, exist_ok=True) - # Don't create index.json to simulate error - - mock_skopeo_copy.side_effect = mock_copy - - with pytest.raises(IIBError, match='OCI index.json not found'): - extract_files_from_image_non_privileged( - 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) - ) - - -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_no_manifests(mock_skopeo_copy, mock_log, tmpdir): - """Test extraction fails when no manifests in OCI index.""" + def fake_oc(cmd, *args, **kwargs): + src, dst, is_dir = _parse_oc_extract_path(cmd) + if is_dir and src == '/manifests': + # oc unpacks the directory's children into dst. + os.makedirs(dst, exist_ok=True) + with open(os.path.join(dst, 'test_manifest.yaml'), 'w') as f: + f.write('test: data') - def mock_copy(source, destination, copy_all, exc_msg): - oci_path = destination.replace('oci:', '') - os.makedirs(oci_path, exist_ok=True) - # Create index.json with empty manifests - index_json = {'manifests': []} - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) - - mock_skopeo_copy.side_effect = mock_copy - - with pytest.raises(IIBError, match='No manifests found in OCI index'): - extract_files_from_image_non_privileged( - 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) - ) - - -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_no_layers(mock_skopeo_copy, mock_log, tmpdir): - """Test extraction fails when no layers in manifest.""" + mock_run_cmd.side_effect = fake_oc + dest_dir = tmpdir.join('dest') - def mock_copy(source, destination, copy_all, exc_msg): - oci_path = destination.replace('oci:', '') - blobs_dir = os.path.join(oci_path, 'blobs', 'sha256') - os.makedirs(blobs_dir, exist_ok=True) + extract_files_from_image_non_privileged('quay.io/ns/test:v1', '/manifests', str(dest_dir)) - # Create index.json - index_json = {'manifests': [{'digest': 'sha256:abc123'}]} - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) + extracted = dest_dir.join('test_manifest.yaml') + assert extracted.check(file=True) + assert extracted.read() == 'test: data' + # Directory form is tried first and is sufficient (single oc invocation). + mock_run_cmd.assert_called_once() + src, _, is_dir = _parse_oc_extract_path(mock_run_cmd.call_args[0][0]) + assert (src, is_dir) == ('/manifests', True) - # Create manifest with no layers - manifest_json = {'layers': []} - with open(os.path.join(blobs_dir, 'abc123'), 'w') as f: - json.dump(manifest_json, f) - mock_skopeo_copy.side_effect = mock_copy +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_success_file(mock_run_cmd, tmpdir): + """A single file is extracted via the no-glob form and copied to dest_path. - with pytest.raises(IIBError, match='No layers found in manifest'): - extract_files_from_image_non_privileged( - 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) - ) + dest_path may have a different basename than the source file (e.g. the hidden + 'do.not.edit.db' is copied to 'extracted_index.db'). + """ + def fake_oc(cmd, *args, **kwargs): + src, dst, is_dir = _parse_oc_extract_path(cmd) + # Directory form finds nothing for a file path; file form places the file + # at /. + if not is_dir and src == '/var/lib/iib/_hidden/do.not.edit.db': + os.makedirs(dst, exist_ok=True) + with open(os.path.join(dst, 'do.not.edit.db'), 'wb') as f: + f.write(b'SQLite format 3\x00') -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_missing_layer_blob( - mock_skopeo_copy, mock_log, tmpdir -): - """Test extraction fails when layer blob file is missing.""" + mock_run_cmd.side_effect = fake_oc + dest_file = tmpdir.join('extracted_index.db') - def mock_copy(source, destination, copy_all, exc_msg): - oci_path = destination.replace('oci:', '') - blobs_dir = os.path.join(oci_path, 'blobs', 'sha256') - os.makedirs(blobs_dir, exist_ok=True) + extract_files_from_image_non_privileged( + 'quay.io/ns/test:v1', '/var/lib/iib/_hidden/do.not.edit.db', str(dest_file) + ) - # Create index.json - index_json = {'manifests': [{'digest': 'sha256:abc123'}]} - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) + assert dest_file.check(file=True) + assert dest_file.read_binary() == b'SQLite format 3\x00' + # Directory form first (empty), then file form. + assert mock_run_cmd.call_count == 2 + forms = [_parse_oc_extract_path(c[0][0])[2] for c in mock_run_cmd.call_args_list] + assert forms == [True, False] - # Create manifest with layer reference - manifest_json = {'layers': [{'digest': 'sha256:missing_layer'}]} - with open(os.path.join(blobs_dir, 'abc123'), 'w') as f: - json.dump(manifest_json, f) - # Don't create the layer blob file - mock_skopeo_copy.side_effect = mock_copy +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_path_not_found(mock_run_cmd, tmpdir): + """When neither form extracts anything, FileNotFoundInImageError is raised.""" + # oc exits 0 and extracts nothing when the path is absent: run_cmd is a no-op. + mock_run_cmd.return_value = '' - with pytest.raises(IIBError, match='Layer blob not found'): + with pytest.raises(FileNotFoundInImageError, match='Path /manifests not found in image'): extract_files_from_image_non_privileged( 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) ) + assert mock_run_cmd.call_count == 2 -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_path_not_found(mock_skopeo_copy, mock_log, tmpdir): - """Test extraction fails when requested path doesn't exist in image.""" - - def mock_copy(source, destination, copy_all, exc_msg): - oci_path = destination.replace('oci:', '') - blobs_dir = os.path.join(oci_path, 'blobs', 'sha256') - os.makedirs(blobs_dir, exist_ok=True) - - # Create index.json - index_json = {'manifests': [{'digest': 'sha256:abc123'}]} - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_oc_failure(mock_run_cmd, tmpdir): + """A failure from 'oc image extract' propagates as IIBError.""" + mock_run_cmd.side_effect = IIBError('Failed to extract /manifests from image') - # Create manifest - manifest_json = {'layers': [{'digest': 'sha256:layer1'}]} - with open(os.path.join(blobs_dir, 'abc123'), 'w') as f: - json.dump(manifest_json, f) - - # Create empty layer tar.gz (no content) - layer_path = os.path.join(blobs_dir, 'layer1') - with tarfile.open(layer_path, 'w:gz'): - pass # Empty tar - - mock_skopeo_copy.side_effect = mock_copy - - with pytest.raises(IIBError, match='Path /manifests not found in image'): + with pytest.raises(IIBError, match='Failed to extract /manifests from image'): extract_files_from_image_non_privileged( 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) ) -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_invalid_layer_tarball( - mock_skopeo_copy, mock_log, tmpdir -): - """Test extraction fails when layer tarball is corrupted.""" - - def mock_copy(source, destination, copy_all, exc_msg): - oci_path = destination.replace('oci:', '') - blobs_dir = os.path.join(oci_path, 'blobs', 'sha256') - os.makedirs(blobs_dir, exist_ok=True) - - # Create index.json - index_json = {'manifests': [{'digest': 'sha256:abc123'}]} - with open(os.path.join(oci_path, 'index.json'), 'w') as f: - json.dump(index_json, f) - - # Create manifest - manifest_json = {'layers': [{'digest': 'sha256:corrupted_layer'}]} - with open(os.path.join(blobs_dir, 'abc123'), 'w') as f: - json.dump(manifest_json, f) - - # Create corrupted layer (not a valid tar.gz) - layer_path = os.path.join(blobs_dir, 'corrupted_layer') - with open(layer_path, 'w') as f: - f.write('not a valid tar.gz file') - - mock_skopeo_copy.side_effect = mock_copy - - with pytest.raises(IIBError, match='Failed to extract layer'): +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_relative_src_rejected(mock_run_cmd, tmpdir): + """A non-absolute src_path is rejected before invoking oc.""" + with pytest.raises(IIBError, match='must be an absolute image path'): extract_files_from_image_non_privileged( - 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) + 'quay.io/ns/test:v1', 'manifests', str(tmpdir.join('dest')) ) + mock_run_cmd.assert_not_called() -@patch('iib.workers.tasks.containerized_utils.log') -@patch('iib.workers.tasks.containerized_utils._skopeo_copy') -def test_extract_files_from_image_non_privileged_skopeo_copy_failure( - mock_skopeo_copy, mock_log, tmpdir -): - """Test extraction fails when skopeo copy fails.""" - mock_skopeo_copy.side_effect = IIBError('Failed to download image') - - with pytest.raises(IIBError, match='Failed to download image'): - extract_files_from_image_non_privileged( - 'quay.io/ns/test:v1', '/manifests', str(tmpdir.join('dest')) - ) - - mock_skopeo_copy.assert_called_once() - # Verify the call was made with correct parameters - call_args = mock_skopeo_copy.call_args - assert call_args[1]['source'] == 'docker://quay.io/ns/test:v1' - assert 'oci:' in call_args[1]['destination'] - assert call_args[1]['copy_all'] is False +@patch('iib.workers.tasks.utils.run_cmd') +def test_extract_files_from_image_non_privileged_root_src_rejected(mock_run_cmd, tmpdir): + """A src_path of '/' names no path under root and is rejected.""" + with pytest.raises(IIBError, match='must name a path under /'): + extract_files_from_image_non_privileged('quay.io/ns/test:v1', '/', str(tmpdir.join('dest'))) + mock_run_cmd.assert_not_called() @patch('iib.workers.tasks.containerized_utils.get_last_commit_sha') From bde9a0cb85f97c1c922c24478ff133550b12b5d2 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Sat, 5 Sep 2026 21:18:21 -0700 Subject: [PATCH 18/18] fix: treat declared-but-empty /configs as an empty catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'oc image extract' unpacks only file entries, so an empty index's /configs (no files) is indistinguishable from an absent path — both raise FileNotFoundInImageError. Rather than guess in the low-level extractor (the reverted dotfile approach), let extract_catalog_and_db_from_image decide: it holds the configs label as the signal that the image declares an FBC root, so it catches FileNotFoundInImageError and uses an empty catalog directory. The hidden index.db remains required and still fails with an onboarding error when absent, regardless of whether /configs is empty. Co-Authored-By: Claude Opus 4.8 --- iib/workers/tasks/containerized_utils.py | 68 ++++++++++++++++--- .../test_tasks/test_containerized_utils.py | 34 +++++++++- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/iib/workers/tasks/containerized_utils.py b/iib/workers/tasks/containerized_utils.py index c8f6bc99e..c506fa93a 100644 --- a/iib/workers/tasks/containerized_utils.py +++ b/iib/workers/tasks/containerized_utils.py @@ -70,14 +70,19 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path A glob on a file path matches nothing, so trying the directory form first is safe; the file form is only reached (a second ``oc`` invocation) when the - directory form yields nothing. An existing-but-empty directory therefore also - reads as "not found" — acceptable for the FBC configs / manifests / metadata / - hidden index.db paths IIB extracts, none of which are ever legitimately empty. + directory form yields nothing. + + ``oc image extract`` only unpacks file entries: an empty directory (a directory + with no files under it, e.g. an empty index's ``/configs``) contributes no + entries and is indistinguishable here from an absent path — both raise + ``FileNotFoundInImageError``. Callers that can legitimately expect an empty + directory (because they hold another signal, such as the image's configs + label) must catch this and treat it as empty rather than missing. :param str image: the pull specification of the container image :param str src_path: the absolute path within the container image to copy from :param str dest_path: the path on the local host to copy into - :raises FileNotFoundInImageError: if src_path is absent in the image + :raises FileNotFoundInImageError: if src_path is absent (or an empty directory) :raises IIBError: if src_path is not absolute or ``oc image extract`` fails """ from iib.workers.tasks.utils import run_cmd @@ -114,8 +119,18 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path # symlinks verbatim (symlinks=True) rather than resolving them against # the host root during the copy. shutil.copytree(dir_staging, dest, dirs_exist_ok=True, symlinks=True) - log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) + log.info( + 'Extracted %s from image %s to %s as a directory (glob form)', + src_path, + image, + dest_path, + ) return + log.debug( + 'Directory-glob form matched nothing for %s in image %s; trying no-glob form', + src_path, + image, + ) # 2) File form: the path is a single file, placed at /. file_staging = temp_path / 'file' @@ -136,12 +151,27 @@ def extract_files_from_image_non_privileged(image: str, src_path: str, dest_path dest = Path(dest_path) dest.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(extracted_file, dest) - log.info('Successfully extracted %s from image %s to %s', src_path, image, dest_path) + log.info( + 'Extracted %s from image %s to %s as a file (no-glob form)', + src_path, + image, + dest_path, + ) return - # 3) Neither shape produced output: the path is absent in the image. Raise - # the specific FileNotFoundInImageError (a subclass of IIBError) so callers - # can distinguish a genuinely absent path from an 'oc' failure above. + # 3) Neither shape produced output. Because 'oc image extract' unpacks only + # file entries, this means src_path is either absent or an empty directory + # (the two are indistinguishable here). Raise the specific + # FileNotFoundInImageError (a subclass of IIBError) so callers can tell this + # apart from an 'oc' failure above and, where they have another signal (e.g. + # a configs label), treat an empty directory as empty rather than missing. + log.error( + 'Nothing extracted for %s in image %s: neither the directory-glob nor the ' + 'no-glob form of "oc image extract" produced output (path is absent or an ' + 'empty directory)', + src_path, + image, + ) raise FileNotFoundInImageError(f'Path {src_path} not found in image {image}.') @@ -163,6 +193,11 @@ def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) - hidden index.db has not been onboarded to the containerized build flow and the request is failed so the image can be onboarded first. + The configs label is the signal that the image declares an FBC root, so a + declared-but-empty configs directory (an empty index, whose ``/configs`` holds + no files) is treated as an empty catalog rather than a missing path — the + divergent add/rm operation then populates it. + :param str from_index_resolved: The digest-resolved from_index image pullspec. :param str temp_dir: Base temp directory for extraction. :return: Tuple of (configs_dir_path, index_db_path). @@ -184,7 +219,20 @@ def extract_catalog_and_db_from_image(from_index_resolved: str, temp_dir: str) - configs_label, configs_dir, ) - extract_files_from_image_non_privileged(from_index_resolved, configs_label, configs_dir) + try: + extract_files_from_image_non_privileged(from_index_resolved, configs_label, configs_dir) + except FileNotFoundInImageError: + # The image declares a configs label but nothing is stored under it. 'oc + # image extract' cannot represent an empty directory, so this is an empty + # FBC catalog (an empty index), not a broken image. Use an empty configs + # directory; the handler's add/rm operation writes the operators into it. + log.info( + 'Configs path %s in %s is empty; using an empty catalog directory at %s', + configs_label, + from_index_resolved, + configs_dir, + ) + Path(configs_dir).mkdir(parents=True, exist_ok=True) index_db_path = str(Path(temp_dir) / 'extracted_index.db') conf = get_worker_config() diff --git a/tests/test_workers/test_tasks/test_containerized_utils.py b/tests/test_workers/test_tasks/test_containerized_utils.py index c8aa9eed6..88cdacd8d 100644 --- a/tests/test_workers/test_tasks/test_containerized_utils.py +++ b/tests/test_workers/test_tasks/test_containerized_utils.py @@ -1149,7 +1149,11 @@ def fake_oc(cmd, *args, **kwargs): @patch('iib.workers.tasks.utils.run_cmd') def test_extract_files_from_image_non_privileged_path_not_found(mock_run_cmd, tmpdir): - """When neither form extracts anything, FileNotFoundInImageError is raised.""" + """When neither form extracts anything, FileNotFoundInImageError is raised. + + 'oc image extract' unpacks only file entries, so an absent path and an empty + directory both extract nothing and are indistinguishable at this layer. + """ # oc exits 0 and extracts nothing when the path is absent: run_cmd is a no-op. mock_run_cmd.return_value = '' @@ -1301,6 +1305,34 @@ def test_extract_catalog_and_db_raises_when_no_hidden_db(mock_extract, mock_labe assert mock_extract.call_count == 2 +@patch('iib.workers.tasks.containerized_utils.get_image_label') +@patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') +def test_extract_catalog_and_db_empty_configs_uses_empty_dir(mock_extract, mock_label, tmp_path): + """A declared-but-empty /configs (empty index) yields an empty catalog, not a failure. + + 'oc image extract' cannot represent an empty directory, so extracting an empty + index's /configs raises FileNotFoundInImageError. Because the image declares a + configs label, this is treated as an empty catalog directory; the hidden db is + still extracted normally. + """ + mock_label.side_effect = lambda image, label: { + 'operators.operatorframework.io.index.configs.v1': '/configs', + }.get(label, '') + # First call (configs) reports nothing under the declared path; second call + # (hidden db) succeeds. + mock_extract.side_effect = [FileNotFoundInImageError('empty /configs'), None] + + configs_dir, index_db = extract_catalog_and_db_from_image( + 'quay.io/redhat/empty-index:test', str(tmp_path) + ) + + assert configs_dir.endswith('extracted_configs') + assert os.path.isdir(configs_dir) + assert os.listdir(configs_dir) == [] # empty catalog + assert index_db.endswith('index.db') + assert mock_extract.call_count == 2 + + @patch('iib.workers.tasks.containerized_utils.get_image_label') @patch('iib.workers.tasks.containerized_utils.extract_files_from_image_non_privileged') def test_extract_catalog_and_db_propagates_real_extraction_error(