Add CEL runtime cost into CR validation #108482

cici37 · 2022-03-03T09:51:06Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is part of #107573. Based on PR: google/cel-go#494. Add runtime cost calculation of CEL into CR validation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added CEL runtime cost calculation into CustomerResource validation. CustomerResource validation will fail if runtime cost exceeds the budget.

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

leilajal · 2022-03-03T17:37:08Z

/triage accepted

leilajal · 2022-03-03T17:38:43Z

/triage accepted

fedebongio · 2022-03-03T17:46:38Z

/cc @jpbetz

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

liggitt · 2022-03-11T18:31:19Z

other than needing testing of the string-prefix-detection branch (strings.HasPrefix(err.Error(), "operation cancelled: actual cost limit exceeded"), this looks good. can go ahead and squash down.

once there's a test in place that fails if that prefix match doesn't catch a cost-exceeded error, this lgtm

cici37 · 2022-03-11T18:49:36Z

go ahead and squash do

The test is in place for the string-prefix-detection branch (strings.HasPrefix(err.Error(), "operation cancelled: actual cost limit exceeded"). It will still pass because all the err got from cel validation will be caught and returned. The only different for this specific err is we wrapped with message call cost exceeds limit for rule. I have updated the message compare in test and now it is working as expected. Thank you for your patience

cici37 · 2022-03-11T19:31:27Z

/test pull-kubernetes-e2e-kind-ipv6

liggitt

Non-blocking for this PR, but make sure there's follow up item to make the construction / detection of "budget exceeded" errors consistent with helper constructor/detector functions. Right now we're returning slightly different error messages in four places and detecting them with strings.Contains in at least 2-3 places

liggitt · 2022-03-11T21:05:58Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation_test.go

+				t.Errorf("expect err of running out of cost budget but did not find")
+			}
+			if meetErr != 3 {
+				t.Errorf("expect 3 errs of running out of cost budget returned but get %v errs", meetErr)


this is surprising... once we exceed our budget, we should stop evaluating further rules, we should get back at most one "budget exceeded" error

if this requires threading/propagating remainingCost through the default validation function, I'm ok doing this in a follow-up, but it does need to be done

I have updated to handle cel cost budget exceed error separately. Would you mind to check the latest commit to see if it makes sense? Thank you

liggitt · 2022-03-14T18:36:09Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go

 }

 // validate is the recursive step func for the validation. insideMeta is true if s specifies
 // TypeMeta or ObjectMeta. The SurroundingObjectFunc f is used to validate defaults of
 // TypeMeta or ObjectMeta fields.
-func validate(pth *field.Path, s *structuralschema.Structural, rootSchema *structuralschema.Structural, f SurroundingObjectFunc, insideMeta, requirePrunedDefaults bool) (field.ErrorList, error) {
+// If CEL validation cost budget exceeded, the error will be saved in returned arg error instead of allErrs. The caller could handle it separately if needed.


If CEL validation cost budget exceeded, the error will be saved in returned arg error instead of allErrs.

Is that accurate? This seems like it is propagating cost-exceeded errors in allErrs:

if remainingCost < 0 { return allErrs, nil, remainingCost }

as an aside, if we did propagate a cost error in err, that would propagate to callers of ValidateDefaults, and would be handled by this block, which has a comment that we never expect to encounter an error:

} else if validationErrors, err := structuraldefaulting.ValidateDefaults(fldPath.Child("openAPIV3Schema"), ss, true, opts.requirePrunedDefaults); err != nil { // this should never happen allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), "", err.Error())) } else {

I think the code (propagating cost errors in allErrs) is actually what we want, and this godoc should change to match what we're actually doing

It was left over from previous change. I have removed the incorrect comment. Sorry for that

liggitt · 2022-03-14T18:55:47Z

/lgtm
/approve

k8s-ci-robot · 2022-03-14T18:57:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, 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:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]

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

k8s-ci-robot requested review from dchen1107, lavalamp and a team March 3, 2022 09:51

k8s-ci-robot added area/code-generation area/dependency Issues or PRs related to dependency changes sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 3, 2022

cici37 force-pushed the vendorCEL branch from 4768fc1 to 5b5153b Compare March 3, 2022 17:02

k8s-ci-robot requested a review from jpbetz March 3, 2022 17:46

cici37 force-pushed the vendorCEL branch from 5b5153b to e309a5a Compare March 3, 2022 17:58

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 3, 2022

cici37 force-pushed the vendorCEL branch from e309a5a to 0d88027 Compare March 3, 2022 19:46

cici37 changed the title ~~[WIP]Add CEL runtime cost into CR validation~~ Add CEL runtime cost into CR validation Mar 3, 2022

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 Mar 3, 2022

cici37 mentioned this pull request Mar 10, 2022

Wire in request context #108646

Merged

cici37 force-pushed the vendorCEL branch from b93ad6d to 94753ad Compare March 11, 2022 00:34

liggitt reviewed Mar 11, 2022

View reviewed changes

cici37 force-pushed the vendorCEL branch from 7610602 to cd92b96 Compare March 11, 2022 17:26

cici37 force-pushed the vendorCEL branch 3 times, most recently from adf92b3 to 5420655 Compare March 11, 2022 18:48

cici37 force-pushed the vendorCEL branch 2 times, most recently from e300bd8 to 50b9a0d Compare March 11, 2022 19:57

liggitt reviewed Mar 11, 2022

View reviewed changes

cici37 force-pushed the vendorCEL branch 3 times, most recently from 744967d to 74e3ae2 Compare March 13, 2022 19:35

liggitt reviewed Mar 14, 2022

View reviewed changes

Wire runtime cost into validation.

6b2907b

cici37 force-pushed the vendorCEL branch from ef674e4 to 6b2907b Compare March 14, 2022 18:55

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 14, 2022

k8s-ci-robot merged commit 866e423 into kubernetes:master Mar 14, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 14, 2022

cici37 deleted the vendorCEL branch March 14, 2022 22:09

This was referenced Mar 22, 2022

Week Ending March 13, 2022 dev-obs/actus#391

Open

Week Ending March 20, 2022 dev-obs/actus#403

Open

This was referenced Jul 26, 2022

Promote feature CustomResourceValidationExpressions to beta #111158

Closed

Promote feature CustomResourceValidationExpressions to beta #111524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CEL runtime cost into CR validation #108482

Add CEL runtime cost into CR validation #108482

cici37 commented Mar 3, 2022

leilajal commented Mar 3, 2022

leilajal commented Mar 3, 2022

fedebongio commented Mar 3, 2022

liggitt commented Mar 11, 2022

cici37 commented Mar 11, 2022

cici37 commented Mar 11, 2022

liggitt left a comment

liggitt Mar 11, 2022

liggitt Mar 11, 2022

cici37 Mar 12, 2022

liggitt Mar 14, 2022

liggitt Mar 14, 2022

cici37 Mar 14, 2022 •

edited

liggitt commented Mar 14, 2022

k8s-ci-robot commented Mar 14, 2022

Add CEL runtime cost into CR validation #108482

Add CEL runtime cost into CR validation #108482

Conversation

cici37 commented Mar 3, 2022

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.:

leilajal commented Mar 3, 2022

leilajal commented Mar 3, 2022

fedebongio commented Mar 3, 2022

liggitt commented Mar 11, 2022

cici37 commented Mar 11, 2022

cici37 commented Mar 11, 2022

liggitt left a comment

Choose a reason for hiding this comment

liggitt Mar 11, 2022

Choose a reason for hiding this comment

liggitt Mar 11, 2022

Choose a reason for hiding this comment

cici37 Mar 12, 2022

Choose a reason for hiding this comment

liggitt Mar 14, 2022

Choose a reason for hiding this comment

liggitt Mar 14, 2022

Choose a reason for hiding this comment

cici37 Mar 14, 2022 • edited

Choose a reason for hiding this comment

liggitt commented Mar 14, 2022

k8s-ci-robot commented Mar 14, 2022

cici37 Mar 14, 2022 •

edited