resourceapply: add improved version which copies over spec.conversion.webhook.clientConfig.caBundle #2064
Conversation
….webhook.clientConfig.caBundle
|
@chrischdi: 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. |
|
/assign @JoelSpeed |
|
/assign @bertinatto @jsafrane |
|
/cc @wking As I think CVO does implement/vendor this same change to resourceapply |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chrischdi, JoelSpeed 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 |
| ) | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
Please double-check but I think that the *Improved versions let callers reuse a cache to avoid unnecessary writes when nothing has changed.
| ) | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
Please also add a unit test
| return existing, false, nil | ||
| } | ||
|
|
||
| if klog.V(2).Enabled() { |
There was a problem hiding this comment.
why not to use klog.V(2).Infof(...) ?
|
|
||
| // 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) { |
There was a problem hiding this comment.
could you rename the params to from and to ? it would be easier to read the code.
| modified := false | ||
| existingCopy := existing.DeepCopy() | ||
|
|
||
| copyCustomResourceDefinitionConversionWebhookCABundle(existing, required) |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| // 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. |
There was a problem hiding this comment.
since this function provides upgrade compatibility with service-ca-bundle operator why shouldn't the ApplyCustomResourceDefinitionV1 receive this improvement ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
6 participants
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.