Prevent webhooks from affecting admission requests for WebhookConfiguration objects #59840

jennybuckley · 2018-02-14T00:16:38Z

What this PR does / why we need it:
As it stands now webhooks can be added to the system which make it impossible for a user to remove that webhook, or two webhooks could be registered which make it impossible to remove each other.

The first commit of this will add a test to make sure webhook deletion is never blocked by a webhook. This test will fail until the second commit is added which will prevent webhooks from affecting admission requests for ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects in the admissionregistration.k8s.io group

Test that webhook deletion is never blocked by a webhook (test fails before second commit)
Prevent webhooks from being called on admission requests for [Validating|Mutating]WebhookConfiguration objects
Document this new behavior maybe in another PR

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of fixing #59124 (Verifies that it can remove the broken webhook.)

Release note:

ValidatingWebhooks and MutatingWebhooks will not be called on admission requests for ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects in the admissionregistration.k8s.io group

jennybuckley · 2018-02-14T18:19:05Z

Besides
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go#L136-L138
and maybe
kubernetes/website#6836
Is there anywhere else that this behavior needs to be documented?

mbohlool · 2018-02-15T21:18:54Z

/assign @caesarxuchao

caesarxuchao · 2018-02-15T22:31:21Z

test/e2e/apimachinery/webhook.go

@@ -123,6 +125,12 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		testCRDWebhook(f, testcrd.Crd, testcrd.DynamicClient)
 	})

+	It("Should not be able to prevent deleting validating-webhook-configurations", func() {


The same is true for the mutating-webhook-configurations, could you expand the test?

caesarxuchao · 2018-02-15T22:35:23Z

test/e2e/apimachinery/webhook.go

+						// NOTE: This path isn't something the service knows how to handle.
+						Path: strPtr("/validating-webhook-configurations"),
+					},
+					// Without CA bundle, the call to webhook always fails


Could you add to the comment that the failure policy is "Fail"? Otherwise this test would have given us false negative.

I am working on changing this to use the correct CA and updating the webhook test image to handle a function that always denies requests so this test doesn't have to rely on a broken CA and failing closed to deny requests.

I think that will be a more clear way of testing this.

caesarxuchao · 2018-02-15T22:36:55Z

Why is the new e2e test passing? I thought we haven't built in the mechanism to skip webhook configurations.

jennybuckley · 2018-02-20T20:24:45Z

@caesarxuchao ~~We haven't built that mechanism yet (this test isn't working how it should)~~

edit: now the test fails as expected before the second commit, and passes after the second commit.

jennybuckley · 2018-02-28T16:50:45Z

/kind bug

lavalamp · 2018-02-28T17:45:06Z

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

+		return true
+	} else {
+		return false
+	}


optional nit (shorter/prettier/clearer):

if gvk.Group == "admissionregistration.k8s.io" { if gvk.Kind == "ValidatingWebhookConfiguration" || gvk.Kind == "MutatingWebhookConfiguration" { return true } } return false

Maybe this could go in k8s.io/apiserver/pkg/admission/plugin/webhook/config or k8s.io/apiserver/pkg/admission/plugin/webhook/request? It looks like there's already a few packages there that might be appropriate.

I think webhook/config is for the plugin config passed to the apiserver at startup, and webhook/request "creates admissionReview request based on admission attributes" according to the doc.go, maybe a new package like "common" or "util"?
or are we trying to avoid that.

lavalamp · 2018-02-28T17:49:17Z

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

@@ -253,6 +257,18 @@ func (a *MutatingWebhook) Admit(attr admission.Attributes) error {
 	return a.convertor.Convert(versionedAttr.Object, attr.GetObject())
 }

