diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index fe48ff6d4ec..0681db890a9 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -15263,7 +15263,7 @@ "description": "CELDeviceSelector contains a CEL expression for selecting a device.", "properties": { "expression": { - "description": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)", + "description": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)\n\nThe length of the expression must be smaller or equal to 10 Ki. The cost of evaluating it is also limited based on the estimated number of logical steps.", "type": "string" } }, diff --git a/api/openapi-spec/v3/apis__resource.k8s.io__v1alpha3_openapi.json b/api/openapi-spec/v3/apis__resource.k8s.io__v1alpha3_openapi.json index 88128b5fb67..a42d2ae8b68 100644 --- a/api/openapi-spec/v3/apis__resource.k8s.io__v1alpha3_openapi.json +++ b/api/openapi-spec/v3/apis__resource.k8s.io__v1alpha3_openapi.json @@ -143,7 +143,7 @@ "properties": { "expression": { "default": "", - "description": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)", + "description": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)\n\nThe length of the expression must be smaller or equal to 10 Ki. The cost of evaluating it is also limited based on the estimated number of logical steps.", "type": "string" } }, diff --git a/pkg/apis/resource/types.go b/pkg/apis/resource/types.go index 2c03a304fdf..909797bf327 100644 --- a/pkg/apis/resource/types.go +++ b/pkg/apis/resource/types.go @@ -526,10 +526,37 @@ type CELDeviceSelector struct { // // cel.bind(dra, device.attributes["dra.example.com"], dra.someBool && dra.anotherBool) // + // The length of the expression must be smaller or equal to 10 Ki. The + // cost of evaluating it is also limited based on the estimated number + // of logical steps. + // // +required Expression string } +// CELSelectorExpressionMaxCost specifies the cost limit for a single CEL selector +// evaluation. +// +// There is no overall budget for selecting a device, so the actual time +// required for that is proportional to the number of CEL selectors and how +// often they need to be evaluated, which can vary depending on several factors +// (number of devices, cluster utilization, additional constraints). +// +// Validation against this limit and [CELSelectorExpressionMaxLength] happens +// only when setting an expression for the first time or when changing it. If +// the limits are changed in a future Kubernetes release, existing users are +// guaranteed that existing expressions will continue to be valid and won't be +// interrupted at runtime after an up- or downgrade. +// +// According to +// https://github.com/kubernetes/kubernetes/blob/4aeaf1e99e82da8334c0d6dddd848a194cd44b4f/staging/src/k8s.io/apiserver/pkg/apis/cel/config.go#L20-L22, +// this gives roughly 0.1 second for each expression evaluation. +// However, this depends on how fast the machine is. +const CELSelectorExpressionMaxCost = 1000000 + +// CELSelectorExpressionMaxLength is the maximum length of a CEL selector expression string. +const CELSelectorExpressionMaxLength = 10 * 1024 + // DeviceConstraint must have exactly one field set besides Requests. type DeviceConstraint struct { // Requests is a list of the one or more requests in this claim which diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 4b4567345f5..7425d00cdff 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -170,10 +170,19 @@ func validateCELSelector(celSelector resource.CELDeviceSelector, fldPath *field. if stored { envType = environment.StoredExpressions } + if len(celSelector.Expression) > resource.CELSelectorExpressionMaxLength { + allErrs = append(allErrs, field.TooLongMaxLength(fldPath.Child("expression"), "", resource.CELSelectorExpressionMaxLength)) + // Don't bother compiling too long expressions. + return allErrs + } + result := dracel.GetCompiler().CompileCELExpression(celSelector.Expression, envType) if result.Error != nil { allErrs = append(allErrs, convertCELErrorToValidationError(fldPath.Child("expression"), celSelector.Expression, result.Error)) + } else if result.MaxCost > resource.CELSelectorExpressionMaxCost { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("expression"), "too complex, exceeds cost limit")) } + return allErrs } diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index 64c1c75797e..6bcfca3d60f 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -18,6 +18,7 @@ package validation import ( "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -316,6 +317,48 @@ func TestValidateClaim(t *testing.T) { return claim }(), }, + "CEL-length": { + wantFailures: field.ErrorList{ + field.TooLongMaxLength(field.NewPath("spec", "devices", "requests").Index(1).Child("selectors").Index(1).Child("cel", "expression"), "", resource.CELSelectorExpressionMaxLength), + }, + claim: func() *resource.ResourceClaim { + claim := testClaim(goodName, goodNS, validClaimSpec) + claim.Spec.Devices.Requests = append(claim.Spec.Devices.Requests, claim.Spec.Devices.Requests[0]) + claim.Spec.Devices.Requests[1].Name += "-2" + expression := `device.driver == ""` + claim.Spec.Devices.Requests[1].Selectors = []resource.DeviceSelector{ + { + // Good selector. + CEL: &resource.CELDeviceSelector{ + Expression: strings.ReplaceAll(expression, `""`, `"`+strings.Repeat("x", resource.CELSelectorExpressionMaxLength-len(expression))+`"`), + }, + }, + { + // Too long by one selector. + CEL: &resource.CELDeviceSelector{ + Expression: strings.ReplaceAll(expression, `""`, `"`+strings.Repeat("x", resource.CELSelectorExpressionMaxLength-len(expression)+1)+`"`), + }, + }, + } + return claim + }(), + }, + "CEL-cost": { + wantFailures: field.ErrorList{ + field.Forbidden(field.NewPath("spec", "devices", "requests").Index(0).Child("selectors").Index(0).Child("cel", "expression"), "too complex, exceeds cost limit"), + }, + claim: func() *resource.ResourceClaim { + claim := testClaim(goodName, goodNS, validClaimSpec) + claim.Spec.Devices.Requests[0].Selectors = []resource.DeviceSelector{ + { + CEL: &resource.CELDeviceSelector{ + Expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + }, + }, + } + return claim + }(), + }, } for name, scenario := range scenarios { diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 840c0c1c6af..1647a05831a 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -45928,7 +45928,7 @@ func schema_k8sio_api_resource_v1alpha3_CELDeviceSelector(ref common.ReferenceCa Properties: map[string]spec.Schema{ "expression": { SchemaProps: spec.SchemaProps{ - Description: "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)", + Description: "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)\n\nThe length of the expression must be smaller or equal to 10 Ki. The cost of evaluating it is also limited based on the estimated number of logical steps.", Default: "", Type: []string{"string"}, Format: "", diff --git a/staging/src/k8s.io/api/resource/v1alpha3/generated.proto b/staging/src/k8s.io/api/resource/v1alpha3/generated.proto index f77d6059527..fe0a55fa7b5 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/generated.proto +++ b/staging/src/k8s.io/api/resource/v1alpha3/generated.proto @@ -128,6 +128,10 @@ message CELDeviceSelector { // // cel.bind(dra, device.attributes["dra.example.com"], dra.someBool && dra.anotherBool) // + // The length of the expression must be smaller or equal to 10 Ki. The + // cost of evaluating it is also limited based on the estimated number + // of logical steps. + // // +required optional string expression = 1; } diff --git a/staging/src/k8s.io/api/resource/v1alpha3/types.go b/staging/src/k8s.io/api/resource/v1alpha3/types.go index cc4de1d6302..9447cfd163a 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/types.go +++ b/staging/src/k8s.io/api/resource/v1alpha3/types.go @@ -532,10 +532,37 @@ type CELDeviceSelector struct { // // cel.bind(dra, device.attributes["dra.example.com"], dra.someBool && dra.anotherBool) // + // The length of the expression must be smaller or equal to 10 Ki. The + // cost of evaluating it is also limited based on the estimated number + // of logical steps. + // // +required Expression string `json:"expression" protobuf:"bytes,1,name=expression"` } +// CELSelectorExpressionMaxCost specifies the cost limit for a single CEL selector +// evaluation. +// +// There is no overall budget for selecting a device, so the actual time +// required for that is proportional to the number of CEL selectors and how +// often they need to be evaluated, which can vary depending on several factors +// (number of devices, cluster utilization, additional constraints). +// +// Validation against this limit and [CELSelectorExpressionMaxLength] happens +// only when setting an expression for the first time or when changing it. If +// the limits are changed in a future Kubernetes release, existing users are +// guaranteed that existing expressions will continue to be valid and won't be +// interrupted at runtime after an up- or downgrade. +// +// According to +// https://github.com/kubernetes/kubernetes/blob/4aeaf1e99e82da8334c0d6dddd848a194cd44b4f/staging/src/k8s.io/apiserver/pkg/apis/cel/config.go#L20-L22, +// this gives roughly 0.1 second for each expression evaluation. +// However, this depends on how fast the machine is. +const CELSelectorExpressionMaxCost = 1000000 + +// CELSelectorExpressionMaxLength is the maximum length of a CEL selector expression string. +const CELSelectorExpressionMaxLength = 10 * 1024 + // DeviceConstraint must have exactly one field set besides Requests. type DeviceConstraint struct { // Requests is a list of the one or more requests in this claim which diff --git a/staging/src/k8s.io/api/resource/v1alpha3/types_swagger_doc_generated.go b/staging/src/k8s.io/api/resource/v1alpha3/types_swagger_doc_generated.go index 5fa01514f21..1cbd5bb0e91 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/types_swagger_doc_generated.go +++ b/staging/src/k8s.io/api/resource/v1alpha3/types_swagger_doc_generated.go @@ -50,7 +50,7 @@ func (BasicDevice) SwaggerDoc() map[string]string { var map_CELDeviceSelector = map[string]string{ "": "CELDeviceSelector contains a CEL expression for selecting a device.", - "expression": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)", + "expression": "Expression is a CEL expression which evaluates a single device. It must evaluate to true when the device under consideration satisfies the desired criteria, and false when it does not. Any other result is an error and causes allocation of devices to abort.\n\nThe expression's input is an object named \"device\", which carries the following properties:\n - driver (string): the name of the driver which defines this device.\n - attributes (map[string]object): the device's attributes, grouped by prefix\n (e.g. device.attributes[\"dra.example.com\"] evaluates to an object with all\n of the attributes which were prefixed by \"dra.example.com\".\n - capacity (map[string]object): the device's capacities, grouped by prefix.\n\nExample: Consider a device with driver=\"dra.example.com\", which exposes two attributes named \"model\" and \"ext.example.com/family\" and which exposes one capacity named \"modules\". This input to this expression would have the following fields:\n\n device.driver\n device.attributes[\"dra.example.com\"].model\n device.attributes[\"ext.example.com\"].family\n device.capacity[\"dra.example.com\"].modules\n\nThe device.driver field can be used to check for a specific driver, either as a high-level precondition (i.e. you only want to consider devices from this driver) or as part of a multi-clause expression that is meant to consider devices from different drivers.\n\nThe value type of each attribute is defined by the device definition, and users who write these expressions must consult the documentation for their specific drivers. The value type of each capacity is Quantity.\n\nIf an unknown prefix is used as a lookup in either device.attributes or device.capacity, an empty map will be returned. Any reference to an unknown field will cause an evaluation error and allocation to abort.\n\nA robust expression should check for the existence of attributes before referencing them.\n\nFor ease of use, the cel.bind() function is enabled, and can be used to simplify expressions that access multiple attributes with the same domain. For example:\n\n cel.bind(dra, device.attributes[\"dra.example.com\"], dra.someBool && dra.anotherBool)\n\nThe length of the expression must be smaller or equal to 10 Ki. The cost of evaluating it is also limited based on the estimated number of logical steps.", } func (CELDeviceSelector) SwaggerDoc() map[string]string { diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go index 4be3fd50765..375b2717eef 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -67,6 +67,10 @@ type CompilationResult struct { OutputType *cel.Type Environment *cel.Env + // MaxCost represents the worst-case cost of the compiled MessageExpression in terms of CEL's cost units, + // as used by cel.EstimateCost. + MaxCost uint64 + emptyMapVal ref.Val } @@ -107,6 +111,10 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty return resultError(fmt.Sprintf("unexpected error loading CEL environment: %v", err), apiservercel.ErrorTypeInternal) } + // We don't have a SizeEstimator. The potential size of the input (= a + // device) is already declared in the definition of the environment. + estimator := &library.CostEstimator{} + ast, issues := env.Compile(expression) if issues != nil { return resultError("compilation failed: "+issues.String(), apiservercel.ErrorTypeInvalid) @@ -122,18 +130,35 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal) } prog, err := env.Program(ast, + // cel.CostLimit is also set to the VAP PerCallLimit as part of + // the base environment. + // + // This call here should override that. In practice it shouldn't + // matter because the limits are the same. + cel.CostLimit(resourceapi.CELSelectorExpressionMaxCost), + cel.CostTracking(estimator), cel.InterruptCheckFrequency(celconfig.CheckFrequency), ) if err != nil { return resultError("program instantiation failed: "+err.Error(), apiservercel.ErrorTypeInternal) } - return CompilationResult{ + + compilationResult := CompilationResult{ Program: prog, Expression: expression, OutputType: ast.OutputType(), Environment: env, emptyMapVal: env.CELTypeAdapter().NativeToValue(map[string]any{}), } + + costEst, err := env.EstimateCost(ast, estimator) + if err != nil { + compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInternal, Detail: "cost estimation failed: " + err.Error()} + return compilationResult + } + + compilationResult.MaxCost = costEst.Max + return compilationResult } // getAttributeValue returns the native representation of the one value that diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go index 8a97813867d..3bc3267aca3 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go @@ -36,6 +36,13 @@ func TestCompile(t *testing.T) { expectCompileError string expectMatchError string expectMatch bool + + // There's no good way to verify that the cost of an expression + // really is what it should be other than eye-balling it. The + // cost should not change in the future unless the expression + // gets changed, so this provides some protection against + // regressions. + expectCost uint64 }{ "true": { expression: "true", @@ -56,92 +63,109 @@ func TestCompile(t *testing.T) { "runtime-error-lookup-identifier": { expression: `device.attributes["no-such-domain"].noSuchAttr.isGreaterThan(quantity("0"))`, expectMatchError: "no such key: noSuchAttr", + expectCost: 6, }, "runtime-error-lookup-map": { expression: `device.attributes["no-such-domain"]["noSuchAttr"].isGreaterThan(quantity("0"))`, expectMatchError: "no such key: noSuchAttr", + expectCost: 6, }, "domain-check-negative": { expression: `"no-such-domain" in device.attributes`, expectMatch: false, + expectCost: 3, }, "domain-check-positive": { expression: `"dra.example.com" in device.attributes`, attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"dra.example.com/something": {BoolValue: ptr.To(true)}}, expectMatch: true, + expectCost: 3, }, "empty-driver-name": { expression: `device.driver == ""`, expectMatch: true, + expectCost: 2, }, "real-driver-name": { expression: `device.driver == "dra.example.com"`, driver: "dra.example.com", expectMatch: true, + expectCost: 4, }, "driver-name-qualifier": { expression: `device.attributes["dra.example.com"].name`, driver: "dra.example.com", attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {BoolValue: ptr.To(true)}}, expectMatch: true, + expectCost: 4, }, "driver-name-qualifier-map-lookup": { expression: `device.attributes["dra.example.com"]["name"]`, driver: "dra.example.com", attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {BoolValue: ptr.To(true)}}, expectMatch: true, + expectCost: 4, }, "bind": { expression: `cel.bind(dra, device.attributes["dra.example.com"], dra.name)`, driver: "dra.example.com", attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {BoolValue: ptr.To(true)}}, expectMatch: true, + expectCost: 15, }, "qualified-attribute-name": { expression: `device.attributes["other.example.com"].name`, driver: "dra.example.com", attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"other.example.com/name": {BoolValue: ptr.To(true)}}, expectMatch: true, + expectCost: 4, }, "bool": { expression: `device.attributes["dra.example.com"].name`, attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {BoolValue: ptr.To(true)}}, driver: "dra.example.com", expectMatch: true, + expectCost: 4, }, "int": { expression: `device.attributes["dra.example.com"].name > 0`, attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {IntValue: ptr.To(int64(1))}}, driver: "dra.example.com", expectMatch: true, + expectCost: 5, }, "string": { expression: `device.attributes["dra.example.com"].name == "fish"`, attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {StringValue: ptr.To("fish")}}, driver: "dra.example.com", expectMatch: true, + expectCost: 5, }, "version": { expression: `device.attributes["dra.example.com"].name.isGreaterThan(semver("0.0.1"))`, attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{"name": {VersionValue: ptr.To("1.0.0")}}, driver: "dra.example.com", expectMatch: true, + expectCost: 6, }, "quantity": { expression: `device.capacity["dra.example.com"].name.isGreaterThan(quantity("1Ki"))`, capacity: map[resourceapi.QualifiedName]resource.Quantity{"name": resource.MustParse("1Mi")}, driver: "dra.example.com", expectMatch: true, + expectCost: 6, }, "check-positive": { expression: `"name" in device.capacity["dra.example.com"] && device.capacity["dra.example.com"].name.isGreaterThan(quantity("1Ki"))`, capacity: map[resourceapi.QualifiedName]resource.Quantity{"name": resource.MustParse("1Mi")}, driver: "dra.example.com", expectMatch: true, + expectCost: 10, }, "check-negative": { expression: `!("name" in device.capacity["dra.example.com"]) || device.capacity["dra.example.com"].name.isGreaterThan(quantity("1Ki"))`, expectMatch: true, + expectCost: 11, }, "all": { expression: ` @@ -167,6 +191,7 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) }, driver: "dra.example.com", expectMatch: true, + expectCost: 52, }, "many": { expression: `device.attributes["dra.example.com"].a && device.attributes["dra.example.com"].b && device.attributes["dra.example.com"].c`, @@ -177,6 +202,15 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) }, driver: "dra.example.com", expectMatch: true, + expectCost: 12, + }, + "expensive": { + // The worst-case is based on the maximum number of + // attributes and the maximum attribute name length. + expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + driver: "dra.example.com", + expectMatch: true, + expectCost: 18446744073709551615, // Exceeds limit! }, } { t.Run(name, func(t *testing.T) { @@ -194,6 +228,10 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) } return } + if expect, actual := scenario.expectCost, result.MaxCost; expect != actual { + t.Errorf("expected CEL cost %d, got %d instead", expect, actual) + } + match, err := result.DeviceMatches(ctx, Device{Attributes: scenario.attributes, Capacity: scenario.capacity, Driver: scenario.driver}) if err != nil { if scenario.expectMatchError == "" {