Support dry run in admission webhooks #66936

jennybuckley · 2018-08-02T21:17:08Z

What this PR does / why we need it:
Follow up to #66391

add DryRun to admission.k8s.io/v1beta1.AdmissionReview
add DryRunnable to admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration
add dry run support to (Valid|Mut)atingAdmissionWebhook

Includes all the api-changes outlined by kubernetes/community#2387

/sig api-machinery

Release note:

To address the possibility dry-run requests overwhelming admission webhooks that rely on side effects and a reconciliation mechanism, a new field is being added to admissionregistration.k8s.io/v1beta1.ValidatingWebhookConfiguration and admissionregistration.k8s.io/v1beta1.MutatingWebhookConfiguration so that webhooks can explicitly register as having dry-run support. If a dry-run request is made on a resource that triggers a non dry-run supporting webhook, the request will be completely rejected, with "400: Bad Request". Additionally, a new field is being added to the admission.k8s.io/v1beta1.AdmissionReview API object, exposing to webhooks whether or not the request being reviewed is a dry-run.

jennybuckley · 2018-08-07T16:56:55Z

Rebased

jennybuckley · 2018-08-07T17:00:30Z

@kubernetes/sig-api-machinery-api-reviews
@kubernetes/api-reviewers

jennybuckley · 2018-08-07T17:04:11Z

@deads2k @liggitt @lavalamp @apelisse

lavalamp · 2018-08-07T17:08:25Z

pkg/apis/admission/types.go

@@ -73,6 +73,10 @@ type AdmissionRequest struct {
 	// OldObject is the existing object. Only populated for UPDATE requests.
 	// +optional
 	OldObject runtime.Object
+	// DryRun indicates that modifications will definitely not be persisted for this request.


And that the call must not have side effects.

+1, what about something also indicating that a value of DryRun false doesn't guarantee that the object will be persisted, so any side effects should have a reconciliation mechanism, or is that redundant because it's already somewhere in the webhook documentation?

It should be documented, but feel free to state it again here if you think it makes it clearer.

Okay, added this

lavalamp · 2018-08-07T17:10:08Z

pkg/apis/admission/types.go

+	// DryRun indicates that modifications will definitely not be persisted for this request.
+	// Defaults to false.
+	// +optional
+	DryRun *bool


We don't want to pass on the different sorts of dry run calls?

I don't think we'd want to get more granular than including/excluding an entire phase of admission. That would mean a particular webhook call would only need to know if the request was dryRun or not. Does that seem sufficient?

I'm not sure, I'm not the one arguing that dryRun should be an array of strings...

I think this was discussed on the wg-apply slack channel, and the conclusion was that giving the webhooks the array of strings would be exposing a lot of complexity to them, which they don't necessarily need. Because we don't want to allow the level of granularity to dry run with only some of the admission webhooks, anyway.

lavalamp · 2018-08-07T17:10:44Z

pkg/apis/admission/types.go

+	// DryRun indicates that modifications will definitely not be persisted for this request.
+	// Defaults to false.
+	// +optional
+	DryRun *bool


Is it really safe to make this optional? Do we need to make a completely new version?

it has to be optional in v1beta1, because it's a new field, and has to default to false, since we can't assume current webhooks handle dry run correctly.

the current plan (in service of pushing all admission webhooks to support dry run) is described in https://github.com/kubernetes/community/blob/master/keps/sig-api-machinery/0015-dry-run.md#admission-controllers:

If dry-run is requested on a non-supported webhook, the request will be completely rejected, as a 400: Bad Request. This field will be defaulted to true and deprecated in v1, and completely removed in v2. All webhooks registered with v2 will be assumed to support dry run.

oops, my comment was actually around the dryRunnable bool on the webhook object. similar reasoning applies to this field though (optional additive field in an existing API version), with the exception of changing defaults and removing it in a future version

Yes, of course if we add it to the existing object it has to be optional.

The comment I'm working on leaving on the other API object will explain my position.

are you proposing to only add dryRun to a v1 (or v1beta2, etc) AdmissionReview object, require webhooks to use the v1 format to indicate support for dry run, and treat all that use the v1beta1 format as not supporting dry run?

upsides:

simpler (though implicit) signal that the webhook knows about dryrun

no default false in v1beta1, default true in v1, remove dryRunnable in v2 path required

downsides:

harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)

