HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides #2947

tiraboschi · 2023-08-24T16:14:01Z

What this PR does / why we need it:
Amend the image address used for the imageStreams
used for the OLM catalog source according to the value
of OpenShiftImageRegistryOverrides as genetically used
in the control plane operator in order to handle the disconnected use case.
This will implicitly assume that the images used for
the 4 default OLM catalogs got mirrored to the internal registry
using the original name and tag.

An annotation based escape mechanism is also defined:
the owner of the hostedcluster will be able to inject custom
addresses (only by digest) for the catalog images.
In that case the imageStream mechanism is completely skipped
and the catalog deployments are directly with the values
in the annotations.
It will be completely up to the user to manually refresh
them when the internal mirror will get refreshed.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes https://issues.redhat.com/browse/HOSTEDCP-591

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

netlify · 2023-08-24T16:27:19Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`01b4580`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/64f5f0ed20786d00083eee81
😎 Deploy Preview	https://deploy-preview-2947--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tiraboschi · 2023-08-28T08:14:22Z

/retest

csrwng

Some comments, thx!

control-plane-operator/controllers/hostedcontrolplane/olm/params.go

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

control-plane-operator/controllers/hostedcontrolplane/olm/catalogs.go

csrwng · 2023-08-31T13:29:26Z

control-plane-operator/controllers/hostedcontrolplane/olm/catalogs.go

