OCPBUGS-87160: skip manifest list signatures by r4f4 · Pull Request #1443 · openshift/oc-mirror

r4f4 · 2026-06-12T14:06:16Z

Description

ART does not sign manifest lists, only the single-arch manifests. The same way, podman/clusterimagepolicy only verify single-arch manifests.

If we require signatures in manifest lists, we break mirroring of multi OCP payloads. So let's ignore them for now. This fix/workaround will be obsolete if we ever introduce some API in container-libs to differentiate images with wrong/missing signatures vs non-signed images.

I have also included a small refactor to simplify the existing code.

Github / Jira issue: OCPBUGS-87160

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Code Improvements (Refactoring, Performance, CI upgrades, etc)
Internal repo assets (diagrams / docs on github repo)
This change requires a documentation update on openshift docs

How Has This Been Tested?

Run d2m with ISC:

kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
  platform:
    architectures:
      - multi
    channels:
      - name: stable-4.20
        minVersion: 4.20.20
        maxVersion: 4.20.20
    graph: false

Expected Outcome

m2d succeeds and the log-level=debug output contains:

2026/06/12 12:38:48  [DEBUG]  : Skip signature gathering for manifest list "sha256:c79de9ca4ae9301bb4a0326e935e33dbd8d945e31a872aff9a8565456de37fcc"
[...]

Summary by CodeRabbit

Refactor
- Updated blob and history digest tracking to use Kubernetes set primitives for collecting, comparing, and merging digests across archive and history flows.
Bug Fixes
- Improved multi-architecture signature handling: manifest-list signature failures are logged without failing the overall gather, while per-architecture signature errors are still surfaced.
Tests
- Updated unit tests to use order-independent set comparisons and set-based mocks for blob/history gather and diff behavior.

openshift-ci-robot · 2026-06-12T14:06:24Z

@r4f4: This pull request references Jira Issue OCPBUGS-87160, which is invalid:

expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Description

ART does not sign manifest lists, only the single-arch manifests. The same way, podman/clusterimagepolicy only verify single-arch manifests.

If we require signatures in manifest lists, we break mirroring of multi OCP payloads. So let's ignore them for now. This fix/workaround will be obsolete if we ever introduce some API in container-libs to differentiate images with wrong/missing signatures vs non-signed images.

I have also included a small refactor to simplify the existing code.

Github / Jira issue: OCPBUGS-87160

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code Improvements (Refactoring, Performance, CI upgrades, etc)

Internal repo assets (diagrams / docs on github repo)

This change requires a documentation update on openshift docs

How Has This Been Tested?

Run d2m with ISC:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
 platform:
   architectures:
     - multi
   channels:
     - name: stable-4.20
       minVersion: 4.20.20
       maxVersion: 4.20.20
   graph: false
Expected Outcome

m2d succeeds and the log-level=debug output contains:
2026/06/12 12:38:48  [DEBUG]  : Skip signature gathering for manifest list "sha256:c79de9ca4ae9301bb4a0326e935e33dbd8d945e31a872aff9a8565456de37fcc"
[...]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

coderabbitai · 2026-06-12T14:06:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 81b637f3-6959-49aa-9b22-744b191153f7

📥 Commits

Reviewing files that changed from the base of the PR and between c562c4e and 65a7bd1.

📒 Files selected for processing (9)

internal/pkg/archive/archive.go
internal/pkg/archive/archive_test.go
internal/pkg/archive/image-blob-gatherer.go
internal/pkg/archive/image-blob-gatherer_test.go
internal/pkg/archive/interface.go
internal/pkg/delete/delete_images_test.go
internal/pkg/history/history.go
internal/pkg/history/history_test.go
internal/pkg/history/interface.go

🚧 Files skipped from review as they are similar to previous changes (9)

internal/pkg/archive/interface.go
internal/pkg/archive/archive_test.go
internal/pkg/history/interface.go
internal/pkg/delete/delete_images_test.go
internal/pkg/archive/image-blob-gatherer_test.go
internal/pkg/history/history_test.go
internal/pkg/history/history.go
internal/pkg/archive/archive.go
internal/pkg/archive/image-blob-gatherer.go

Walkthrough

This pull request refactors blob and history tracking from map[string]struct{} to sets.Set[string] across archive, history, and related tests. Interfaces, implementations, and mocks now use set membership, union, and insertion operations.

Changes

Blob Set Tracking Refactor

Layer / File(s)	Summary
Interface contracts `internal/pkg/history/interface.go`, `internal/pkg/archive/interface.go`	`History.Read()`/`Append()` and `BlobsGatherer.GatherBlobs()` now use `sets.Set[string]`, with `sets` imports added.
History storage and tests `internal/pkg/history/history.go`, `internal/pkg/history/history_test.go`	`history.Read` builds sets with `Insert`, `history.Append` merges via `Union`, and tests construct/compare sets with `sets.New[string](...)` and `sets.List(...)`.
Image blob gathering `internal/pkg/archive/image-blob-gatherer.go`, `internal/pkg/archive/image-blob-gatherer_test.go`	`GatherBlobs` returns `sets.Set[string]`; multi-arch and single-arch collection now use set operations, and tests were updated to set fixtures and comparisons.
Archive diff tracking `internal/pkg/archive/archive.go`, `internal/pkg/archive/archive_test.go`	`addImagesDiff` and `addBlobsDiff` now operate on `sets.Set[string]`, using `Has`, `Insert`, and `Union`; archive tests and mocks were updated accordingly.
Delete test mock `internal/pkg/delete/delete_images_test.go`	The blob gatherer mock now returns `sets.Set[string]`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	`image-blob-gatherer.go` logs `err.Error()` for signature-gather failures; wrapped errors can include full image refs/registry hostnames from `ImageManifest`.	Log a generic message or redact image refs/hostnames before emitting the error; avoid passing raw `err.Error()` to debug logs.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: skipping manifest list signatures for the OCPBUGS-87160 mirroring issue.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	No unstable test titles found: touched tests use static table-driven names, and no Ginkgo It/Describe titles with dynamic data were added.
Test Structure And Quality	✅ Passed	PASS: The touched tests are plain table-driven unit tests, not Ginkgo; no cluster waits, cleanup gaps, or single-responsibility regressions were introduced.
Microshift Test Compatibility	✅ Passed	Touched test files are plain Go unit tests; no new Ginkgo e2e tests or MicroShift-unsupported APIs were added.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the changed files are ordinary `testing.T` unit tests under `internal/pkg`, so the SNO multi-node check is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	Only archive/history set refactors changed; no manifests, controllers, replicas, affinity, nodeSelectors, PDBs, or topology logic were added.
Ote Binary Stdout Contract	✅ Passed	PR only changes internal archive/history packages and tests; no main/init/TestMain/suite setup or stdout writes were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the touched tests are unit tests using local fixtures/httptest, with no public internet or IPv4-only assumptions.
No-Weak-Crypto	✅ Passed	No weak crypto primitives, custom crypto, or secret/token comparisons were introduced in the touched archive/history/signature paths.
Container-Privileges	✅ Passed	PR only changes Go source/tests in internal/pkg/archive, history, and delete; no K8s/container manifests or privilege settings were added.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-06-12T14:07:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [r4f4]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adolfo-ab

lgtm once failing jobs are fixed

r4f4 · 2026-06-15T12:20:01Z

lgtm once failing jobs are fixed

@adolfo-ab sorry, I changed the approach a little. Instead of just ignoring signatures for manifest lists, we still try to copy them but ignore errors. WDYT?

r4f4 · 2026-06-22T14:35:34Z

/jira refresh

openshift-ci-robot · 2026-06-22T14:35:38Z

@r4f4: This pull request references Jira Issue OCPBUGS-87160, which is invalid:

expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

r4f4 · 2026-06-22T14:37:02Z

/jira refresh

openshift-ci-robot · 2026-06-22T14:37:09Z

