Skip to content

OCPBUGS-85585: apply registry overrides to component images in CPO sub-resources #8509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raelga wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-85585: apply registry overrides to component images in CPO sub-resources #8509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,7 @@ func (r *HostedControlPlaneReconciler) update(ctx context.Context, hostedControl
}

userReleaseImageProvider := imageprovider.New(userReleaseImage)
releaseImageProvider := imageprovider.New(releaseImage)
releaseImageProvider := imageprovider.NewWithRegistryOverrides(releaseImage, r.ReleaseProvider.GetRegistryOverrides())
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

Hey, I traced the image flow and I think the releaseImage here already has overrides applied. When r.ReleaseProvider.Lookup() runs at line 1070, it goes through the RegistryMirrorProviderDecorator (wired up at main.go:308-314) which does strings.Replace on all ImageStream tags — including the static ones like availability-prober from StaticProviderDecorator.

So by this point, releaseImage.ComponentImages() should already return overridden refs. Calling NewWithRegistryOverrides again would try to match the original prefix (e.g. quay.io) but it's already been replaced, so it's effectively a no-op.

Do you have logs or a specific cluster config where you saw the un-overridden image? That would help nail down if the gap is really here or maybe further up the chain — e.g. ControlPlaneOperatorImageAnnotation bypassing the release provider entirely (support/util/util.go:477).


var errs []error
if err := r.reconcileCPOV2(ctx, hostedControlPlane, infraStatus, releaseImageProvider, userReleaseImageProvider); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,8 @@ func TestEventHandling(t *testing.T) {
mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().
Lookup(gomock.Any(), gomock.Any(), gomock.Any()).
Return(testutils.InitReleaseImageOrDie("4.15.0"), nil).AnyTimes()
mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().
GetRegistryOverrides().Return(map[string]string{}).AnyTimes()
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

These mocks return empty overrides, so they only hit the no-op path. It'd be great to have at least one test with actual non-empty overrides to exercise the real logic end-to-end from the controller level.

mockEC2 := awsapi.NewMockEC2API(mockCtrl)
mockEC2.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVpcEndpointsOutput{}, fmt.Errorf("not ready")).AnyTimes()

Expand Down Expand Up @@ -810,6 +812,8 @@ func TestNonReadyInfraTriggersRequeueAfter(t *testing.T) {
mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().
Lookup(gomock.Any(), gomock.Any(), gomock.Any()).
Return(testutils.InitReleaseImageOrDie("4.15.0"), nil).AnyTimes()
mockedProviderWithOpenshiftImageRegistryOverrides.EXPECT().
GetRegistryOverrides().Return(map[string]string{}).AnyTimes()
mockEC2 := awsapi.NewMockEC2API(mockCtrl)
mockEC2.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVpcEndpointsOutput{}, fmt.Errorf("not ready")).AnyTimes()
hcp := sampleHCP(t)
Expand Down
32 changes: 31 additions & 1 deletion control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package imageprovider

import "github.com/openshift/hypershift/support/releaseinfo"
import (
"strings"

"github.com/openshift/hypershift/support/releaseinfo"
)

//go:generate ../../../../hack/tools/bin/mockgen -source=imageprovider.go -package=imageprovider -destination=imageprovider_mock.go

