Matchconditions admission webhooks alpha implementation for kep-3716 #116261

ivelichkovich · 2023-03-03T20:26:53Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Adds MatchConditions field to ValidatingWebhookConfiguration and MutatingWebhookConfiguration which contains CEL expressions for matching on the webhooks.

Which issue(s) this PR fixes:

kubernetes/enhancements#3716

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added the `MatchConditions` field to `ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration` for the v1beta and v1 apis. 

The `AdmissionWebhookMatchConditions` featuregate is now in Alpha

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[<link>](https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3716-admission-webhook-match-conditions)

k8s-ci-robot · 2023-03-03T20:26:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tallclair

API review comments. Will follow up with code review.

pkg/apis/admissionregistration/types.go

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

pkg/apis/admissionregistration/types.go

tallclair

I'd recommend pulling the migrate versionedattr to avoid circular dependency commit into a separate PR. Should be a trivial refactor, and helps focus this PR.

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

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

pkg/apis/admissionregistration/validation/validation.go

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

tallclair · 2023-03-03T23:17:28Z

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

+
+	evalResults, err := v.filter.ForInput(versionedAttr, nil, cel.CreateAdmissionRequest(versionedAttr.Attributes))
+	if err != nil {
+		// on error ignore the match condition, don't apply matching


I wonder if we should consider emitting a (rate limited) event on the webhook object?

If so, you can leave it as a TODO for now.

Sure, added a todo about an error event

Should that code eventually live here (which is shared with embedded admission) or in webhook.go?

fail open vs. fail closed on error here ins interesting.

Fail closed could be one of:

sending the request to the webhook for admission? If the webhook is not designed to handle request that the matchCondition was suppose to filter out, then that could cause problems.

denying admission? For webhooks with FailurePolicy: Ignore this might be suprising.

Fail open would mean skipping the webhook? For webhooks that are suppose to enforce something critical, and have FailurePolicy: Fail, this might be surprising?

Should FailurePolicy be extended control how errors are handled here?

So what's interesting is that the failPolicy here is different than the failPolicy for ValidatingAdmissionPolicy in a way. For VAP the validation is the CEL evaluations so failurePolicy handling errors there makes sense. When I think of failurePolicy for VWH it's a failure calling the webhook. This is an error deciding if it should even call the webhook so imo it's a little different, that's why initially we just went with ignoring the errors. I'm open to doing it either way though

So it wouldn't fail open in the sense that things would avoid the webhook, what will happen here is that additional things would be included and sent to the webhook as the matchConditions can only be used to reduce the scope of a webhook.

If we go the other way for example (or made it configurable), a bad evaluation of a rule would basically make the webhook useless as it wouldn't target anything at all because we're doing an AND of the matchConditions.

With type checking, I'm assuming that errors here should be rare. I'm guessing the most likely cause of an error would be using secondary authz, and a failure calling an authorization webhook? In that case, I'm guessing that would be used for skipping the webhook if a user had a certain privilege. In that case, the safe thing would be to call the webhook anyway.

For the case of the webhook being called on a resource it doesn't support - that should really be handled by the rules, not the MatchConditions. In fact, we may want to add a comment that rules & namespace selector should be used over MatchConditions when possible.

Will type checking always be effective? Thinking of CRDs and uses of embedded resources here.

There is also the possibility of non-schema errors, like divide-by-zero.

In my mind, the CEL code here is essentially an extension of the webhook code itself and using FailurePolicy makes sense:

If failure policy is Fail, then I read that as the user saying "if I'm not sure what the webhook's response to this would have been, we should be pessimistic", Ignore would have the opposite interpretation.

One benefit of the current "on failure, call the webhook anyway" approach is that on the off chance the webhook should have been called, the system will behave appropriately, maybe allowing resources that should be allowed that would otherwise be rejected.

One downside is that for "ignore on failure" webhooks, you may get over-enforcement: the exclude logic that someone wrote is no longer properly excluding things.

The comparison seems like a wash to me, so I'd go with whatever would be least surprising to a user. IMO that would be to use failure policy, but I could certainly see using docs or similar to set a different expectation.

TL;DR either works, but I have a slight preference for @jpbetz 's approach.

So one thing though is if the user wants the webhook to be pessimistic and block things when its unsure, using the same failurePolicy for matchConditions would have the opposite effect. For example, if the failPolicy causes the match condition to evaluate to false, then the match condition will not match and the webhook will not get called and the request will be allowed in.

Failure policy == fail doesn't mean match condition error => no match, it would mean match condition error => reject the request

tallclair · 2023-03-03T23:22:27Z

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

+				klog.Error("Invalid type conversion to MatchCondition")
+				continue


I wonder if this should return an error instead? It should never happen...

Yeah we can do that. How should we handle errors returned from matcher in generic/webhook.go? Log the at high verbosity?

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

ivelichkovich · 2023-03-04T15:33:31Z

Moved versionattr package migration for circular dependency to this PR #116264

k8s-ci-robot · 2023-03-06T15:02:47Z

@rjsadow: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

release-note Added the `MatchConditions` field to `ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration` for the v1beta and v1 apis. 

The `AdmissionWebhookMatchConditions` featuregate is now in Alpha

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.

rjsadow · 2023-03-06T15:04:06Z

/release-note-edit

Added the `MatchConditions` field to `ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration` for the v1beta and v1 apis. 

The `AdmissionWebhookMatchConditions` featuregate is now in Alpha

pkg/apis/admissionregistration/types.go

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

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go

liggitt · 2023-03-14T23:47:41Z

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

+		if m.failPolicy == v1.Fail {
+			return MatchResult{
+				Error: err,
+			}
+		} else if m.failPolicy == v1.Ignore {
+			return MatchResult{
+				Matches: false,
+			}
+		}


Make a note to adjust this in a follow-up to add a default case (probably with an internal server error or something) if the failurePolicy is neither of these expected values

That's not possible in the API today, but falling through to range evalResults isn't what we'd want to do.

+1

In the same follow up, I'd extract this to a helper function since the logic is used in 2 places.

tallclair · 2023-03-15T00:04:15Z

/lgtm

k8s-ci-robot · 2023-03-15T00:04:21Z

LGTM label has been added.

Git tree hash: 8e8bdc73059105643136720a812defd5dd81c8fc

tallclair · 2023-03-15T01:08:53Z

/lgtm

k8s-ci-robot · 2023-03-15T01:08:59Z

LGTM label has been added.

Git tree hash: a45dbfd0b9a3b5f544ef9043b39ee9419211aef8

This PR is based off of, and dependent on the following PR: kubernetes#116261 Signed-off-by: Max Smythe <smythe@google.com>

* Add custom match conditions for CEL admission This PR is based off of, and dependent on the following PR: #116261 Signed-off-by: Max Smythe <smythe@google.com> * run `make update` Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Update compatibility test data Signed-off-by: Max Smythe <smythe@google.com> * Revert "Update compatibility test data" This reverts commit 312ba7f. * Allow params during validation; make match conditions optional Signed-off-by: Max Smythe <smythe@google.com> * Add conditional ignoring of matcher CEL expression validation on update Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Add more validation tests Signed-off-by: Max Smythe <smythe@google.com> * Short-circuit CEL matcher when no matchers specified Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Address review comments Signed-off-by: Max Smythe <smythe@google.com> --------- Signed-off-by: Max Smythe <smythe@google.com>

* Add custom match conditions for CEL admission This PR is based off of, and dependent on the following PR: kubernetes/kubernetes#116261 Signed-off-by: Max Smythe <smythe@google.com> * run `make update` Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Update compatibility test data Signed-off-by: Max Smythe <smythe@google.com> * Revert "Update compatibility test data" This reverts commit 312ba7f9e74e0ec4a7ac1f07bf575479c608af28. * Allow params during validation; make match conditions optional Signed-off-by: Max Smythe <smythe@google.com> * Add conditional ignoring of matcher CEL expression validation on update Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Add more validation tests Signed-off-by: Max Smythe <smythe@google.com> * Short-circuit CEL matcher when no matchers specified Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Address review comments Signed-off-by: Max Smythe <smythe@google.com> --------- Signed-off-by: Max Smythe <smythe@google.com> Kubernetes-commit: e5fd204c33e90a7e8f5a0ee70242f1296a5ec7af

Description: * Allows for the bypassing of admission controller webhook for certain resources. Upstream PR, Issue, KEP, etc. links: * See below. If this patch is based on an upstream commit, how (if at all) do this patch and the upstream source differ? * N/A If this patch's changes have not been added by upstream, why not? * This patch has not been added by upstream, but work is being actively done to add the ability to filter webhooks. See: - KEP-3716 -- https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3716-admission-webhook-match-conditions and kubernetes/enhancements#3717 - Tracking issue -- kubernetes/enhancements#3716 - Kubernetes PR kubernetes#116261 -- kubernetes#116261 Other patches related to this patch: * None Changes made to this patch after its initial creation and reasons for these changes: * February 2, 2023 -- This patch replaced 0002-EKS-PATCH-Bypassed-admission-controller-webhook-for-.patch (https://github.com/aws/eks-distro/blob/v1-22-eks-18/projects/kubernetes/kubernetes/1-22/patches/0002-EKS-PATCH-Bypassed-admission-controller-webhook-for-.patch). The difference between this patch and its predecessor is that this patch allows for users to supply a config file that defines which webhooks should be excluded. If this files is not provided, this patch and the previous one function the same. Kubernetes version this patch can be dropped: * When the upstream efforts to implement these changes are merged. Signed-off-by: Davanum Srinivas <davanum@gmail.com>

Description: * Allows for the bypassing of admission controller webhook for certain resources. Upstream PR, Issue, KEP, etc. links: * See below. If this patch is based on an upstream commit, how (if at all) do this patch and the upstream source differ? * N/A If this patch's changes have not been added by upstream, why not? * This patch has not been added by upstream, but work is being actively done to add the ability to filter webhooks. See: - KEP-3716 -- https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3716-admission-webhook-match-conditions and kubernetes/enhancements#3717 - Tracking issue -- kubernetes/enhancements#3716 - Kubernetes PR kubernetes#116261 -- kubernetes#116261 Other patches related to this patch: * None Changes made to this patch after its initial creation and reasons for these changes: * February 2, 2023 -- This patch replaced 0002-EKS-PATCH-Bypassed-admission-controller-webhook-for-.patch (https://github.com/aws/eks-distro/blob/v1-22-eks-18/projects/kubernetes/kubernetes/1-22/patches/0002-EKS-PATCH-Bypassed-admission-controller-webhook-for-.patch). The difference between this patch and its predecessor is that this patch allows for users to supply a config file that defines which webhooks should be excluded. If this files is not provided, this patch and the previous one function the same. Kubernetes version this patch can be dropped: * When the upstream efforts to implement these changes are merged. * We can replace with new match using match conditions in 1.28+ or when match conditions become GA Signed-off-by: Jyoti Mahapatra <jyotima@amazon.com>

…ubernetes#116261) * api changes adding match conditions * feature gate and registry strategy to drop fields * matchConditions logic for admission webhooks * feedback * update test * import order * bears.com * update fail policy ignore behavior * update docs and matcher to hold fail policy as non-pointer * update matcher error aggregation, fix early fail failpolicy ignore, update docs * final cleanup * openapi gen

* Add custom match conditions for CEL admission This PR is based off of, and dependent on the following PR: kubernetes#116261 Signed-off-by: Max Smythe <smythe@google.com> * run `make update` Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Fix unit tests Signed-off-by: Max Smythe <smythe@google.com> * Update compatibility test data Signed-off-by: Max Smythe <smythe@google.com> * Revert "Update compatibility test data" This reverts commit 312ba7f. * Allow params during validation; make match conditions optional Signed-off-by: Max Smythe <smythe@google.com> * Add conditional ignoring of matcher CEL expression validation on update Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Add more validation tests Signed-off-by: Max Smythe <smythe@google.com> * Short-circuit CEL matcher when no matchers specified Signed-off-by: Max Smythe <smythe@google.com> * Run codegen Signed-off-by: Max Smythe <smythe@google.com> * Address review comments Signed-off-by: Max Smythe <smythe@google.com> --------- Signed-off-by: Max Smythe <smythe@google.com>

k8s-ci-robot requested review from alexzielenski and apelisse March 3, 2023 20:27

ivelichkovich force-pushed the matchconditions branch from 7807278 to 52740ba Compare March 3, 2023 20:54

tallclair reviewed Mar 3, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/attributes.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/request/admissionreview.go Outdated Show resolved Hide resolved

ivelichkovich mentioned this pull request Mar 3, 2023

migrate versionedattr to avoid circular dependency #116264

Merged

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

tallclair reviewed Mar 4, 2023

View reviewed changes

tallclair self-assigned this Mar 6, 2023

andrewsykim reviewed Mar 6, 2023

View reviewed changes

pkg/apis/admissionregistration/types.go Outdated Show resolved Hide resolved

staging/src/k8s.io/api/admissionregistration/v1/types.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go Outdated Show resolved Hide resolved

k8s-ci-robot added the area/test label Mar 6, 2023

tallclair added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 14, 2023

liggitt reviewed Mar 14, 2023

View reviewed changes

final cleanup

c6814b0

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2023

k8s-ci-robot requested review from liggitt and tallclair March 14, 2023 23:50

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

openapi gen

357cb60

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2023

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

k8s-ci-robot merged commit 5e5b302 into kubernetes:master Mar 15, 2023

maxsmythe added a commit to maxsmythe/kubernetes that referenced this pull request Mar 15, 2023

Add custom match conditions for CEL admission

8d03bfb

This PR is based off of, and dependent on the following PR: kubernetes#116261 Signed-off-by: Max Smythe <smythe@google.com>

benluddy mentioned this pull request Jun 21, 2023

CEL lib: Expose errors on authz decisions instead of raising them from check() #118804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matchconditions admission webhooks alpha implementation for kep-3716 #116261

Matchconditions admission webhooks alpha implementation for kep-3716 #116261

ivelichkovich commented Mar 3, 2023 •

edited by k8s-ci-robot

k8s-ci-robot commented Mar 3, 2023

tallclair left a comment

tallclair left a comment

tallclair Mar 3, 2023

ivelichkovich Mar 7, 2023

maxsmythe Mar 7, 2023

jpbetz Mar 9, 2023 •

edited

ivelichkovich Mar 9, 2023

ivelichkovich Mar 9, 2023

tallclair Mar 9, 2023

maxsmythe Mar 10, 2023

ivelichkovich Mar 10, 2023

maxsmythe Mar 13, 2023

tallclair Mar 3, 2023

ivelichkovich Mar 7, 2023

ivelichkovich commented Mar 4, 2023

k8s-ci-robot commented Mar 6, 2023

rjsadow commented Mar 6, 2023

liggitt Mar 14, 2023 •

edited

ivelichkovich Mar 14, 2023 •

edited

tallclair Mar 14, 2023

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

		klog.Error("Invalid type conversion to MatchCondition")
		continue

Matchconditions admission webhooks alpha implementation for kep-3716 #116261

Matchconditions admission webhooks alpha implementation for kep-3716 #116261

Conversation

ivelichkovich commented Mar 3, 2023 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 3, 2023

tallclair left a comment

Choose a reason for hiding this comment

tallclair left a 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

jpbetz Mar 9, 2023 • edited

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

ivelichkovich commented Mar 4, 2023

k8s-ci-robot commented Mar 6, 2023

rjsadow commented Mar 6, 2023

liggitt Mar 14, 2023 • edited

Choose a reason for hiding this comment

ivelichkovich Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

ivelichkovich commented Mar 3, 2023 •

edited by k8s-ci-robot

jpbetz Mar 9, 2023 •

edited

liggitt Mar 14, 2023 •

edited

ivelichkovich Mar 14, 2023 •

edited