+// TODO: factor into a common place along with the validating webhook version.
+func (a *MutatingWebhook) shouldSkipAllHooks(attr admission.Attributes) bool {


Optional nit: in this case I think it's best to highlight the reason for skipping-- maybe "isWebhookConfigurationResource"?

If we grow additional reasons here, then I'd go for the current name.

lavalamp · 2018-02-28T17:51:38Z

This is clearly a bug fix, seems OK.

jennybuckley · 2018-03-01T21:40:27Z

/retest

jennybuckley · 2018-03-01T22:26:48Z

/retest

jennybuckley · 2018-03-02T19:18:00Z

/priority important-soon

lavalamp · 2018-03-05T22:51:11Z

staging/src/k8s.io/api/admissionregistration/v1beta1/types.go

+	// However, in order to prevent ValidatingAdmissionWebhooks and MutatingAdmissionWebhooks
+	// from putting the cluster in a state which cannot be recovered from without completely
+	// disabling the plugin, ValidatingAdmissionWebhooks and MutatingAdmissionWebhooks are never called
+	// on admission requests for ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects


Nit: missing final period. (this goes into the api documentation or I wouldn't care)

lavalamp · 2018-03-05T22:59:42Z

test/e2e/apimachinery/webhook.go

+					Service: &v1beta1.ServiceReference{
+						Namespace: namespace,
+						Name:      serviceName,
+						Path:      strPtr(""),


Add a comment explaining what the webhook will do when called?

lavalamp · 2018-03-05T23:02:39Z

test/e2e/apimachinery/webhook.go

+	}
+}
+
+func testWebhookForWebhookConfigurations(f *framework.Framework) {


Perhaps add a comment that this test assumes the deletion-rejecting webhook from registerWebhookForWebhookConfigurations is in place?

lavalamp · 2018-03-05T23:03:54Z

Sorry I found a few more things--no more, I promise. Can you fix and squash non-generated changes, then I will lgtm & approve.

jennybuckley · 2018-03-05T23:43:24Z

@lavalamp should be good now

lavalamp · 2018-03-06T01:25:04Z

/retest

lavalamp · 2018-03-06T01:25:46Z

/lgtm
/approve

k8s-ci-robot · 2018-03-06T01:25:52Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]
~~docs/api-reference/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]

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-03-06T01:26:07Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@caesarxuchao @jennybuckley @lavalamp

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-ci-robot · 2018-03-06T01:27:16Z