+	catalogWithOverride := make(map[string]string)
+	for name, image := range catalogToImage {
+		for registrySource, registryDest := range openShiftImageRegistryOverrides {
+			if strings.Contains(image, registrySource) {


instead of doing a string check, I'd rather parse the image ref and match the registry exactly, using reference.Parse() from

hypershift/support/thirdparty/library-go/pkg/image/reference/reference.go

Line 32 in 8b4b529

func Parse(spec string) (DockerImageReference, error) {

then replace the registry in the reference object

This is a bit ambiguous: reference.Parse("registry.redhat.io/redhat/redhat-operator-index:v4.14") will return a DockerImageReference where Registry=registry.redhat.io and Namespace=redhat but (probably) we don't want to replace just the registry forcing the customers to keep using the same namespace in their mirrored registry.

And indeed in the cmd help of the control-plane-operator (see: https://github.com/openshift/hypershift/blob/main/control-plane-operator/main.go#L177 ) we say:
Images before being applied are scanned for the source registry string and if found the string is replaced with the destination registry string.

which is exactly what is done in:

hypershift/support/releaseinfo/registry_image_content_policies.go

Lines 26 to 28 in 068f642

if strings.Contains(image, registrySource) {

for _, registryReplacement := range registryDest {

image = strings.Replace(image, registrySource, registryReplacement, 1)

I'm not directly calling that method since here we lack the pullSecret that is used to validate the replacement upfront, but at least the implementation is coherent.

csrwng · 2023-09-05T17:12:50Z

/approve

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

Amend the image address used for the imageStreams used for the OLM catalog source according to the value of OpenShiftImageRegistryOverrides as genetically used in the control plane operator in order to handle the disconnected use case. This will implicitly assume that the images used for the 4 default OLM catalogs got mirrored to the internal registry using the original name and tag. An annotation based escape mechanism is also defined: the owner of the hostedcluster will be able to inject custom addresses (only by digest) for the catalog images. In that case the imageStream mechanism is completely skipped and the catalog deployments are directly with the values in the annotations. It will be completely up to the user to manually refresh them when the internal mirror will get refreshed. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

tiraboschi · 2023-09-06T12:15:40Z

/test e2e-aws

tiraboschi · 2023-09-06T12:15:48Z

/test e2e-kubevirt-aws-ovn

bryan-cox · 2023-09-06T15:27:40Z

Updating the title so the Jira ticket is linked and the label is added. This is going to be required soon "DPTP will be adding the requirement of PRs having a jira/valid-reference label for a PR to merge. Currently PRs will get this label added automatically by referencing a Jira in their title...".

/retitle HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides

openshift-ci-robot · 2023-09-06T15:27:46Z

@tiraboschi: This pull request references HOSTEDCP-591 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
Amend the image address used for the imageStreams
used for the OLM catalog source according to the value
of OpenShiftImageRegistryOverrides as genetically used
in the control plane operator in order to handle the disconnected use case.
This will implicitly assume that the images used for
the 4 default OLM catalogs got mirrored to the internal registry
using the original name and tag.

An annotation based escape mechanism is also defined:
the owner of the hostedcluster will be able to inject custom
addresses (only by digest) for the catalog images.
In that case the imageStream mechanism is completely skipped
and the catalog deployments are directly with the values
in the annotations.
It will be completely up to the user to manually refresh
them when the internal mirror will get refreshed.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes https://issues.redhat.com/browse/HOSTEDCP-591

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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/test-infra repository.

bryan-cox

/lgtm

openshift-ci · 2023-09-06T15:31:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox, csrwng, tiraboschi

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [csrwng]

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

openshift-ci-robot · 2023-09-06T16:06:54Z

/retest-required

Remaining retests: 0 against base HEAD 32f3ced and 2 for PR HEAD 5af4213 in total

openshift-ci · 2023-09-06T18:05:33Z

@tiraboschi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ibmcloud-iks	`5af4213`	link	false	`/test e2e-ibmcloud-iks`
ci/prow/e2e-ibmcloud-roks	`5af4213`	link	false	`/test e2e-ibmcloud-roks`

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-09-06T20:16:28Z

/retest-required

Remaining retests: 0 against base HEAD 8c24b8c and 1 for PR HEAD 5af4213 in total

With openshift#2947 we introduced a mechanism to amend the OLM catalog ImageStream according to OpenShiftImageRegistryOverrides that are always automatically initializaed according to any ImageContentSourcePolicy (ICSP) or any ImageDigestMirrorSet (IDMS) instance from an OpenShift management cluster. But assuming that the whole default OLM catalogs got properly mirrored to a different registry just because an ICSP is there on the management cluster is overblown. Let's make this optional introducing an additional annotation to opt-in only if really needed, if not let's continue pointing the catalogs imagestream to the public registry. Automatically mirror the 5 involved annotations from the HC CR to the HCO one. Fixes: https://issues.redhat.com/browse/OCPBUGS-18720 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

With openshift#2947 we introduced a mechanism to amend the OLM catalog ImageStream according to OpenShiftImageRegistryOverrides that are always automatically initializaed according to any ImageContentSourcePolicy (ICSP) or any ImageDigestMirrorSet (IDMS) instance from an OpenShift management cluster. But assuming that the whole default OLM catalogs got properly mirrored to a different registry just because an ICSP is there on the management cluster is overblown and with the imagestream the replacements cannot be validated ahead ot time. Let's not do it by default according to ICSP/IDMS on the management cluster but only according to the value of a new explicit annotation. Automatically mirror the 5 involved annotations from the HC CR to the HCP one. Fixes: https://issues.redhat.com/browse/OCPBUGS-18720 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-area labels Aug 24, 2023

openshift-ci bot requested review from hasueki and isco-rodriguez August 24, 2023 16:14

openshift-ci bot added area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release and removed do-not-merge/needs-area labels Aug 24, 2023

tiraboschi force-pushed the olm_catalogs_disconnected branch from 2897fc6 to 482a316 Compare August 24, 2023 16:26

openshift-ci bot added the area/documentation Indicates the PR includes changes for documentation label Aug 24, 2023

tiraboschi force-pushed the olm_catalogs_disconnected branch 2 times, most recently from c0aa4fa to d830010 Compare August 25, 2023 13:25

openshift-ci bot added the area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release label Aug 25, 2023

tiraboschi changed the title ~~WIP: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides~~ Amend OLM catalog IS according to OpenShiftImageRegistryOverrides Aug 25, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2023

tiraboschi force-pushed the olm_catalogs_disconnected branch from d830010 to baf49c7 Compare August 25, 2023 13:29

csrwng reviewed Aug 31, 2023

View reviewed changes

tiraboschi force-pushed the olm_catalogs_disconnected branch from baf49c7 to 01b4580 Compare September 4, 2023 14:59

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2023

csrwng reviewed Sep 5, 2023

View reviewed changes

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go Outdated Show resolved Hide resolved

tiraboschi force-pushed the olm_catalogs_disconnected branch from 01b4580 to 5af4213 Compare September 5, 2023 21:53

openshift-ci bot changed the title ~~Amend OLM catalog IS according to OpenShiftImageRegistryOverrides~~ HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides Sep 6, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 6, 2023

bryan-cox approved these changes Sep 6, 2023

View reviewed changes

openshift-ci bot assigned bryan-cox Sep 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023

openshift-merge-robot merged commit 9f32daa into openshift:main Sep 7, 2023
13 of 15 checks passed

tiraboschi deleted the olm_catalogs_disconnected branch September 7, 2023 08:39

tiraboschi mentioned this pull request Sep 11, 2023

OCPBUGS-18720: amend OLM catalogs ImageStream according to annotation #3001

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides #2947

HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides #2947

tiraboschi commented Aug 24, 2023 •

edited

netlify bot commented Aug 24, 2023 •

edited

tiraboschi commented Aug 28, 2023

csrwng left a comment

csrwng Aug 31, 2023

tiraboschi Sep 4, 2023

csrwng commented Sep 5, 2023

tiraboschi commented Sep 6, 2023

tiraboschi commented Sep 6, 2023

bryan-cox commented Sep 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 •

edited by openshift-ci bot

bryan-cox left a comment

openshift-ci bot commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

	if strings.Contains(image, registrySource) {
	for _, registryReplacement := range registryDest {
	image = strings.Replace(image, registrySource, registryReplacement, 1)

HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides #2947

HOSTEDCP-591: Amend OLM catalog IS according to OpenShiftImageRegistryOverrides #2947

Conversation

tiraboschi commented Aug 24, 2023 • edited

netlify bot commented Aug 24, 2023 • edited

✅ Deploy Preview for hypershift-docs ready!

tiraboschi commented Aug 28, 2023

csrwng left a comment

Choose a reason for hiding this comment

csrwng Aug 31, 2023

Choose a reason for hiding this comment

tiraboschi Sep 4, 2023

Choose a reason for hiding this comment

csrwng commented Sep 5, 2023

tiraboschi commented Sep 6, 2023

tiraboschi commented Sep 6, 2023

bryan-cox commented Sep 6, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 • edited by openshift-ci bot

bryan-cox left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

tiraboschi commented Aug 24, 2023 •

edited

netlify bot commented Aug 24, 2023 •

edited

bryan-cox commented Sep 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 •

edited by openshift-ci bot