OCPBUGS-85585: tighten registry override matching to strict longest-prefix across release-image consumers#8509
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughThe PR adds a deterministic registry-prefix remapping utility and applies it when building release image providers. imageprovider.New delegates to NewWithRegistryOverrides(releaseImage, registryOverrides) which clones and rewrites component image references using registryoverride.Replace. The registry mirror decorator was updated to use the new helper. Unit tests cover Replace and NewWithRegistryOverrides behaviors and idempotency. Two controller reconciliation tests were updated to mock non-empty registry overrides returned by GetRegistryOverrides(). Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant RP as ReleaseProvider
participant IP as ImageProvider
participant K as Kubernetes API
C->>RP: GetReleaseImage()
C->>RP: GetRegistryOverrides()
RP-->>C: releaseImage, registryOverrides
C->>IP: NewWithRegistryOverrides(releaseImage, registryOverrides)
IP-->>C: SimpleReleaseImageProvider (componentsImages remapped)
C->>K: Reconcile components using provider (image references)
K-->>C: Reconciliation status
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go`:
- Around line 200-277: Add a unit test covering the registry-prefix edge case
where an override key like "quay.io" should not match a hostname subdomain
"quay.io.example.com": add a new t.Run in imageprovider_test.go that constructs
a release image with a component image
"quay.io.example.com/namespace/image:tag", supplies an overrides map containing
"quay.io" -> "mirror.example.com", calls NewWithRegistryOverrides(releaseImage,
overrides) and asserts provider.GetImage("component") remains the original
"quay.io.example.com/namespace/image:tag". This ensures the matching logic used
by NewWithRegistryOverrides/GetImage treats registry keys as exact registry
hostnames (not substrings) and prevents incorrect remapping of subdomains.
In
`@control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.go`:
- Line 70: The map returned by releaseImage.ComponentImages() (assigned to
images) is being modified directly, which can mutate the releaseImage internal
state; clone that map into a new map[string]string (copying all key/value pairs)
before performing any modifications in imageprovider.go (where images is
used/updated) so all changes occur on the cloned map and the original
releaseImage remains unmodified.
- Around line 74-76: The prefix check using strings.HasPrefix(image, source) can
match partial hostnames; update the condition in the loop that sets images[key]
so it only treats source as a match when it equals the registry component
exactly or is followed by a '/' (e.g. use a predicate like image == source or
strings.HasPrefix(image, source+"/") instead of plain HasPrefix). Modify the if
that currently reads strings.HasPrefix(image, source) to this stricter check
before performing images[key] = strings.Replace(image, source, target, 1) so
only full registry hosts are replaced.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d098d99a-9102-4bfb-b08a-fda47dfc0431
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go
|
/area control-plane-operator |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8509 +/- ##
==========================================
+ Coverage 41.88% 41.90% +0.01%
==========================================
Files 759 760 +1
Lines 94155 94177 +22
==========================================
+ Hits 39434 39461 +27
+ Misses 51961 51956 -5
Partials 2760 2760
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go (1)
200-294: ⚡ Quick winAdd test case for registry port number handling.
The test suite comprehensively covers main scenarios, including the subdomain non-match case. All tests follow best practices with proper "When ... should ..." naming, gomega assertions, and parallel execution.
One scenario not currently tested: registry hostnames with explicit port numbers. The implementation handles this correctly as long as the override key includes the port (e.g.,
"registry.example.com:5000": "mirror.example.com"will successfully matchregistry.example.com:5000/namespace/image). Consider adding a test to document this expected behavior:t.Run("When registry has port number, override matching includes the port", func(t *testing.T) { t.Parallel() g := NewWithT(t) releaseImage := newTestReleaseImage(map[string]string{ "component": "registry.example.com:5000/namespace/image:tag", }) overrides := map[string]string{ "registry.example.com:5000": "mirror.example.com", } provider := NewWithRegistryOverrides(releaseImage, overrides) g.Expect(provider.GetImage("component")).To(Equal( "mirror.example.com/namespace/image:tag")) })🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go` around lines 200 - 294, Add a test case to TestNewWithRegistryOverrides that verifies registry hostnames with ports are matched when the override key includes the port: create a subtest using newTestReleaseImage with an image like "registry.example.com:5000/namespace/image:tag", supply overrides map with key "registry.example.com:5000" and value "mirror.example.com", construct provider via NewWithRegistryOverrides and assert provider.GetImage("component") returns "mirror.example.com/namespace/image:tag".
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go`:
- Around line 200-294: Add a test case to TestNewWithRegistryOverrides that
verifies registry hostnames with ports are matched when the override key
includes the port: create a subtest using newTestReleaseImage with an image like
"registry.example.com:5000/namespace/image:tag", supply overrides map with key
"registry.example.com:5000" and value "mirror.example.com", construct provider
via NewWithRegistryOverrides and assert provider.GetImage("component") returns
"mirror.example.com/namespace/image:tag".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 451a24c9-3b71-43db-a6dc-02b6393dbd55
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.go
raelga
force-pushed
the
raelga/fix-cpo-registry-overrides-init-containers
branch
2 times, most recently
from
009bb28 to
6f542db
Compare
|
/retitle OCPBUGS-85585: apply registry overrides to component images in CPO sub-resources |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@raelga: This pull request references Jira Issue OCPBUGS-85585, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-85585, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
…lper Three small refactors that align this provider with the decorator change in the previous commit and address review feedback on openshift#8509: * New() now delegates to NewWithRegistryOverrides(releaseImage, nil) instead of duplicating the field initialisation. registryoverride.Replace is a no-op for a nil override map, so callers of New() see exactly the same component images as before. * NewWithRegistryOverrides() uses maps.Clone (Go 1.25+, already required by go.mod) to copy ComponentImages() and registryoverride.Replace to remap each image. The inline duplicate-detection loop is gone. * The matching semantics now match RegistryMirrorProviderDecorator: strict slash-boundary prefix matching with longest-prefix-wins, instead of first-match-in-map-iteration-order. As a small defensive bonus, New() now also returns a SimpleReleaseImageProvider that owns a private copy of ComponentImages(), rather than aliasing the map embedded in the ReleaseImage. Related: OCPBUGS-85585 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… overrides Cover the two properties highlighted in review feedback on openshift#8509: * NewWithRegistryOverrides must not mutate either its overrides argument or the source release image's ComponentImages map. This is the contract that lets multiple callers share the same release image and overrides map without surprises. * Applying the same overrides twice must be a no-op (idempotency). Regressions here would manifest as compounding rewrites, e.g. mirror.example.com/quay-cache/mirror.example.com/quay-cache/... Also adds a focused longest-prefix-wins assertion at this layer (the shared helper already has full coverage for the matching algorithm, but exercising it through the imageprovider keeps the layers honest). Related: OCPBUGS-85585 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er tests
Flip the two registry-override mocks added by the previous commits from
map[string]string{} to a non-empty override so the full Reconcile path
actually walks the override-application logic at
hostedcontrolplane_controller.go:1106 (and the related propagation through
configoperatorv2.NewComponent and the ignitionserver --registry-overrides
flag).
The release image fixture has no images matching the override key, so the
existing semantic assertions of these tests are unchanged: this is purely
added coverage of the override code path through reconcile, matching the
convention already used at lines 176 and 1040 of the same file. Addresses
review feedback on openshift#8509.
Related: OCPBUGS-85585
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
openshift-ci
Bot
added
the
area/hypershift-operator
|
@raelga: This pull request references Jira Issue OCPBUGS-85585, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go (1)
730-731: ⚡ Quick winUse a matching override key so this path validates real remapping.
On Line 731 and Line 816, the key
"registry"is effectively a non-match for typical release image refs, so these tests still mostly cover the no-op path. Prefer a realistic key (for examplequay.io) and assert at least one rewritten image path in the reconciled output.Also applies to: 815-816
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go` around lines 730 - 731, The test uses mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().GetRegistryOverrides().Return(map[string]string{"registry":"override"}) which uses a non-matching key and therefore exercises the no-op path; change the map key to a realistic registry host (e.g. "quay.io" or "registry.redhat.io") so the remapping code is exercised, then update the test assertions in the same test that call the reconciler to check that at least one image string in the reconciled output has been rewritten (assert that an expected original prefix like "quay.io" is replaced with "override" in the returned image list), leaving the mock call name mockedProviderWithOpenshiftImageRegistryOverrides and its GetRegistryOverrides() usage intact.
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go`:
- Around line 730-731: The test uses
mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().GetRegistryOverrides().Return(map[string]string{"registry":"override"})
which uses a non-matching key and therefore exercises the no-op path; change the
map key to a realistic registry host (e.g. "quay.io" or "registry.redhat.io") so
the remapping code is exercised, then update the test assertions in the same
test that call the reconciler to check that at least one image string in the
reconciled output has been rewritten (assert that an expected original prefix
like "quay.io" is replaced with "override" in the returned image list), leaving
the mock call name mockedProviderWithOpenshiftImageRegistryOverrides and its
GetRegistryOverrides() usage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dcb45b87-cb18-487c-9119-6692ce758ff6
📒 Files selected for processing (6)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.gocontrol-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/util/registryoverride/registryoverride.gosupport/util/registryoverride/registryoverride_test.go
raelga
changed the title
|
Scheduling tests matching the |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee, raelga 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
|
/verified by unit |
openshift-ci-robot
added
the
verified
|
@muraee: This PR has been marked as verified by 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. |
|
@raelga: all tests passed! 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. |
|
@raelga: Jira Issue Verification Checks: Jira Issue OCPBUGS-85585 Jira Issue OCPBUGS-85585 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-06-22-235733 |
…ROSLSRE-1318) The latest HyperShift operator image includes a regression in registryoverride.Replace (openshift/hypershift#8509) that breaks repository-level --registry-overrides for digest-based images. This causes all CAPI and component image rewrites to fail in environments using ACR mirrors, blocking cluster creation. Pin to the known-good build (a101e669, 2026-06-05) until the upstream fix (openshift/hypershift#8824, OCPBUGS-92034) merges and a new image is published.
|
/cherrypick release-4.22 release-4.21 release-4.20 |
|
@avollmer-redhat: new pull request created: #8873 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 kubernetes-sigs/prow repository. |
… containers Backports openshift#8509 and openshift#8824 to release-4.22: 1. Add support/util/registryoverride package with strict longest-prefix matching that correctly handles digest (@sha256:) and tag (:) separators, preventing false substring matches (e.g. "quay.io" matching "quay.io.example.com"). 2. Fix RegistryMirrorProviderDecorator.Lookup to use registryoverride.Replace instead of strings.Replace, eliminating the original substring-match bug. 3. Add imageprovider.NewWithRegistryOverrides to apply registry overrides to all component images at provider creation time, ensuring init containers (availability-prober) and other CPO sub-resources use overridden images. 4. Wire NewWithRegistryOverrides into the HCP controller reconcile loop so the control-plane release image provider applies overrides. Without this fix, CPO-managed init containers (e.g. availability-prober) retain original registry references, causing ValidatingAdmissionPolicies in Deny mode to block HCP creation in environments that restrict image sources.
… containers Backports openshift#8509 and openshift#8824 to release-4.21: 1. Add support/util/registryoverride package with strict longest-prefix matching that correctly handles digest (@sha256:) and tag (:) separators, preventing false substring matches (e.g. "quay.io" matching "quay.io.example.com"). 2. Fix RegistryMirrorProviderDecorator.Lookup to use registryoverride.Replace instead of strings.Replace, eliminating the original substring-match bug. 3. Add imageprovider.NewWithRegistryOverrides to apply registry overrides to all component images at provider creation time, ensuring init containers (availability-prober) and other CPO sub-resources use overridden images. 4. Wire NewWithRegistryOverrides into the HCP controller reconcile loop so the control-plane release image provider applies overrides. Without this fix, CPO-managed init containers (e.g. availability-prober) retain original registry references, causing ValidatingAdmissionPolicies in Deny mode to block HCP creation in environments that restrict image sources.
… containers Backports openshift#8509 and openshift#8824 to release-4.20: 1. Add support/util/registryoverride package with strict longest-prefix matching that correctly handles digest (@sha256:) and tag (:) separators, preventing false substring matches (e.g. "quay.io" matching "quay.io.example.com"). 2. Fix RegistryMirrorProviderDecorator.Lookup to use registryoverride.Replace instead of strings.Replace, eliminating the original substring-match bug. 3. Add imageprovider.NewWithRegistryOverrides to apply registry overrides to all component images at provider creation time, ensuring init containers (availability-prober) and other CPO sub-resources use overridden images. 4. Wire NewWithRegistryOverrides into the HCP controller reconcile loop so the control-plane release image provider applies overrides. Without this fix, CPO-managed init containers (e.g. availability-prober) retain original registry references, causing ValidatingAdmissionPolicies in Deny mode to block HCP creation in environments that restrict image sources.
…t containers Combined manual backport of openshift#8509 and openshift#8824 to release-4.21. Introduces strict longest-prefix registry override matching and wires overrides into CPO init container image resolution.
…t containers Combined manual backport of openshift#8509 and openshift#8824 to release-4.20. Introduces strict longest-prefix registry override matching and wires overrides into CPO init container image resolution.
…t containers Combined manual backport of openshift#8509 and openshift#8824 to release-4.22. Introduces strict longest-prefix registry override matching and wires overrides into CPO init container image resolution.
8 participants
Summary
The
RegistryMirrorProviderDecoratorused byLookup()rewrites everyImageStream tag with a bare
strings.Replace(image, source, target, 1). Thismatches anywhere in the image reference, so:
quay.ioinside
quay.io.example.com),sub-resources (e.g. the
availability-proberinit container injected intocapi-providerand other deployments) still pointing at the originalregistry.
Downstream, this breaks clusters where a ValidatingAdmissionPolicy in Deny
mode only allows images from approved registries: HCP creation hangs
indefinitely.
Root cause
support/releaseinfo/registry_mirror_provider.goran an unboundedstrings.Replaceper(source, target)pair across the entire image string.Match boundaries were neither anchored to the start of the reference nor to a
path separator, and iteration order over the override map was undefined, so
which override "won" depended on Go's randomized map iteration.
Fix
Extract a shared
registryoverride.Replace(image, overrides)helper atsupport/util/registryoverride/and use it from both:RegistryMirrorProviderDecorator.Lookup()(the actual bug fix), andimageprovider.NewWithRegistryOverrides()(defense-in-depth: callers thatconstruct a provider directly still get the same semantics).
The helper:
image == sourceorimagehas prefixsource + "/"(no substring/false-host matches),
iteration order),
imageprovider.New(releaseImage)now delegates toNewWithRegistryOverrides(releaseImage, nil), and the controller wiring athostedcontrolplane_controller.go:1106keeps the explicitNewWithRegistryOverrides(..., r.ReleaseProvider.GetRegistryOverrides())call so the override path is exercised even when callers don't go through
Lookup()first.Impact
capi-provider,ignition server, etc.) now reliably honor
--registry-overrides.environments.
pre-existing latent bug) are also fixed.
Test plan
registryoverride.Replacecovering 12 cases incl. boundary,empty overrides, longest-prefix, no mutation of the overrides map.
imageprovider_test.gocovers no-overrides, no-match, single-match,multi-override, longest-prefix-wins, mutation safety (release-image and
overrides untouched), and idempotency (no double-apply when image was
already mirrored).
hostedcontrolplane_controller_test.go(TestEventHandling,TestNonReadyInfraTriggersRequeueAfter) now exercise the override pathwith non-empty overrides through full reconcile.
--registry-overrides+ VAP in Deny mode (manual).Related