Adding ObjectSelector to admission webhooks by caesarxuchao · Pull Request #78226 · kubernetes/kubernetes

caesarxuchao · 2019-05-23T00:54:32Z

/assign
/sig api-machinery

I only implemented it for the main resource. The behavior for subresources is undefined.

I added to the admission webhook integration test to make sure webhooks are called if labels matches the object selector. The test also makes sure in-flight label changes made by the mutation webhooks are not counted when compared to the object selector.

TODO:

update the API doc to warn people that the objectSelector should only be used for opt-in webhooks.

I still need to do some code cleanup, so marked as WIP. cc @liggitt @jpbetz @roycaihw

Added object selector to admission webhook configurations.
The object selector applies to an object's labels. For CREATE and UPDATE, the object is the new object. For DELETE, it is existing object (passed as oldObject in the admissionRequest).
Admission behavior for subresources with non-empty object selector is undefined currently.
Use the object selector only if the webhook is opt-in, because end users may skip the admission webhook by setting the labels.

caesarxuchao · 2019-05-23T22:27:46Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

The changes to the patch handler are complicated. I need to extract the labels after the patch is applied to the existing object, but before the running the mutation webhooks (applyAdmission), and pass the labels to the validation webhooks (encapsuled in createValidation and updateValidation). I used the *ctx.LabelsHolder to pass the labels.

caesarxuchao · 2019-05-24T00:14:45Z

/assign @jpbetz @liggitt
/unassign

I can still clean up the integration test more, but I want to get some early feedback. Thank you.

roycaihw · 2019-05-24T21:00:23Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

in what cases do we allow error?

extensions/v1beta1/deployments/rollback does not implement the Object interface. That seems to be the only exceptions.

I updated the update.go to return NewInternalError if meta.Accessor() fails.

Looks safe. Log an error if it ever does happen just so we're not entirely silent about the error?

roycaihw · 2019-05-24T21:14:04Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

is the returning error behavior here different from other verbs?

roycaihw · 2019-05-24T21:59:12Z

test/integration/apiserver/admissionwebhook/admission_test.go

it seems that the update doesn't tolerate modified object. I got the following flake locally:

--- FAIL: TestWebhookV1beta1/apiextensions.k8s.io.v1beta1.customresourcedefinitions/delete (0.35s) admission_test.go:608: Operation cannot be fulfilled on customresourcedefinitions.apiextensions.k8s.io "openshiftwebconsoleconfigs.webconsole.operator.openshift.io": the object has been modified; please apply your changes to the latest version and try again

Thanks. I'll update the code.

caesarxuchao · 2019-05-24T22:44:57Z

I believe that the flake I encountered was due to the watch cache. The object in watch cache contains already removed labels, thus the request was dispatched to wrong webhooks.

It verifies that object selector is respected. It verifies that labels added by the mutation webhooks are not affecting object selector decision in the same round of admission.

if the obj does not implement the Object interface. The only exception is the CREATE handler, as extension/v1beta1/deployments/rollback does not implement the interface.

caesarxuchao · 2019-05-25T00:39:04Z

/retest

Integration test failure not related.

k8s-ci-robot · 2019-05-25T00:52:32Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`3264ace`	link	`/test pull-kubernetes-verify`

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.

Details

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.

jpbetz · 2019-05-25T05:46:54Z

pkg/apis/admissionregistration/types.go

+	// object (passed as oldObject in the admissionRequest).
+	// Admission behavior for subresources with non-empty object selector
+	// is undefined currently.
+	// Use the object selector only if the webhook is opt-in, because end


In addition to allowing users to opt-in to a webhook, labels also make it possible to ensure that all objects with a particular label have been admitted by the webhooks for that label.

jpbetz · 2019-05-25T05:50:19Z

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

+type Matcher struct {
+}
+
+// MatchObjectSelector decideds whether the request matches the ObjectSelector


s/decideds/decides

jpbetz · 2019-05-25T05:52:55Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

Looks safe. Log an error if it ever does happen just so we're not entirely silent about the error?

jpbetz · 2019-05-25T06:02:46Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

+	if !ok {
+		return nil, errors.NewInternalError(fmt.Errorf("expect labelsHolder set in context"))
+	}
+	labelsHolder.SetLabels(labels)


Add comment here summarizing why/how labels are messaged via the context? Seems like we should comment about that somewhere..

sttts · 2019-05-27T08:50:41Z

pkg/apis/admissionregistration/types.go

+	// users may skip the admission webhook by setting the labels.
+	// Default to the empty LabelSelector, which matches everything.
+	// +optional
+	ObjectSelector *metav1.LabelSelector


Why not ObjectLabelSelector ?

sttts · 2019-05-27T08:51:22Z

pkg/apis/admissionregistration/types.go

+	// UPDATE, the object is the new object. For DELETE, it is existing
+	// object (passed as oldObject in the admissionRequest).
+	// Admission behavior for subresources with non-empty object selector
+	// is undefined currently.


can we define it?

sttts · 2019-05-27T09:43:03Z

staging/src/k8s.io/apiserver/pkg/admission/attributes.go

 		dryRun:      dryRun,
 		object:      object,
 		oldObject:   oldObject,
+		labels:      labels,


