⚠ Add admissionregisteration/v1 support for webhook #469

jiachengxu · 2020-08-11T08:51:03Z

This PR implements support to generate {Mutating, Validating}WebhookConfiguration with version v1.
Fixes #419

The first intention was to add a package marker webhookVersions=<[]string> for the user to specify the versions that want to generate. It will default to v1 if the webhookVersions is not specified. However, after discussion with @estroz (#469 (comment)), we found out this cannot work nicely because, for different versions of WebhookConfiguraiton, Some fields (SideEffects, AdmissionReviewVersions) can have different available values, and if one value in that marker is invalid for one version then you can't generate the config for either version.
So we decided to use an optional field marker +kubebuilder:webhookVersions=<[]string> to specify the versions in the +kubebuilder:webhookVersions directly, it works as follows:

We default to generate v1 configuration if the marker is not specified.
Generate v1 if the marker is +kubebuilder:webhook:webhookVersions=v1
Generate v1beta1 if the marker is +kubebuilder:webhook:webhookVersions=v1beta1
Generate both v1 and v1beta1 if the marker is +kubebuilder:webhook:webhookVersions=v1;v1beta1 or +kubebuilder:webhook:webhookVersions=v1beta1;v1, we throw error out if the sideEffects (later we also needed to handle AdmissionReviewVersions) cannot meet requirement for either versions.

This is a breaking change since we generate v1 version configurations by default if the marker is not specified.

jiachengxu · 2020-08-11T08:51:17Z

/cc @alvaroaleman @vincepri

pkg/webhook/parser.go

jiachengxu · 2020-08-11T08:54:20Z

pkg/webhook/testdata/webhook.go

@@ -27,7 +27,7 @@ func (c *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
 		Complete()
 }

-// +kubebuilder:webhook:verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=Some
+// +kubebuilder:webhook:verbs=create;update,path=/validate-testdata-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=testdata.kubebuiler.io,resources=cronjobs,versions=v1,name=validation.cronjob.testdata.kubebuilder.io,sideEffects=NoneOnDryRun


This change is because in admissionregistration/v1, the sideEffects can only be None and NoneOnDryRun when creates Configurations

Are you checking for this case anywhere? There should be an error if anything other than either of those two is set.

No, there is no such check, please check this comment: #469 (comment) What do you think about this? Or you think it is better to forbid users to create config with Some and Unknown?

In general I would want as much static validation as possible for my operator project so that I don't have to wait for my CI's apiserver to tell me my webhook config is invalid when I upgrade from v1beta1 to v1. Is there a good reason not to statically validate side effects?

I guess the counter-argument is that having one marker for two versions is nice, and if one value in that marker is invalid for one version then you can't generate the config for either version, which I think is what you're suggesting in your comment.

The right solution IMO is to add an optional +kubebuilder:webhook:webhookVersion=<v1|v1beta1> field so you can specify which version to generate for a marker if you want to generate both and configure them differently. The case where this field wasn't added to the marker is the current case, so no version list is needed and this isn't a breaking change. Thoughts?

I guess the counter-argument is that having one marker for two versions is nice, and if one value in that marker is invalid for one version then you can't generate the config for either version, which I think is what you're suggesting in your comment.

Yeah, exactly. This is my main concern so I decided not to handle the static validation and let kube-apiserver handle it, but it may not be a good solution because users have to modify the configuration every time they re-generate it.

The right solution IMO is to add an optional +kubebuilder:webhook:webhookVersion=<v1|v1beta1> field so you can specify which version to generate for a marker if you want to generate both and configure them differently.

I think this will work, but it will double the number of markers if the user wants to generate both v1 and v1beta1 webhooks. How about we do the following, with a marker +kubebuilder:webhook:webhookVersions=<[]string>:

We default to generate v1 configuration if the marker is not specified.

Generate v1 if the marker is +kubebuilder:webhook:webhookVersions=v1

Generate v1beta1 if the marker is +kubebuilder:webhook:webhookVersions=v1beta1

Generate both v1 and v1beta1 if the marker is +kubebuilder:webhook:webhookVersions=v1;v1beta1 or +kubebuilder:webhook:webhookVersions=v1beta1;v1, we throw error out if the sideEffects (later we also needed to handle AdmissionReviewVersions) cannot meet requirement for either versions.
In this way, we still provide a chance for the user to generate both v1 and v1beta1 in one marker.

The case where this field wasn't added to the marker is the current case, so no version list is needed and this isn't a breaking change.

I think it will still be a breaking change because we will generate v1 configurations by default (currently v1beta1)?

WDYT @estroz ?

Works for me.

this isn't a breaking change.

To the marker definition I mean, since current markers will work as-is.

@estroz This has been implemented, PTAL :)

pkg/webhook/parser.go

pkg/webhook/conv.go

pkg/webhook/parser.go

pkg/webhook/parser_integration_test.go

pkg/webhook/testdata/cronjob_types.go

