resourceapply: add improved version which copies over spec.conversion.webhook.clientConfig.caBundle by chrischdi · Pull Request #2064 · openshift/library-go

chrischdi · 2025-12-09T12:05:33Z

This PR adds an improved version of ApplyCustomResourceDefinitionV1, analog to the pattern used in other packages, named ApplyCustomResourceDefinitionV1Improved.

The only change is to also copy over the caBundle of a conversion webhook, to preserve an injected ca bundle and to not cause indefinite reconciliations between some controller and service-ca-bundle operator.

This was found for https://github.com/openshift/cluster-capi-operator which makes use of this library.

CVO already today does this: https://github.com/openshift/cluster-version-operator/blob/0e6c916f99e05983190202575bb530200560acb9/lib/resourcemerge/apiext.go#L34

I adjusted the code though to match what library-go does for MutatingWebhookConfigurations.

….webhook.clientConfig.caBundle

openshift-ci · 2025-12-09T12:14:42Z

@chrischdi: all tests passed!

Full PR test history. Your PR dashboard.

Details

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

damdo · 2025-12-09T12:34:19Z

/assign @JoelSpeed

damdo · 2025-12-09T12:34:54Z

/assign @bertinatto @jsafrane

damdo · 2025-12-09T12:35:50Z

/cc @wking

As I think CVO does implement/vendor this same change to resourceapply

JoelSpeed · 2025-12-09T13:29:19Z

/lgtm

openshift-ci · 2025-12-09T13:29:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chrischdi, JoelSpeed
Once this PR has been reviewed and has the lgtm label, please ask for approval from bertinatto. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

pkg/operator/OWNERS

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

p0lyn0mial · 2025-12-16T08:33:48Z

pkg/operator/resource/resourceapply/apiextensions.go

 )

+// ApplyCustomResourceDefinitionV1Improved applies the required CustomResourceDefinition to the cluster, preserving an injected ca bundle.
+func ApplyCustomResourceDefinitionV1Improved(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, recorder events.Recorder, required *apiextensionsv1.CustomResourceDefinition) (*apiextensionsv1.CustomResourceDefinition, bool, error) {


Please double-check but I think that the *Improved versions let callers reuse a cache to avoid unnecessary writes when nothing has changed.

That's right, *Improved variants typically use the ResourceCache. While the suffix isn’t ideal, it’s probably best to continue following the existing convention for consistency. Let's try to come up with a different suffix for now (ApplyCustomResourceDefinitionV1WithCABundle?).

p0lyn0mial · 2025-12-16T08:35:17Z

pkg/operator/resource/resourceapply/apiextensions.go

 )

