Skip to content

Commit

Permalink
Adding the feature gates to fix cost for VAP and webhook matchConditi…
Browse files Browse the repository at this point in the history
…ons.

Kubernetes-commit: 3d896724760a957e8059ff80e9f399248eacac66
  • Loading branch information
cici37 authored and k8s-publishing-bot committed May 1, 2024
1 parent cb47ad4 commit 5e9c693
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 10 deletions.
6 changes: 4 additions & 2 deletions pkg/apis/apiextensions/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/apiextensions/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()},
Expand Down Expand Up @@ -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"))

Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions pkg/apiserver/schema/cel/compilation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{})},
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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()
Expand Down
3 changes: 2 additions & 1 deletion pkg/apiserver/schema/cel/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
44 changes: 43 additions & 1 deletion pkg/apiserver/schema/cel/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -2006,14 +2007,48 @@ 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 {
i := i
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]
Expand All @@ -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 {
Expand Down

0 comments on commit 5e9c693

Please sign in to comment.