Fix v1beta1 webhook #2358
Merged
Fix v1beta1 webhook #2358
Conversation
This file contains hidden or 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
The v1 webhook did work as expected, but not the v1beta1 webhook before this commit. To fix this, it: * creates 2 distinct webhooks with distinct names (`elastic-es-validation-v1beta1.k8s.elastic.co` and `elastic-es-validation-v1.k8s.elastic.co`) in the ValidatingWebhookConfiguration * ensures the v1beta1 webhook is created along with the v1 webhook at operator startup * registers v1beta1 resources in the schema, required for the v1beta1 webhook to run To test it, I deployed the operator and webhook configurations, then: - tried applying a v1beta1 resource: the v1beta1 webhook was triggered, returning an error mentioning v1beta1.NodeSet - tried applying a v1 resource: the v1 webhook was triggered, returning an error mentioning v1.NodeSet
barkbay
reviewed
Jan 7, 2020
docs/webhook.asciidoc
Outdated
| The webhook is composed of 4 main components. Here is a brief description of each of them to understand how they interact, their naming, and how they are managed. | ||
|
|
||
| . A `ValidatingWebhookConfiguration` object that defines the validating webhook, named `elastic-es-validation.k8s.elastic.co`. It must be created before starting the operator. The `caBundle` field can be automatically managed as part of the automatic certificate management _(see below)_. | ||
| . A `ValidatingWebhookConfiguration` object that defines the validating webhook, named `elastic-es-validation-{eck_crd_version}.k8s.elastic.co`. It must be created before starting the operator. The `caBundle` field can be automatically managed as part of the automatic certificate management _(see below)_. |
Contributor
There was a problem hiding this comment.
The ValidatingWebhookConfiguration object is named elastic-webhook.k8s.elastic.co
It is the clientConfig that is named elastic-es-validation-{eck_crd_version}.k8s.elastic.co
Contributor
Author
There was a problem hiding this comment.
The ValidatingWebhookConfiguration contains a webhooks array of Webhook, for which we define the name. That's not the name of the clientConfig.
I think in "A ValidatingWebhookConfiguration object that defines the validating webhook, named elastic-es-validation-{eck_crd_version}.k8s.elastic.co", named refers to the inner validating webhook, not to the ValidatingWebhookConfiguration.
WDYT?
Contributor
Author
There was a problem hiding this comment.
We can maybe simplify that sentence to A ValidatingWebhookConfiguration object that defines the validating webhooks(without any name reference) so we avoid the confusion?
I think the user can name them any way they want as long as they point to the correct path.
barkbay
approved these changes
Jan 7, 2020
Contributor
barkbay
left a comment
There was a problem hiding this comment.
Just left a nit in the doc, otherwise lgtm
sebgl
added a commit
to sebgl/cloud-on-k8s
that referenced
this pull request
Jan 7, 2020
* Fix v1beta1 webhook The v1 webhook did work as expected, but not the v1beta1 webhook before this commit. To fix this, it: * creates 2 distinct webhooks with distinct names (`elastic-es-validation-v1beta1.k8s.elastic.co` and `elastic-es-validation-v1.k8s.elastic.co`) in the ValidatingWebhookConfiguration * ensures the v1beta1 webhook is created along with the v1 webhook at operator startup * registers v1beta1 resources in the schema, required for the v1beta1 webhook to run To test it, I deployed the operator and webhook configurations, then: - tried applying a v1beta1 resource: the v1beta1 webhook was triggered, returning an error mentioning v1beta1.NodeSet - tried applying a v1 resource: the v1 webhook was triggered, returning an error mentioning v1.NodeSet * Remove ambiguity in webhooks docs
sebgl
added a commit
that referenced
this pull request
Jan 7, 2020
* Fix v1beta1 webhook The v1 webhook did work as expected, but not the v1beta1 webhook before this commit. To fix this, it: * creates 2 distinct webhooks with distinct names (`elastic-es-validation-v1beta1.k8s.elastic.co` and `elastic-es-validation-v1.k8s.elastic.co`) in the ValidatingWebhookConfiguration * ensures the v1beta1 webhook is created along with the v1 webhook at operator startup * registers v1beta1 resources in the schema, required for the v1beta1 webhook to run To test it, I deployed the operator and webhook configurations, then: - tried applying a v1beta1 resource: the v1beta1 webhook was triggered, returning an error mentioning v1beta1.NodeSet - tried applying a v1 resource: the v1 webhook was triggered, returning an error mentioning v1.NodeSet * Remove ambiguity in webhooks docs
mjmbischoff
pushed a commit
to mjmbischoff/cloud-on-k8s
that referenced
this pull request
Jan 13, 2020
* Fix v1beta1 webhook The v1 webhook did work as expected, but not the v1beta1 webhook before this commit. To fix this, it: * creates 2 distinct webhooks with distinct names (`elastic-es-validation-v1beta1.k8s.elastic.co` and `elastic-es-validation-v1.k8s.elastic.co`) in the ValidatingWebhookConfiguration * ensures the v1beta1 webhook is created along with the v1 webhook at operator startup * registers v1beta1 resources in the schema, required for the v1beta1 webhook to run To test it, I deployed the operator and webhook configurations, then: - tried applying a v1beta1 resource: the v1beta1 webhook was triggered, returning an error mentioning v1beta1.NodeSet - tried applying a v1 resource: the v1 webhook was triggered, returning an error mentioning v1.NodeSet * Remove ambiguity in webhooks docs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The v1 webhook did work as expected, but not the v1beta1 webhook before
this commit.
To fix this, it:
(
elastic-es-validation-v1beta1.k8s.elastic.coandelastic-es-validation-v1.k8s.elastic.co) in theValidatingWebhookConfiguration
operator startup
webhook to run
To test it, I deployed the operator and webhook configurations, then:
returning an error mentioning v1beta1.NodeSet
an error mentioning v1.NodeSet
Fixes #2355.