@r4f4: This pull request references Jira Issue OCPBUGS-87160, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (5.0.0) matches configured target version for branch (5.0.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

aguidirh

Thanks for the PR @r4f4.

A lot of good refactorings. I only added one nit suggestion, please let me know if you will take it so I'll wait to add LGTM.

I'm not sure if we should be concerned about it but I got the following when building locally:

go build -mod=readonly -tags "json1 " -ldflags "-X github.com/openshift/oc-mirror/pkg/version.versionFromGit="v0.2.0-alpha.1-645-g7a74c01" -X github.com/openshift/oc-mirror/pkg/version.commitFromGit="7a74c01d" -X github.com/openshift/oc-mirror/pkg/version.gitTreeState="clean" -X github.com/openshift/oc-mirror/pkg/version.buildDate="2026-06-25T13:56:56Z" " -o './bin/oc-mirror' github.com/openshift/oc-mirror/cmd/oc-mirror
# github.com/mattn/go-sqlite3
sqlite3-binding.c: In function ‘sqlite3ShadowTableName’:
sqlite3-binding.c:123934:9: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
123934 |   zTail = strrchr(zName, '_');
       |         ^
make[1]: Leaving directory '/home/aguidi/go/src/github.com/aguidirh/oc-mirror/v1'

But at the end the binary was built. did you face anything similar?

aguidirh · 2026-06-25T13:50:31Z

+		if historyBlobs.Has(hash) || alreadyAddedBlobs.Has(hash) {
+			continue
 		}


Much cleaner now, thanks!

aguidirh · 2026-06-25T13:54:35Z

-	blobs := make(map[string]struct{})
-
-	blobs[in.digest.String()] = struct{}{}
+	blobs := sets.New[string](in.digest.String())


nit: it seems that when you initialize sets.New inline, it does not need the type.

Suggested change

blobs := sets.New[string](in.digest.String())

blobs := sets.New(in.digest.String())

This is valid for all the places where sets.New is initialized (I saw it over few files).

aguidirh · 2026-06-25T13:55:34Z

+			expectedBlobs: sets.New[string](
+				"sha256:467829ca4ff134ef9762a8f69647fdf2515b974dfc94a8474c493a45ef922e51",
+				"sha256:728191dbaae078c825ffb518e15d33956353823d4da6c2e81fe9b1ed60ddef7d",
+				"sha256:50b9402635dd4b312a86bed05dcdbda8c00120d3789ec2e9b527045100b3bdb4",
+			),


nit: another example of initialization inline:

Suggested change

expectedBlobs: sets.New[string](

"sha256:467829ca4ff134ef9762a8f69647fdf2515b974dfc94a8474c493a45ef922e51",

"sha256:728191dbaae078c825ffb518e15d33956353823d4da6c2e81fe9b1ed60ddef7d",

"sha256:50b9402635dd4b312a86bed05dcdbda8c00120d3789ec2e9b527045100b3bdb4",

),

expectedBlobs: sets.New(

"sha256:467829ca4ff134ef9762a8f69647fdf2515b974dfc94a8474c493a45ef922e51",

"sha256:728191dbaae078c825ffb518e15d33956353823d4da6c2e81fe9b1ed60ddef7d",

"sha256:50b9402635dd4b312a86bed05dcdbda8c00120d3789ec2e9b527045100b3bdb4",

),

ART does not sign manifest lists, only the single-arch manifests. The same way, podman/clusterimagepolicy only verify single-arch manifests. If we require signatures in manifest lists, we break mirroring of `multi` OCP payloads. So let's ignore them for now. This fix/workaround will be obsolete if we ever introduce some API in container-libs to differentiate images with wrong/missing signatures vs non-signed images.

r4f4 · 2026-06-25T20:16:11Z

Thanks for the PR @r4f4.

A lot of good refactorings. I only added one nit suggestion, please let me know if you will take it so I'll wait to add LGTM.

I'm not sure if we should be concerned about it but I got the following when building locally:

go build -mod=readonly -tags "json1 " -ldflags "-X github.com/openshift/oc-mirror/pkg/version.versionFromGit="v0.2.0-alpha.1-645-g7a74c01" -X github.com/openshift/oc-mirror/pkg/version.commitFromGit="7a74c01d" -X github.com/openshift/oc-mirror/pkg/version.gitTreeState="clean" -X github.com/openshift/oc-mirror/pkg/version.buildDate="2026-06-25T13:56:56Z" " -o './bin/oc-mirror' github.com/openshift/oc-mirror/cmd/oc-mirror
# github.com/mattn/go-sqlite3
sqlite3-binding.c: In function ‘sqlite3ShadowTableName’:
sqlite3-binding.c:123934:9: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
123934 |   zTail = strrchr(zName, '_');
       |         ^
make[1]: Leaving directory '/home/aguidi/go/src/github.com/aguidirh/oc-mirror/v1'

But at the end the binary was built. did you face anything similar?

I don't see this even when I build after a make clean. Maybe it happened when the mattn/go-sqlite3 was updated in 78fa31f and I didn't even realize it. Anyway, this PR doesn't touch any deps so this was not introduced here.

r4f4 · 2026-06-25T20:19:19Z

Update: addressed review comments.

openshift-ci-robot · 2026-06-25T20:19:39Z

@r4f4: This pull request references Jira Issue OCPBUGS-87160, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (5.0.0) matches configured target version for branch (5.0.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

Description

ART does not sign manifest lists, only the single-arch manifests. The same way, podman/clusterimagepolicy only verify single-arch manifests.

If we require signatures in manifest lists, we break mirroring of multi OCP payloads. So let's ignore them for now. This fix/workaround will be obsolete if we ever introduce some API in container-libs to differentiate images with wrong/missing signatures vs non-signed images.

I have also included a small refactor to simplify the existing code.

Github / Jira issue: OCPBUGS-87160

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code Improvements (Refactoring, Performance, CI upgrades, etc)

Internal repo assets (diagrams / docs on github repo)

This change requires a documentation update on openshift docs

How Has This Been Tested?

Run d2m with ISC:
kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
 platform:
   architectures:
     - multi
   channels:
     - name: stable-4.20
       minVersion: 4.20.20
       maxVersion: 4.20.20
   graph: false
Expected Outcome

m2d succeeds and the log-level=debug output contains:
2026/06/12 12:38:48  [DEBUG]  : Skip signature gathering for manifest list "sha256:c79de9ca4ae9301bb4a0326e935e33dbd8d945e31a872aff9a8565456de37fcc"
[...]
Summary by CodeRabbit

Refactor

Updated blob and history digest tracking to use Kubernetes set primitives for collecting, comparing, and merging digests across archive and history flows.

Bug Fixes

Improved multi-architecture signature handling: manifest-list signature failures are logged without failing the overall gather, while per-architecture signature errors are still surfaced.

Tests

Updated unit tests to use order-independent set comparisons and set-based mocks for blob/history gather and diff behavior.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2026-06-25T20:56:26Z

@r4f4: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

aguidirh

/lgtm

aguidirh · 2026-06-26T07:36:15Z

/assign @nidangavali for verified label

openshift-ci · 2026-06-26T07:36:18Z

@aguidirh: GitHub didn't allow me to assign the following users: for, verified, label.

Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @nidangavali for verified label

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci Bot requested review from adolfo-ab and aguidirh June 12, 2026 14:07

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2026

adolfo-ab reviewed Jun 15, 2026

View reviewed changes

r4f4 force-pushed the ocpbugs-87160-multi-sigs branch from a493b23 to 7b13f01 Compare June 15, 2026 12:17

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2026

r4f4 force-pushed the ocpbugs-87160-multi-sigs branch from 7b13f01 to c562c4e Compare June 15, 2026 12:18

openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2026

r4f4 force-pushed the ocpbugs-87160-multi-sigs branch from c562c4e to 7a74c01 Compare June 18, 2026 14:21

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jun 22, 2026

aguidirh reviewed Jun 25, 2026

View reviewed changes

r4f4 added 2 commits June 25, 2026 22:13

pkg/archive: simplify code with sets library

e81bc78

r4f4 force-pushed the ocpbugs-87160-multi-sigs branch from 7a74c01 to 65a7bd1 Compare June 25, 2026 20:16

aguidirh reviewed Jun 26, 2026

View reviewed changes

openshift-ci Bot assigned aguidirh Jun 26, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2026

openshift-ci Bot assigned nidangavali Jun 26, 2026

	blobs := sets.New[string](in.digest.String())
	blobs := sets.New(in.digest.String())

Uh oh!

Conversation

r4f4 commented Jun 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Expected Outcome

Summary by CodeRabbit

Uh oh!

openshift-ci-robot commented Jun 12, 2026

Description

Type of change

How Has This Been Tested?

Expected Outcome

Uh oh!

coderabbitai Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks failed

❌ Failed checks (1 error, 1 warning)

Uh oh!

openshift-ci Bot commented Jun 12, 2026

Uh oh!

adolfo-ab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r4f4 commented Jun 15, 2026

Uh oh!

r4f4 commented Jun 22, 2026

Uh oh!

openshift-ci-robot commented Jun 22, 2026

Uh oh!

r4f4 commented Jun 22, 2026

Uh oh!

openshift-ci-robot commented Jun 22, 2026

Uh oh!

aguidirh left a comment

Choose a reason for hiding this comment

Uh oh!

aguidirh Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

aguidirh Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

aguidirh Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

r4f4 commented Jun 25, 2026

Uh oh!

r4f4 commented Jun 25, 2026

Uh oh!

openshift-ci-robot commented Jun 25, 2026

Description

Type of change

How Has This Been Tested?

Expected Outcome

Summary by CodeRabbit

Uh oh!

openshift-ci Bot commented Jun 25, 2026

Uh oh!

aguidirh left a comment

Choose a reason for hiding this comment

Uh oh!

aguidirh commented Jun 26, 2026

Uh oh!

openshift-ci Bot commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

r4f4 commented Jun 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 12, 2026 •

edited

Loading

adolfo-ab left a comment •

edited

Loading