From ff9ef0737088083ffbafc8d10ef77550b08b52af Mon Sep 17 00:00:00 2001 From: cici37 Date: Wed, 21 Aug 2024 16:50:00 +0000 Subject: [PATCH 1/9] Apply strictCost for DRA --- staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3c140a1a220..4be3fd50765 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -209,7 +209,7 @@ func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (boo } func mustBuildEnv() *environment.EnvSet { - envset := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), false /* strictCost */) + envset := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true /* strictCost */) field := func(name string, declType *apiservercel.DeclType, required bool) *apiservercel.DeclField { return apiservercel.NewDeclField(name, declType, required, nil, nil) } From f548fc2264488001a4b7b979b44ba64895663b7f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 30 Sep 2024 18:09:33 +0200 Subject: [PATCH 2/9] DRA API: implement CEL cost limit The main purpose is to protect against denial-of-service attacks. Scheduling time depends a lot on unpredictable factors and expected scheduling time also varies, so no attempt is made to limit the overall time spent on evaluating CEL expressions per claim. --- api/openapi-spec/swagger.json | 2 +- ...is__resource.k8s.io__v1alpha3_openapi.json | 2 +- pkg/apis/resource/types.go | 27 ++++++++++++ pkg/apis/resource/validation/validation.go | 9 ++++ .../validation_resourceclaim_test.go | 43 +++++++++++++++++++ pkg/generated/openapi/zz_generated.openapi.go | 2 +- .../api/resource/v1alpha3/generated.proto | 4 ++ .../src/k8s.io/api/resource/v1alpha3/types.go | 27 ++++++++++++ .../v1alpha3/types_swagger_doc_generated.go | 2 +- .../cel/compile.go | 27 +++++++++++- .../cel/compile_test.go | 38 ++++++++++++++++ 11 files changed, 178 insertions(+), 5 deletions(-) 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 == "" { From 7995b6f18224fc8fe5c95548e5d815cc8d53a4ef Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 17 Oct 2024 17:28:31 +0200 Subject: [PATCH 3/9] DRA CEL: add test case for runtime cost limit check At the moment, the cost also gets checked at runtime. This test case ensures that this check is really active. --- .../cel/compile_test.go | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) 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 3bc3267aca3..33ed7724e98 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 @@ -17,6 +17,7 @@ limitations under the License. package cel import ( + "fmt" "strings" "testing" @@ -207,10 +208,29 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) "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! + // To actually reach that expected cost at runtime, we must + // have many attributes. + attributes: func() map[resourceapi.QualifiedName]resourceapi.DeviceAttribute { + attributes := make(map[resourceapi.QualifiedName]resourceapi.DeviceAttribute) + prefix := "dra.example.com/" + attribute := resourceapi.DeviceAttribute{ + StringValue: ptr.To("abc"), + } + // If the cost estimate was accurate, using exactly as many attributes + // as allowed at most should exceed the limit. In practice, the estimate + // is an upper bound and significantly more attributes are needed before + // the runtime cost becomes too large. + for i := 0; i < 1000*resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice; i++ { + suffix := fmt.Sprintf("-%d", i) + name := prefix + strings.Repeat("x", resourceapi.DeviceMaxIDLength-len(suffix)) + suffix + attributes[resourceapi.QualifiedName(name)] = attribute + } + return attributes + }(), + expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + driver: "dra.example.com", + expectMatchError: "actual cost limit exceeded", + expectCost: 18446744073709551615, // Exceeds limit! }, } { t.Run(name, func(t *testing.T) { From 5e514f5fcb844538b81278660994fe013e4f60da Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 17 Oct 2024 17:47:42 +0200 Subject: [PATCH 4/9] DRA CEL: fix error checking in unit test Not getting an expected error wasn't checked. --- .../k8s.io/dynamic-resource-allocation/cel/compile_test.go | 6 ++++++ 1 file changed, 6 insertions(+) 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 33ed7724e98..ee697a0495b 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 @@ -248,6 +248,9 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) } return } + if scenario.expectCompileError != "" { + t.Fatalf("expected compile error %q, got none", scenario.expectCompileError) + } if expect, actual := scenario.expectCost, result.MaxCost; expect != actual { t.Errorf("expected CEL cost %d, got %d instead", expect, actual) } @@ -262,6 +265,9 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) } return } + if scenario.expectMatchError != "" { + t.Fatalf("expected match error %q, got none", scenario.expectMatchError) + } if match != scenario.expectMatch { t.Fatalf("expected result %v, got %v", scenario.expectMatch, match) } From 39f259286356204ebf0527026256e988b22278c0 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 17 Oct 2024 17:50:41 +0200 Subject: [PATCH 5/9] DRA CEL: avoid redundant cel.CostTracking It's already called by the base environment. --- staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go | 1 - 1 file changed, 1 deletion(-) 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 375b2717eef..53418314a47 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -136,7 +136,6 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty // 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 { From 7b0071d71b9bdbd921ad157a4d2642aa596aa923 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 17 Oct 2024 17:52:05 +0200 Subject: [PATCH 6/9] DRA CEL: disable runtime cost check In DRA, the cost check is done only at validation time. At runtime, any expression that passed validation gets executed without interrupting it. The advantage is that it becomes easier to change the limit because stored expression do not suddenly fail after an up- or downgrade. The limit could even become a configuration parameter of the apiserver because that is the only place where the limit gets checked --- .../cel/compile.go | 24 +++++++++++++++---- .../cel/compile_test.go | 7 +++--- 2 files changed, 22 insertions(+), 9 deletions(-) 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 53418314a47..663e550be92 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "math" "reflect" "strings" "sync" @@ -130,12 +131,25 @@ 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. + // The Kubernetes CEL base environment sets the VAP cost limit. // - // This call here should override that. In practice it shouldn't - // matter because the limits are the same. - cel.CostLimit(resourceapi.CELSelectorExpressionMaxCost), + // In DRA, the cost check is done only at validation time. At + // runtime, any expression that passed validation gets executed + // without interrupting it. The advantage is that it becomes + // easier to change the limit because stored expression do not + // suddenly fail after an up- or downgrade. The limit could + // even become a configuration parameter of the apiserver + // because that is the only place where the limit gets checked + // + // We cannot unset the cost limit + // (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/cel/options.go#L512-L518). But + // we can set a high value that then never triggers + // (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/interpreter/runtimecost.go#L104-L106). + // + // Even better would be to enable cost tracking only during + // validation, but for that k8s.io/apiserver/pkg/cel/environment would + // have to be changed to not set the limit. + cel.CostLimit(math.MaxInt64), cel.InterruptCheckFrequency(celconfig.CheckFrequency), ) if err != nil { 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 ee697a0495b..b0fa08f30f9 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 @@ -227,10 +227,9 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) } return attributes }(), - expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, - driver: "dra.example.com", - expectMatchError: "actual cost limit exceeded", - expectCost: 18446744073709551615, // Exceeds limit! + expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + driver: "dra.example.com", + expectCost: 18446744073709551615, // Exceeds limit! }, } { t.Run(name, func(t *testing.T) { From 425f694fe6c5a64f095bf4f10bbc2f39311bb66f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 18 Oct 2024 10:11:34 +0200 Subject: [PATCH 7/9] DRA CEL: log actual cost This may be useful for validating the cost estimate. --- .../dynamic-resource-allocation/cel/compile.go | 14 +++++++------- .../cel/compile_test.go | 13 ++++++++++++- .../structured/allocator.go | 7 ++++--- 3 files changed, 23 insertions(+), 11 deletions(-) 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 663e550be92..c8a33e5487a 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -198,14 +198,14 @@ func getAttributeValue(attr resourceapi.DeviceAttribute) (any, error) { var boolType = reflect.TypeOf(true) -func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (bool, error) { +func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (bool, *cel.EvalDetails, error) { // TODO (future): avoid building these maps and instead use a proxy // which wraps the underlying maps and directly looks up values. attributes := make(map[string]any) for name, attr := range input.Attributes { value, err := getAttributeValue(attr) if err != nil { - return false, fmt.Errorf("attribute %s: %w", name, err) + return false, nil, fmt.Errorf("attribute %s: %w", name, err) } domain, id := parseQualifiedName(name, input.Driver) if attributes[domain] == nil { @@ -231,19 +231,19 @@ func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (boo }, } - result, _, err := c.Program.ContextEval(ctx, variables) + result, details, err := c.Program.ContextEval(ctx, variables) if err != nil { - return false, err + return false, details, err } resultAny, err := result.ConvertToNative(boolType) if err != nil { - return false, fmt.Errorf("CEL result of type %s could not be converted to bool: %w", result.Type().TypeName(), err) + return false, details, fmt.Errorf("CEL result of type %s could not be converted to bool: %w", result.Type().TypeName(), err) } resultBool, ok := resultAny.(bool) if !ok { - return false, fmt.Errorf("CEL native result value should have been a bool, got instead: %T", resultAny) + return false, details, fmt.Errorf("CEL native result value should have been a bool, got instead: %T", resultAny) } - return resultBool, nil + return resultBool, details, nil } func mustBuildEnv() *environment.EnvSet { 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 b0fa08f30f9..519477a84ce 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 @@ -254,7 +254,18 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) 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}) + match, details, err := result.DeviceMatches(ctx, Device{Attributes: scenario.attributes, Capacity: scenario.capacity, Driver: scenario.driver}) + // details.ActualCost can be called for nil details, no need to check. + actualCost := ptr.Deref(details.ActualCost(), 0) + if scenario.expectCost > 0 { + t.Logf("actual cost %d, %d%% of worst-case estimate", actualCost, actualCost*100/scenario.expectCost) + } else { + t.Logf("actual cost %d, expected zero costs", actualCost) + if actualCost > 0 { + t.Errorf("expected zero costs for (presumably) constant expression %q, got instead %d", scenario.expression, actualCost) + } + } + if err != nil { if scenario.expectMatchError == "" { t.Fatalf("unexpected evaluation error: %v", err) diff --git a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go index c7bdd1f4247..7ec9ca61e21 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go @@ -30,6 +30,7 @@ import ( resourcelisters "k8s.io/client-go/listers/resource/v1alpha3" "k8s.io/dynamic-resource-allocation/cel" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ) // ClaimLister returns a subset of the claims that a @@ -665,11 +666,11 @@ func (alloc *allocator) selectorsMatch(r requestIndices, device *resourceapi.Bas return false, fmt.Errorf("claim %s: selector #%d: CEL compile error: %w", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), i, expr.Error) } - matches, err := expr.DeviceMatches(alloc.ctx, cel.Device{Driver: deviceID.Driver, Attributes: device.Attributes, Capacity: device.Capacity}) + matches, details, err := expr.DeviceMatches(alloc.ctx, cel.Device{Driver: deviceID.Driver, Attributes: device.Attributes, Capacity: device.Capacity}) if class != nil { - alloc.logger.V(7).Info("CEL result", "device", deviceID, "class", klog.KObj(class), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "err", err) + alloc.logger.V(7).Info("CEL result", "device", deviceID, "class", klog.KObj(class), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "actualCost", ptr.Deref(details.ActualCost(), 0), "err", err) } else { - alloc.logger.V(7).Info("CEL result", "device", deviceID, "claim", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "err", err) + alloc.logger.V(7).Info("CEL result", "device", deviceID, "claim", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), "selector", i, "expression", selector.CEL.Expression, "actualCost", ptr.Deref(details.ActualCost(), 0), "matches", matches, "err", err) } if err != nil { From 021c9fb92f9cc7b6db898277345d2d3a0288e611 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 18 Oct 2024 10:12:41 +0200 Subject: [PATCH 8/9] DRA CEL: add benchmark Expression evaluation in all scenarios gets benchmarked where compilation works. A pending optimization in another PR caches compiled expressions, so the time for compilation will become less important. What matters is the actual evaluation. --- .../cel/compile_test.go | 419 ++++++++++-------- 1 file changed, 230 insertions(+), 189 deletions(-) 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 519477a84ce..33c9254773a 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 @@ -28,148 +28,147 @@ import ( "k8s.io/utils/ptr" ) -func TestCompile(t *testing.T) { - for name, scenario := range map[string]struct { - expression string - driver string - attributes map[resourceapi.QualifiedName]resourceapi.DeviceAttribute - capacity map[resourceapi.QualifiedName]resource.Quantity - expectCompileError string - expectMatchError string - expectMatch bool +var testcases = map[string]struct { + expression string + driver string + attributes map[resourceapi.QualifiedName]resourceapi.DeviceAttribute + capacity map[resourceapi.QualifiedName]resource.Quantity + 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", - expectMatch: true, - }, - "false": { - expression: "false", - expectMatch: false, - }, - "syntax-error": { - expression: "?!", - expectCompileError: "Syntax error", - }, - "type-error": { - expression: `1`, - expectCompileError: "must evaluate to bool or the unknown type, not int", - }, - "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: ` + // 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", + expectMatch: true, + }, + "false": { + expression: "false", + expectMatch: false, + }, + "syntax-error": { + expression: "?!", + expectCompileError: "Syntax error", + }, + "type-error": { + expression: `1`, + expectCompileError: "must evaluate to bool or the unknown type, not int", + }, + "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: ` device.capacity["dra.example.com"].quantity.isGreaterThan(quantity("1Ki")) && device.attributes["dra.example.com"].bool && device.attributes["dra.example.com"].int > 0 && @@ -181,57 +180,60 @@ device.attributes["dra.example.com"]["int"] > 0 && device.attributes["dra.example.com"]["string"] == "fish" && device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) `, - attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{ - "bool": {BoolValue: ptr.To(true)}, - "int": {IntValue: ptr.To(int64(1))}, - "string": {StringValue: ptr.To("fish")}, - "version": {VersionValue: ptr.To("1.0.0")}, - }, - capacity: map[resourceapi.QualifiedName]resource.Quantity{ - "quantity": resource.MustParse("1Mi"), - }, - driver: "dra.example.com", - expectMatch: true, - expectCost: 52, + attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{ + "bool": {BoolValue: ptr.To(true)}, + "int": {IntValue: ptr.To(int64(1))}, + "string": {StringValue: ptr.To("fish")}, + "version": {VersionValue: ptr.To("1.0.0")}, }, - "many": { - expression: `device.attributes["dra.example.com"].a && device.attributes["dra.example.com"].b && device.attributes["dra.example.com"].c`, - attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{ - "a": {BoolValue: ptr.To(true)}, - "b": {BoolValue: ptr.To(true)}, - "c": {BoolValue: ptr.To(true)}, - }, - driver: "dra.example.com", - expectMatch: true, - expectCost: 12, + capacity: map[resourceapi.QualifiedName]resource.Quantity{ + "quantity": resource.MustParse("1Mi"), }, - "expensive": { - // The worst-case is based on the maximum number of - // attributes and the maximum attribute name length. - // To actually reach that expected cost at runtime, we must - // have many attributes. - attributes: func() map[resourceapi.QualifiedName]resourceapi.DeviceAttribute { - attributes := make(map[resourceapi.QualifiedName]resourceapi.DeviceAttribute) - prefix := "dra.example.com/" - attribute := resourceapi.DeviceAttribute{ - StringValue: ptr.To("abc"), - } - // If the cost estimate was accurate, using exactly as many attributes - // as allowed at most should exceed the limit. In practice, the estimate - // is an upper bound and significantly more attributes are needed before - // the runtime cost becomes too large. - for i := 0; i < 1000*resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice; i++ { - suffix := fmt.Sprintf("-%d", i) - name := prefix + strings.Repeat("x", resourceapi.DeviceMaxIDLength-len(suffix)) + suffix - attributes[resourceapi.QualifiedName(name)] = attribute - } - return attributes - }(), - expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, - driver: "dra.example.com", - expectCost: 18446744073709551615, // Exceeds limit! + 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`, + attributes: map[resourceapi.QualifiedName]resourceapi.DeviceAttribute{ + "a": {BoolValue: ptr.To(true)}, + "b": {BoolValue: ptr.To(true)}, + "c": {BoolValue: ptr.To(true)}, }, - } { + 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. + // To actually reach that expected cost at runtime, we must + // have many attributes. + attributes: func() map[resourceapi.QualifiedName]resourceapi.DeviceAttribute { + attributes := make(map[resourceapi.QualifiedName]resourceapi.DeviceAttribute) + prefix := "dra.example.com/" + attribute := resourceapi.DeviceAttribute{ + StringValue: ptr.To("abc"), + } + // If the cost estimate was accurate, using exactly as many attributes + // as allowed at most should exceed the limit. In practice, the estimate + // is an upper bound and significantly more attributes are needed before + // the runtime cost becomes too large. + for i := 0; i < 1000*resourceapi.ResourceSliceMaxAttributesAndCapacitiesPerDevice; i++ { + suffix := fmt.Sprintf("-%d", i) + name := prefix + strings.Repeat("x", resourceapi.DeviceMaxIDLength-len(suffix)) + suffix + attributes[resourceapi.QualifiedName(name)] = attribute + } + return attributes + }(), + expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + driver: "dra.example.com", + expectCost: 18446744073709551615, // Exceeds limit! + }, +} + +func TestCEL(t *testing.T) { + for name, scenario := range testcases { t.Run(name, func(t *testing.T) { _, ctx := ktesting.NewTestContext(t) result := GetCompiler().CompileCELExpression(scenario.expression, environment.StoredExpressions) @@ -284,3 +286,42 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) }) } } + +func BenchmarkDeviceMatches(b *testing.B) { + for name, scenario := range testcases { + if scenario.expectCompileError != "" { + continue + } + b.Run(name, func(b *testing.B) { + _, ctx := ktesting.NewTestContext(b) + result := GetCompiler().CompileCELExpression(scenario.expression, environment.StoredExpressions) + if result.Error != nil { + b.Fatalf("unexpected compile error: %s", result.Error.Error()) + } + + for i := 0; i < b.N; i++ { + // It would be nice to measure + // time/actual_cost, but the time as observed + // here also includes additional preparations + // in result.DeviceMatches and thus cannot be + // used. + match, _, err := result.DeviceMatches(ctx, Device{Attributes: scenario.attributes, Capacity: scenario.capacity, Driver: scenario.driver}) + if err != nil { + if scenario.expectMatchError == "" { + b.Fatalf("unexpected evaluation error: %v", err) + } + if !strings.Contains(err.Error(), scenario.expectMatchError) { + b.Fatalf("expected evaluation error to contain %q, but got instead: %v", scenario.expectMatchError, err) + } + return + } + if scenario.expectMatchError != "" { + b.Fatalf("expected match error %q, got none", scenario.expectMatchError) + } + if match != scenario.expectMatch { + b.Fatalf("expected result %v, got %v", scenario.expectMatch, match) + } + } + }) + } +} From d53cb79cec11094099205a8c114a416b430a6c2c Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 23 Oct 2024 21:15:05 +0200 Subject: [PATCH 9/9] DRA cel: enforce runtime limit by default again As pointed out during code review, the CEL cost estimates are not considered perfectly reliable. Therefore it is better to also do runtime checks. Some downstream users might decide to allow CEL expressions to run longer. Therefore the cost limit is now part of an Options struct. kube-scheduler uses the default cost limit defined in the resource.k8s.io API, which is the same cost limit that also the apiserver uses during validation. --- pkg/apis/resource/types.go | 9 ++++- pkg/apis/resource/validation/validation.go | 2 +- .../src/k8s.io/api/resource/v1alpha3/types.go | 9 ++++- .../cel/compile.go | 40 +++++++++---------- .../cel/compile_test.go | 12 +++--- .../structured/allocator.go | 3 +- 6 files changed, 40 insertions(+), 35 deletions(-) diff --git a/pkg/apis/resource/types.go b/pkg/apis/resource/types.go index 909797bf327..77e0617926a 100644 --- a/pkg/apis/resource/types.go +++ b/pkg/apis/resource/types.go @@ -545,8 +545,13 @@ type CELDeviceSelector struct { // 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. +// guaranteed that existing expressions will continue to be valid. +// +// However, the kube-scheduler also applies this cost limit at runtime, so it +// could happen that a valid expression fails at runtime after an up- or +// downgrade. This can also happen without version skew when the cost estimate +// underestimated the actual cost. That this might happen is the reason why +// kube-scheduler enforces the runtime limit instead of relying on validation. // // According to // https://github.com/kubernetes/kubernetes/blob/4aeaf1e99e82da8334c0d6dddd848a194cd44b4f/staging/src/k8s.io/apiserver/pkg/apis/cel/config.go#L20-L22, diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 7425d00cdff..a41d0c69349 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -176,7 +176,7 @@ func validateCELSelector(celSelector resource.CELDeviceSelector, fldPath *field. return allErrs } - result := dracel.GetCompiler().CompileCELExpression(celSelector.Expression, envType) + result := dracel.GetCompiler().CompileCELExpression(celSelector.Expression, dracel.Options{EnvType: &envType}) if result.Error != nil { allErrs = append(allErrs, convertCELErrorToValidationError(fldPath.Child("expression"), celSelector.Expression, result.Error)) } else if result.MaxCost > resource.CELSelectorExpressionMaxCost { diff --git a/staging/src/k8s.io/api/resource/v1alpha3/types.go b/staging/src/k8s.io/api/resource/v1alpha3/types.go index 9447cfd163a..7055fd31a2b 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/types.go +++ b/staging/src/k8s.io/api/resource/v1alpha3/types.go @@ -551,8 +551,13 @@ type CELDeviceSelector struct { // 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. +// guaranteed that existing expressions will continue to be valid. +// +// However, the kube-scheduler also applies this cost limit at runtime, so it +// could happen that a valid expression fails at runtime after an up- or +// downgrade. This can also happen without version skew when the cost estimate +// underestimated the actual cost. That this might happen is the reason why +// kube-scheduler enforces the runtime limit instead of relying on validation. // // According to // https://github.com/kubernetes/kubernetes/blob/4aeaf1e99e82da8334c0d6dddd848a194cd44b4f/staging/src/k8s.io/apiserver/pkg/apis/cel/config.go#L20-L22, 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 c8a33e5487a..a0ea09f7d8d 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "math" "reflect" "strings" "sync" @@ -39,6 +38,7 @@ import ( apiservercel "k8s.io/apiserver/pkg/cel" "k8s.io/apiserver/pkg/cel/environment" "k8s.io/apiserver/pkg/cel/library" + "k8s.io/utils/ptr" ) const ( @@ -93,10 +93,21 @@ func newCompiler() *compiler { return &compiler{envset: mustBuildEnv()} } +// Options contains several additional parameters +// for [CompileCELExpression]. All of them have reasonable +// defaults. +type Options struct { + // EnvType allows to override the default environment type [environment.StoredExpressions]. + EnvType *environment.Type + + // CostLimit allows overriding the default runtime cost limit [resourceapi.CELSelectorExpressionMaxCost]. + CostLimit *uint64 +} + // CompileCELExpression returns a compiled CEL expression. It evaluates to bool. // // TODO (https://github.com/kubernetes/kubernetes/issues/125826): validate AST to detect invalid attribute names. -func (c compiler) CompileCELExpression(expression string, envType environment.Type) CompilationResult { +func (c compiler) CompileCELExpression(expression string, options Options) CompilationResult { resultError := func(errorString string, errType apiservercel.ErrorType) CompilationResult { return CompilationResult{ Error: &apiservercel.Error{ @@ -107,7 +118,7 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty } } - env, err := c.envset.Env(envType) + env, err := c.envset.Env(ptr.Deref(options.EnvType, environment.StoredExpressions)) if err != nil { return resultError(fmt.Sprintf("unexpected error loading CEL environment: %v", err), apiservercel.ErrorTypeInternal) } @@ -131,25 +142,10 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal) } prog, err := env.Program(ast, - // The Kubernetes CEL base environment sets the VAP cost limit. - // - // In DRA, the cost check is done only at validation time. At - // runtime, any expression that passed validation gets executed - // without interrupting it. The advantage is that it becomes - // easier to change the limit because stored expression do not - // suddenly fail after an up- or downgrade. The limit could - // even become a configuration parameter of the apiserver - // because that is the only place where the limit gets checked - // - // We cannot unset the cost limit - // (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/cel/options.go#L512-L518). But - // we can set a high value that then never triggers - // (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/interpreter/runtimecost.go#L104-L106). - // - // Even better would be to enable cost tracking only during - // validation, but for that k8s.io/apiserver/pkg/cel/environment would - // have to be changed to not set the limit. - cel.CostLimit(math.MaxInt64), + // The Kubernetes CEL base environment sets the VAP limit as runtime cost limit. + // DRA has its own default cost limit and also allows the caller to change that + // limit. + cel.CostLimit(ptr.Deref(options.CostLimit, resourceapi.CELSelectorExpressionMaxCost)), cel.InterruptCheckFrequency(celconfig.CheckFrequency), ) if err != nil { 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 33c9254773a..60d38af6fb7 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 @@ -23,7 +23,6 @@ import ( resourceapi "k8s.io/api/resource/v1alpha3" "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apiserver/pkg/cel/environment" "k8s.io/klog/v2/ktesting" "k8s.io/utils/ptr" ) @@ -226,9 +225,10 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) } return attributes }(), - expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, - driver: "dra.example.com", - expectCost: 18446744073709551615, // Exceeds limit! + expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`, + driver: "dra.example.com", + expectMatchError: "actual cost limit exceeded", + expectCost: 18446744073709551615, // Exceeds limit! }, } @@ -236,7 +236,7 @@ func TestCEL(t *testing.T) { for name, scenario := range testcases { t.Run(name, func(t *testing.T) { _, ctx := ktesting.NewTestContext(t) - result := GetCompiler().CompileCELExpression(scenario.expression, environment.StoredExpressions) + result := GetCompiler().CompileCELExpression(scenario.expression, Options{}) if scenario.expectCompileError != "" && result.Error == nil { t.Fatalf("expected compile error %q, got none", scenario.expectCompileError) } @@ -294,7 +294,7 @@ func BenchmarkDeviceMatches(b *testing.B) { } b.Run(name, func(b *testing.B) { _, ctx := ktesting.NewTestContext(b) - result := GetCompiler().CompileCELExpression(scenario.expression, environment.StoredExpressions) + result := GetCompiler().CompileCELExpression(scenario.expression, Options{}) if result.Error != nil { b.Fatalf("unexpected compile error: %s", result.Error.Error()) } diff --git a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go index 7ec9ca61e21..0d5a942af54 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go @@ -26,7 +26,6 @@ import ( v1 "k8s.io/api/core/v1" resourceapi "k8s.io/api/resource/v1alpha3" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apiserver/pkg/cel/environment" resourcelisters "k8s.io/client-go/listers/resource/v1alpha3" "k8s.io/dynamic-resource-allocation/cel" "k8s.io/klog/v2" @@ -653,7 +652,7 @@ func (alloc *allocator) isSelectable(r requestIndices, slice *resourceapi.Resour func (alloc *allocator) selectorsMatch(r requestIndices, device *resourceapi.BasicDevice, deviceID DeviceID, class *resourceapi.DeviceClass, selectors []resourceapi.DeviceSelector) (bool, error) { for i, selector := range selectors { - expr := cel.GetCompiler().CompileCELExpression(selector.CEL.Expression, environment.StoredExpressions) + expr := cel.GetCompiler().CompileCELExpression(selector.CEL.Expression, cel.Options{}) if expr.Error != nil { // Could happen if some future apiserver accepted some // future expression and then got downgraded. Normally