why do we need labels globally throughout the code base and not only in the webhooks. This looks wrong. Please move that logic into the webhook.

sttts · 2019-05-27T09:43:59Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

 }

-type mutateObjectUpdateFunc func(obj, old runtime.Object) error
+type mutateObjectUpdateFunc func(ctx context.Context, obj, old runtime.Object) error


why this change?

sttts · 2019-05-27T09:47:26Z

I am not happy about the ripples of the change. I am against merging it in this way.

/hold

caesarxuchao · 2019-05-29T06:50:29Z

The requirement of extracting labels "at the admission time before running any plugins and will be used for all webhooks during the admission process" makes the implementation complicated. Especially for PATCH operations, I needed to extract the labels after the patch is applied, but before applying the mutation webhooks in this line, thus I used context to hold and pass the labels.

One alternative I had considered was letting the mutation webhook controller extract the labels. The drawbacks are:

built-in admission plugins can change object labels (e.g., storage/pv/label/admission)
it doesn't work for subresources. According to the KEP, sometimes we need to extract the labels from the parent object, which is not accessible by the mutation webhook controller.

@sttts

k8s-ci-robot · 2019-05-29T17:53:47Z

@caesarxuchao: PR needs rebase.

Details

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.

liggitt · 2019-05-29T20:47:29Z

Talked through the various issues with @sttts, @caesarxuchao, @jpbetz, and @roycaihw. most of the difficulties with this implementation came from the original behavior specified in the KEP, which in retrospect led to very counter-intuitive behavior from an end-user's perspective.

The updated behavior we landed on (which is much easier to explain and to implement... double win):

To determine whether the objectSelector matches given admission attributes:

An empty objectSelector always matches
A non-empty objectSelector is evaluated against both the oldObject and newObject that would be sent to the webhook, and is considered to match if either object matches the selector. A null object (for example, oldObject in the case of create, or newObject in the case of delete) or an object that cannot have labels (like a DeploymentRollback object) is not considered to match.

sttts · 2019-05-29T21:04:44Z

I very much prefer what @liggitt suggests in #78226 (comment). Feel free to unhold the PR with such an implementation.

caesarxuchao · 2019-05-30T00:35:22Z

Superseded by #78505

k8s-ci-robot assigned caesarxuchao May 23, 2019

caesarxuchao changed the title ~~Adding ObjectSelector to admission webhooks~~ [WIP] Adding ObjectSelector to admission webhooks May 23, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2019

k8s-ci-robot requested review from andrewsykim and deads2k May 23, 2019 00:55

caesarxuchao force-pushed the object-selector-admission-webhook branch from 252773a to 13ef626 Compare May 23, 2019 22:14

caesarxuchao commented May 23, 2019

View reviewed changes

caesarxuchao force-pushed the object-selector-admission-webhook branch from 13ef626 to c2558a1 Compare May 23, 2019 23:44

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 23, 2019

caesarxuchao force-pushed the object-selector-admission-webhook branch from 59426bd to 32297f4 Compare May 24, 2019 00:13

caesarxuchao changed the title ~~[WIP] Adding ObjectSelector to admission webhooks~~ Adding ObjectSelector to admission webhooks May 24, 2019

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 May 24, 2019

k8s-ci-robot assigned jpbetz and unassigned caesarxuchao May 24, 2019

roycaihw reviewed May 24, 2019

View reviewed changes

Chao Xu added 6 commits May 24, 2019 16:37

Add integration test to verify object selector.

9478fac

It verifies that object selector is respected. It verifies that labels added by the mutation webhooks are not affecting object selector decision in the same round of admission.

implementation of objectSelector

ece1d60

minor changes, populating the signatures changes

3d75028

unit test fix

cf4ed08

api disclaimer for subresources

b661161

Address Haowei's comments to return internal error in endpoint handlers

3264ace

if the obj does not implement the Object interface. The only exception is the CREATE handler, as extension/v1beta1/deployments/rollback does not implement the interface.

caesarxuchao force-pushed the object-selector-admission-webhook branch from 9cf4343 to 3264ace Compare May 25, 2019 00:04

jpbetz reviewed May 25, 2019

View reviewed changes

sttts reviewed May 27, 2019

View reviewed changes

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

sttts self-assigned this May 27, 2019

liggitt added this to the v1.15 milestone May 29, 2019

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

liggitt added the area/admission-control label May 29, 2019

caesarxuchao mentioned this pull request May 30, 2019

Adding ObjectSelector to admission webhooks #78505

Merged

caesarxuchao closed this May 30, 2019

liggitt mentioned this pull request May 31, 2019

update objectSelector design kubernetes/enhancements#1079

Merged

Conversation

caesarxuchao commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 24, 2019

Uh oh!

caesarxuchao commented May 25, 2019

Uh oh!

k8s-ci-robot commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented May 27, 2019

Uh oh!

caesarxuchao commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 29, 2019

Uh oh!

liggitt commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sttts commented May 29, 2019

Uh oh!

caesarxuchao commented May 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

caesarxuchao commented May 23, 2019 •

edited

Loading

k8s-ci-robot commented May 25, 2019 •

edited

Loading

caesarxuchao commented May 29, 2019 •

edited

Loading

liggitt commented May 29, 2019 •

edited

Loading