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.
  • Loading branch information
cici37 committed May 10, 2024
1 parent f97ac22 commit d6e4115
Show file tree
Hide file tree
Showing 34 changed files with 1,199 additions and 126 deletions.
43 changes: 34 additions & 9 deletions pkg/apis/admissionregistration/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import (
"k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions"
"k8s.io/apiserver/pkg/cel"
"k8s.io/apiserver/pkg/cel/environment"
"k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/apiserver/pkg/util/webhook"
"k8s.io/client-go/util/jsonpath"

Expand Down Expand Up @@ -221,6 +223,7 @@ func ValidateValidatingWebhookConfiguration(e *admissionregistration.ValidatingW
requireRecognizedAdmissionReviewVersion: true,
requireUniqueWebhookNames: true,
allowInvalidLabelValueInSelector: false,
strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForWebhooks),
})
}

Expand Down Expand Up @@ -250,6 +253,7 @@ func ValidateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebho
requireRecognizedAdmissionReviewVersion: true,
requireUniqueWebhookNames: true,
allowInvalidLabelValueInSelector: false,
strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForWebhooks),
})
}

Expand All @@ -261,6 +265,7 @@ type validationOptions struct {
requireUniqueWebhookNames bool
allowInvalidLabelValueInSelector bool
preexistingExpressions preexistingExpressions
strictCostEnforcement bool
}

type preexistingExpressions struct {
Expand Down Expand Up @@ -687,6 +692,7 @@ func ValidateValidatingWebhookConfigurationUpdate(newC, oldC *admissionregistrat
requireUniqueWebhookNames: validatingHasUniqueWebhookNames(oldC.Webhooks),
allowInvalidLabelValueInSelector: validatingWebhookHasInvalidLabelValueInSelector(oldC.Webhooks),
preexistingExpressions: findValidatingPreexistingExpressions(oldC),
strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForWebhooks),
})
}

Expand All @@ -700,6 +706,7 @@ func ValidateMutatingWebhookConfigurationUpdate(newC, oldC *admissionregistratio
requireUniqueWebhookNames: mutatingHasUniqueWebhookNames(oldC.Webhooks),
allowInvalidLabelValueInSelector: mutatingWebhookHasInvalidLabelValueInSelector(oldC.Webhooks),
preexistingExpressions: findMutatingPreexistingExpressions(oldC),
strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForWebhooks),
})
}

Expand All @@ -713,7 +720,7 @@ const (

// ValidateValidatingAdmissionPolicy validates a ValidatingAdmissionPolicy before creation.
func ValidateValidatingAdmissionPolicy(p *admissionregistration.ValidatingAdmissionPolicy) field.ErrorList {
return validateValidatingAdmissionPolicy(p, validationOptions{ignoreMatchConditions: false})
return validateValidatingAdmissionPolicy(p, validationOptions{ignoreMatchConditions: false, strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForVAP)})
}