would require rebuilding existing webhooks that have no side effects (and so can support dry run mode without changes) to support a new version

harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)

Why?

Also, if a cluster admin wants to use dry run but some of their webhooks don't support it, they could manually add dryRunnable: true to the webhooks themself if they are willing to accept the risk in doing so

Yeah, this is a foot-gun :)

harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)

Why?

They'd need to request v1beta1 AdmissionReview objects pre-1.12, and v1 AdmissionReview objects in 1.12+. I actually thought we already had the requested AdmissionReview version in the webhook registration object, but it turns out we don't.

Thinking about that a little more, we'll likely want a list of AdmissionReview versions the webhook can accept. Each apiserver, when sending an AdmissionReview object, would find the first version in the webhook's supported list it is able to send. That would let 1.12 and pre-1.12 servers both talk to the same webhook, as long as it supported both v1 and v1beta1 AdmissionReview objects.

if a cluster admin wants to use dry run but some of their webhooks don't support it, they could manually add dryRunnable: true to the webhooks themself if they are willing to accept the risk in doing so

Yeah, this is a foot-gun :)

I don't think it's catastrophic... admission already has to tolerate and recover from being called in cases where the object isn't actually persisted to storage (because of a conflict, etc). telling the admission plugin this is a dry-run operation is largely an optimization that lets it skip eventually reconciling away a side-effect from a non-persisted request.

They'd need to request v1beta1 AdmissionReview ...

Webhooks don't request anything, they get what we send them. I am confused about what you mean.

apiserver can try with the new version and retry with an old version if that fails.

lavalamp · 2018-08-07T17:45:35Z

pkg/apis/admissionregistration/types.go

+	// will be completely rejected and the webhook will not be called.
+	// Defaults to false.
+	// +optional
+	DryRunnable *bool


So I understand that it's much more expedient to put this here, but it's IMO the wrong way to do this.

I think this should be solved by revving the admissionreview API version, and checking at runtime if the webhook understands it.

If we put this here, then:

administrators will be tempted to flip the bit without checking whether the webhook actually supports it (how can they even check?)

administrators have to flip the bit in every configuration that references the webhook.

Did we decide that the expediency outweighs these considerations?

I think it was done this way more out of consistency with how we add fields to other API types than a concern about expediency.

It doesn't look like we give webhooks a way to indicate what version of AdmissionReview they want today. As soon as we support more than one version, we need that information. I'd be on board with adding that, only adding a dryRun indicator to a new rev of AdmissionReview, and requiring webhooks to request a version of AdmissionReview that supports dryRun.

@jennybuckley's point about this requiring rebuilding admission plugins that currently don't have side effects is a good one. All the conversations we had around this are coming back to me now.

It doesn't look like we give webhooks a way to indicate what version of AdmissionReview they want today.

We don't need to know this. We can try a call with the new version, if it fails, retry with the old version. We don't need a place for that in the configuration.

@jennybuckley's point about this requiring rebuilding admission plugins that currently don't have side effects is a good one.

That's still an argument from expediency.

I'm not arguing we shouldn't do the expedient thing, I just want to know that we actually considered doing the correct thing and are making a good trade off. Given that the correct thing is apparently non-obvious, maybe it is better to just do it this way :/

@lavalamp

…on-2 Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Block dry-run if a webhook would be called **What this PR does / why we need it**: Follow up to kubernetes#66391 Suggested in kubernetes#66391 (comment) Makes dry-run safe in case kubernetes#66936 takes a long time to merge **Release note**: ```release-note NONE ``` /sig api-machinery cc @lavalamp

