Calculate CEL cost totals

[DangerOnTheRanger]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is a part of #107573, and adds support at the CRD level for CEL expression cost calculation as per the KEP, and emits an error message if the CRD CEL cost limit is exceeded. This PR builds off of #108419 by taking into account maxLength and associated fields when calculating the total maximum cost for a CRD's CEL expressions.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[fedebongio]

/triage accepted

[jpbetz]

@DangerOnTheRanger Just to make sure we're on the same page, I'm expecting that:

per-expression estimated cost is: *

per-CRD estimated cost: sum(per-expression estimated costs)

I wanted to point this out because this PR is titled "per-CRD" but includes logic to calculate number of times an expression can be evaluated.

[DangerOnTheRanger]

Kermit Alexander II Just to make sure we're on the same page, I'm expecting that:

per-expression estimated cost is: *

per-CRD estimated cost: sum(per-expression estimated costs)

I wanted to point this out because this PR is titled "per-CRD" but includes logic to calculate number of times an expression can be evaluated.

Yeah, I think there was unfortunately some ambiguity there. I've renamed the PR so the changes make a bit more sense/have a bit more context, hopefully.

[cici37]

Hi @DangerOnTheRanger, when you rebase, please remove TODO in compilation_test and update the cost limit with const PerCallLimit. Thanks

[jpbetz]

Maybe use a name different than "CRD cost"? This computes the cost of a single CEL expression, not the cost of all the CEL expressions in a CRD.

[DangerOnTheRanger]

Yes, I think it could be more descriptive. I've renamed it to getExpressionCost - how does that sound?

[DangerOnTheRanger]

/retest

[liggitt]

what does a nil return mean?

[jpbetz]

godoc:

extractMaxElements returns the factor by which the schema increases the number of
possible data elements for its children.  If schema is a map and has MaxProperties or an
array has MaxItems, the int pointer of the max value is returned.
If schema is a map or array and does not have MaxProperties or MaxItems, nil is returned to indicate
that there is no limit to the possible number of data elements imposed by the current
schema.  If the schema is an object, 1 is returned to indicate that there
is not increase to the number of possible data elements for its children.  Primitives
do not have children, but 1 is returned for simplicity in this case.

[liggitt]

is this being called in ways that will make us repeatedly recursively evaluate the size of a schema?

if I have a schema 40 nesting levels deep, and have a cel rule at each level, does this call:

• estimateMinSizeJSON(root) (traversing all child schemas to compute the min size of the root)
• estimateMinSizeJSON(level 1) (re-traversing all child schemas to compute the min size of level 1)
• ...
  ?

[liggitt]

beyond the scope of this PR, but a similar question exists for other callers of estimateMinSizeJSON via SchemaDeclType / estimateMaxArrayItemsPerRequest / estimateMaxAdditionalPropertiesPerRequest

we want to make sure a deep schema with cel rules at the root and other levels isn't going to be super expensive to compute cost on

[jpbetz]

This general problem is fairly pervasive in the CRD validation side of things. While I was looking at how we could do more work as a post traversal step to make the min calculations cheap, I noticed that we construct a new structural schema whenever we compile CEL programs, which is another case of use doing a recursive traversal at every level (for the worst case). We also convert those schemas to the "decl" format that CEL accepts in compile (which is another recursive traversal).

How would you feel about a beta task where we construct a benchmark that reproduces this problem well and then optimize it away? A traversal that starts at the first branch in the tree where a CEL rule encountered that accumulates some base facts (like min sizes), and prepares the structural and "decls" schemas should allow for a lot of reuse. But it's a larger change.

[liggitt]

sounds ok for beta, as long as the calculations that do recursive traversals in this PR and #108990 are behind the cel validation feature gate

[jpbetz]

Feedback applied on new commits.

[liggitt]

needs squash, and has an unused import compilation error:

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go:21:2: "math" imported but not used (typecheck)
	"math"
	^

[liggitt]

lgtm otherwise

[dims]

@DangerOnTheRanger please visit the red CI jobs!

[liggitt]

/approve
/retest

@jpbetz has final lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DangerOnTheRanger, 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]
• test/integration/apiserver/OWNERS [liggitt]

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

[jpbetz]

/lgtm

[jpbetz]

/hold cancel

[k8s-triage-robot]

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.