DRA CEL: add missing size estimator by pohly · Pull Request #129661 · kubernetes/kubernetes

pohly · 2025-01-16T11:03:20Z

What type of PR is this?

/kind bug
/priority important-soon
/assign @jpbetz

Needs to be backported to 1.32.

What this PR does / why we need it:

Not implementing a size estimator had the effect that strings retrieved from the attributes were treated as "unknown size", leading to wildly overestimating the cost and validation errors even for even simple expressions like this:

device.attributes["qat.intel.com"].services.matches("[^a]?sym")

Maximum number of elements in maps and the maximum length of the driver name string were also ignored resp. missing. Pre-defined types like apiservercel.StringType must be avoided because they are defined as having a zero maximum size.

Which issue(s) this PR fixes:

Fixes #129523

Special notes for your reviewer:

This might change the cost estimate for some potentially stored CEL expressions involving the driver name. This is okay because the validation was implemented so that all stored expressions are assumed to be valid, i.e. a down- or upgrade will not lead to validation errors when the different versions of the apiserver implement cost estimation differently.

Does this PR introduce a user-facing change?

DRA: CEL expressions using attribute strings exceeded the cost limit because their cost estimation was incomplete.

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

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

k8s-ci-robot · 2025-01-16T11:03:23Z

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

k8s-ci-robot · 2025-01-16T11:03:30Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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-sigs/prow repository.

Adarsh-verma-14 · 2025-01-16T13:27:25Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-unit

k8s-triage-robot · 2025-01-16T14:05:11Z

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.

pohly · 2025-01-16T14:50:07Z

@Adarsh-verma-14: the unit test failure is genuine, I'll have to update validation.

jpbetz · 2025-01-16T15:05:11Z

-		field(driverVar, apiservercel.StringType, true),
-		field(attributesVar, apiservercel.NewMapType(apiservercel.StringType, apiservercel.NewMapType(apiservercel.StringType, apiservercel.AnyType, resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice), resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice), true),
-		field(capacityVar, apiservercel.NewMapType(apiservercel.StringType, apiservercel.NewMapType(apiservercel.StringType, apiservercel.QuantityDeclType, resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice), resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice), true),
+		field(driverVar, driverType, true),


Just brainstorming on how to ensure someone doesn't accidentally forget to set a MaxElements on a type..

Option: field() takes the MaxElements as an argument, you could define consts for the values provided.
Option: field() takes a type alias to apiservercel.AnyType and this package provides a newType function that requires the MaxElements argument and returns the alias type.
Option: field() asserts that passed types (at least the string and bytes typed ones, maybe others) have MaxElements set.

This isn't limited to fields, nor to this particular source code.

Someone else in some entirely different package might do apiservercel.NewMapType(apiservercel.StringType, apiservercel.StringType, myMaxNumberOfKeys) without realizing that they should use some custom string type with MaxElements set.

To disarm this footgun everywhere, those pre-defined types would have to be removed in favor of constructor methods which force passing a maxElements parameters.

Alternatively, the size estimator could refuse to use an unset MaxElements. But that then only fails if someone is careful with unit testing.

True.. I'd be OK with opening a separate issue for the pre-defined types -> constructor method idea.

WDYT @cici37 is this something the CEL contributors would be willing to work on?

Not implementing a size estimator had the effect that strings retrieved from the attributes were treated as "unknown size", leading to wildly overestimating the cost and validation errors even for even simple expressions like this: device.attributes["qat.intel.com"].services.matches("[^a]?sym") Maximum number of elements in maps and the maximum length of the driver name string were also ignored resp. missing. Pre-defined types like apiservercel.StringType must be avoided because they are defined as having a zero maximum size.

jpbetz · 2025-01-16T15:50:13Z

/lgtm
For API machinery bits and the use of MaxElements.
I'm okay with #129661 (comment) being further improved as a follow up, possibly by CEL maintainers.

k8s-ci-robot · 2025-01-16T15:50:21Z

LGTM label has been added.

Details

Git tree hash: fe5a09234b007e7ae58eb414661716901e0c72ab

pohly · 2025-01-16T16:04:04Z

/retest

"Pod scheduling timeout".

pohly · 2025-01-16T17:09:11Z

/label api-review

pohly · 2025-01-16T17:12:44Z


+// DriverNameMaxLength is the maximum valid length of a driver name in the
+// ResourceSliceSpec and other places. It's the same as for CSI driver names.
+const DriverNameMaxLength = 63


For reference:

kubernetes/pkg/apis/core/validation/validation.go

Lines 1688 to 1690 in 50fc400

if len(driverName) > 63 {

allErrs = append(allErrs, field.TooLong(fldPath, "" /*unused*/, 63))

}

And

kubernetes/pkg/apis/resource/validation/validation.go

Line 46 in 50fc400

validateDriverName = corevalidation.ValidateCSIDriverName

liggitt · 2025-01-16T22:13:27Z

