CLID-290: feat: Add sparse manifest platform filtering for multi-arch images#1435
Open
aguidirh wants to merge 10 commits into
Open
CLID-290: feat: Add sparse manifest platform filtering for multi-arch images#1435aguidirh wants to merge 10 commits into
aguidirh wants to merge 10 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aguidirh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
aguidirh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@aguidirh: This pull request references CLID-290 which is a valid jira issue. DetailsIn response to this:
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces per-image platform filtering across the oc-mirror pipeline. A new ChangesPer-image platform filtering across oc-mirror
Sequence Diagram(s)sequenceDiagram
participant Collector as Collector (Release/Helm/Operator/Additional)
participant RelatedImage as RelatedImage
participant BatchWorker as ChannelConcurrentBatch.Worker
participant MirrorEngine as mirror.copy()
participant determinePlatformSelection as determinePlatformSelection
participant parseMultiArch as parseMultiArch
Collector->>RelatedImage: sets Platforms *[]string from ConvertPlatformsToStringSlice
Collector->>BatchWorker: enqueues CopyImageSchema with Platforms
BatchWorker->>MirrorEngine: sets options.InstancePlatforms from img.Platforms
MirrorEngine->>determinePlatformSelection: opts.InstancePlatforms or MultiArch/All
determinePlatformSelection->>parseMultiArch: comma-separated OS/ARCH or legacy mode
parseMultiArch-->>determinePlatformSelection: ImageListSelection + []InstancePlatformFilter
determinePlatformSelection-->>MirrorEngine: imageListSelection + instancePlatforms
MirrorEngine->>MirrorEngine: copy.Options{InstancePlatforms: instancePlatforms}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/api/v2alpha1/type_config.go (1)
116-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Platform.DeepCopy()drops scalar fields and returns an incomplete copy.Line 117 only seeds
Graph;ReleaseandKubeVirtContainerare lost in the copied struct. That can silently alter mirroring behavior when callers depend onDeepCopy().Proposed fix
func (p Platform) DeepCopy() Platform { platformCopy := Platform{ - Graph: p.Graph, + Graph: p.Graph, + Release: p.Release, + KubeVirtContainer: p.KubeVirtContainer, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/api/v2alpha1/type_config.go` around lines 116 - 130, Platform.DeepCopy() currently initializes platformCopy with only Graph, causing scalar fields like Release and KubeVirtContainer to be lost; update Platform.DeepCopy to copy all scalar and value fields from the receiver (e.g., set platformCopy.Release = p.Release and platformCopy.KubeVirtContainer = p.KubeVirtContainer) in addition to the existing deep copies of slices (Channels, Architectures, Platforms) so the returned copy is complete; locate the DeepCopy method on type Platform and add assignments for any other scalar/value fields to mirror the source struct.
🧹 Nitpick comments (2)
internal/pkg/operator/filtered_collector_test.go (1)
1052-1061: ⚡ Quick winMake the mock validate the forwarded
platformsargument.The new parameter is currently ignored, so these tests won’t detect broken propagation from operator config to catalog handler calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/operator/filtered_collector_test.go` around lines 1052 - 1061, The mock getRelatedImagesFromCatalog in MockHandler ignores the new platforms parameter; update it to validate the forwarded platforms pointer (the platforms *[]string argument) before returning results — e.g., check platforms is not nil and that *platforms matches the expected slice (or contains required entries) and return an error (or fail) when it does not so tests detect propagation failures; keep the existing relatedImages behavior otherwise and leave copyImageSchemaMap untouched.internal/pkg/operator/catalog_handler_test.go (1)
43-43: ⚡ Quick winAdd one non-nil
platformscase to validate propagation in returned related images.Current update only checks the
nilpath, so the new behavior can regress without test failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/operator/catalog_handler_test.go` at line 43, Add a test entry in catalog_handler_test.go that calls handler.getRelatedImagesFromCatalog with a non-nil platforms argument (e.g., a slice like []string{"linux/amd64"}) in addition to the existing nil-case; call the same function used in the diff (getRelatedImagesFromCatalog) with that non-nil platforms and copyImageSchemaMap, assert err == nil, and verify that each returned related image in res has its Platforms (or equivalent field) set to the same non-nil slice (or contains the provided platforms), thereby validating propagation of the platforms parameter through the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 8: Update the pinned module version for
github.com/distribution/distribution/v3 in go.mod from v3.0.0 to a patched
release (>= v3.1.1) to address the listed GHSA advisories; after editing the
version string, run the appropriate Go module commands (e.g., go get
github.com/distribution/distribution/[email protected] and go mod tidy) to update go.sum
and ensure build/test pass, and verify no other transitive constraints (e.g.,
from github.com/containerd/containerd or github.com/moby/spdystream) prevent the
upgrade.
- Line 138: The project currently pins github.com/moby/spdystream v0.5.0; update
the dependency to v0.5.1+ by bumping the parent module that pulls it (or
directly requesting the newer version) so the vulnerable transitive dependency
is resolved, then run go mod tidy to refresh go.sum; locate references to
github.com/moby/spdystream in go.mod (and the parent module that requires it)
and run a go get or otherwise update that parent to force
github.com/moby/[email protected] (or later), followed by go mod tidy to update
go.sum.
- Line 63: Update the indirect dependency github.com/containerd/containerd from
v1.7.29 to at least v1.7.32 in go.mod to address GHSA-fqw6-gf59-qr4w; edit the
require directive for "github.com/containerd/containerd" to "v1.7.32" or a newer
compatible v1.7.x+ version, run "go get
github.com/containerd/[email protected]" (or higher) and then "go mod tidy" to
refresh go.sum and ensure the indirect upgrade is applied across the module
graph.
In `@internal/pkg/archive/image-blob-gatherer_test.go`:
- Line 293: The test currently hard-codes the full transport error string in the
assert.Equal call (the assert.Equal(t, "...", err.Error()) in
image-blob-gatherer_test.go), which is brittle across environments; change it to
use substring/contains assertions (e.g., assert.Contains or strings.Contains on
err.Error()) to check key phrases such as "error when creating a new image
source", "fetching manifest latest", and "connection refused" (or "pinging
container registry") instead of the full message so the test is robust to
platform-specific network wording.
In `@internal/pkg/mirror/mirror.go`:
- Around line 359-368: The loop that parses multiArch tokens (using variable
multiArch and splitting into parts) currently only checks len(parts)==2 and can
accept empty or whitespace OS/ARCH; update the validation in that block (the
code that builds copy.InstancePlatformFilter) to trim and reject empty parts and
enforce an allow-list pattern for OS and Architecture (e.g., only
letters/numbers, '-', '_' and dots) before appending to platforms; on invalid
tokens return the same error path (the fmt.Errorf currently returned alongside
copy.CopySystemImage) so malformed tokens are rejected at this trust boundary.
In `@internal/pkg/operator/filtered_collector.go`:
- Around line 251-255: The computed platforms slice is passed into
getRelatedImagesFromCatalog but not applied to the operator catalog image copy,
so the catalog copy ends up unfiltered; update the code that assembles or
creates the operator catalog image later in this function to accept and use the
platforms filter (e.g., pass the platforms variable into the catalog image
creation call or set the Platforms field on the catalog image struct before
invoking the copy), ensuring the same platforms value used with
o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig, platforms,
copyImageSchemaMap) is also applied to the operator catalog image copy.
In `@internal/pkg/release/local_stored_collector.go`:
- Around line 56-73: getPlatformFilters in LocalStorageCollector currently
returns nil when both o.Config.Mirror.Platform.Platforms and Architectures are
empty, causing "mirror all" behavior; change it to return a pointer to a slice
containing the default "linux/amd64" string instead. Specifically, in
getPlatformFilters (and where it falls back from Architectures), replace the
final "return nil" with creation of a []string{"linux/amd64"} and return &slice
so callers expecting *[]string receive the documented default platform.
---
Outside diff comments:
In `@internal/pkg/api/v2alpha1/type_config.go`:
- Around line 116-130: Platform.DeepCopy() currently initializes platformCopy
with only Graph, causing scalar fields like Release and KubeVirtContainer to be
lost; update Platform.DeepCopy to copy all scalar and value fields from the
receiver (e.g., set platformCopy.Release = p.Release and
platformCopy.KubeVirtContainer = p.KubeVirtContainer) in addition to the
existing deep copies of slices (Channels, Architectures, Platforms) so the
returned copy is complete; locate the DeepCopy method on type Platform and add
assignments for any other scalar/value fields to mirror the source struct.
---
Nitpick comments:
In `@internal/pkg/operator/catalog_handler_test.go`:
- Line 43: Add a test entry in catalog_handler_test.go that calls
handler.getRelatedImagesFromCatalog with a non-nil platforms argument (e.g., a
slice like []string{"linux/amd64"}) in addition to the existing nil-case; call
the same function used in the diff (getRelatedImagesFromCatalog) with that
non-nil platforms and copyImageSchemaMap, assert err == nil, and verify that
each returned related image in res has its Platforms (or equivalent field) set
to the same non-nil slice (or contains the provided platforms), thereby
validating propagation of the platforms parameter through the function.
In `@internal/pkg/operator/filtered_collector_test.go`:
- Around line 1052-1061: The mock getRelatedImagesFromCatalog in MockHandler
ignores the new platforms parameter; update it to validate the forwarded
platforms pointer (the platforms *[]string argument) before returning results —
e.g., check platforms is not nil and that *platforms matches the expected slice
(or contains required entries) and return an error (or fail) when it does not so
tests detect propagation failures; keep the existing relatedImages behavior
otherwise and leave copyImageSchemaMap untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c8d1649-2c7f-439b-a8ae-62be25a36dee
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/pkg/additional/local_stored_collector.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_internal.gointernal/pkg/api/v2alpha1/type_platform.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/batch/concurrent_chan_worker.gointernal/pkg/helm/local_stored_collector.gointernal/pkg/mirror/mirror.gointernal/pkg/mirror/mirror_test.gointernal/pkg/mirror/options.gointernal/pkg/operator/catalog_handler.gointernal/pkg/operator/catalog_handler_test.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/operator/interface.gointernal/pkg/release/local_stored_collector.go
Comment on lines
+359
to
+368
| for _, platform := range strings.Split(multiArch, ",") { | ||
| platform = strings.TrimSpace(platform) | ||
| parts := strings.Split(platform, "/") | ||
| if len(parts) != 2 { | ||
| return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch) | ||
| } | ||
| platforms = append(platforms, copy.InstancePlatformFilter{ | ||
| OS: parts[0], | ||
| Architecture: parts[1], | ||
| }) |
There was a problem hiding this comment.
Harden platform token validation before constructing filters.
Line 362 only validates len(parts) == 2; empty or whitespace OS/ARCH values still pass. Reject invalid tokens at this boundary to avoid propagating malformed platform filters.
Proposed fix
- for _, platform := range strings.Split(multiArch, ",") {
+ for _, platform := range strings.Split(multiArch, ",") {
platform = strings.TrimSpace(platform)
- parts := strings.Split(platform, "/")
- if len(parts) != 2 {
+ parts := strings.SplitN(platform, "/", 2)
+ if len(parts) != 2 {
return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
}
+ osPart := strings.TrimSpace(parts[0])
+ archPart := strings.TrimSpace(parts[1])
+ if osPart == "" || archPart == "" {
+ return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: expected non-empty os/architecture", platform)
+ }
platforms = append(platforms, copy.InstancePlatformFilter{
- OS: parts[0],
- Architecture: parts[1],
+ OS: osPart,
+ Architecture: archPart,
})
}As per coding guidelines, “Validate at trust boundaries with allow-lists, not deny-lists.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, platform := range strings.Split(multiArch, ",") { | |
| platform = strings.TrimSpace(platform) | |
| parts := strings.Split(platform, "/") | |
| if len(parts) != 2 { | |
| return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch) | |
| } | |
| platforms = append(platforms, copy.InstancePlatformFilter{ | |
| OS: parts[0], | |
| Architecture: parts[1], | |
| }) | |
| for _, platform := range strings.Split(multiArch, ",") { | |
| platform = strings.TrimSpace(platform) | |
| parts := strings.SplitN(platform, "/", 2) | |
| if len(parts) != 2 { | |
| return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch) | |
| } | |
| osPart := strings.TrimSpace(parts[0]) | |
| archPart := strings.TrimSpace(parts[1]) | |
| if osPart == "" || archPart == "" { | |
| return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: expected non-empty os/architecture", platform) | |
| } | |
| platforms = append(platforms, copy.InstancePlatformFilter{ | |
| OS: osPart, | |
| Architecture: archPart, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pkg/mirror/mirror.go` around lines 359 - 368, The loop that parses
multiArch tokens (using variable multiArch and splitting into parts) currently
only checks len(parts)==2 and can accept empty or whitespace OS/ARCH; update the
validation in that block (the code that builds copy.InstancePlatformFilter) to
trim and reject empty parts and enforce an allow-list pattern for OS and
Architecture (e.g., only letters/numbers, '-', '_' and dots) before appending to
platforms; on invalid tokens return the same error path (the fmt.Errorf
currently returned alongside copy.CopySystemImage) so malformed tokens are
rejected at this trust boundary.
Source: Coding guidelines
Comment on lines
+56
to
+73
| func (o LocalStorageCollector) getPlatformFilters() *[]string { | ||
| // Use new Platforms field if available | ||
| if len(o.Config.Mirror.Platform.Platforms) > 0 { | ||
| return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms) | ||
| } | ||
|
|
||
| // Fall back to deprecated Architectures field (assume linux) | ||
| if len(o.Config.Mirror.Platform.Architectures) > 0 { | ||
| platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures)) | ||
| for i, arch := range o.Config.Mirror.Platform.Architectures { | ||
| platformStrs[i] = "linux/" + arch | ||
| } | ||
| return &platformStrs | ||
| } | ||
|
|
||
| // No platforms specified - mirror all | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Default platform behavior conflicts with config contract.
When both Platforms and Architectures are empty, Line 72 returns nil (mirror all). But Platform docs declare the default should be linux/amd64. This mismatch can unexpectedly mirror every platform.
Proposed fix
func (o LocalStorageCollector) getPlatformFilters() *[]string {
@@
// No platforms specified - mirror all
- return nil
+ defaultPlatform := []string{"linux/" + v2alpha1.DefaultPlatformArchitecture}
+ return &defaultPlatform
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (o LocalStorageCollector) getPlatformFilters() *[]string { | |
| // Use new Platforms field if available | |
| if len(o.Config.Mirror.Platform.Platforms) > 0 { | |
| return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms) | |
| } | |
| // Fall back to deprecated Architectures field (assume linux) | |
| if len(o.Config.Mirror.Platform.Architectures) > 0 { | |
| platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures)) | |
| for i, arch := range o.Config.Mirror.Platform.Architectures { | |
| platformStrs[i] = "linux/" + arch | |
| } | |
| return &platformStrs | |
| } | |
| // No platforms specified - mirror all | |
| return nil | |
| } | |
| func (o LocalStorageCollector) getPlatformFilters() *[]string { | |
| // Use new Platforms field if available | |
| if len(o.Config.Mirror.Platform.Platforms) > 0 { | |
| return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms) | |
| } | |
| // Fall back to deprecated Architectures field (assume linux) | |
| if len(o.Config.Mirror.Platform.Architectures) > 0 { | |
| platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures)) | |
| for i, arch := range o.Config.Mirror.Platform.Architectures { | |
| platformStrs[i] = "linux/" + arch | |
| } | |
| return &platformStrs | |
| } | |
| // No platforms specified - mirror all | |
| defaultPlatform := []string{"linux/" + v2alpha1.DefaultPlatformArchitecture} | |
| return &defaultPlatform | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pkg/release/local_stored_collector.go` around lines 56 - 73,
getPlatformFilters in LocalStorageCollector currently returns nil when both
o.Config.Mirror.Platform.Platforms and Architectures are empty, causing "mirror
all" behavior; change it to return a pointer to a slice containing the default
"linux/amd64" string instead. Specifically, in getPlatformFilters (and where it
falls back from Architectures), replace the final "return nil" with creation of
a []string{"linux/amd64"} and return &slice so callers expecting *[]string
receive the documented default platform.
aguidirh
force-pushed
the
CLID-290
branch
2 times, most recently
from
June 8, 2026 15:46
3219c3e to
74ce905
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/pkg/mirror/mirror.go (1)
368-377:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen platform token validation at this trust boundary.
The current validation only checks
len(parts) == 2and doesn't validate the content of OS/Architecture tokens after splitting. Empty or whitespace-only values (e.g.," / ","linux/ ") can pass validation and create malformed platform filters. As per coding guidelines, input validation at trust boundaries should use allow-lists, not just structural checks.🛡️ Proposed fix to validate and sanitize platform tokens
for _, platform := range strings.Split(multiArch, ",") { platform = strings.TrimSpace(platform) - parts := strings.Split(platform, "/") + parts := strings.SplitN(platform, "/", 2) if len(parts) != 2 { return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch) } + osPart := strings.TrimSpace(parts[0]) + archPart := strings.TrimSpace(parts[1]) + if osPart == "" || archPart == "" { + return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: OS and architecture must be non-empty", platform) + } + // Validate against allow-list: alphanumeric, hyphen, underscore, dot + validPattern := regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) + if !validPattern.MatchString(osPart) || !validPattern.MatchString(archPart) { + return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: OS and architecture must contain only letters, numbers, hyphens, underscores, and dots", platform) + } platforms = append(platforms, copy.InstancePlatformFilter{ - OS: parts[0], - Architecture: parts[1], + OS: osPart, + Architecture: archPart, }) }Note: You'll need to import
"regexp"at the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/mirror/mirror.go` around lines 368 - 377, The loop that parses multiArch platforms accepts tokens with only a structural check (len(parts)==2) which allows empty or whitespace OS/Architecture values; update the logic in the function containing this loop (the multi-arch platform parsing block in mirror.go) to (1) Trim and validate each token and each part after splitting, (2) enforce an allow-list/regex for OS and Architecture (e.g., permit only lowercase letters, digits and hyphens/underscores) and reject empty strings, and (3) return a clear error when a token fails validation; add the required import ("regexp") and use the validated parts when building copy.InstancePlatformFilter to prevent malformed platform filters.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/pkg/mirror/mirror.go`:
- Around line 368-377: The loop that parses multiArch platforms accepts tokens
with only a structural check (len(parts)==2) which allows empty or whitespace
OS/Architecture values; update the logic in the function containing this loop
(the multi-arch platform parsing block in mirror.go) to (1) Trim and validate
each token and each part after splitting, (2) enforce an allow-list/regex for OS
and Architecture (e.g., permit only lowercase letters, digits and
hyphens/underscores) and reject empty strings, and (3) return a clear error when
a token fails validation; add the required import ("regexp") and use the
validated parts when building copy.InstancePlatformFilter to prevent malformed
platform filters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6601800d-c8a5-472a-8855-b3444425902e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/pkg/additional/local_stored_collector.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_internal.gointernal/pkg/api/v2alpha1/type_platform.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/batch/concurrent_chan_worker.gointernal/pkg/helm/local_stored_collector.gointernal/pkg/mirror/mirror.gointernal/pkg/mirror/mirror_test.gointernal/pkg/mirror/options.gointernal/pkg/operator/catalog_handler.gointernal/pkg/operator/catalog_handler_test.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/operator/interface.gointernal/pkg/release/local_stored_collector.go
✅ Files skipped from review due to trivial changes (1)
- internal/pkg/archive/image-blob-gatherer_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/pkg/operator/filtered_collector_test.go
- internal/pkg/operator/interface.go
- internal/pkg/operator/catalog_handler.go
- internal/pkg/api/v2alpha1/type_platform.go
- internal/pkg/operator/catalog_handler_test.go
- internal/pkg/batch/concurrent_chan_worker.go
- internal/pkg/additional/local_stored_collector.go
- internal/pkg/operator/filtered_collector.go
- internal/pkg/helm/local_stored_collector.go
- internal/pkg/api/v2alpha1/type_internal.go
- internal/pkg/api/v2alpha1/type_config.go
- internal/pkg/release/local_stored_collector.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
8-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve known vulnerable pinned modules before merge.
go.modstill pins versions flagged as HIGH severity:
- Line 8:
github.com/distribution/distribution/v3 v3.0.0- Line 63:
github.com/containerd/containerd v1.7.29- Line 138:
github.com/moby/spdystream v0.5.0Please bump to patched releases and refresh
go.sum(go get ... && go mod tidy) to satisfy the repo’s dependency security policy.#!/bin/bash set -euo pipefail echo "== Current versions in go.mod ==" rg -n 'github.com/distribution/distribution/v3|github.com/containerd/containerd|github.com/moby/spdystream' go.mod echo echo "== OSV check: distribution/v3 ==" curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \ -d '{"package":{"ecosystem":"Go","name":"github.com/distribution/distribution/v3"}}' \ | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}' echo echo "== OSV check: containerd ==" curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \ -d '{"package":{"ecosystem":"Go","name":"github.com/containerd/containerd"}}' \ | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}' echo echo "== OSV check: moby/spdystream ==" curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \ -d '{"package":{"ecosystem":"Go","name":"github.com/moby/spdystream"}}' \ | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}'Also applies to: 63-63, 138-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 8, go.mod pins vulnerable module versions for github.com/distribution/distribution/v3, github.com/containerd/containerd and github.com/moby/spdystream; update each to the latest patched release, run “go get github.com/distribution/distribution/v3@<patched> github.com/containerd/containerd@<patched> github.com/moby/spdystream@<patched>” (replace <patched> with the secure versions) and then run “go mod tidy” to refresh go.sum; ensure the updated module lines (github.com/distribution/distribution/v3, github.com/containerd/containerd, github.com/moby/spdystream) in go.mod reflect the new versions and commit the resulting go.mod and go.sum changes.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@go.mod`:
- Line 8: go.mod pins vulnerable module versions for
github.com/distribution/distribution/v3, github.com/containerd/containerd and
github.com/moby/spdystream; update each to the latest patched release, run “go
get github.com/distribution/distribution/v3@<patched>
github.com/containerd/containerd@<patched> github.com/moby/spdystream@<patched>”
(replace <patched> with the secure versions) and then run “go mod tidy” to
refresh go.sum; ensure the updated module lines
(github.com/distribution/distribution/v3, github.com/containerd/containerd,
github.com/moby/spdystream) in go.mod reflect the new versions and commit the
resulting go.mod and go.sum changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc19da77-8286-4674-9b7f-072db74d31b2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/pkg/additional/local_stored_collector.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_internal.gointernal/pkg/api/v2alpha1/type_platform.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/batch/concurrent_chan_worker.gointernal/pkg/helm/local_stored_collector.gointernal/pkg/mirror/mirror.gointernal/pkg/mirror/mirror_test.gointernal/pkg/mirror/options.gointernal/pkg/operator/catalog_handler.gointernal/pkg/operator/catalog_handler_test.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/operator/interface.gointernal/pkg/release/local_stored_collector.go
✅ Files skipped from review due to trivial changes (1)
- internal/pkg/archive/image-blob-gatherer_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/pkg/operator/catalog_handler_test.go
- internal/pkg/operator/interface.go
- internal/pkg/mirror/options.go
- internal/pkg/operator/filtered_collector.go
- internal/pkg/additional/local_stored_collector.go
- internal/pkg/batch/concurrent_chan_worker.go
- internal/pkg/mirror/mirror_test.go
- internal/pkg/operator/filtered_collector_test.go
- internal/pkg/api/v2alpha1/type_platform.go
- internal/pkg/api/v2alpha1/type_config.go
- internal/pkg/release/local_stored_collector.go
- internal/pkg/helm/local_stored_collector.go
- internal/pkg/api/v2alpha1/type_internal.go
- internal/pkg/operator/catalog_handler.go
- internal/pkg/mirror/mirror.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/pkg/operator/filtered_collector.go (1)
251-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatalog image misses platform filter propagation
Line 306 builds the catalog
RelatedImagewithoutPlatforms, so the catalog copy path can still mirror all architectures even though Line 254 now filters related images.Suggested patch
relatedImages[componentName] = []v2alpha1.RelatedImage{ { Name: catalogName, Image: catalogImage, Type: v2alpha1.TypeOperatorCatalog, TargetTag: targetTag, TargetCatalog: op.TargetCatalog, RebuiltTag: rebuiltTag, FullCatalog: isFullCatalog(op), + Platforms: platforms, }, }Also applies to: 306-315
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/operator/filtered_collector.go` around lines 251 - 255, The catalog-related image construction is missing propagation of platform filters: after you call o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig, platforms, copyImageSchemaMap) (where platforms := v2alpha1.ConvertPlatformsToStringSlice(op.Platforms)), ensure that each RelatedImage created for the catalog copy path includes the same Platforms value (set RelatedImage.Platforms = platforms or equivalent) so the catalog copy honors op.Platforms; update the code paths that build RelatedImage instances (the block referenced around getRelatedImagesFromCatalog and the construction in the 306–315 region) to copy/set the platforms slice from the platforms variable into the RelatedImage before returning or appending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 3: The go.mod specifies Go 1.25.6 but the build sandbox uses
GO_VERSION=go1.23.5 causing mismatched toolchains; update the hack/build.sh,
hack/e2e.sh and the Dockerfile (local/go-toolset) to either install/use Go
1.25.6 (set GO_VERSION=go1.25.6 and ensure the local/go-toolset image installs
that toolchain) or make the scripts export GOTOOLCHAIN=auto so the runtime
respects the go.mod directive; change occurrences of GO_VERSION and any image
build steps that pull the older toolset, and verify createToolchain/install
steps in local/go-toolset use the 1.25.6 installer to keep builds consistent
with go.mod.
---
Duplicate comments:
In `@internal/pkg/operator/filtered_collector.go`:
- Around line 251-255: The catalog-related image construction is missing
propagation of platform filters: after you call
o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig, platforms,
copyImageSchemaMap) (where platforms :=
v2alpha1.ConvertPlatformsToStringSlice(op.Platforms)), ensure that each
RelatedImage created for the catalog copy path includes the same Platforms value
(set RelatedImage.Platforms = platforms or equivalent) so the catalog copy
honors op.Platforms; update the code paths that build RelatedImage instances
(the block referenced around getRelatedImagesFromCatalog and the construction in
the 306–315 region) to copy/set the platforms slice from the platforms variable
into the RelatedImage before returning or appending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3d59dc56-7672-41f8-a6be-eaf1d8c7f04a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.modinternal/pkg/additional/local_stored_collector.gointernal/pkg/api/v2alpha1/type_config.gointernal/pkg/api/v2alpha1/type_internal.gointernal/pkg/api/v2alpha1/type_platform.gointernal/pkg/archive/image-blob-gatherer_test.gointernal/pkg/batch/concurrent_chan_worker.gointernal/pkg/helm/local_stored_collector.gointernal/pkg/mirror/mirror.gointernal/pkg/mirror/mirror_test.gointernal/pkg/mirror/options.gointernal/pkg/operator/catalog_handler.gointernal/pkg/operator/catalog_handler_test.gointernal/pkg/operator/filtered_collector.gointernal/pkg/operator/filtered_collector_test.gointernal/pkg/operator/interface.gointernal/pkg/release/local_stored_collector.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/pkg/mirror/options.go
- internal/pkg/archive/image-blob-gatherer_test.go
- internal/pkg/additional/local_stored_collector.go
- internal/pkg/batch/concurrent_chan_worker.go
- internal/pkg/operator/catalog_handler_test.go
- internal/pkg/api/v2alpha1/type_platform.go
- internal/pkg/api/v2alpha1/type_internal.go
- internal/pkg/operator/interface.go
- internal/pkg/mirror/mirror_test.go
- internal/pkg/operator/catalog_handler.go
- internal/pkg/release/local_stored_collector.go
- internal/pkg/mirror/mirror.go
- internal/pkg/api/v2alpha1/type_config.go
adolfo-ab
suggested changes
Jun 15, 2026
adolfo-ab
left a comment
adolfo-ab
left a comment
Contributor
There was a problem hiding this comment.
Can you check if you need to also update internal/pkg/config/defaults.go and cincinnati.go?
Also, maybe you could add one or two integration tests, so that this feature is already covered.
|
|
||
| // Fall back to deprecated Architectures field (assume linux) | ||
| //nolint:staticcheck // SA1019: Architectures is deprecated but we maintain backward compatibility | ||
| if len(o.Config.Mirror.Platform.Architectures) > 0 { |
Contributor
There was a problem hiding this comment.
Since Architectures is deprecated, maybe we can add a log line mentioning it here, and asking to consider using Platform instead
Contributor
Author
There was a problem hiding this comment.
Please mark it as resolved in case there is no further suggestions for this thread.
Contributor
There was a problem hiding this comment.
It won't let me resolve this one, but feel free to do it yourself in you can or just move on as if it was
| // InstancePlatformFilter defines OS and Architecture for filtering | ||
| // multi-architecture manifest lists. This allows selecting specific | ||
| // platforms (e.g., linux/amd64, linux/arm64) when mirroring images. | ||
| type InstancePlatformFilter struct { |
Contributor
There was a problem hiding this comment.
What happens if a user only sets the OS or the Architecture? Should we check that we always have both? Otherwise we might get linux/ or /amd64, no?
Contributor
Author
There was a problem hiding this comment.
Please mark it as resolved in case there is no further suggestions for this thread.
Contributor
Author
|
/test v1-e2e |
Implement InstancePlatformFilter feature with sparse manifest support to allow users to specify OS/Architecture pairs for filtering multi-architecture images across all image types (additional images, operators, helm charts, and releases). This feature brings the platform filtering capability from skopeo (containers/image PR #2874) to oc-mirror, enabling users to mirror only specific platforms instead of all architectures. The implementation uses sparse manifests where the manifest list references all platforms but only the specified platforms have their actual layer blobs downloaded, significantly reducing disk usage and bandwidth. Platform filtering is applied during Mirror-to-Disk and Mirror-to-Mirror workflows. Disk-to-Mirror workflow uses already-filtered images from disk without re-applying platform filters. The implementation includes backward compatibility with the deprecated Platform.Architectures field and follows the same strategy as skopeo for consistency. Fixes: CLID-290 Signed-off-by: Alex Guidi <[email protected]>
…ages refactor: Replace per-image Platforms with CollectorSchema.PlatformFilters map This is a cleaner approach than the previous commit. Instead of storing platform filters on RelatedImage and CopyImageSchema (polluting image structs with filtering concerns), each collector now populates a PlatformFilters map[string][]string on CollectorSchema, keyed by image origin. The batch worker reads from this map at copy time. Note: platform filtering for release content images is not yet functional end-to-end. For it to work, the Cincinnati query must return the multi-arch payload (arch=multi) so that image-references contains manifest list digests rather than single-arch digests. This is tracked as a follow-up. Please review the TBD comments in the code — particularly in executor.go (cross-collector platform merge semantics) and filtered_collector.go (localRelatedImages approach) — as these decisions benefit from team input.
…ages fix: Address code review suggestions from adolfo-ab and coderabbitai - Add Validate() to InstancePlatformFilter to reject entries with empty OS or Architecture, preventing malformed strings like "linux/" or "/amd64" - Update ConvertPlatformsToStringSlice to skip invalid entries rather than producing malformed platform strings - Add deprecation warning log when the Architectures field is used, directing users to switch to the new platform.platforms field
…ages fix: Reduce cyclomatic complexity in collector functions to pass linter Extract helpers to bring three functions within the cyclop threshold (max 10): - ReleaseImageCollector: extract compareByOriginSourceDest sort helper - AdditionalImagesCollector: extract resolveAdditionalImageSrcDest and resolveTargetRepoTag helpers - HelmImageCollector: extract collectHelmImagesM2D and collectHelmImagesD2M helpers, reducing the main function to a simple mode dispatch
…ages fix: Enable sparse manifest support in the local cache registry Add validation.manifests.indexes.platforms: none to the local cache registry configuration so it matches the behavior of a target registry configured for sparse manifests. Without this setting, the distribution registry validates that all blobs referenced in a manifest list are present before accepting the push, causing "blob unknown to registry" errors when platform filtering leaves some architecture blobs uncopied.
…ages Refactor release platform handling to preserve backward compatibility while enabling sparse manifest filtering for users who adopt the new Platforms field. Cincinnati GetReleaseReferenceImages now branches on Platform.Platforms vs Platform.Architectures: - Platforms set: always queries Cincinnati with ?arch=multi (the only manifest list payload) and warns that the target registry must support sparse manifest lists (validation.manifests.indexes.platforms: none). - Architectures set (or defaulted to ["amd64"] by Complete()): preserves the original per-arch Cincinnati loop unchanged. The two fields are mutually exclusive — a new validateReleasePlatformFields check in Validate() returns an error if both are specified. defaults.go no longer auto-fills Architectures when Platforms is already set, preventing spurious defaults from being added alongside an explicit Platforms configuration. ReleaseImageCollector sets MultiArch="system" by default (original behavior) and overrides to "all" only when Platforms is configured, so that ensureReleaseInOCIFormat downloads all arch payloads from the multi-arch release payload and can read image-references with manifest-list component digests. RemoveSignatures=true is restored in ensureReleaseInOCIFormat: converting docker to OCI format changes the image digest, which invalidates any attached signatures. Since this copy is for metadata extraction only, signatures are not needed and keeping them would cause downstream failures.
Contributor
Author
|
Next round:
|
…ages Simplify getPlatformFilters(): the Architectures field is only for Cincinnati (?arch=<arch>), so the batch worker does not need InstancePlatforms for that path. Returning nil for the Architectures case preserves the original behavior where per-arch single-arch payloads are copied whole without platform filtering. Move the Architectures deprecation warning from getPlatformFilters() to the Cincinnati Architectures branch in GetReleaseReferenceImages, where the field is actually consumed.
Contributor
Author
|
/retest |
…ages Fix archive creation failing for M2D when platform filtering is active. When sparse manifest filtering is used, the local cache registry stores a complete manifest list index but only the blobs for the selected platforms. The archive step calls ImageManifest for every digest in the manifest list, including platforms that were never mirrored, which returns "manifest unknown" from the registry and caused the archive build to abort. Fix: skip absent platform manifests in multiArchBlobs with the same "manifest unknown" check already used in mirror.go, logging a debug message and continuing to the next platform.
…ages Fix M2D archive creation failing when sparse manifest platform filtering is active. The archive step iterates over every digest in a release manifest list to collect the blobs that need to be packaged. With sparse filtering, the local cache only stores blobs for the configured platforms — other platforms are absent. Two bugs caused the archive build to fail: 1. ImageManifest returned "manifest unknown" for absent platform digests, which was treated as a fatal error instead of an expected condition. 2. The absent platform digest was inserted into the blobs set before the manifest check, so the archive later tried to locate a blob file that was never stored. Fix: skip absent platform digests that are not in the image's allowed platform set (CollectorSchema.PlatformFilters[img.Origin]). The digest is only added to the blobs set after a successful manifest fetch. Platforms that are in the allowed set but return "manifest unknown" still fail — that indicates a genuine error. To make PlatformFilters available to the archive builder without a separate setter or interface change, BuildArchive now accepts a CollectorSchema instead of a plain image slice, and the batch worker propagates PlatformFilters into the returned CollectorSchema.
Contributor
Author
|
/retest |
aguidirh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold I will unhold it as soon as I send the integration tests commit. |
openshift-ci
Bot
added
the
do-not-merge/hold
adolfo-ab
reviewed
Jul 16, 2026
adolfo-ab
left a comment
adolfo-ab
left a comment
Contributor
There was a problem hiding this comment.
Currently, if I understand correctly, if a user writes a platform that doesn't exist in the ISC, we always continue the mirroring process.
In the case that the image is a manifest list but doesn't contain the architecture defined in the ISC, we retry ignoring the platform filter. In other cases (typo in the platform, or the image being single-arch) we just ignore the filter without warning.
IMHO, if the user defines a platform in the ISC, we can assume that he wants that specific architecture and nothing else, and if that platform doesn't exist for that image for whatever reason, we should just fail for that image, print a warning, and continue with the next image in the ISC, instead of retrying without filter or ignoring the filter altogether.
Wdyt @aguidirh ?
adolfo-ab
reviewed
Jul 17, 2026
| collectorSchema.CopyImageSchemaMap = operatorImgs.CopyImageSchemaMap | ||
| collectorSchema.CatalogToFBCMap = operatorImgs.CatalogToFBCMap | ||
|
|
||
| // TODO: if images are shared between types, maps.Copy will override platform filters for |
Contributor
There was a problem hiding this comment.
Maybe we can perform this merge in removeDuplicatedImages, which is called once per collector in CollectAll. That way we can have Platforms in CopyImageSchema, populated by each collector and we don't need the PlatformFilter map in CollectorSchema. I think this would simplify the PR, although it's similar to your first approach so maybe I'm missing something here.
|
|
||
| // Priority 2: Fall back to MultiArch flag (for CLI direct usage or default "all") | ||
| if len(opts.MultiArch) > 0 && opts.All { | ||
| return copy.CopySystemImage, nil, fmt.Errorf("cannot use --all and --multi-arch flags together") |
Contributor
There was a problem hiding this comment.
These error messages make it seem like oc-mirror had --all and --multi-arch flags, which might confuse the user, since these are skopeo CLI flags.
…ages Add integration tests and multi-arch release image builder infrastructure: - sparse_manifest_test.go: 10 Ginkgo specs covering all 7 manual test scenarios (M2M filtered, M2M backward compat, M2D+D2M filtered, M2D+D2M backward compat, config validation, deprecated architectures + release channel, negative test) - sparse_manifest_helpers_test.go: test helpers for platform blob presence assertions - ISC fixtures: 8 image set config files for the sparse manifest test scenarios - registry-config-sparse.yaml: distribution registry config with platforms:none - create-release-multi.sh: build and push the 4-arch fake OCP release manifest list - generate-release-signature-multi.sh: GPG-sign both single and multi-arch releases with a shared key so a single release-pk.asc validates all signatures - Per-arch OCI image config blobs (arm64, s390x, ppc64le) alongside existing amd64 - config-multi.json: 4-placeholder manifest template for multi-arch builds - release-payload-multi/index.json + oci-layout: committed OCI manifest list - Makefile: add sign-multi cosign target - testdata/keys: updated GPG public key and signatures for both release images - helpers_test.go: copy all signature files in setupWorkDir (not just one)
|
@aguidirh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements sparse manifest platform filtering for multi-architecture images in oc-mirror. Users can now specify OS/Architecture pairs to filter which platforms are mirrored, significantly reducing disk usage and bandwidth requirements.
The implementation brings the platform filtering capability from containers/image (containers/container-libs PR #656) to oc-mirror. It uses sparse manifests where the manifest list references all platforms but only the specified platforms have their actual layer blobs downloaded.
Platform filtering works across all image types:
architecturesfield)Github / Jira issue: CLID-290
Type of change
How Has This Been Tested?
Sparse Manifest Platform Filtering — Manual Test Plan
What the feature does
When
platform.platformsis set in the ISC, oc-mirror copies only the requestedplatform blobs from multi-arch manifest lists. The manifest list index in the target
registry is preserved intact, but only the specified architecture blobs are present.
This requires the target registry to have blob-presence validation disabled
(
validation.manifests.indexes.platforms: none).Prerequisites
1. Registries
Registry for filtered ISC run (
isc-platforms.yaml) —localhost:5000:Registry for baseline ISC run (
isc.yaml) —localhost:6000:Both registries use the same config (which includes
platforms: none). The differenceis which ISC is mirrored to each one.
2. ISC files used
alex-tests/alex-isc/isc-platforms.yamlplatformsfield — primary test ISCalex-tests/alex-isc/isc.yamlplatforms— backward compat + comparison baseline3. Verification script
Test Suite
TEST 1 — New Platforms field: all image types in one run (M2M)
ISC:
alex-tests/alex-isc/isc-platforms.yamlExpected log: Warning printed before copying:
Verify — report mode:
What to look for in the report output:
openshift/release-images(top-level release payload)openshift/release:4.18.1-multi-*(component images)rh_ee_aguidi/multi-platform-containerrh_ee_aguidi/empty-imagestefanprodan/podinfoprojectsigstore/cosignedBoth
openshift/release-images(top-level manifest list from Cincinnati) and theopenshift/releasecomponent images (extracted from image-references) must show onlythe configured platforms. The verify script checks both.
TEST 2 — Backward compat: old behavior unchanged (M2M)
ISC:
alex-tests/alex-isc/isc.yaml(noplatformsfield anywhere)Expected:
single-manifest (no index)— Cincinnati called with?arch=amd64, only single-arch amd64 payload[N present, 0 missing])Verify:
Cleanup between M2M and M2D runs
Before running M2D tests, free up disk space:
TEST 3 — New Platforms field: M2D then D2M
Step 1 — M2D:
rm -rf ~/.oc-mirror ./bin/oc-mirror --v2 \ --config alex-tests/alex-isc/isc-platforms.yaml \ file://alex-tests/clid-290-platforms/m2d-outputStep 2 — Prepare for D2M (simulate disconnected environment):
rm -rf ~/.oc-mirror mkdir -p alex-tests/clid-290-platforms/d2m-input mv alex-tests/clid-290-platforms/m2d-output/mirror_000001.tar \ alex-tests/clid-290-platforms/d2m-input/ rm -rf alex-tests/clid-290-platforms/m2d-output/working-dirStep 3 — D2M:
Verify — same checks as TEST 1:
Expected: Identical results to TEST 1. Platform filtering preserved through archive.
Check logs to confirm no external HTTP calls.
TEST 4 — Backward compat: M2D then D2M with isc.yaml
Step 1 — M2D:
rm -rf ~/.oc-mirror ./bin/oc-mirror --v2 \ --config alex-tests/alex-isc/isc.yaml \ file://alex-tests/clid-290/m2d-outputStep 2 — Prepare:
rm -rf ~/.oc-mirror mkdir -p alex-tests/clid-290/d2m-input mv alex-tests/clid-290/m2d-output/mirror_000001.tar \ alex-tests/clid-290/d2m-input/ rm -rf alex-tests/clid-290/m2d-output/working-dirStep 3 — D2M:
Verify:
What to look for:
single-manifest (no index)— single-arch amd64[N present, 0 missing]— all platforms presentTEST 5 — Validation error: both Platforms and Architectures set
Create
/tmp/invalid-isc.yaml:Expected: oc-mirror exits immediately with:
TEST 6 — Deprecated Architectures field with
multivalueCreate
/tmp/isc-archs-multi.yaml:Expected:
platform.architectures is deprecated; use platform.platforms insteadmultiin architectures)multi-platform-containerrepo exists in:6000Verify:
TEST 7 — Negative: target registry without sparse manifest support
Start a plain registry (no
platforms: none):Expected:
Summary Table
Known Limitations
Release component images may remain single-arch. The top-level release image is
a manifest list and is correctly filtered. Component images from
image-referencesmay still be single-arch depending on the OCP version.
D2M can only copy what M2D stored. Requesting a platform in D2M that wasn't
stored in M2D results in "no instances found" fallback — the whole image is copied.
Single-arch operator/helm images. Many operator bundle images are published
as single-arch (amd64 only). Platform filtering is a no-op for these; they are
always copied whole via the fallback retry.
Script to verify archs on registries:
Summary by CodeRabbit