Avoid reconciling field Webhook.ClientConfig.CABundle
#711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9496671093Details
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9498846290Details
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9577793570Details
💛 - Coveralls |
|
Hi @zeeke, just a general comment we need to take care of this also for the cert-manager in vanilla kubernetes we will have the same issue https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/bindata/manifests/operator-webhook/003-webhook.yaml#L12 |
Good point. Though the unit test I implemented refers to the Openshift CA injector, the fix should be valid for the Cert Manager as well. |
zeeke
commented
Jun 21, 2024
pkg/apply/merge.go
Outdated
| return nil | ||
| } | ||
|
|
||
| injectCABundle, ok := currentAnnotations["service.beta.openshift.io/inject-cabundle"] |
There was a problem hiding this comment.
@SchSeba I think you refer to this. Removing
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9626639262Details
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9626718921Details
💛 - Coveralls |
ykulazhenkov
approved these changes
Jun 24, 2024
SchSeba
reviewed
Jun 25, 2024
pkg/apply/merge.go
Outdated
| @@ -116,6 +122,92 @@ func MergeServiceAccountForUpdate(current, updated *uns.Unstructured) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // MergeWebhookForUpdate ensures the Webhook.ClientConfig.CABundle is never removed from a webhook | |||
| // if the resource has the `service.beta.openshift.io/inject-cabundle` annotation | |||
There was a problem hiding this comment.
nit: please remove this one
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9658912326Details
💛 - Coveralls |
adrianchiris
approved these changes
Jun 25, 2024
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9675205060Details
💛 - Coveralls |
Webhook resources (`ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration`) in OpenShift are configured with `service.beta.openshift.io/inject-cabundle` in a way that a third component fills the ClientConfig.CABundle field of the webhook. When reconciling webhooks, do not override the field and avoid a flakiness, as there might be a time slot in which the API server is not configured with a valid client certificate: ``` Error from server (InternalError): error when creating "policies": Internal error occurred: failed calling webhook "operator-webhook.sriovnetwork.openshift.io": failed to call webhook: Post "https://operator-webhook-service.openshift-sriov-network-operator.svc:443/mutating-custom-resource?timeout=10s": tls: failed to verify certificate: x509: certificate signed by unknown authority ``` The same behavior also happens when using CertManager Refs: - https://docs.openshift.com/container-platform/4.15/security/certificates/service-serving-certificate.html - https://issues.redhat.com/browse/OCPBUGS-32139 - https://cert-manager.io/docs/concepts/ca-injector/ Signed-off-by: Andrea Panattoni <[email protected]>
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9676241549Details
💛 - Coveralls |
SchSeba
approved these changes
Jun 27, 2024
|
3 approvals merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Webhook resources (
ValidatingWebhookConfigurationandMutatingWebhookConfiguration) in OpenShift are configured withservice.beta.openshift.io/inject-cabundlein a way that a third component fills the ClientConfig.CABundle field of the webhook. When reconciling webhooks, do not override the field and avoid a flakiness, as there might be a time slot in which the API server is not configured with a valid client certificate:Refs: