Add more detailed plan for admission controller and webhook support to dry-run kep #2387

jennybuckley · 2018-07-18T21:56:11Z

This is to address #2301 (comment), #2301 (comment), and #2301 (comment)

jennybuckley · 2018-07-18T21:59:05Z

cc @liggitt since you noticed these details being missing in the first place
cc @apelisse since you wrote the kep in the first place
/sig api-machinery

liggitt · 2018-07-19T13:58:11Z

keps/0015-dry-run.md

+All built-in admission controllers will have to be checked, and the ones with side
+effects will have to be skipped or changed to only have side effects in the non
+dry-run case. Some examples of built-in admission controllers with the possibility
+forside effects are:


"for side-effects"

liggitt · 2018-07-19T14:02:52Z

keps/0015-dry-run.md

@@ -53,10 +53,52 @@ either as an argument for built-in admission plugins or through a dryRun
 query-parameter for dynamic webhooks, will give them a chance not to have any


clarify the following:

While admission controllers are not supposed to have side-effects when triggered, some of them still do. Quotas for example uses the side-effects when triggered

Side-effects are not prohibited as long as there is a reconciliation loop that accounts for the admission plugin getting called and the mutation not being persisted. The reason we want to provide a dry run indicator to admission plugins is so they can improve user experience in cases where waiting for that reconciliation would be disruptive (like quota).

liggitt · 2018-07-19T14:07:05Z

keps/0015-dry-run.md

+	// will be completely rejected and the webhook will not be called.
+	// Defaults to false.
+	// +optional
+	SupportsDryRun *bool `json:"supportsDryRun,omitempty" protobuf:"varint,6,number,opt,name=supportsDryRun"`


DryRunnable *bool? Per https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#naming-conventions:

The name of a field expressing a boolean property called 'fooable' should be called Fooable, not IsFooable.

That doc also says to think twice about bool fields. Having thought twice about it here, I think subdividing aspects of an admission plugin's dry runnability is too fine-grained, and a binary "supported" / "not supported" is better (especially since we want to default this on in future versions and require full support.

I'll add some justification for this to the doc

liggitt · 2018-07-19T14:11:26Z

keps/0015-dry-run.md