+// ApplyCustomResourceDefinitionV1Improved applies the required CustomResourceDefinition to the cluster, preserving an injected ca bundle.
+func ApplyCustomResourceDefinitionV1Improved(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, recorder events.Recorder, required *apiextensionsv1.CustomResourceDefinition) (*apiextensionsv1.CustomResourceDefinition, bool, error) {


Please also add a unit test

p0lyn0mial · 2025-12-16T08:36:14Z

pkg/operator/resource/resourceapply/apiextensions.go

+		return existing, false, nil
+	}
+
+	if klog.V(2).Enabled() {


why not to use klog.V(2).Infof(...) ?

p0lyn0mial · 2025-12-16T08:36:52Z

pkg/operator/resource/resourceapply/apiextensions.go

+
+// copyCustomResourceDefinitionConversionWebhookCABundle populates spec.conversion.webhook.clientConfig.caBundle fields from
+// existing resource if it was set before and is not set in present. This provides upgrade compatibility with service-ca-bundle operator.
+func copyCustomResourceDefinitionConversionWebhookCABundle(existing *apiextensionsv1.CustomResourceDefinition, required *apiextensionsv1.CustomResourceDefinition) {


could you rename the params to from and to ? it would be easier to read the code.

p0lyn0mial · 2025-12-16T08:51:28Z

pkg/operator/resource/resourceapply/apiextensions.go

+	modified := false
+	existingCopy := existing.DeepCopy()
+
+	copyCustomResourceDefinitionConversionWebhookCABundle(existing, required)


you pass the original pointer to the required obj which is modified. you need to make a copy of that obj and pass the pointer to the copy

p0lyn0mial · 2025-12-16T08:53:08Z

pkg/operator/resource/resourceapply/apiextensions.go

 }
+
+// copyCustomResourceDefinitionConversionWebhookCABundle populates spec.conversion.webhook.clientConfig.caBundle fields from
+// existing resource if it was set before and is not set in present. This provides upgrade compatibility with service-ca-bundle operator.


since this function provides upgrade compatibility with service-ca-bundle operator why shouldn't the ApplyCustomResourceDefinitionV1 receive this improvement ?

I thought about creating a separate function to not "surprisingly" change behavior of the existing function.

I'm okay with doing the adjustment to ApplyCustomResourceDefinitionV1

I think creating an "Improved" function has generally been the approach around here right?
To not surprise importers with a "breaking change" in behaviour.
That said if we think we are ok with that, +1

I think that the *Improved versions also let callers reuse a cache to avoid unnecessary writes when nothing has changed. Usually by accepting the cache ResourceCache param.

I'm ok with a new function but I think the new function should also take the cache ResourceCache param.

We should also consider deprecating the old function. WDYT?

Sounds reasonable thanks

bertinatto

Could you also create a proof PR where you bump library-go in your operator to use this version?

bertinatto · 2025-12-30T17:50:56Z

pkg/operator/resource/resourceapply/apiextensions.go

 )

+// ApplyCustomResourceDefinitionV1Improved applies the required CustomResourceDefinition to the cluster, preserving an injected ca bundle.
+func ApplyCustomResourceDefinitionV1Improved(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, recorder events.Recorder, required *apiextensionsv1.CustomResourceDefinition) (*apiextensionsv1.CustomResourceDefinition, bool, error) {


That's right, *Improved variants typically use the ResourceCache. While the suffix isn’t ideal, it’s probably best to continue following the existing convention for consistency. Let's try to come up with a different suffix for now (ApplyCustomResourceDefinitionV1WithCABundle?).

chrischdi · 2026-01-08T12:32:41Z

As I'm not at the company anymore, one of the team would need to takeover, cc @damdo

damdo · 2026-01-09T10:05:40Z

Thanks @chrischdi for your work
Yes, someone else will take over in a bit

p0lyn0mial · 2026-02-03T09:56:38Z

is this pr still relevant ?

resourceapply: add improved version which copies over spec.conversion…

495830d

….webhook.clientConfig.caBundle

openshift-ci bot requested review from deads2k and p0lyn0mial December 9, 2025 12:05

chrischdi mentioned this pull request Dec 9, 2025

NO-JIRA: capiinstaller: keep caBundle for CRDs and conversion webhooks openshift/cluster-capi-operator#423

Merged

openshift-ci bot assigned JoelSpeed Dec 9, 2025

openshift-ci bot assigned bertinatto and jsafrane Dec 9, 2025

openshift-ci bot requested a review from wking December 9, 2025 12:35

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2025

chrischdi mentioned this pull request Dec 10, 2025

NO-JIRA: capiinstaller: fix error wrapping openshift/cluster-capi-operator#427

Merged

p0lyn0mial reviewed Dec 16, 2025

View reviewed changes

bertinatto reviewed Dec 30, 2025

View reviewed changes

Comments

Conversation

chrischdi commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Dec 9, 2025

Uh oh!

damdo commented Dec 9, 2025

Uh oh!

damdo commented Dec 9, 2025

Uh oh!

damdo commented Dec 9, 2025

Uh oh!

JoelSpeed commented Dec 9, 2025

Uh oh!

openshift-ci bot commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bertinatto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrischdi commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damdo commented Jan 9, 2026

Uh oh!

p0lyn0mial commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chrischdi commented Dec 9, 2025 •

edited

Loading

chrischdi commented Jan 8, 2026 •

edited

Loading