pkg/webhook/parser.go

pkg/webhook/parser_integration_test.go

estroz

/lgtm

jiachengxu · 2020-08-15T04:13:06Z

kindly ping @vincepri , can you please take a look?

vincepri

/approve
/assign @alvaroaleman
/hold

Feel free to unhold when ready

k8s-ci-robot · 2020-08-18T15:57:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiachengxu, vincepri

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~~ [vincepri]

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

vincepri · 2020-08-18T15:57:23Z

Should we merge this after we release v0.3.1, I guess this would be v0.4.0?

vincepri · 2020-08-18T15:58:50Z

Actually it seems we already merged #468, which is a breaking change, so this can go in and the next release is v0.4.0

jiachengxu · 2020-08-19T15:00:58Z

kindly ping @alvaroaleman , do you have any further comments? If not, I will just unhold the PR, and rock and roll :)

alvaroaleman

/hold cancel

caboteria · 2020-10-28T19:48:11Z

The problem with making v1 the default is that controller-runtime doesn't yet handle v1 so now controller-gen by default generates manifests that don't work with controller-runtime.

https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/webhook.go#L44

estroz · 2020-10-28T20:31:39Z

@caboteria this was fixed in #474, which forces you to set admissionReviewVersions={<version>}, in this case v1beta1.

estroz · 2020-10-28T20:36:02Z

@vincepri there should probably be another release for this, but since it breaks the v0.4.0 marker scheme, should it be v0.5.0?

caboteria · 2020-10-28T21:05:11Z

Thanks @estroz for the pointer, that helps! I'm "go get"ing the current master rev of controller-tools so I'm all set, but my concern is that the defaults for controller-tools don't work with the current controller-runtime so it's easy for noobs like me to stub their toe when trying to build an operator. Should v1beta1 be the default until controller-runtime works with v1?

estroz · 2020-10-28T21:21:08Z

the defaults for controller-tools don't work with the current controller-runtime

There is no admissionReviewVersion default anymore (logic for removing the default is here), you are forced to set that marker field. If you try running controller-gen webhook without that field set, you'll get a fairly indicative error. Does that answer your question?

jiachengxu · 2020-10-28T21:32:45Z

For making controller-runtime work with admissionReviewVersion v1, we should fix this:kubernetes-sigs/controller-runtime#1123

caboteria · 2020-10-29T01:45:57Z

Thanks @estroz, the current master version is better in that respect than 0.4.0 which happily generates manifests that deploy but cause errors at run-time:

2020-10-28T15:18:03.677Z	ERROR	controller-runtime.webhook.webhooks	unable to decode the request	{"webhook": "/validate-egw-acnodal-io-v1-loadbalancer", "error": "no kind \"AdmissionReview\" is registered for version \"admission.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:101\""}

I guess one person's "fairly indicative" is another's "wtf"...

Don't get me wrong, I appreciate the effort that you all are making to move controller-tools forward and I second both the suggestion to release controller-tools 0.5.0 and to make controller-runtime work with v1. At the moment we're in a situation where it's easy for noobs (like me) to waste time having to debug things that should just work.

estroz · 2020-10-29T16:32:04Z

I guess one person's "fairly indicative" is another's "wtf"...

🤣 I meant the static error your see when generating admission webhooks for markers that don't have admissionReviewVersions={<versions>}:

$ controller-gen --version
Version: v0.4.1-0.20200902055707-2f203e490e8a
$ tree api/v1
api/v1
├── cronjob_types.go
├── cronjob_webhook.go
├── groupversion_info.go
└── zz_generated.deepcopy.go
$ cat api/v1/cronjob_webhook.go
package v1

// +kubebuilder:webhook:path=/mutate-batch-my-domain-v1-cronjob,mutating=true,failurePolicy=fail,sideEffects=None,groups=batch.my.domain,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io
...
$ controller-gen webhook paths=./...
AdmissionReviewVersions is mandatory for v1 {Mutating,Validating}WebhookConfiguration

The deploy-time errors you get when deploying a v1 webhook config without admissionReviewVersions aren't under the purview of controller-tools. Also you shouldn't be seeing these deploy-time errors when using a webhook config generated using the current master's controller-gen binary; if you don't specify admissionReviewVersions={<versions>} you'll see the static error from the example above and the manifests.yaml won't be generated.

jiachengxu · 2020-10-30T09:32:30Z

@caboteria A PR is created here: kubernetes-sigs/controller-runtime#1233 to tackle this issue.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2020

k8s-ci-robot requested review from DirectXMan12 and pwittrock August 11, 2020 08:51

k8s-ci-robot requested review from alvaroaleman and vincepri August 11, 2020 08:51

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2020

jiachengxu commented Aug 11, 2020

View reviewed changes

pkg/webhook/parser.go Outdated Show resolved Hide resolved

jiachengxu commented Aug 11, 2020

View reviewed changes

pkg/webhook/parser.go Outdated Show resolved Hide resolved

