Enfoce per-CRD estimated cost limit

This commit is contained in:
Joe Betz 2022-03-24 15:35:55 -04:00
parent f25c0e5f09
commit ff3d67d76a
3 changed files with 233 additions and 35 deletions

View File

@ -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

View File

@ -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
}

View File

@ -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": {