diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 94e74f869fd..b97493d040b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -22,6 +22,7 @@ import ( "math" "reflect" "regexp" + "sort" "strings" "unicode" "unicode/utf8" @@ -52,6 +53,8 @@ var ( const ( // StaticEstimatedCostLimit represents the largest-allowed static CEL cost on a per-expression basis. StaticEstimatedCostLimit = 10000000 + // StaticEstimatedCRDCostLimit represents the largest-allowed total cost for the x-kubernetes-validations rules of a CRD. + StaticEstimatedCRDCostLimit = 100000000 ) // ValidateCustomResourceDefinition statically validates @@ -720,7 +723,20 @@ func validateCustomResourceDefinitionValidation(ctx context.Context, customResou requireValidPropertyType: opts.requireValidPropertyType, } - allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true, &opts, rootCostInfo())...) + costInfo := rootCostInfo() + allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema, true, &opts, costInfo)...) + + if costInfo.TotalCost != nil { + if costInfo.TotalCost.totalCost > StaticEstimatedCRDCostLimit { + for _, expensive := range costInfo.TotalCost.mostExpensive { + costErrorMsg := fmt.Sprintf("contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema") + allErrs = append(allErrs, field.Forbidden(expensive.path, costErrorMsg)) + } + + costErrorMsg := getCostErrorMessage("x-kubernetes-validations estimated rule cost total for entire OpenAPIv3 schema", costInfo.TotalCost.totalCost, StaticEstimatedCRDCostLimit) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("openAPIV3Schema"), costErrorMsg)) + } + } if opts.requireStructuralSchema { if ss, err := structuralschema.NewStructural(schema); err != nil { @@ -760,6 +776,43 @@ type costInfo struct { // that the parent schemas offer no bound to the number of times a data element for the current // schema can exist. MaxCardinality *uint64 + // TotalCost accumulates the x-kubernetes-validators estimated rule cost total for an entire custom resource + // definition. A single totalCost is allocated for each validation call and passed through the stack as the + // custom resource definition's OpenAPIv3 schema is recursively validated. + TotalCost *totalCost +} + +type totalCost struct { + // totalCost accumulates the x-kubernetes-validators estimated rule cost total. + totalCost uint64 + // mostExpensive accumulates the top 4 most expensive rules contributing to the totalCost. Only rules + // that accumulate at least 1% of total cost limit are included. + mostExpensive []ruleCost +} + +func (c *totalCost) observeExpressionCost(path *field.Path, cost uint64) { + if math.MaxUint64-c.totalCost < cost { + c.totalCost = math.MaxUint64 + } else { + c.totalCost += cost + } + + if cost < StaticEstimatedCRDCostLimit/100 { // ignore rules that contribute < 1% of total cost limit + return + } + c.mostExpensive = append(c.mostExpensive, ruleCost{path: path, cost: cost}) + sort.Slice(c.mostExpensive, func(i, j int) bool { + // sort in descending order so the most expensive rule is first + return c.mostExpensive[i].cost > c.mostExpensive[j].cost + }) + if len(c.mostExpensive) > 4 { + c.mostExpensive = c.mostExpensive[:4] + } +} + +type ruleCost struct { + path *field.Path + cost uint64 } // MultiplyByElementCost returns a costInfo where the MaxCardinality is multiplied by the @@ -767,7 +820,7 @@ type costInfo struct { // MaxCardinality is unbounded (nil) or the factor that the schema increase the cardinality // is unbounded, the resulting costInfo's MaxCardinality is also unbounded. func (c *costInfo) MultiplyByElementCost(schema *apiextensions.JSONSchemaProps) costInfo { - result := costInfo{MaxCardinality: unbounded} + result := costInfo{TotalCost: c.TotalCost, MaxCardinality: unbounded} if schema == nil { // nil schemas can be passed since we call MultiplyByElementCost // before ValidateCustomResourceDefinitionOpenAPISchema performs its nil check @@ -831,6 +884,7 @@ func rootCostInfo() costInfo { rootCardinality := uint64(1) return costInfo{ MaxCardinality: &rootCardinality, + TotalCost: &totalCost{}, } } @@ -1050,9 +1104,12 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch for i, cr := range compResults { expressionCost := getExpressionCost(cr, nodeCostInfo) if expressionCost > StaticEstimatedCostLimit { - costErrorMsg := getCostErrorMessage(expressionCost, StaticEstimatedCostLimit) + costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit) allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg)) } + if nodeCostInfo.TotalCost != nil { + nodeCostInfo.TotalCost.observeExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost) + } if cr.Error != nil { if cr.Error.Type == cel.ErrorTypeRequired { allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail)) @@ -1096,15 +1153,20 @@ func getExpressionCost(cr cel.CompilationResult, cardinalityCost costInfo) uint6 return multiplyWithOverflowGuard(cr.MaxCost, cr.MaxCardinality) } -func getCostErrorMessage(expressionCost, costLimit uint64) string { - exceedFactor := float64(expressionCost) / float64(StaticEstimatedCostLimit) +func getCostErrorMessage(costName string, expressionCost, costLimit uint64) string { + exceedFactor := float64(expressionCost) / float64(costLimit) + var factor string if exceedFactor > 100.0 { // if exceedFactor is greater than 2 orders of magnitude, the rule is likely O(n^2) or worse // and will probably never validate without some set limits // also in such cases the cost estimation is generally large enough to not add any value - return fmt.Sprintf("CEL rule exceeded budget by more than 100x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are used)") + factor = fmt.Sprintf("more than 100x") + } else if exceedFactor < 1.5 { + factor = fmt.Sprintf("%fx", exceedFactor) // avoid reporting "exceeds budge by a factor of 1.0x" + } else { + factor = fmt.Sprintf("%.1fx", exceedFactor) } - return fmt.Sprintf("CEL rule exceeded budget by factor of %.1fx (try adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are used)", exceedFactor) + return fmt.Sprintf("%s exceeds budget by factor of %s (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)", costName, factor) } var newlineMatcher = regexp.MustCompile(`[\n\r]+`) // valid newline chars in CEL grammar diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index d0324f220c7..56ead47c0b7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -24,6 +24,8 @@ import ( "strings" "testing" + "k8s.io/utils/pointer" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" @@ -33,7 +35,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/utils/pointer" ) type validationMatch struct { @@ -7972,7 +7973,11 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, expectedErrors: []validationMatch{ + // exceeds per-rule limit and contributes to total limit being exceeded (1 error for each) forbidden("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), + forbidden("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), + // total limit is exceeded + forbidden("spec.validation.openAPIV3Schema"), }, }, { @@ -8005,7 +8010,11 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, expectedErrors: []validationMatch{ + // exceeds per-rule limit and contributes to total limit being exceeded (1 error for each) forbidden("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), + forbidden("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), + // total limit is exceeded + forbidden("spec.validation.openAPIV3Schema"), }, }, { @@ -8090,6 +8099,58 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, expectedErrors: []validationMatch{}, }, + { + name: "forbid validation rules where cost total exceeds total limit", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "list": { + Type: "array", + MaxItems: int64Ptr(100000), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + MaxLength: int64Ptr(5000), + XValidations: apiextensions.ValidationRules{ + {Rule: "self.contains('keyword')"}, + }, + }, + }, + }, + "map": { + Type: "object", + MaxProperties: int64Ptr(1000), + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + MaxLength: int64Ptr(5000), + XValidations: apiextensions.ValidationRules{ + {Rule: "self.contains('keyword')"}, + }, + }, + }, + }, + "field": { // include a validation rule that does not contribute to total limit being exceeded (i.e. it is less than 1% of the limit) + Type: "integer", + XValidations: apiextensions.ValidationRules{ + {Rule: "self > 50 && self < 100"}, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + // exceeds per-rule limit and contributes to total limit being exceeded (1 error for each) + forbidden("spec.validation.openAPIV3Schema.properties[list].items.x-kubernetes-validations[0].rule"), + forbidden("spec.validation.openAPIV3Schema.properties[list].items.x-kubernetes-validations[0].rule"), + // contributes to total limit being exceeded, but does not exceed per-rule limit + forbidden("spec.validation.openAPIV3Schema.properties[map].additionalProperties.x-kubernetes-validations[0].rule"), + // total limit is exceeded + forbidden("spec.validation.openAPIV3Schema"), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -8463,6 +8524,83 @@ func TestCostInfo(t *testing.T) { } } +func TestPerCRDEstimatedCost(t *testing.T) { + tests := []struct { + name string + costs []uint64 + expectedExpensive []uint64 + expectedTotal uint64 + }{ + { + name: "no costs", + costs: []uint64{}, + expectedExpensive: []uint64{}, + expectedTotal: uint64(0), + }, + { + name: "one cost", + costs: []uint64{1000000}, + expectedExpensive: []uint64{1000000}, + expectedTotal: uint64(1000000), + }, + { + name: "one cost, ignored", // costs < 1% of the per-CRD cost limit are not considered expensive + costs: []uint64{900000}, + expectedExpensive: []uint64{}, + expectedTotal: uint64(900000), + }, + { + name: "2 costs", + costs: []uint64{5000000, 25000000}, + expectedExpensive: []uint64{25000000, 5000000}, + expectedTotal: uint64(30000000), + }, + { + name: "3 costs, one ignored", + costs: []uint64{5000000, 25000000, 900000}, + expectedExpensive: []uint64{25000000, 5000000}, + expectedTotal: uint64(30900000), + }, + { + name: "4 costs", + costs: []uint64{16000000, 50000000, 34000000, 50000000}, + expectedExpensive: []uint64{50000000, 50000000, 34000000, 16000000}, + expectedTotal: uint64(150000000), + }, + { + name: "5 costs, one trimmed, one ignored", // only the top 4 most expensive are tracked + costs: []uint64{16000000, 50000000, 900000, 34000000, 50000000, 50000001}, + expectedExpensive: []uint64{50000001, 50000000, 50000000, 34000000}, + expectedTotal: uint64(200900001), + }, + { + name: "costs do not overflow", + costs: []uint64{math.MaxUint64 / 2, math.MaxUint64 / 2, 1, 10, 100, 1000}, + expectedExpensive: []uint64{math.MaxUint64 / 2, math.MaxUint64 / 2}, + expectedTotal: uint64(math.MaxUint64), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + crdCost := rootCostInfo().TotalCost + for _, cost := range tt.costs { + crdCost.observeExpressionCost(nil, cost) + } + if len(crdCost.mostExpensive) != len(tt.expectedExpensive) { + t.Fatalf("expected %d largest costs but got %d: %v", len(tt.expectedExpensive), len(crdCost.mostExpensive), crdCost.mostExpensive) + } + for i, expensive := range crdCost.mostExpensive { + if tt.expectedExpensive[i] != expensive.cost { + t.Errorf("expected largest cost of %d at index %d but got %d", tt.expectedExpensive[i], i, expensive.cost) + } + } + if tt.expectedTotal != crdCost.totalCost { + t.Errorf("expected total cost of %d but got %d", tt.expectedTotal, crdCost.totalCost) + } + }) + } +} + func int64ptr(i int64) *int64 { return &i } diff --git a/test/integration/apiserver/crd_validation_expressions_test.go b/test/integration/apiserver/crd_validation_expressions_test.go index d5db2796eae..0a6c24d9724 100644 --- a/test/integration/apiserver/crd_validation_expressions_test.go +++ b/test/integration/apiserver/crd_validation_expressions_test.go @@ -511,7 +511,7 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) { "spec": map[string]interface{}{ "x": int64(2), "y": int64(2), - "extra": "skipValidation?", + "extra": strings.Repeat("x", 201), "floatMap": map[string]interface{}{ "key1": 0.2, "key2": 0.3, @@ -541,18 +541,18 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) { "key1": 0.2, "key2": 0.3, }, - "assocList": []interface{}{ - map[string]interface{}{ - "k": "a", - "v": "1", - }, - map[string]interface{}{ - "a": "a", - }, - }, - "limit": nil, + "assocList": []interface{}{}, + "limit": nil, }, }} + assocList := cr.Object["spec"].(map[string]interface{})["assocList"].([]interface{}) + for i := 1; i <= 101; i++ { + assocList = append(assocList, map[string]interface{}{ + "k": "a", + "v": fmt.Sprintf("%d", i), + }) + } + cr.Object["spec"].(map[string]interface{})["assocList"] = assocList _, err = crClient.Create(context.TODO(), cr, metav1.CreateOptions{}) if err == nil || !strings.Contains(err.Error(), "some validation rules were not checked because the object was invalid; correct the existing errors to complete validation") { @@ -569,13 +569,9 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) { "name": name2, }, "spec": map[string]interface{}{ - "x": int64(2), - "y": int64(2), - "floatMap": map[string]interface{}{ - "key1": 0.2, - "key2": 0.3, - "key3": 0.4, - }, + "x": int64(2), + "y": int64(2), + "floatMap": map[string]interface{}{}, "assocList": []interface{}{ map[string]interface{}{ "k": "a", @@ -585,6 +581,10 @@ func TestCustomResourceValidatorsWithBlockingErrors(t *testing.T) { "limit": nil, }, }} + floatMap := cr.Object["spec"].(map[string]interface{})["floatMap"].(map[string]interface{}) + for i := 1; i <= 101; i++ { + floatMap[fmt.Sprintf("key%d", i)] = float64(i) / 10 + } _, err = crClient.Create(context.TODO(), cr, metav1.CreateOptions{}) if err == nil || !strings.Contains(err.Error(), "some validation rules were not checked because the object was invalid; correct the existing errors to complete validation") { @@ -751,13 +751,12 @@ var structuralSchemaWithValidators = []byte(` }, "assocList": { "type": "array", - "maxItems": 10, + "maxItems": 100, "items": { "type": "object", - "maxProperties": 12, "properties": { - "k": { "type": "string", "maxLength": 3}, - "v": { "type": "string", "maxLength": 3} + "k": { "type": "string", "maxLength": 200}, + "v": { "type": "string", "maxLength": 200} }, "required": ["k"] }, @@ -817,7 +816,7 @@ var structuralSchemaWithBlockingErr = []byte(` }, "extra": { "type": "string", - "maxLength": 0, + "maxLength": 200, "x-kubernetes-validations": [ { "rule": "self.startsWith('anything')" @@ -826,7 +825,7 @@ var structuralSchemaWithBlockingErr = []byte(` }, "floatMap": { "type": "object", - "maxProperties": 2, + "maxProperties": 100, "additionalProperties": { "type": "number" }, "x-kubernetes-validations": [ { @@ -836,10 +835,9 @@ var structuralSchemaWithBlockingErr = []byte(` }, "assocList": { "type": "array", - "maxItems": 1, + "maxItems": 100, "items": { "type": "object", - "maxItems": 1, "properties": { "k": { "type": "string" }, "v": { "type": "string" } @@ -1008,7 +1006,7 @@ var structuralSchemaWithDefaultMapKeyTransitionRule = []byte(` "k2" ], "x-kubernetes-list-type": "map", - "maxItems": 100, + "maxItems": 1000, "items": { "type": "object", "properties": {