alvaroaleman reviewed Aug 11, 2020

View reviewed changes

pkg/webhook/conv.go Outdated Show resolved Hide resolved

pkg/webhook/conv.go Outdated Show resolved Hide resolved

pkg/webhook/parser.go Outdated Show resolved Hide resolved

pkg/webhook/parser_integration_test.go Outdated Show resolved Hide resolved

estroz reviewed Aug 11, 2020

View reviewed changes

pkg/webhook/testdata/cronjob_types.go Outdated Show resolved Hide resolved

estroz mentioned this pull request Aug 11, 2020

Migrate existing KB project to v1 CRDs and Webhooks with minimal user effort kubernetes-sigs/kubebuilder#1065

Closed

estroz reviewed Aug 12, 2020

View reviewed changes

pkg/webhook/parser.go Show resolved Hide resolved

estroz reviewed Aug 12, 2020

View reviewed changes

pkg/webhook/parser.go Outdated Show resolved Hide resolved

estroz reviewed Aug 12, 2020

View reviewed changes

pkg/webhook/parser_integration_test.go Outdated Show resolved Hide resolved

⚠ Add admissionregisteration/v1 support for webhook

0a3ac09

estroz approved these changes Aug 14, 2020

View reviewed changes

k8s-ci-robot assigned estroz Aug 14, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2020

vincepri approved these changes Aug 18, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman Aug 18, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2020

alvaroaleman approved these changes Aug 19, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2020

k8s-ci-robot merged commit 04609bd into kubernetes-sigs:master Aug 19, 2020

This was referenced Aug 21, 2020

⚠ Add AdmissionReviewVersions support for webhook #474

Merged

⚠️ (go/v3-alpha) default to v1 CRDs and webhooks kubernetes-sigs/kubebuilder#1644

Merged

jiachengxu mentioned this pull request Oct 30, 2020

⚠ pkg/webhook: Add support for k8s.io/api/admission/v1 kubernetes-sigs/controller-runtime#1233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ Add admissionregisteration/v1 support for webhook #469

⚠ Add admissionregisteration/v1 support for webhook #469

jiachengxu commented Aug 11, 2020 •

edited

Loading

jiachengxu commented Aug 11, 2020

jiachengxu Aug 11, 2020 •

edited

Loading

estroz Aug 12, 2020

jiachengxu Aug 12, 2020 •

edited

Loading

estroz Aug 12, 2020

estroz Aug 12, 2020 •

edited

Loading

jiachengxu Aug 13, 2020

estroz Aug 13, 2020

jiachengxu Aug 14, 2020

estroz left a comment

jiachengxu commented Aug 15, 2020

vincepri left a comment

k8s-ci-robot commented Aug 18, 2020

vincepri commented Aug 18, 2020

vincepri commented Aug 18, 2020

jiachengxu commented Aug 19, 2020

alvaroaleman left a comment

caboteria commented Oct 28, 2020

estroz commented Oct 28, 2020

estroz commented Oct 28, 2020

caboteria commented Oct 28, 2020

estroz commented Oct 28, 2020

jiachengxu commented Oct 28, 2020

caboteria commented Oct 29, 2020

estroz commented Oct 29, 2020 •

edited

Loading

jiachengxu commented Oct 30, 2020 •

edited

Loading

⚠ Add admissionregisteration/v1 support for webhook #469

⚠ Add admissionregisteration/v1 support for webhook #469

Conversation

jiachengxu commented Aug 11, 2020 • edited Loading

jiachengxu commented Aug 11, 2020

jiachengxu Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

estroz Aug 12, 2020

Choose a reason for hiding this comment

jiachengxu Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

estroz Aug 12, 2020

Choose a reason for hiding this comment

estroz Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

jiachengxu Aug 13, 2020

Choose a reason for hiding this comment

estroz Aug 13, 2020

Choose a reason for hiding this comment

jiachengxu Aug 14, 2020

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

jiachengxu commented Aug 15, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 18, 2020

vincepri commented Aug 18, 2020

vincepri commented Aug 18, 2020

jiachengxu commented Aug 19, 2020

alvaroaleman left a comment

Choose a reason for hiding this comment

caboteria commented Oct 28, 2020

estroz commented Oct 28, 2020

estroz commented Oct 28, 2020

caboteria commented Oct 28, 2020

estroz commented Oct 28, 2020

jiachengxu commented Oct 28, 2020

caboteria commented Oct 29, 2020

estroz commented Oct 29, 2020 • edited Loading

jiachengxu commented Oct 30, 2020 • edited Loading

jiachengxu commented Aug 11, 2020 •

edited

Loading

jiachengxu Aug 11, 2020 •

edited

Loading

jiachengxu Aug 12, 2020 •

edited

Loading

estroz Aug 12, 2020 •

edited

Loading

estroz commented Oct 29, 2020 •

edited

Loading

jiachengxu commented Oct 30, 2020 •

edited

Loading