Skip to content

Fix cache digest comparison by using oras manifest fetch instead of client-side hash#1358

Merged
yashvardhannanavati merged 1 commit into
mainfrom
fix_iib_cache
Jun 26, 2026
Merged

Fix cache digest comparison by using oras manifest fetch instead of client-side hash#1358
yashvardhannanavati merged 1 commit into
mainfrom
fix_iib_cache

Conversation

@yashvardhannanavati

Copy link
Copy Markdown
Collaborator

get_image_digest computes a client-side sha256 of the raw manifest bytes from skopeo, which doesn't match the registry-assigned digest stored in the OpenShift ImageStream. Additionally, skopeo inspect without --raw fails entirely on ORAS artifacts.

Replace with oras manifest fetch --descriptor which returns the registry-assigned digest, matching what oc import-image stores in the ImageStream.

lipoja
lipoja previously approved these changes Jun 26, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 26, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:24 AM UTC · Completed 9:36 AM UTC
Commit: 8207ad4 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review

Reason: stale-head

The review agent reviewed commit 0c47018da7b659bf3f3df2927b8a8c4b8d16437a but the PR HEAD is now 5a2a8b2fd2ffbedca805bfd6a2f3287e43eb3729. This review was discarded to avoid approving unreviewed code.

Previous run

Review

Findings

Medium

  • [missing config key] iib/workers/tasks/oras_utils.py:268conf['iib_index_db_oras_auth_path'] uses bracket access, which will raise KeyError if the key is missing from the worker config. While sibling keys (iib_index_db_artifact_registry, iib_index_db_artifact_template) follow the same external-config pattern, using conf.get('iib_index_db_oras_auth_path', '') would provide a safe default and is consistent with how optional config is accessed elsewhere (e.g., in validate_celery_config).

  • [missing error handling] iib/workers/tasks/oras_utils.py:277json.loads(descriptor)['digest'] can fail with JSONDecodeError (if oras returns non-JSON output) or KeyError (if the descriptor lacks a digest field). Neither exception is caught or wrapped in an IIBError with context about what went wrong.
    Remediation: Wrap in try/except that raises IIBError(f'Failed to parse digest from manifest descriptor for {artifact_pullspec}: {e}').

  • [test coverage gap] tests/test_workers/test_tasks/test_oras_utils.py:434 — Both test cases set iib_index_db_oras_auth_path to '' (falsy). The code path where the auth path is set and os.path.exists() returns True — which appends ['--registry-config', path] to the command — has zero test coverage.
    Remediation: Add a test case with a non-empty iib_index_db_oras_auth_path, mock os.path.exists to return True, and assert run_cmd is called with the --registry-config argument.

  • [missing config documentation] README.md — The new iib_index_db_oras_auth_path configuration variable is not documented in the "Configuring the Worker(s)" section of the README alongside other iib_* config options.

Low

  • [unused import] iib/workers/tasks/oras_utils.py:13get_image_digest is imported with # noqa: F401 but is no longer used in this module after the PR replaces its only call site. No other module imports get_image_digest from oras_utils. Consider removing the import entirely.

  • [command-construction] iib/workers/tasks/oras_utils.py:269 — The cmd_args = [] + *cmd_args unpacking pattern works but deviates from the dominant codebase pattern of building a single cmd list with cmd.extend() (e.g., push_oras_artifact at lines 87–96).

  • [architectural-coherence] iib/workers/tasks/oras_utils.py:269 — The new --registry-config flag-based auth differs from the set_registry_auths() context manager used by other ORAS functions. This may be intentional (the PR description notes skopeo fails on ORAS artifacts), but worth documenting why the auth approaches differ.

  • [docstring-staleness] iib/workers/tasks/oras_utils.py — The verify_indexdb_cache_sync() docstring could note the switch from client-side hashing to registry-assigned digest retrieval.


Labels: PR fixes a bug in ORAS digest comparison logic

Previous run (2)

Review

Reason: stale-head

The review agent reviewed commit 2d1e3c69b3749c5c9934f7db693461bf5cdea10e but the PR HEAD is now a44d5bccbd387b909aef135305e94f03c137ac43. This review was discarded to avoid approving unreviewed code.

@fullsend-ai-review

Copy link
Copy Markdown

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 26, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:39 AM UTC · Completed 9:52 AM UTC
Commit: 8207ad4 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread iib/workers/tasks/oras_utils.py
Comment thread iib/workers/tasks/oras_utils.py
Comment thread tests/test_workers/test_tasks/test_oras_utils.py
Comment thread iib/workers/tasks/oras_utils.py
Comment thread iib/workers/tasks/oras_utils.py
Comment thread iib/workers/tasks/oras_utils.py
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment bug Something isn't working labels Jun 26, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 26, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:03 AM UTC · Completed 10:16 AM UTC
Commit: 8207ad4 · View workflow run →

…lient-side hash

get_image_digest computes a client-side sha256 of the raw manifest
bytes from skopeo, which doesn't match the registry-assigned digest
stored in the OpenShift ImageStream. Additionally, skopeo inspect
without --raw fails entirely on ORAS artifacts.

Replace with oras manifest fetch --descriptor which returns the
registry-assigned digest, matching what oc import-image stores in
the ImageStream.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@yashvardhannanavati yashvardhannanavati dismissed lipoja’s stale review June 26, 2026 10:07

dismissing because flake8 and black fixes were added

@fullsend-ai-review

Copy link
Copy Markdown

/fs-review

@yashvardhannanavati yashvardhannanavati merged commit 735f2ae into main Jun 26, 2026
12 of 13 checks passed
@yashvardhannanavati yashvardhannanavati deleted the fix_iib_cache branch June 26, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants