Apply cost constraints to ValidatingAdmissionPolicy #115747

cici37 · 2023-02-14T07:47:53Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Apply cost constraints to ValidatingAdmissionPolicy

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This change contains:

Apply cel runtime cost to ValidatingAdmissionPolicy
Add params for testing purpose with comments
Move cel cost related config under API review(since the numbers should be API facing and changes to those could make existing persisted CRDs or ValidatingAdmissionPolicy fail validation).

Does this PR introduce a user-facing change?

Added CEL runtime cost calculation into ValidatingAdmissionPolicy, matching the evaluation cost
restrictions that already apply to CustomResourceDefinition.
If rule evaluation uses more compute than the limit, the API server aborts the evaluation and the
admission check that was being performed is aborted; the `failurePolicy` for the ValidatingAdmissionPolicy
determines the outcome.

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/3488

k8s-triage-robot · 2023-02-14T08:50:48Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

cici37 · 2023-02-14T17:11:01Z

/cc @jpbetz

cici37 · 2023-02-14T21:11:32Z

/triage accepted

sftim · 2023-02-15T15:19:37Z

Changelog suggestion:

Added CEL runtime cost calculation into ValidatingAdmissionPolicy. ValidatingAdmissionPolicy will fail if the
runtime cost exceeds the per-call limit for each validation expression or cost of all expressions exceed `runtimeCELBudget`.

However, I'd prefer to be more clear:

is this a new admission time check?
what does “fail” mean: fail-open or fail-closed.
where does somebody specify runtimeCELBudget, and is that the capitalization that they'd see?

sftim · 2023-02-15T15:21:24Z

staging/src/k8s.io/apiserver/pkg/apis/cel/config.go

+	PerCallLimit = 1000000
+
+	// RuntimeCELCostBudget is the overall cost budget for runtime CEL validation cost per ValidatingAdmissionPolicy or CustomResource
+	// current RuntimeCELCostBudget gives roughly 1 seconds for the validation
+	RuntimeCELCostBudget = 10000000


Can we include a unit into these constant names? Even if it's PerCallLimitFooBars or PerCallLimitApproximateSeconds.

It is a little tricky to define the unit.. The cost is the cost of operation defined by cel-go. Open to suggestions on this one :)

I don't know if this helps, but I'll make a few statements about cost:

CEL cost units represent an evaluation of a basic CEL compute operation. CEL cost units do not have a 1:1 correspondence with machine instructions (or CPU cycles), but each can be though of roughly as the compute required for CEL to evaluate simple operations like a integer comparison.

For any CEL expression and input, the CEL cost will always be exactly the same regardless or platform or system load.

CEL cost units are tallied during CEL evaluation, but do not directly represent the metering of CPU utilization.

cici37 · 2023-02-15T19:18:49Z

Thanks for the suggestion!

is this a new admission time check?

It is the cel-go cost check which prevent us from long running on cel validation. CRD validation rules has this check for quite some time. This PR is to apply the same constraints on ValidatingAdmissionPolicy.

what does “fail” mean: fail-open or fail-closed.

Fail means the admission validation will fail and the FailurePolicy will apply.

where does somebody specify runtimeCELBudget, and is that the capitalization that they'd see?

This is the number we assigned based on evaluation hence not allow users to specify

sftim · 2023-02-15T19:27:17Z

OK, try this release note:

Added CEL runtime cost calculation into ValidatingAdmissionPolicy, matching the evaluation cost
restrictions that already apply to CustomResourceDefinition.
If rule evaluation uses more compute than the limit, the API server aborts the evaluation and the
admission check that was being performed is aborted; the `failurePolicy` for the ValidatingAdmissionPolicy
determines the outcome.

cici37 · 2023-02-15T22:24:52Z

/assign @liggitt @jpbetz
since you have reviewed the similar changes in CRD validation rules :)

cici37 · 2023-02-22T18:31:07Z

/retest

jpbetz

Approach looks great. The "per binding" approach to the budget was what I was hoping to see here! I added a few minor comments but nothing blocking. LGTM once those are addressed.

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

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

staging/src/k8s.io/apiserver/pkg/apis/cel/config.go

cici37 · 2023-02-27T17:14:32Z

/test pull-kubernetes-e2e-gce

staging/src/k8s.io/apiserver/pkg/apis/cel/config.go

cici37 · 2023-03-05T23:15:30Z

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

+				}
+				remainingBudget -= int64(*rtCost)
+			}
+		}


Note: here we make the behavior same as the CRD validation rule path, which is whenever issues in retrieving cost, per expression cost exceed or runtime budge exceed, we stop running following expressions and return error.
However, it will make the cost exceed error higher priority than other errors and return only per expression exceed error back even there are another validation failures existing.
Should we treat the per expression cost exceed error same as other errors and save per evaluationResult? And we could also consider keeping the following validations running. What do you think? @jpbetz

I think it's fine to only return one per expression exceeded error back. Users shouldn't expect to get full information back when the cost system kicks in, since the point of the cost system is to prevent running CEL expressions once the limit is exceeded.

EDIT: What I mean is that I think the code in this PR is correct. Just error out when the limit is hit.

jpbetz · 2023-03-06T20:34:27Z

/approve
For CEL packages

/assign @liggitt for API review approval. The only change here is passing the runtime limit when compiling expressions during API validation (which doesn't actually change anything).

liggitt · 2023-03-06T20:44:47Z

/approve

CI looks unhappy, but the API const relocation and package addition in vendor.txt lgtm
will defer review/lgtm of admission changes to @jpbetz

k8s-ci-robot · 2023-03-06T20:45:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/OWNERS~~ [cici37,jpbetz,liggitt]
~~staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/OWNERS~~ [cici37,jpbetz,liggitt]
~~staging/src/k8s.io/apiserver/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/cel/OWNERS~~ [cici37,jpbetz,liggitt]
~~vendor/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 · 2023-03-06T20:45:31Z

/approve

CI looks unhappy, but the API const relocation and package addition in vendor.txt lgtm will defer review/lgtm of admission changes to @jpbetz

An duplicated imports get in while rebasing. I removed it and CI should be happy now. Thank you!

liggitt · 2023-03-06T22:11:59Z

still some relevant lint/vet/unit failures, looks like

jpbetz · 2023-03-06T23:50:26Z

/lgtm

k8s-ci-robot · 2023-03-06T23:50:32Z

LGTM label has been added.

Git tree hash: 15f783c147ded42118aa8d1fdeea53bde5d0f9c7

k8s-ci-robot requested review from alexzielenski and justinsb February 14, 2023 07:49

cici37 force-pushed the rc branch 3 times, most recently from 27c62f8 to f99fad0 Compare February 14, 2023 08:41

k8s-ci-robot requested a review from jpbetz February 14, 2023 17:11

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 Feb 14, 2023

sftim reviewed Feb 15, 2023

View reviewed changes

k8s-ci-robot assigned jpbetz and liggitt Feb 15, 2023

liggitt added the api-review Categorizes an issue or PR as actively needing an API review. label Feb 16, 2023

jpbetz reviewed Feb 23, 2023

View reviewed changes

cici37 force-pushed the rc branch from 32d4bf8 to 25f0bed Compare February 24, 2023 17:51

liggitt reviewed Mar 2, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/apis/cel/config.go Outdated Show resolved Hide resolved

liggitt moved this from In progress to API review completed, 1.27 in API Reviews Mar 2, 2023

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

cici37 force-pushed the rc branch from 25f0bed to 6abe0d0 Compare March 5, 2023 23:06

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

cici37 commented Mar 5, 2023

View reviewed changes

cici37 mentioned this pull request Mar 6, 2023

KEP-3488: Implement Enforcement Actions and Audit Annotations #115973

Merged

cici37 force-pushed the rc branch from 6abe0d0 to c625edb Compare March 6, 2023 20:25

cici37 added 2 commits March 6, 2023 20:43

Apply resource constraints to ValidatingAdmissionPolicy.

244c63a

Update CRD validation rules path accordingly.

1f4a9dd

cici37 force-pushed the rc branch from c625edb to 1f4a9dd Compare March 6, 2023 20:44

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

Fix CI

6d08211

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

k8s-ci-robot merged commit 8c61473 into kubernetes:master Mar 7, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 7, 2023

cici37 deleted the rc branch March 7, 2023 02:03

jpbetz mentioned this pull request Mar 9, 2023

Matchconditions admission webhooks alpha implementation for kep-3716 #116261

Merged

cici37 mentioned this pull request Mar 16, 2023

CEL for Admission Control kubernetes/enhancements#3488

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply cost constraints to ValidatingAdmissionPolicy #115747

Apply cost constraints to ValidatingAdmissionPolicy #115747

cici37 commented Feb 14, 2023 •

edited

k8s-triage-robot commented Feb 14, 2023

cici37 commented Feb 14, 2023

cici37 commented Feb 14, 2023

sftim commented Feb 15, 2023

sftim Feb 15, 2023

cici37 Feb 15, 2023

jpbetz Feb 15, 2023 •

edited

cici37 commented Feb 15, 2023 •

edited

sftim commented Feb 15, 2023 •

edited

cici37 commented Feb 15, 2023

cici37 commented Feb 22, 2023

jpbetz left a comment

cici37 commented Feb 27, 2023

cici37 Mar 5, 2023

jpbetz Mar 6, 2023 •

edited

jpbetz commented Mar 6, 2023

liggitt commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

cici37 commented Mar 6, 2023

liggitt commented Mar 6, 2023

jpbetz commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

Apply cost constraints to ValidatingAdmissionPolicy #115747

Apply cost constraints to ValidatingAdmissionPolicy #115747

Conversation

cici37 commented Feb 14, 2023 • edited

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-triage-robot commented Feb 14, 2023

cici37 commented Feb 14, 2023

cici37 commented Feb 14, 2023

sftim commented Feb 15, 2023

sftim Feb 15, 2023

Choose a reason for hiding this comment

cici37 Feb 15, 2023

Choose a reason for hiding this comment

jpbetz Feb 15, 2023 • edited

Choose a reason for hiding this comment

cici37 commented Feb 15, 2023 • edited

sftim commented Feb 15, 2023 • edited

cici37 commented Feb 15, 2023

cici37 commented Feb 22, 2023

jpbetz left a comment

Choose a reason for hiding this comment

cici37 commented Feb 27, 2023

cici37 Mar 5, 2023

Choose a reason for hiding this comment

jpbetz Mar 6, 2023 • edited

Choose a reason for hiding this comment

jpbetz commented Mar 6, 2023

liggitt commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

cici37 commented Mar 6, 2023

liggitt commented Mar 6, 2023

jpbetz commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

cici37 commented Feb 14, 2023 •

edited

jpbetz Feb 15, 2023 •

edited

cici37 commented Feb 15, 2023 •

edited

sftim commented Feb 15, 2023 •

edited

jpbetz Mar 6, 2023 •

edited