@lavalamp

Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Block dry-run if a webhook would be called **What this PR does / why we need it**: Follow up to kubernetes/kubernetes#66391 Suggested in kubernetes/kubernetes#66391 (comment) Makes dry-run safe in case kubernetes/kubernetes#66936 takes a long time to merge **Release note**: ```release-note NONE ``` /sig api-machinery cc @lavalamp Kubernetes-commit: 47878f2bd1642145e311d2336e3103c6edcd50de

@lavalamp

Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Block dry-run if a webhook would be called **What this PR does / why we need it**: Follow up to kubernetes/kubernetes#66391 Suggested in kubernetes/kubernetes#66391 (comment) Makes dry-run safe in case kubernetes/kubernetes#66936 takes a long time to merge **Release note**: ```release-note NONE ``` /sig api-machinery cc @lavalamp Kubernetes-commit: 47878f2bd1642145e311d2336e3103c6edcd50de

lavalamp · 2018-08-08T20:12:31Z

Is anything blocking this PR other than me?

jennybuckley · 2018-08-08T20:23:52Z

@lavalamp

I would also want to see if @deads2k takes issue with anything in the API changes being made here, because on the wg-apply slack channel he said "we still get api review during pulls I presume?" before approving the KEP. I don't want to roll this back because it merged too soon without getting everyone's input

lavalamp · 2018-08-08T20:40:00Z

Yeah, both Jordan and I are API approvers, and I'm pondering whether I should hold my nose or get a 3rd opinion.

…

On Wed, Aug 8, 2018 at 1:24 PM Jenny Buckley ***@***.***> wrote: @lavalamp <https://github.com/lavalamp> I would also want to see if @deads2k <https://github.com/deads2k> takes issue with anything in the API changes being made here, because on the wg-apply slack channel he said "we still get api review during pulls I presume?" before approving the KEP. I don't want to roll this back because it merged too soon without getting everyone's input — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66936 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglv_l2p2OxLIGYPXS_XEW_5tXvG1Iks5uO0kSgaJpZM4VtBP9> .

jennybuckley · 2018-08-08T21:43:44Z

@lavalamp besides the doc change you pointed out, is there anything in this that you think needs to be changed before you would approve this?

lavalamp · 2018-08-08T23:42:28Z

Let's proceed as if this is fine. I'll get a 2nd opinion in parallel. Yes, if we're going to do it this way, this is good with the doc change.

liggitt · 2018-08-21T01:58:16Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go