func validateValidatingAdmissionPolicy(p *admissionregistration.ValidatingAdmissionPolicy, opts validationOptions) field.ErrorList {
Expand All @@ -728,7 +735,7 @@ func validateValidatingAdmissionPolicySpec(meta metav1.ObjectMeta, spec *admissi
getCompiler := func() plugincel.Compiler {
if compiler == nil {
needsComposition := len(spec.Variables) > 0
compiler = createCompiler(needsComposition)
compiler = createCompiler(needsComposition, opts.strictCostEnforcement)
}
return compiler
}
Expand Down Expand Up @@ -973,6 +980,7 @@ func validateVariable(compiler plugincel.Compiler, v *admissionregistration.Vari
result := compiler.CompileAndStoreVariable(variable, plugincel.OptionalVariableDeclarations{
HasParams: paramKind != nil,
HasAuthorizer: true,
StrictCost: opts.strictCostEnforcement,
}, envType)
if result.Error != nil {
allErrors = append(allErrors, convertCELErrorToValidationError(fldPath.Child("expression"), variable, result.Error))
Expand Down Expand Up @@ -1047,6 +1055,7 @@ func validateValidationExpression(compiler plugincel.Compiler, expression string
}, plugincel.OptionalVariableDeclarations{
HasParams: hasParams,
HasAuthorizer: true,
StrictCost: opts.strictCostEnforcement,
}, envType, fldPath)
}

Expand All @@ -1055,11 +1064,18 @@ func validateMatchConditionsExpression(expression string, opts validationOptions
if opts.preexistingExpressions.matchConditionExpressions.Has(expression) {
envType = environment.StoredExpressions
}
return validateCELCondition(statelessCELCompiler, &matchconditions.MatchCondition{
var compiler plugincel.Compiler
if opts.strictCostEnforcement {
compiler = strictStatelessCELCompiler
} else {
compiler = nonStrictStatelessCELCompiler
}
return validateCELCondition(compiler, &matchconditions.MatchCondition{
Expression: expression,
}, plugincel.OptionalVariableDeclarations{
HasParams: opts.allowParamsInMatchConditions,
HasAuthorizer: true,
StrictCost: opts.strictCostEnforcement,
}, envType, fldPath)
}

Expand All @@ -1073,6 +1089,7 @@ func validateMessageExpression(compiler plugincel.Compiler, expression string, o
}, plugincel.OptionalVariableDeclarations{
HasParams: opts.allowParamsInMatchConditions,
HasAuthorizer: false,
StrictCost: opts.strictCostEnforcement,
}, envType, fldPath)
}

Expand All @@ -1097,7 +1114,7 @@ func validateAuditAnnotation(compiler plugincel.Compiler, meta metav1.ObjectMeta
}
result := compiler.CompileCELExpression(&validatingadmissionpolicy.AuditAnnotationCondition{
ValueExpression: trimmedValueExpression,
}, plugincel.OptionalVariableDeclarations{HasParams: paramKind != nil, HasAuthorizer: true}, envType)
}, plugincel.OptionalVariableDeclarations{HasParams: paramKind != nil, HasAuthorizer: true, StrictCost: opts.strictCostEnforcement}, envType)
if result.Error != nil {
switch result.Error.Type {
case cel.ErrorTypeRequired:
Expand Down Expand Up @@ -1191,6 +1208,7 @@ func ValidateValidatingAdmissionPolicyUpdate(newC, oldC *admissionregistration.V
return validateValidatingAdmissionPolicy(newC, validationOptions{
ignoreMatchConditions: ignoreValidatingAdmissionPolicyMatchConditions(newC, oldC),
preexistingExpressions: findValidatingPolicyPreexistingExpressions(oldC),
strictCostEnforcement: utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForVAP),
})
}

Expand Down Expand Up @@ -1250,17 +1268,24 @@ func validateFieldRef(fieldRef string, fldPath *field.Path) field.ErrorList {

// statelessCELCompiler does not support variable composition (and thus is stateless). It should be used when
// variable composition is not allowed, for example, when validating MatchConditions.
var statelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
// strictStatelessCELCompiler is a cel Compiler that enforces strict cost enforcement.
// nonStrictStatelessCELCompiler is a cel Compiler that does not enforce strict cost enforcement.
var strictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true))
var nonStrictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), false))

func createCompiler(allowComposition bool) plugincel.Compiler {
func createCompiler(allowComposition, strictCost bool) plugincel.Compiler {
if !allowComposition {
return statelessCELCompiler
if strictCost {
return strictStatelessCELCompiler
} else {
return nonStrictStatelessCELCompiler
}
}
compiler, err := plugincel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
compiler, err := plugincel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), strictCost))
if err != nil {
// should never happen, but cannot panic either.
utilruntime.HandleError(err)
return plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
return plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), strictCost))
}
return compiler
}
Expand Down
20 changes: 15 additions & 5 deletions pkg/apis/admissionregistration/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
plugincel "k8s.io/apiserver/pkg/admission/plugin/cel"
"k8s.io/apiserver/pkg/cel/environment"
"k8s.io/apiserver/pkg/cel/library"
"k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/apis/admissionregistration"
)

Expand Down Expand Up @@ -3412,19 +3414,27 @@ func TestValidateValidatingAdmissionPolicyUpdate(t *testing.T) {
},
// TODO: CustomAuditAnnotations: string valueExpression with {oldObject} is allowed
}
strictCost := utilfeature.DefaultFeatureGate.Enabled(features.StrictCostEnforcementForVAP)
// Include the test library, which includes the test() function in the storage environment during test
base := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())
base := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), strictCost)
extended, err := base.Extend(environment.VersionedOptions{
IntroducedVersion: version.MustParseGeneric("1.999"),
EnvOptions: []cel.EnvOption{library.Test()},
})
if err != nil {
t.Fatal(err)
}
statelessCELCompiler = plugincel.NewCompiler(extended)
defer func() {
statelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
}()
if strictCost {
strictStatelessCELCompiler = plugincel.NewCompiler(extended)
defer func() {
strictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), strictCost))
}()
} else {
nonStrictStatelessCELCompiler = plugincel.NewCompiler(extended)
defer func() {
nonStrictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), strictCost))
}()
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/features/kube_features.go
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,10 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

genericfeatures.StorageVersionHash: {Default: true, PreRelease: featuregate.Beta},

genericfeatures.StrictCostEnforcementForVAP: {Default: false, PreRelease: featuregate.Beta},

genericfeatures.StrictCostEnforcementForWebhooks: {Default: false, PreRelease: featuregate.Beta},

genericfeatures.StructuredAuthenticationConfiguration: {Default: true, PreRelease: featuregate.Beta},

genericfeatures.StructuredAuthorizationConfiguration: {Default: true, PreRelease: featuregate.Beta},
Expand Down
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
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
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
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
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 d6e4115

Please sign in to comment.