Expand Down Expand Up @@ -58,3 +62,29 @@ func (p *SimpleReleaseImageProvider) ImageExist(key string) (string, bool) {
func (p *SimpleReleaseImageProvider) ComponentImages() map[string]string {
return p.componentsImages
}

// NewWithRegistryOverrides creates a SimpleReleaseImageProvider that applies
// registry overrides to all component images. This ensures init containers
// and other sub-resources created by CPO use the overridden image references.
func NewWithRegistryOverrides(releaseImage *releaseinfo.ReleaseImage, registryOverrides map[string]string) *SimpleReleaseImageProvider {
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

Minor: since this shares the same struct construction as New(), you could have New() just delegate:

func New(releaseImage *releaseinfo.ReleaseImage) *SimpleReleaseImageProvider {
    return NewWithRegistryOverrides(releaseImage, nil)
}

Keeps initialization in one place.

originalImages := releaseImage.ComponentImages()
images := make(map[string]string, len(originalImages))
for k, v := range originalImages {
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

Tiny nit: since we're on Go 1.25+, this could be maps.Clone(releaseImage.ComponentImages()). Totally optional.

images[k] = v
}
if len(registryOverrides) > 0 {
for key, image := range images {
for source, target := range registryOverrides {
if image == source || strings.HasPrefix(image, source+"/") {
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

Nice — this prefix guard is actually safer than what RegistryMirrorProviderDecorator does at registry_mirror_provider.go:38, which is a bare strings.Replace that would incorrectly match quay.io inside quay.io.evil.com.

One concern though: now we have two different matching algorithms for the same operation in the same pipeline. Worth extracting this into a shared helper in support/util/ and fixing the decorator too — either here or as a follow-up. WDYT?

images[key] = strings.Replace(image, source, target, 1)
break
}
}
}
}
return &SimpleReleaseImageProvider{
componentsImages: images,
missingImages: make([]string, 0),
ReleaseImage: releaseImage,
}
}
119 changes: 119 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/imageprovider/imageprovider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ import (
"testing"

. "github.com/onsi/gomega"

"github.com/openshift/hypershift/support/releaseinfo"

imageapi "github.com/openshift/api/image/v1"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestNewFromImages(t *testing.T) {
Expand Down Expand Up @@ -174,3 +181,115 @@ func TestComponentImages(t *testing.T) {
g.Expect(result).To(Equal(images))
})
}

func newTestReleaseImage(images map[string]string) *releaseinfo.ReleaseImage {
tags := make([]imageapi.TagReference, 0, len(images))
for name, image := range images {
tags = append(tags, imageapi.TagReference{
Name: name,
From: &corev1.ObjectReference{Name: image},
})
Copy link
Copy Markdown
Contributor

@jparrill jparrill May 14, 2026

Choose a reason for hiding this comment

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

Tests look solid! Two cases I think would be worth adding:

  1. Mutation safety — make sure the original ReleaseImage isn't modified (matters because ReleaseImage gets cached in the decorator chain):
t.Run("When overrides are applied, it should not mutate the original release image", func(t *testing.T) {
    t.Parallel()
    g := NewWithT(t)
    releaseImage := newTestReleaseImage(map[string]string{"etcd": "quay.io/etcd:v3"})
    NewWithRegistryOverrides(releaseImage, map[string]string{"quay.io": "mirror.io"})
    g.Expect(releaseImage.ComponentImages()["etcd"]).To(Equal("quay.io/etcd:v3"))
})
  1. Double-override idempotency — this is actually the real runtime scenario since RegistryMirrorProviderDecorator.Lookup() already applies overrides before we get here:
t.Run("When images are already overridden, it should not double-apply", func(t *testing.T) {
    t.Parallel()
    g := NewWithT(t)
    releaseImage := newTestReleaseImage(map[string]string{
        "availability-prober": "mirror.example.com/quay-cache/ns/image:tag",
    })
    overrides := map[string]string{"quay.io": "mirror.example.com/quay-cache"}
    provider := NewWithRegistryOverrides(releaseImage, overrides)
    g.Expect(provider.GetImage("availability-prober")).To(Equal(
        "mirror.example.com/quay-cache/ns/image:tag"))
})

}
return &releaseinfo.ReleaseImage{
ImageStream: &imageapi.ImageStream{
ObjectMeta: metav1.ObjectMeta{Name: "4.20.0"},
Spec: imageapi.ImageStreamSpec{Tags: tags},
},
}
}

func TestNewWithRegistryOverrides(t *testing.T) {
t.Parallel()

t.Run("When overrides match, component images should be remapped", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

releaseImage := newTestReleaseImage(map[string]string{
"availability-prober": "quay.io/redhat-user-workloads/crt-redhat-acm-tenant/control-plane-operator-4-20:latest",
"kube-apiserver": "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:abc123",
"etcd": "registry.access.redhat.com/rhel8/etcd:latest",
})
overrides := map[string]string{
"quay.io": "mirror.example.com/quay-cache",
}

provider := NewWithRegistryOverrides(releaseImage, overrides)

g.Expect(provider.GetImage("availability-prober")).To(Equal(
"mirror.example.com/quay-cache/redhat-user-workloads/crt-redhat-acm-tenant/control-plane-operator-4-20:latest"))
g.Expect(provider.GetImage("kube-apiserver")).To(Equal(
"mirror.example.com/quay-cache/openshift-release-dev/ocp-v4.0-art-dev@sha256:abc123"))
g.Expect(provider.GetImage("etcd")).To(Equal(
"registry.access.redhat.com/rhel8/etcd:latest"))
})

t.Run("When no overrides provided, images should be unchanged", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

releaseImage := newTestReleaseImage(map[string]string{
"availability-prober": "quay.io/redhat-user-workloads/crt-redhat-acm-tenant/cpo:latest",
})

provider := NewWithRegistryOverrides(releaseImage, nil)

g.Expect(provider.GetImage("availability-prober")).To(Equal(
"quay.io/redhat-user-workloads/crt-redhat-acm-tenant/cpo:latest"))
})

t.Run("When overrides don't match any image, images should be unchanged", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

releaseImage := newTestReleaseImage(map[string]string{
"etcd": "registry.access.redhat.com/rhel8/etcd:latest",
})
overrides := map[string]string{
"quay.io": "mirror.example.com",
}

provider := NewWithRegistryOverrides(releaseImage, overrides)

g.Expect(provider.GetImage("etcd")).To(Equal(
"registry.access.redhat.com/rhel8/etcd:latest"))
})

t.Run("When override prefix matches subdomain, it should not apply", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

releaseImage := newTestReleaseImage(map[string]string{
"component": "quay.io.example.com/namespace/image:tag",
})
overrides := map[string]string{
"quay.io": "mirror.example.com",
}

provider := NewWithRegistryOverrides(releaseImage, overrides)

g.Expect(provider.GetImage("component")).To(Equal(
"quay.io.example.com/namespace/image:tag"))
})

t.Run("When multiple overrides exist, only the matching one should apply", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

releaseImage := newTestReleaseImage(map[string]string{
"availability-prober": "quay.io/redhat-user-workloads/crt-redhat-acm-tenant/cpo:latest",
"etcd": "gcr.io/etcd-development/etcd:v3.5",
})
overrides := map[string]string{
"quay.io": "acr.example.com/quay-cache",
"gcr.io": "acr.example.com/gcr-cache",
}

provider := NewWithRegistryOverrides(releaseImage, overrides)

g.Expect(provider.GetImage("availability-prober")).To(Equal(
"acr.example.com/quay-cache/redhat-user-workloads/crt-redhat-acm-tenant/cpo:latest"))
g.Expect(provider.GetImage("etcd")).To(Equal(
"acr.example.com/gcr-cache/etcd-development/etcd:v3.5"))
})
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.