+	if len(path) == 0 {
+		// Path() can return an empty list, early exit if it does since we can't
+		// provide size estimates when that happens
+		return nil


does a nil return turn into the worst-case size estimate?

Yes, I believe so. This is copy-and-pasted from the size estimator

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

Lines 313 to 345 in ab54e44

func (c *sizeEstimator) EstimateSize(element checker.AstNode) *checker.SizeEstimate {

if len(element.Path()) == 0 {

// Path() can return an empty list, early exit if it does since we can't

// provide size estimates when that happens

return nil

}

currentNode := c.root

// cut off "self" from path, since we always start there

for _, name := range element.Path()[1:] {

switch name {

case "@items", "@values":

if currentNode.ElemType == nil {

return nil

}

currentNode = currentNode.ElemType

case "@keys":

if currentNode.KeyType == nil {

return nil

}

currentNode = currentNode.KeyType

default:

field, ok := currentNode.Fields[name]

if !ok {

return nil

}

if field.Type == nil {

return nil

}

currentNode = field.Type

}

}

return &checker.SizeEstimate{Min: 0, Max: uint64(currentNode.MaxElements)}

}

The main difference compared to that one is injecting stronger typing of the attribute values:
https://github.com/pohly/kubernetes/blob/2cc3dbf2250177c60b2967e3e8481ac3f7e33829/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go#L398-L404

liggitt · 2025-01-16T22:16:01Z

Needs to be backported to 1.32.

What's the rationale for backporting? At first glance, this looks like a pretty complex change to backport, and this isn't fixing a regression, right?

liggitt · 2025-01-16T22:17:45Z

/approve

for API constants

k8s-ci-robot · 2025-01-16T22:17:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [liggitt,pohly]

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

pohly · 2025-01-17T06:52:01Z

What's the rationale for backporting? At first glance, this looks like a pretty complex change to backport, and this isn't fixing a regression, right?

Not a regression, but a pretty significant bug that limits what users can do with DRA in 1.32. I'll elaborate further on this in the cherry-pick PR and we can discuss there.

…1-origin-release-1.32 Automated cherry pick of #129661: DRA CEL: add missing size estimator

k8s-ci-robot assigned jpbetz Jan 16, 2025

k8s-ci-robot requested review from bart0sh and klueska January 16, 2025 11:03

pohly force-pushed the dra-cell-cost-limit-increase branch from 23dcfed to d979e57 Compare January 16, 2025 12:30

pohly changed the title ~~WIP: DRA CEL: add missing size estimator~~ DRA CEL: add missing size estimator Jan 16, 2025

pohly marked this pull request as ready for review January 16, 2025 12:31

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 Jan 16, 2025

pohly force-pushed the dra-cell-cost-limit-increase branch from d979e57 to e3d839f Compare January 16, 2025 14:47

jpbetz reviewed Jan 16, 2025

View reviewed changes

pohly force-pushed the dra-cell-cost-limit-increase branch from e3d839f to 2cc3dbf Compare January 16, 2025 15:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2025

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jan 16, 2025

pohly commented Jan 16, 2025

View reviewed changes

liggitt reviewed Jan 16, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2025

k8s-ci-robot merged commit 488fac4 into kubernetes:master Jan 17, 2025

k8s-ci-robot added this to the v1.33 milestone Jan 17, 2025

pohly mentioned this pull request Jan 17, 2025

Automated cherry pick of #129661: DRA CEL: add missing size estimator #129690

Merged

pacoxu mentioned this pull request Feb 8, 2025

DRA: structured parameters kubernetes/enhancements#4381

Closed

41 tasks

k8s-ci-robot added a commit that referenced this pull request Mar 3, 2025

Merge pull request #129690 from pohly/automated-cherry-pick-of-#12966…

8207fb4

…1-origin-release-1.32 Automated cherry pick of #129661: DRA CEL: add missing size estimator

	if len(driverName) > 63 {
	allErrs = append(allErrs, field.TooLong(fldPath, "" /unused/, 63))
	}

	func (c sizeEstimator) EstimateSize(element checker.AstNode) checker.SizeEstimate {
	if len(element.Path()) == 0 {
	// Path() can return an empty list, early exit if it does since we can't
	// provide size estimates when that happens
	return nil
	}
	currentNode := c.root
	// cut off "self" from path, since we always start there
	for _, name := range element.Path()[1:] {
	switch name {
	case "@items", "@values":
	if currentNode.ElemType == nil {
	return nil
	}
	currentNode = currentNode.ElemType
	case "@keys":
	if currentNode.KeyType == nil {
	return nil
	}
	currentNode = currentNode.KeyType
	default:
	field, ok := currentNode.Fields[name]
	if !ok {
	return nil
	}
	if field.Type == nil {
	return nil
	}
	currentNode = field.Type
	}
	}
	return &checker.SizeEstimate{Min: 0, Max: uint64(currentNode.MaxElements)}
	}

Conversation

pohly commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Jan 16, 2025

Uh oh!

k8s-ci-robot commented Jan 16, 2025

Uh oh!

Adarsh-verma-14 commented Jan 16, 2025

Uh oh!

k8s-triage-robot commented Jan 16, 2025

Uh oh!

pohly commented Jan 16, 2025

Uh oh!

jpbetz Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jpbetz Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jpbetz commented Jan 16, 2025

Uh oh!

k8s-ci-robot commented Jan 16, 2025

Uh oh!

pohly commented Jan 16, 2025

Uh oh!

pohly commented Jan 16, 2025

Uh oh!

pohly Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

pohly Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

pohly Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

pohly Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jan 16, 2025

Uh oh!

liggitt commented Jan 16, 2025

Uh oh!

k8s-ci-robot commented Jan 16, 2025

Uh oh!

pohly commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pohly commented Jan 16, 2025 •

edited

Loading

jpbetz Jan 16, 2025 •

edited

Loading