From 5e9c6936bb35d5aa41bb056d130d852e2f0c8954 Mon Sep 17 00:00:00 2001 From: Cici Huang Date: Wed, 1 May 2024 16:26:41 -0700 Subject: [PATCH] Adding the feature gates to fix cost for VAP and webhook matchConditions. Kubernetes-commit: 3d896724760a957e8059ff80e9f399248eacac66 --- .../apiextensions/validation/validation.go | 6 ++- .../validation/validation_test.go | 6 +-- pkg/apiserver/schema/cel/compilation_test.go | 6 +-- pkg/apiserver/schema/cel/validation.go | 3 +- pkg/apiserver/schema/cel/validation_test.go | 44 ++++++++++++++++++- 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/pkg/apis/apiextensions/validation/validation.go b/pkg/apis/apiextensions/validation/validation.go index 970a6519c..7133776fb 100644 --- a/pkg/apis/apiextensions/validation/validation.go +++ b/pkg/apis/apiextensions/validation/validation.go @@ -92,7 +92,8 @@ func ValidateCustomResourceDefinition(ctx context.Context, obj *apiextensions.Cu requirePrunedDefaults: true, requireAtomicSetType: true, requireMapListKeysMapSetValidation: true, - celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), + // strictCost is always true to enforce cost limits. + celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true), } allErrs := genericvalidation.ValidateObjectMeta(&obj.ObjectMeta, false, nameValidationFn, field.NewPath("metadata")) @@ -231,7 +232,8 @@ func ValidateCustomResourceDefinitionUpdate(ctx context.Context, obj, oldObj *ap requireMapListKeysMapSetValidation: requireMapListKeysMapSetValidation(&oldObj.Spec), preexistingExpressions: findPreexistingExpressions(&oldObj.Spec), versionsWithUnchangedSchemas: findVersionsWithUnchangedSchemas(obj, oldObj), - celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), + // strictCost is always true to enforce cost limits. + celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true), } return validateCustomResourceDefinitionUpdate(ctx, obj, oldObj, opts) } diff --git a/pkg/apis/apiextensions/validation/validation_test.go b/pkg/apis/apiextensions/validation/validation_test.go index 760a37edc..a1492939c 100644 --- a/pkg/apis/apiextensions/validation/validation_test.go +++ b/pkg/apis/apiextensions/validation/validation_test.go @@ -7312,7 +7312,7 @@ func TestValidateCustomResourceDefinitionValidationRuleCompatibility(t *testing. } // Include the test library, which includes the test() function in the storage environment during test - base := environment.MustBaseEnvSet(version.MajorMinor(1, 998)) + base := environment.MustBaseEnvSet(version.MajorMinor(1, 998), true) envSet, err := base.Extend(environment.VersionedOptions{ IntroducedVersion: version.MajorMinor(1, 999), EnvOptions: []cel.EnvOption{library.Test()}, @@ -10104,7 +10104,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx := context.TODO() if tt.opts.celEnvironmentSet == nil { - tt.opts.celEnvironmentSet = environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()) + tt.opts.celEnvironmentSet = environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true) } got := validateCustomResourceDefinitionValidation(ctx, &tt.input, tt.statusEnabled, tt.opts, field.NewPath("spec", "validation")) @@ -10635,7 +10635,7 @@ func TestCelContext(t *testing.T) { celContext := RootCELContext(tt.schema) celContext.converter = converter opts := validationOptions{ - celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), + celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true), } openAPIV3Schema := &specStandardValidatorV3{ allowDefaults: opts.allowDefaults, diff --git a/pkg/apiserver/schema/cel/compilation_test.go b/pkg/apiserver/schema/cel/compilation_test.go index f76d71a07..da946e475 100644 --- a/pkg/apiserver/schema/cel/compilation_test.go +++ b/pkg/apiserver/schema/cel/compilation_test.go @@ -850,7 +850,7 @@ func TestCelCompilation(t *testing.T) { for _, tt := range cases { t.Run(tt.name, func(t *testing.T) { - env, err := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()).Extend( + env, err := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true).Extend( environment.VersionedOptions{ IntroducedVersion: version.MajorMinor(1, 999), EnvOptions: []celgo.EnvOption{celgo.Lib(&fakeLib{})}, @@ -1327,7 +1327,7 @@ func genMapWithCustomItemRule(item *schema.Structural, rule string) func(maxProp // if expectedCostExceedsLimit is non-zero. Typically, only expectedCost or expectedCostExceedsLimit is non-zero, not both. func schemaChecker(schema *schema.Structural, expectedCost uint64, expectedCostExceedsLimit uint64, t *testing.T) func(t *testing.T) { return func(t *testing.T) { - compilationResults, err := Compile(schema, model.SchemaDeclType(schema, false), celconfig.PerCallLimit, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), NewExpressionsEnvLoader()) + compilationResults, err := Compile(schema, model.SchemaDeclType(schema, false), celconfig.PerCallLimit, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true), NewExpressionsEnvLoader()) if err != nil { t.Fatalf("Expected no error, got: %v", err) } @@ -1885,7 +1885,7 @@ func TestCostEstimation(t *testing.T) { } func BenchmarkCompile(b *testing.B) { - env := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()) // prepare the environment + env := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true) // prepare the environment s := genArrayWithRule("number", "true")(nil) b.ReportAllocs() b.ResetTimer() diff --git a/pkg/apiserver/schema/cel/validation.go b/pkg/apiserver/schema/cel/validation.go index b4c8afa9a..d9b595805 100644 --- a/pkg/apiserver/schema/cel/validation.go +++ b/pkg/apiserver/schema/cel/validation.go @@ -90,7 +90,8 @@ func NewValidator(s *schema.Structural, isResourceRoot bool, perCallLimit uint64 // exist. declType is expected to be a CEL DeclType corresponding to the structural schema. // perCallLimit was added for testing purpose only. Callers should always use const PerCallLimit from k8s.io/apiserver/pkg/apis/cel/config.go as input. func validator(s *schema.Structural, isResourceRoot bool, declType *cel.DeclType, perCallLimit uint64) *Validator { - compiledRules, err := Compile(s, declType, perCallLimit, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), StoredExpressionsEnvLoader()) + // strictCost is always true to enforce cost limits. + compiledRules, err := Compile(s, declType, perCallLimit, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true), StoredExpressionsEnvLoader()) var itemsValidator, additionalPropertiesValidator *Validator var propertiesValidators map[string]Validator if s.Items != nil { diff --git a/pkg/apiserver/schema/cel/validation_test.go b/pkg/apiserver/schema/cel/validation_test.go index 654f6aecd..ef75b7c5a 100644 --- a/pkg/apiserver/schema/cel/validation_test.go +++ b/pkg/apiserver/schema/cel/validation_test.go @@ -61,6 +61,7 @@ func TestValidationExpressions(t *testing.T) { costBudget int64 isRoot bool expectSkipped bool + expectedCost int64 }{ // tests where val1 and val2 are equal but val3 is different // equality, comparisons and type specific functions @@ -2006,6 +2007,38 @@ func TestValidationExpressions(t *testing.T) { `quantity(self.val1).isInteger()`, }, }, + {name: "cost for extended lib calculated correctly: isSorted", + obj: objs("20", "200M"), + schema: schemas(stringType, stringType), + valid: []string{ + "[1,2,3,4].isSorted()", + }, + expectedCost: 4, + }, + {name: "cost for extended lib calculated correctly: url", + obj: objs("20", "200M"), + schema: schemas(stringType, stringType), + valid: []string{ + "url('https:://kubernetes.io/').getHostname() != 'test'", + }, + expectedCost: 4, + }, + {name: "cost for extended lib calculated correctly: split", + obj: objs("20", "200M"), + schema: schemas(stringType, stringType), + valid: []string{ + "size('abc 123 def 123'.split(' ')) > 0", + }, + expectedCost: 5, + }, + {name: "cost for extended lib calculated correctly: join", + obj: objs("20", "200M"), + schema: schemas(stringType, stringType), + valid: []string{ + "size(['aa', 'bb', 'cc', 'd', 'e', 'f', 'g', 'h', 'i', 'j'].join(' ')) > 0", + }, + expectedCost: 7, + }, } for i := range tests { @@ -2013,7 +2046,9 @@ func TestValidationExpressions(t *testing.T) { t.Run(tests[i].name, func(t *testing.T) { t.Parallel() tt := tests[i] - tt.costBudget = celconfig.RuntimeCELCostBudget + if tt.costBudget == 0 { + tt.costBudget = celconfig.RuntimeCELCostBudget + } ctx := context.TODO() for j := range tt.valid { validRule := tt.valid[j] @@ -2032,6 +2067,13 @@ func TestValidationExpressions(t *testing.T) { for _, err := range errs { t.Errorf("unexpected error: %v", err) } + + if tt.expectedCost != 0 { + if remainingBudget != tt.costBudget-tt.expectedCost { + t.Errorf("expected cost to be %d, but got %d", tt.expectedCost, tt.costBudget-remainingBudget) + } + return + } if tt.expectSkipped { // Skipped validations should have no cost. The only possible false positive here would be the CEL expression 'true'. if remainingBudget != tt.costBudget {