@jennybuckley: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`ed109f7`	link	`/test pull-kubernetes-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jennybuckley · 2018-03-06T01:35:04Z

/retest

k8s-github-robot · 2018-03-06T02:23:01Z

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

k8s-github-robot · 2018-03-06T03:09:33Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from mikedanese and rmmh February 14, 2018 00:16

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

jennybuckley changed the title ~~[WIP] Prevent webhooks from affecting admission requests for WebhookConfiguration objects~~ Prevent webhooks from affecting admission requests for WebhookConfiguration objects Feb 14, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2018

jennybuckley force-pushed the webhooks-on-webhooks branch from 8ecac7c to c71f1c2 Compare February 14, 2018 17:58

k8s-ci-robot assigned caesarxuchao Feb 15, 2018

caesarxuchao reviewed Feb 15, 2018

View reviewed changes

jennybuckley force-pushed the webhooks-on-webhooks branch 2 times, most recently from bdbf2c0 to 174033d Compare February 27, 2018 22:34

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 28, 2018

lavalamp reviewed Feb 28, 2018

View reviewed changes

lavalamp added this to the v1.10 milestone Feb 28, 2018

lavalamp added the status/approved-for-milestone label Feb 28, 2018

jennybuckley force-pushed the webhooks-on-webhooks branch from 174033d to 6bd940b Compare February 28, 2018 18:31

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

jennybuckley force-pushed the webhooks-on-webhooks branch from 6bd940b to a65bbe2 Compare February 28, 2018 18:41

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

jennybuckley force-pushed the webhooks-on-webhooks branch from a65bbe2 to 9f1be2f Compare February 28, 2018 19:38

k8s-github-robot added the milestone/incomplete-labels label Mar 2, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 2, 2018

k8s-github-robot removed the milestone/incomplete-labels label Mar 2, 2018

jennybuckley mentioned this pull request Mar 5, 2018

Use semantic equality in client ca post start hook #60771

Merged

lavalamp reviewed Mar 5, 2018

View reviewed changes

jennybuckley force-pushed the webhooks-on-webhooks branch from 3059ae4 to bba4aee Compare March 5, 2018 23:42

jennybuckley added 2 commits March 5, 2018 16:35

Prevent webhooks from affecting admission requests for webhooks

58b43ad

Run hack/update-all.sh

b4abf56

jennybuckley force-pushed the webhooks-on-webhooks branch from bba4aee to b4abf56 Compare March 6, 2018 00:36

k8s-ci-robot assigned lavalamp Mar 6, 2018

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

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

k8s-github-robot merged commit 5066a67 into kubernetes:master Mar 6, 2018

jennybuckley mentioned this pull request Mar 22, 2018

Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

Merged

jennybuckley mentioned this pull request Apr 4, 2018

admission/webhook: deduplicate mutating and validating webhook code #58381

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent webhooks from affecting admission requests for WebhookConfiguration objects #59840

Prevent webhooks from affecting admission requests for WebhookConfiguration objects #59840

jennybuckley commented Feb 14, 2018 •

edited

jennybuckley commented Feb 14, 2018

mbohlool commented Feb 15, 2018

caesarxuchao Feb 15, 2018

caesarxuchao Feb 15, 2018

jennybuckley Feb 20, 2018 •

edited

caesarxuchao commented Feb 15, 2018

jennybuckley commented Feb 20, 2018 •

edited

jennybuckley commented Feb 28, 2018

lavalamp Feb 28, 2018

jennybuckley Feb 28, 2018 •

edited

lavalamp Feb 28, 2018

lavalamp commented Feb 28, 2018

jennybuckley commented Mar 1, 2018

jennybuckley commented Mar 1, 2018

jennybuckley commented Mar 2, 2018

lavalamp Mar 5, 2018

lavalamp Mar 5, 2018

lavalamp Mar 5, 2018

lavalamp commented Mar 5, 2018

jennybuckley commented Mar 5, 2018

lavalamp commented Mar 6, 2018

lavalamp commented Mar 6, 2018

k8s-ci-robot commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

k8s-ci-robot commented Mar 6, 2018 •

edited

jennybuckley commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

Prevent webhooks from affecting admission requests for WebhookConfiguration objects #59840

Prevent webhooks from affecting admission requests for WebhookConfiguration objects #59840

Conversation

jennybuckley commented Feb 14, 2018 • edited

jennybuckley commented Feb 14, 2018

mbohlool commented Feb 15, 2018

caesarxuchao Feb 15, 2018

Choose a reason for hiding this comment

caesarxuchao Feb 15, 2018

Choose a reason for hiding this comment

jennybuckley Feb 20, 2018 • edited

Choose a reason for hiding this comment

caesarxuchao commented Feb 15, 2018

jennybuckley commented Feb 20, 2018 • edited

jennybuckley commented Feb 28, 2018

lavalamp Feb 28, 2018

Choose a reason for hiding this comment

jennybuckley Feb 28, 2018 • edited

Choose a reason for hiding this comment

lavalamp Feb 28, 2018

Choose a reason for hiding this comment

lavalamp commented Feb 28, 2018

jennybuckley commented Mar 1, 2018

jennybuckley commented Mar 1, 2018

jennybuckley commented Mar 2, 2018

lavalamp Mar 5, 2018

Choose a reason for hiding this comment

lavalamp Mar 5, 2018

Choose a reason for hiding this comment

lavalamp Mar 5, 2018

Choose a reason for hiding this comment

lavalamp commented Mar 5, 2018

jennybuckley commented Mar 5, 2018

lavalamp commented Mar 6, 2018

lavalamp commented Mar 6, 2018

k8s-ci-robot commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

k8s-ci-robot commented Mar 6, 2018 • edited

jennybuckley commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

k8s-github-robot commented Mar 6, 2018

jennybuckley commented Feb 14, 2018 •

edited

jennybuckley Feb 20, 2018 •

edited

jennybuckley commented Feb 20, 2018 •

edited

jennybuckley Feb 28, 2018 •

edited

k8s-ci-robot commented Mar 6, 2018 •

edited