Custom match criteria #116350

maxsmythe · 2023-03-08T01:21:39Z

~~Currently blocked/dependent on #116261~~

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This PR adds custom match conditions per this section of the CEL Admission KEP:

https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3488-cel-admission-control#match-conditions

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added matchConditions field to ValidatingAdmissionPolicy, enabled support for CEL based custom match criteria.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/e80ad39a21e6f1be90e2d514fb33df34fd496814/keps/sig-api-machinery/3488-cel-admission-control#match-conditions

k8s-ci-robot · 2023-03-08T01:21:49Z

Hi @maxsmythe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

cici37 · 2023-03-08T06:12:45Z

pkg/apis/admissionregistration/util.go

Revert change here?

cici37 · 2023-03-08T06:14:39Z

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

+	MatchConditions []MatchCondition `json:"matchConditions" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,5,rep,name=matchConditions"`
+}
+
+var _ cel.ExpressionAccessor = &MatchCondition{}


This should not sit in this file?

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

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go

jpbetz · 2023-03-09T02:50:17Z

Once params is wired in through the matcher this should be able to mostly piggy-back off of webhook matchConditions, so I don't see much to do here. Even wiring in params should be timed carefully to avoid inducing excess rebase pain.

Places to consider adding tests while waiting:

pkg/apis/admissionregistration/validation/validation_test.go - Just a sanity test that matchConditions validation doesn't get broken in the future
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go - Maaaybe add a test to make sure the failurePolicy settings are respected in the case of error
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go - This is a mini-integration test that covers quite a bit of the code path from controller actuation to validation.

For auditAnnotations (which also adds new expressions to ValidationAdmissionPolicy), I believe these were the places I added test cases before writing some full integration tests (which go in test/integration/apiserver/cel/validatingadmissionpolicy_test.go when you get to that step).

cici37 · 2023-03-09T17:39:02Z

/triage accepted

maxsmythe · 2023-03-15T21:14:14Z

I believe I've addressed all the outstanding comments

maxsmythe · 2023-03-15T21:52:22Z

interesting, I can't re-request from both liggitt and tallclair at the same time

k8s-ci-robot · 2023-03-15T21:57:50Z

@maxsmythe: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features	509501a378d3feb948059259d844dd52c603d2aa	link	false	`/test pull-kubernetes-e2e-gce-cos-alpha-features`
check-dependency-stats	509501a378d3feb948059259d844dd52c603d2aa	link	false	`/test check-dependency-stats`
pull-kubernetes-conformance-kind-ipv6-parallel	509501a378d3feb948059259d844dd52c603d2aa	link	false	`/test pull-kubernetes-conformance-kind-ipv6-parallel`

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.

maxsmythe · 2023-03-15T22:03:43Z

It looks like some auto-gen stuff failed even though update-codegen.sh yielded no results, re-running full make update

Signed-off-by: Max Smythe <smythe@google.com>

maxsmythe · 2023-03-15T22:20:30Z

Codegen re-ran

liggitt · 2023-03-15T22:23:10Z

It looks like some auto-gen stuff failed even though update-codegen.sh yielded no results, re-running full make update

oh, looks like update-openapi-spec isn't included in update-codegen

liggitt · 2023-03-15T22:25:09Z

/approve
API and validation lgtm
squash at will
@tallclair has lgtm

k8s-ci-robot · 2023-03-15T22:25:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, liggitt, maxsmythe

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~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/OWNERS~~ [cici37,liggitt]
~~staging/src/k8s.io/client-go/applyconfigurations/OWNERS~~ [liggitt]

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

cici37

Nothing is merging blocker. LGTM. Thank you!

cici37 · 2023-03-15T22:37:49Z

pkg/apis/admissionregistration/validation/validation.go

@@ -520,6 +523,17 @@ func ignoreValidatingWebhookMatchConditions(new, old []admissionregistration.Val
 	return true
 }

+// ignoreMatchConditions returns true if any new expressions are added


nit: ignoreValidatingAdmissionPolicyMatchConditions?
I understand the comment tried to say that we set opt ignoreMatchConditions to true if any new expressions are added while updating but it might not so clear as the func name doesn't match the comment :). Maybe something like ignoreValidatingAdmissionPolicyMatchConditions returns true if both paramsKind and matchConditions remain the same while updating hence the update in ValidatingAdmissionPolicySpec will skip matchConditions. ?

In the spirit of making comments semantically valuable and avoiding describing logic, how about:

"ignoreValidatingAdmissionPolicyMatchConditions returns true if there have been no updates that could invalidate previously-valid match conditions"?

cici37 · 2023-03-15T22:50:15Z

.../src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go

@@ -501,13 +502,24 @@ func (c *policyController) latestPolicyData() []policyData {
 				if definitionInfo.lastReconciledValue.Spec.ParamKind != nil {
 					hasParam = true
 				}
+				matchConditions := definitionInfo.lastReconciledValue.Spec.MatchConditions
+				matchExpressionAccessors := make([]cel.ExpressionAccessor, len(matchConditions))


nit: do we wanna check the len before construct matchExpressionAccessors and matchconditions.Matcher? something like:

matchConditions := definitionInfo.lastReconciledValue.Spec.MatchConditions var matcher matchconditions.Matcher = nil if len(matchConditions) > 0 { matchExpressionAccessors := make([]cel.ExpressionAccessor, len(matchConditions)) for i := range matchConditions { matchExpressionAccessors[i] = (*matchconditions.MatchCondition)(&matchConditions[i]) } matcher = matchconditions.NewMatcher(c.filterCompiler.Compile(matchExpressionAccessors, optionalVars, celconfig.PerCallLimit), c.authz, failurePolicy, "validatingadmissionpolicy", definitionInfo.lastReconciledValue.Name) }

Yeah, that seems a bit clearer, changing.

Signed-off-by: Max Smythe <smythe@google.com>

tallclair · 2023-03-15T23:17:16Z

/lgtm
/label tide/merge-method-squash

k8s-ci-robot · 2023-03-15T23:17:22Z

LGTM label has been added.

Git tree hash: 3207223f1b9306ae73616112359fb37f50019ce8

* 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>

maxsmythe marked this pull request as draft March 8, 2023 01:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/apiserver labels Mar 8, 2023

k8s-ci-robot requested review from alexzielenski and apelisse March 8, 2023 01:21

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 8, 2023

cici37 reviewed Mar 8, 2023

View reviewed changes

jpbetz reviewed Mar 9, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go Outdated Show resolved Hide resolved

jpbetz reviewed Mar 9, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go Outdated Show resolved Hide resolved

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 9, 2023

liggitt assigned liggitt and tallclair Mar 10, 2023

maxsmythe requested review from liggitt and tallclair and removed request for liggitt and tallclair March 15, 2023 21:14

Run codegen

5984b5e

Signed-off-by: Max Smythe <smythe@google.com>

liggitt moved this from Changes requested to API review completed, 1.27 in API Reviews Mar 15, 2023

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

cici37 reviewed Mar 15, 2023

View reviewed changes

Address review comments

83f62ff

Signed-off-by: Max Smythe <smythe@google.com>

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 15, 2023

k8s-ci-robot merged commit e5fd204 into kubernetes:master Mar 16, 2023

SIG Node CI/Test Board automation moved this from Triage to Done Mar 16, 2023

SIG Node PR Triage automation moved this from Triage to Done Mar 16, 2023

cici37 mentioned this pull request Mar 16, 2023

CEL for Admission Control kubernetes/enhancements#3488

Open

16 tasks

alexzielenski mentioned this pull request Jul 20, 2023

KEP-3488: Promote ValidatingAdmissionPolicy to Beta #118644

Merged

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom match criteria #116350

Custom match criteria #116350

maxsmythe commented Mar 8, 2023 •

edited by liggitt

k8s-ci-robot commented Mar 8, 2023

cici37 Mar 8, 2023

cici37 Mar 8, 2023

jpbetz commented Mar 9, 2023 •

edited

cici37 commented Mar 9, 2023

maxsmythe commented Mar 15, 2023

maxsmythe commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023 •

edited

maxsmythe commented Mar 15, 2023

maxsmythe commented Mar 15, 2023

liggitt commented Mar 15, 2023

liggitt commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

cici37 left a comment

cici37 Mar 15, 2023

maxsmythe Mar 15, 2023

cici37 Mar 15, 2023

cici37 Mar 15, 2023

maxsmythe Mar 15, 2023

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

Custom match criteria #116350

Custom match criteria #116350

Conversation

maxsmythe commented Mar 8, 2023 • edited by liggitt

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 8, 2023

cici37 Mar 8, 2023

Choose a reason for hiding this comment

cici37 Mar 8, 2023

Choose a reason for hiding this comment

jpbetz commented Mar 9, 2023 • edited

cici37 commented Mar 9, 2023

maxsmythe commented Mar 15, 2023

maxsmythe commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023 • edited

maxsmythe commented Mar 15, 2023

maxsmythe commented Mar 15, 2023

liggitt commented Mar 15, 2023

liggitt commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

cici37 left a comment

Choose a reason for hiding this comment

cici37 Mar 15, 2023

Choose a reason for hiding this comment

maxsmythe Mar 15, 2023

Choose a reason for hiding this comment

cici37 Mar 15, 2023

Choose a reason for hiding this comment

cici37 Mar 15, 2023

Choose a reason for hiding this comment

maxsmythe Mar 15, 2023

Choose a reason for hiding this comment

tallclair commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

maxsmythe commented Mar 8, 2023 •

edited by liggitt

jpbetz commented Mar 9, 2023 •

edited

k8s-ci-robot commented Mar 15, 2023 •

edited