OCPBUGS-85585: apply registry overrides to component images in CPO sub-resources#8509
OCPBUGS-85585: apply registry overrides to component images in CPO sub-resources#8509raelga wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe controller now constructs the image provider with registry override data by calling 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
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raelga The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8509 +/- ##
==========================================
+ Coverage 40.00% 40.01% +0.01%
==========================================
Files 751 751
Lines 92838 92880 +42
==========================================
+ Hits 37137 37164 +27
- Misses 53014 53024 +10
- Partials 2687 2692 +5
... and 1 file with indirect coverage changes
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
from
b0ada44 to
009bb28
Compare
When CPO creates sub-resources (e.g. capi-provider deployments), it injects init containers like availability-prober using image references from the release image. These references were not being remapped by --registry-overrides, causing the init containers to reference the original registry (e.g. quay.io) instead of the override target. Add NewWithRegistryOverrides to the imageprovider package that clones the component images map and applies registry overrides using safe prefix matching (source+"/") to prevent subdomain false matches. Use it when creating the releaseImageProvider in the HostedControlPlane reconciler. Bug: https://redhat.atlassian.net/browse/OCPBUGS-85585
raelga
force-pushed
the
raelga/fix-cpo-registry-overrides-init-containers
branch
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. |
3 participants
Summary
CPO does not propagate
--registry-overridesto init containers it injects in HCP sub-resources. When CPO creates deployments likecapi-provider, it injectsavailability-proberinit containers using the originalquay.ioimage reference instead of the overridden ACR path.Root cause
In
hostedcontrolplane_controller.go:1106, thereleaseImageProvideris created withimageprovider.New(releaseImage)which returns component images as-is from the release image stream. The registry overrides available viar.ReleaseProvider.GetRegistryOverrides()are not applied to these component images.Fix
Add
NewWithRegistryOverrides()to theimageproviderpackage that applies registry overrides to all component images when creating the provider. Use it in the HostedControlPlane reconciler instead ofimageprovider.New().This ensures when CPO calls
GetImage("availability-prober")(or any other component), the returned image reference has the registry override applied.Impact
Without this fix, any ValidatingAdmissionPolicy that restricts container images to approved registries will block CPO init containers, causing HCP cluster creation to hang indefinitely.
Test plan
--registry-overridesand a VAP in Deny modeRelated
Summary by CodeRabbit
New Features
Tests