Fix index.db namespace collisions and support divergent-tag builds - #1378
Fix index.db namespace collisions and support divergent-tag builds#1378yashvardhannanavati wants to merge 18 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 11:06 AM UTC · Completed 11:24 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.00 |
PR Summary by QodoPrevent index.db collisions and support divergent-tag builds
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1.
|
![qodo-for-releng[bot]](https://avatars.githubusercontent.com/u/9591189?s=60&v=4){kind=small}
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 1:26 AM UTC · Completed 1:45 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.84 |
|
Risk Assessment: moderate (2/5) DetailsSignals are essentially unchanged from the prior assessment (21 files, ~2,517 lines, 0.38 test ratio, no security or CI changes, low 30d churn, stable file history), yielding the same composite of ~1.90, which rounds to 2 - moderate. Previous runRisk Assessment: moderate (2/5) DetailsA large refactor across 21 files with 2,525 lines changed drives a high change-size signal, but adequate test coverage (38% test ratio), zero security or CI changes, low recent churn, and no regression history across primarily stable files produce a composite score of 1.95, rounding to moderate. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge change with 2049 lines across 21 files, but mitigated by stable git history (no recent fixes), reasonable test coverage (38%), and experienced author. Previous run (3)Risk Assessment: moderate (2/5) DetailsLarge PR (19 files, 1253 lines) with one protected path and moderate test coverage (42%), mostly new files with stable existing files showing low churn and long stability (avg 236 days since last change). |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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' |
There was a problem hiding this comment.
This means that the index image is not FBC index image and I thought that we do not support non-FBC images in new IIB.
There was a problem hiding this comment.
There should not exist an image with both LABELs set.
It is either this operators.operatorframework.io.index.database.v1 or this operators.operatorframework.io.index.configs.v1
There was a problem hiding this comment.
Especially when we have this code above:
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_resolved} does not contain a file-based catalog.")Then db_label will never be filleted. It will be always empty.
| # Content from the image (source of truth), scaffolding from the OCP branch. | ||
| # 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 |
There was a problem hiding this comment.
I thought we do not extract db from index image anymore.
I think this will now work anymore, since we do not include index.db in the image we are building in konflux.
Derive the index.db artifact and ImageStream tag from the index image's manifest digest (idb-<sha256>) 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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 <noreply@anthropic.com>
bddca65 to
b73a69f
Compare
|
🤖 Finished Review · ✅ Success · Started 8:46 AM UTC · Completed 9:25 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.47 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 <noreply@anthropic.com>
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 12:07 AM UTC · Completed 12:49 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.83 |
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
('<dir>/*:<dst>') first, then the file form ('<file>:<dst>'), 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 <noreply@anthropic.com>
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 9:20 AM UTC · Completed 10:02 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
🤖 Finished Review · ✅ Success · Started 4:19 AM UTC · Completed 4:40 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.83 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
'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 <noreply@anthropic.com>
ab758de to
bde9a0c
Compare
|
🤖 Finished Review · ✅ Success · Started 8:14 AM UTC · Completed 8:36 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.76 |
| - **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. 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. |
There was a problem hiding this comment.
[high] scope-creep
The content-key migration orphans every existing ORAS-cached index.db artifact in production. The README explicitly documents this: cached entries under the previous naming scheme are orphaned, and affected images fail with 'no index.db found' until re-onboarded. Every onboarded image will begin failing immediately after deployment with no fallback path or migration mechanism.
Suggested fix: Add an explicit deployment checklist item, or introduce a transient fallback that checks the digest-keyed artifact and, if absent, falls back to the old pullspec-keyed artifact for one release cycle.
There was a problem hiding this comment.
this is intentional. There are no production index.db atm
|
|
||
| set_request_state(request_id, 'in_progress', 'Pushing updated index database') | ||
| conf = get_worker_config() | ||
| output_tag = f'idb-{_get_index_digest(output_image)}' |
There was a problem hiding this comment.
[medium] API-contract-violation
push_index_db_artifact hardcodes the artifact tag format as f'idb-{_get_index_digest(output_image)}' instead of using the configurable iib_index_db_artifact_tag_template from the worker config. The lookup path in _get_artifact_combined_tag (oras_utils.py:96) correctly uses the config template. If the template is overridden, pushed tags will not match lookup tags, causing cache misses or lookup failures.
Suggested fix: Replace the hardcoded tag with the config template: output_tag = conf['iib_index_db_artifact_tag_template'].format(digest=_get_index_digest(output_image)).
| catalog_path = local_git_repo_path / 'configs' | ||
| if catalog_path.exists(): | ||
| shutil.rmtree(catalog_path) | ||
| shutil.copytree(extracted_configs, catalog_path) |
There was a problem hiding this comment.
[medium] path-traversal
On the divergent-tag path, shutil.copytree(extracted_configs, catalog_path) uses the default symlinks=False, which follows symlinks on the host filesystem. The upstream extract_files_from_image_non_privileged (line 121) explicitly uses symlinks=True. A malicious from_index image embedding an absolute symlink in its /configs directory could exfiltrate host-local files into the git-committed catalog_path.
Suggested fix: Pass symlinks=True to the shutil.copytree call at line 690, or add a post-extraction pass that detects and removes symlinks targeting paths outside the temp directory.
| 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}' |
There was a problem hiding this comment.
[medium] breaking-config-interface
The format variable names in iib_index_db_artifact_tag_template changed from {image_name} and {tag} to {digest}. Any deployment overriding this setting in a site-level settings.py will get an unhandled KeyError at runtime when _get_artifact_combined_tag calls .format(digest=...) on a template referencing {image_name} and {tag}.
Suggested fix: Validate the config key's format string at startup (check that {digest} is present, warn if {image_name}/{tag} are found), or document that the template format is not operator-configurable.
| # 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], |
There was a problem hiding this comment.
[low] secret-exposure
The new remote_branch_exists function injects credentials into the git URL. The error-path log.error in run_cmd (utils.py line 923) logs the raw command without sanitization. Pre-existing pattern, but adds another affected call site.
Suggested fix: Fix run_cmd to use _sanitize_cmd_log in the log.error call at utils.py line 923.
| ) | ||
|
|
||
|
|
||
| def prepare_git_repository_for_build( |
There was a problem hiding this comment.
[low] naming-abstraction
prepare_git_repository_for_build is retained alongside the new prepare_build_sources for request types where divergent-tag detection does not apply (merge, create_empty_index), but lacks any comment explaining why it was not migrated.
Suggested fix: Add a docstring note stating it is intentionally retained for request types where divergent-tag detection does not apply.
| :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 |
There was a problem hiding this comment.
[low] documentation-comment-format
remote_branch_exists uses :rtype: bool with no :return: description, diverging from the file's established :return: convention.
Suggested fix: Replace ':rtype: bool' with ':return: True if the branch exists on the remote, False otherwise.'
| 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. |
There was a problem hiding this comment.
[low] documentation-comment-format
Three new functions use :return: + :rtype: on separate lines, while pre-existing functions use :returns Type: description inline. Introduces a mixed docstring style.
Suggested fix: Adopt one consistent style across all functions in the file.
2 participants
Summary
Fixes two architectural collisions in the containerized (git + ORAS + Konflux) worker architecture, where the
index.dbartifact identity and the git branch were derived from keys that are too weak.Problem 1 — namespace collision. The
index.dbORAS-artifact and ImageStream tags derived from repo-name + OCP version only, dropping registry/namespace. Soquay.io/redhat/fooandquay.io/redhat-pending/foocollapsed onto the same artifact reference and read/overwrote each other'sindex.db→ corruption.Problem 2 — divergent tags. The git branch was the OCP version read from image labels, so a divergent tag (
:test, or a timestampedv4.14-<ts>) resolved to the same branch as mainlinev4.14and collided in git.What changed
Part A — namespace-safe artifact naming (clean cutover). The artifact/ImageStream tag now includes a short 8-char sha256 hash of the full pullspec (registry + namespace + repo + tag):
{image_name}-{tag}-{pullspec_hash}. Distinct namespaces now get distinct tags and never collide. No dual-read (which would reintroduce the bug) — the first request after deploy is a normal cache miss that repopulates from Quay. Old tags become orphaned and are pruned separately.Part B — branch = image tag, with a normal/divergent discriminator. The git branch is now keyed on the image tag. For prod indexes
tag == ocp_version, so this is a no-op for the existing fleet.configs/+index.dbfrom the image using the unprivileged extractor only (skopeo → OCI → untar; precedence hidden-db → labeled-db → empty db for pure-FBC images), reuse the base OCP branch's Konflux Component via a throw-away MR that is never merged, and reject overwrite. The divergentindex.dbis never sourced from ORAS. Timestamped/point-in-time tags stay throw-away by design.Commits
fcdaadbfix: make index.db artifact tag namespace-safe via pullspec hash23e76dffeat: addremote_branch_existsandget_index_taghelpers8128a7dfeat: unprivileged extraction of configs+index.db from index image0c1f478feat:prepare_build_sourcesorchestrates normal vs divergent build pathse3f9dcb/2d892dd/6102d86feat: route add / rm / fbc-operations through normal/divergent build sourcesa1ed741test: prove divergent never-merge guard withoverwrite=True892cb25docs: document branch=tag convention and divergent-tag buildsScope
In scope: single-
from_indexrequest types —add,rm,fbc-operations. Out of scope (unchanged):merge-index-image,regenerate-bundle,create-empty-index(its empty-artifact ref is preserved byte-identical). Digest-referencedfrom_indexis deferred. The old (non-containerized) worker handlers are untouched.Type of change
Checklist
tox -e py312→ 1211 passed, 93.37% coverage)tox -m static: black / flake8 / yamllint / mypy all OK)Unreleasedsection is empty by convention; entries are generated from PR titles at release time)🤖 Generated with Claude Code