@@ -98,8 +98,9 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr *generic.Versi

 func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.Webhook, attr *generic.VersionedAttributes) error {
 	if attr.IsDryRun() {
-		// TODO: support this
-		return webhookerrors.NewDryRunUnsupportedErr(h.Name)
+		if h.SideEffects == nil || *h.SideEffects == v1beta1.SideEffectClassUnknown || *h.SideEffects == v1beta1.SideEffectClassSome {


same question here... requiring a positive match on an enum value we understand that means dry run is safe seems better

liggitt · 2018-08-21T02:00:54Z

pkg/apis/admissionregistration/types.go

+	// Requests with the dryRun attribute will be auto-rejected if they match a webhook with
+	// sideEffects == Unknown or Some. Defaults to Unknown.
+	// +optional
+	SideEffects *SideEffectClass


did validation get added for this field?

liggitt · 2018-08-21T02:04:34Z

pkg/apis/admissionregistration/v1beta1/defaults.go

@@ -35,4 +35,8 @@ func SetDefaults_Webhook(obj *admissionregistrationv1beta1.Webhook) {
 		selector := metav1.LabelSelector{}
 		obj.NamespaceSelector = &selector
 	}
+	if obj.SideEffects == nil {


worth adding a TODO here (or in a "v1 graduation criteria" list if we have one) to revisit/remove this default and possibly make the field required when promoting to v1, so we don't forget

jennybuckley · 2018-08-21T23:07:41Z

Addressed comments and squashed

jennybuckley · 2018-08-22T17:36:16Z

/retest

lavalamp · 2018-08-22T19:36:21Z

Looks like comments have been addressed.

/lgtm
/approved

jennybuckley · 2018-08-22T23:55:52Z

Just had to rebase, hopefully everything still passes

liggitt · 2018-08-23T00:26:12Z

/lgtm

k8s-ci-robot · 2018-08-23T00:26:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, lavalamp, liggitt

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:

~~api/OWNERS~~ [lavalamp,liggitt]
~~docs/api-reference/OWNERS~~ [lavalamp,liggitt]
~~pkg/apis/OWNERS~~ [lavalamp,liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp,liggitt]

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

k8s-github-robot · 2018-08-23T02:19:29Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-23T02:41:58Z

Automatic merge from submit-queue (batch tested with PRs 67576, 66936). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2018

k8s-ci-robot requested review from deads2k and derekwaynecarr August 2, 2018 21:17

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 2, 2018

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 2, 2018

jennybuckley force-pushed the dry-run-webhooks branch from 45099df to 17f254a Compare August 6, 2018 18:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2018

jennybuckley mentioned this pull request Aug 7, 2018

Block dry-run if a webhook would be called #67085

Merged

jennybuckley force-pushed the dry-run-webhooks branch from 17f254a to 9b63e5c Compare August 7, 2018 16:56

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 7, 2018

lavalamp reviewed Aug 7, 2018

View reviewed changes

jennybuckley force-pushed the dry-run-webhooks branch from 9b63e5c to 68c608f Compare August 7, 2018 21:36

liggitt reviewed Aug 21, 2018

View reviewed changes

jennybuckley mentioned this pull request Aug 21, 2018

Dry run feature documentation 1.12 kubernetes/website#10033

Merged

k8s-ci-robot added the area/apiserver label Aug 21, 2018

jennybuckley force-pushed the dry-run-webhooks branch 5 times, most recently from 7057df3 to 50d9cec Compare August 21, 2018 23:06

k8s-ci-robot assigned lavalamp Aug 22, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 22, 2018

jennybuckley added 2 commits August 22, 2018 16:26

Support dry run in admission webhooks

2d0ec48

Update generated

c61eac7

jennybuckley force-pushed the dry-run-webhooks branch from 50d9cec to c61eac7 Compare August 22, 2018 23:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2018

k8s-ci-robot assigned liggitt Aug 23, 2018

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

k8s-github-robot merged commit 5a16163 into kubernetes:master Aug 23, 2018

ayj mentioned this pull request Feb 5, 2019

Istio yaml structure istio/istio#10429

Closed

ChangyuWang mentioned this pull request Jun 20, 2019

support dry run in galley validation webhook istio/istio#15018

Closed

Support dry run in admission webhooks #66936

Support dry run in admission webhooks #66936

Conversation

jennybuckley commented Aug 2, 2018 • edited Loading

jennybuckley commented Aug 7, 2018

jennybuckley commented Aug 7, 2018

jennybuckley commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Aug 8, 2018

jennybuckley commented Aug 8, 2018

lavalamp commented Aug 8, 2018 via email

jennybuckley commented Aug 8, 2018

lavalamp commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley commented Aug 21, 2018

jennybuckley commented Aug 22, 2018

lavalamp commented Aug 22, 2018

jennybuckley commented Aug 22, 2018

liggitt commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

k8s-github-robot commented Aug 23, 2018

k8s-github-robot commented Aug 23, 2018

jennybuckley commented Aug 2, 2018 •

edited

Loading

liggitt Aug 7, 2018 •

edited

Loading

liggitt Aug 7, 2018 •

edited

Loading

liggitt Aug 7, 2018 •

edited

Loading