+}
+```
+
+


also need an update to the admission.Attributes interface (e.g. DryRun() bool)

I wasn't sure if that goes in the KEP because it wasn't an API change, but since it's an interface people could import I see how it could still go here.

liggitt · 2018-07-19T14:13:08Z

thanks for the update, this file moved under the api-machinery folder, will need to rebase there

@apelisse were you planning to add details about the proposed behavior of generated fields separately (uid, creationTimestamp, resourceVersion, generateName, etc)?

jennybuckley · 2018-07-19T17:36:32Z

Addressed comments and rebased

liggitt · 2018-07-19T19:29:33Z

keps/sig-api-machinery/0015-dry-run.md

+	// will be completely rejected and the webhook will not be called.
+	// Defaults to false.
+	// +optional
+	DryRunnable *bool `json:"supportsDryRun,omitempty" protobuf:"varint,6,number,opt,name=supportsDryRun"`


nit: switch json and proto tags to match field name (dryRunnable)

liggitt · 2018-07-19T19:31:10Z

keps/sig-api-machinery/0015-dry-run.md

+can explicitly register as having dry-run support.
+If dry-run is requested on a non-supported webhook, the request will be completely rejected,
+as a 400: Bad Request, and the name of the webhook/webhooks which don't support dry run will be
+listed. This field will be defaulted to true and deprecated in v1, and completely removed in v2.


the name of the webhook/webhooks which don't support dry run will be listed

I wouldn't promise that this will surface via the API. We can log it, and consider whether we want this information leaking to the end user.

liggitt · 2018-07-19T19:31:42Z

a couple nits, then this iteration looks good to go in to me. thanks

liggitt · 2018-07-19T20:46:52Z

/lgtm

liggitt · 2018-07-19T20:54:32Z

/assign @deads2k
for approval

deads2k · 2018-07-23T20:34:52Z

/approve

k8s-ci-robot · 2018-07-23T20:34:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

~~keps/sig-api-machinery/OWNERS~~ [deads2k]

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

Automatic merge from submit-queue. 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>. Support dry run in admission plugins **What this PR does / why we need it**: Adds support for dry run to admission controllers as outlined by kubernetes/community#2387 - [x] add IsDryRun() to admission.Attributes interface - [x] add dry run support to NamespaceAutoProvision - [x] add dry run support to ResourceQuota - [x] add dry run support to EventRateLimit The following is being done in a follow up PR: - [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview``` - [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration``` - [x] add dry run support to (Valid|Mut)atingAdmissionWebhook /sig api-machinery **Release note**: ```release-note In clusters where the DryRun feature is enabled, dry-run requests will go through the normal admission chain. Because of this, ImagePolicyWebhook authors should especially make sure that their webhooks do not rely on side effects. ``` Here is a list of the admission controllers that were considered when making this PR: - AlwaysAdmit: No side effects - AlwaysPullImages: No side effects - LimitPodHardAntiAffinityTopology: No side effects - DefaultTolerationSeconds: No side effects - AlwaysDeny: No side effects - EventRateLimit: Has side possible effect of affecting the rate, skipping this entire plugin in dry-run case since it won't correspond to an actual write to etcd anyway - DenyEscalatingExec: No side effects - DenyExecOnPrivileged: Deprecated, and has no side effects - ExtendedResourceToleration: No side effects - OwnerReferencesPermissionEnforcement: No side effects - ImagePolicyWebhook: No side effects* (*this uses a webhook but it is very specialized. It only sees pod container images, for the purpose of accepting or rejecting certain image sources, so it is very unlikely that it would rely on side effects.) - LimitRanger: No side effects - NamespaceAutoProvision: Has possible side effect of creating a namespace, skipping the create in the dry-run case - NamespaceExists: No side effects - NodeRestriction: No side effects - PodNodeSelector: No side effects - PodPreset: No side effects - PodTolerationRestriction: No side effects - Priority: No side effects - ResourceQuota: Has side possible effect of taking up quota, will only check quota but skip changing quota in the dry-run case - PodSecurityPolicy: No side effects - SecurityContextDeny: No side effects - ServiceAccount: No side effects - PersistentVolumeLabel: No side effects - PersistentVolumeClaimResize: No side effects - DefaultStorageClass: No side effects - StorageObjectInUseProtection: No side effects - Initializers: No side effects - NamespaceLifecycle: No side effects - MutatingAdmissionWebhook: Same as below - ValidatingAdmissionWebhook: Has possible side effects depending on if webhook authors depend on side effects and a reconciliation mechanism. To fix this we will expose whether or not a request is dry-run to webhooks through AdmissionReview, and require that all called webhooks understand the field by checking if DryRunnable true is specified in the webhook config. This will be done in a separate PR because it requires an api-change

Automatic merge from submit-queue. 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>. Support dry run in admission plugins **What this PR does / why we need it**: Adds support for dry run to admission controllers as outlined by kubernetes/community#2387 - [x] add IsDryRun() to admission.Attributes interface - [x] add dry run support to NamespaceAutoProvision - [x] add dry run support to ResourceQuota - [x] add dry run support to EventRateLimit The following is being done in a follow up PR: - [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview``` - [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration``` - [x] add dry run support to (Valid|Mut)atingAdmissionWebhook /sig api-machinery **Release note**: ```release-note In clusters where the DryRun feature is enabled, dry-run requests will go through the normal admission chain. Because of this, ImagePolicyWebhook authors should especially make sure that their webhooks do not rely on side effects. ``` Here is a list of the admission controllers that were considered when making this PR: - AlwaysAdmit: No side effects - AlwaysPullImages: No side effects - LimitPodHardAntiAffinityTopology: No side effects - DefaultTolerationSeconds: No side effects - AlwaysDeny: No side effects - EventRateLimit: Has side possible effect of affecting the rate, skipping this entire plugin in dry-run case since it won't correspond to an actual write to etcd anyway - DenyEscalatingExec: No side effects - DenyExecOnPrivileged: Deprecated, and has no side effects - ExtendedResourceToleration: No side effects - OwnerReferencesPermissionEnforcement: No side effects - ImagePolicyWebhook: No side effects* (*this uses a webhook but it is very specialized. It only sees pod container images, for the purpose of accepting or rejecting certain image sources, so it is very unlikely that it would rely on side effects.) - LimitRanger: No side effects - NamespaceAutoProvision: Has possible side effect of creating a namespace, skipping the create in the dry-run case - NamespaceExists: No side effects - NodeRestriction: No side effects - PodNodeSelector: No side effects - PodPreset: No side effects - PodTolerationRestriction: No side effects - Priority: No side effects - ResourceQuota: Has side possible effect of taking up quota, will only check quota but skip changing quota in the dry-run case - PodSecurityPolicy: No side effects - SecurityContextDeny: No side effects - ServiceAccount: No side effects - PersistentVolumeLabel: No side effects - PersistentVolumeClaimResize: No side effects - DefaultStorageClass: No side effects - StorageObjectInUseProtection: No side effects - Initializers: No side effects - NamespaceLifecycle: No side effects - MutatingAdmissionWebhook: Same as below - ValidatingAdmissionWebhook: Has possible side effects depending on if webhook authors depend on side effects and a reconciliation mechanism. To fix this we will expose whether or not a request is dry-run to webhooks through AdmissionReview, and require that all called webhooks understand the field by checking if DryRunnable true is specified in the webhook config. This will be done in a separate PR because it requires an api-change Kubernetes-commit: 6fe7f9f4b70fce08050e8211af6dee09517baef7

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Support dry run in admission webhooks **What this PR does / why we need it**: Follow up to #66391 - [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview``` - [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration``` - [x] add dry run support to (Valid|Mut)atingAdmissionWebhook Includes all the api-changes outlined by kubernetes/community#2387 /sig api-machinery **Release note**: ```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. ```

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Support dry run in admission webhooks **What this PR does / why we need it**: Follow up to kubernetes/kubernetes#66391 - [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview``` - [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration``` - [x] add dry run support to (Valid|Mut)atingAdmissionWebhook Includes all the api-changes outlined by kubernetes/community#2387 /sig api-machinery **Release note**: ```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. ``` Kubernetes-commit: 5a16163c87fe2a90916a51b52771a668bcaf2a0d

…ssion Add more detailed plan for admission controller and webhook support to dry-run kep

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Jul 18, 2018

k8s-ci-robot requested review from bgrant0607 and calebamiles July 18, 2018 21:56

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2018

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 18, 2018

liggitt reviewed Jul 19, 2018

View reviewed changes

jennybuckley force-pushed the dry-run-kep-admission branch from cb0d5b2 to 2421896 Compare July 19, 2018 17:28

jennybuckley mentioned this pull request Jul 19, 2018

Support dry run in admission plugins kubernetes/kubernetes#66391

Merged

7 tasks

jennybuckley changed the title ~~Add more detailed plan for webhook support to dry-run kep~~ Add more detailed plan for admission controller and webhook support to dry-run kep Jul 19, 2018

liggitt reviewed Jul 19, 2018

View reviewed changes

Add admission api changes to dry run kep

c32cfac

jennybuckley force-pushed the dry-run-kep-admission branch from 2421896 to c32cfac Compare July 19, 2018 19:44

k8s-ci-robot assigned liggitt Jul 19, 2018

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

k8s-ci-robot assigned deads2k Jul 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2018

k8s-ci-robot merged commit b5ab8a9 into kubernetes:master Jul 23, 2018

jennybuckley mentioned this pull request Aug 2, 2018

Support dry run in admission webhooks kubernetes/kubernetes#66936

Merged

3 tasks

calebamiles pushed a commit to calebamiles/community that referenced this pull request Sep 5, 2018

Merge pull request kubernetes#2387 from jennybuckley/dry-run-kep-admi…

667e8b2

…ssion Add more detailed plan for admission controller and webhook support to dry-run kep

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more detailed plan for admission controller and webhook support to dry-run kep #2387

Add more detailed plan for admission controller and webhook support to dry-run kep #2387

jennybuckley commented Jul 18, 2018

jennybuckley commented Jul 18, 2018

liggitt Jul 19, 2018

liggitt Jul 19, 2018

liggitt Jul 19, 2018

jennybuckley Jul 19, 2018

liggitt Jul 19, 2018

jennybuckley Jul 19, 2018

liggitt commented Jul 19, 2018

jennybuckley commented Jul 19, 2018

liggitt Jul 19, 2018

liggitt Jul 19, 2018

liggitt commented Jul 19, 2018 •

edited

Loading

liggitt commented Jul 19, 2018

liggitt commented Jul 19, 2018

deads2k commented Jul 23, 2018

k8s-ci-robot commented Jul 23, 2018

		@@ -53,10 +53,52 @@ either as an argument for built-in admission plugins or through a dryRun
		query-parameter for dynamic webhooks, will give them a chance not to have any

Add more detailed plan for admission controller and webhook support to dry-run kep #2387

Add more detailed plan for admission controller and webhook support to dry-run kep #2387

Conversation

jennybuckley commented Jul 18, 2018

jennybuckley commented Jul 18, 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

liggitt commented Jul 19, 2018

jennybuckley commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jul 19, 2018 • edited Loading

liggitt commented Jul 19, 2018

liggitt commented Jul 19, 2018

deads2k commented Jul 23, 2018

k8s-ci-robot commented Jul 23, 2018

liggitt commented Jul 19, 2018 •

edited

Loading