diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go index 02945b14f4c..e529d5a3eb6 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go @@ -26,10 +26,12 @@ import ( schemaobjectmeta "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning" apiservervalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" celconfig "k8s.io/apiserver/pkg/apis/cel" + utilfeature "k8s.io/apiserver/pkg/util/feature" ) // ValidateDefaults checks that default values validate and are properly pruned. @@ -91,8 +93,27 @@ func validate(ctx context.Context, pth *field.Path, s *structuralschema.Structur allErrs = append(allErrs, errs...) } else if celValidator := cel.NewValidator(s, isResourceRoot, celconfig.PerCallLimit); celValidator != nil { celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) - remainingCost = rmCost allErrs = append(allErrs, celErrs...) + + if len(celErrs) == 0 && utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { + // If ratcheting is enabled some CEL rules may use optionalOldSelf + // For such rules the above validation is not sufficient for + // determining if the default value is a valid value to introduce + // via create or uncorrelated update. + // + // Validate an update from nil to the default value to ensure + // that the default value pass + celErrs, rmCostWithoutOldObject := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, nil, remainingCost) + allErrs = append(allErrs, celErrs...) + + // capture the cost of both types of runs and take whichever + // leaves less remaining cost + if rmCostWithoutOldObject < rmCost { + rmCost = rmCostWithoutOldObject + } + } + + remainingCost = rmCost if remainingCost < 0 { return allErrs, nil, remainingCost } @@ -116,8 +137,27 @@ func validate(ctx context.Context, pth *field.Path, s *structuralschema.Structur allErrs = append(allErrs, errs...) } else if celValidator := cel.NewValidator(s, isResourceRoot, celconfig.PerCallLimit); celValidator != nil { celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) - remainingCost = rmCost allErrs = append(allErrs, celErrs...) + + if len(celErrs) == 0 && utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { + // If ratcheting is enabled some CEL rules may use optionalOldSelf + // For such rules the above validation is not sufficient for + // determining if the default value is a valid value to introduce + // via create or uncorrelated update. + // + // Validate an update from nil to the default value to ensure + // that the default value pass + celErrs, rmCostWithoutOldObject := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, nil, remainingCost) + allErrs = append(allErrs, celErrs...) + + // capture the cost of both types of runs and take whichever + // leaves less remaining cost + if rmCostWithoutOldObject < rmCost { + rmCost = rmCostWithoutOldObject + } + } + + remainingCost = rmCost if remainingCost < 0 { return allErrs, nil, remainingCost } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation_test.go index b3cb4d9cb19..c5a14e87986 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation_test.go @@ -23,8 +23,13 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/component-base/featuregate" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/ptr" ) func jsonPtr(x interface{}) *apiextensions.JSON { @@ -34,8 +39,9 @@ func jsonPtr(x interface{}) *apiextensions.JSON { func TestDefaultValidationWithCostBudget(t *testing.T) { tests := []struct { - name string - input apiextensions.CustomResourceValidation + name string + input apiextensions.CustomResourceValidation + features []featuregate.Feature }{ { name: "default cel validation", @@ -98,6 +104,10 @@ func TestDefaultValidationWithCostBudget(t *testing.T) { for _, tt := range tests { ctx := context.TODO() t.Run(tt.name, func(t *testing.T) { + for _, f := range tt.features { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, true)() + } + schema := tt.input.OpenAPIV3Schema ss, err := structuralschema.NewStructural(schema) if err != nil { @@ -148,3 +158,103 @@ func TestDefaultValidationWithCostBudget(t *testing.T) { }) } } + +func TestDefaultValidationWithOptionalOldSelf(t *testing.T) { + tests := []struct { + name string + input apiextensions.CustomResourceValidation + errors []string + }{ + { + name: "invalid default", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "defaultFailsRatcheting": { + Type: "string", + Default: jsonPtr("default"), + XValidations: apiextensions.ValidationRules{ + { + Rule: "oldSelf.hasValue()", + OptionalOldSelf: ptr.To(true), + Message: "foobarErrorMessage", + }, + }, + }, + }, + }, + }, + errors: []string{"foobarErrorMessage"}, + }, + { + name: "valid default", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "defaultFailsRatcheting": { + Type: "string", + Default: jsonPtr("default"), + XValidations: apiextensions.ValidationRules{ + { + Rule: "oldSelf.orValue(self) == self", + OptionalOldSelf: ptr.To(true), + Message: "foobarErrorMessage", + }, + }, + }, + }, + }, + }, + errors: []string{}, + }, + } + + for _, tt := range tests { + ctx := context.TODO() + t.Run(tt.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)() + schema := tt.input.OpenAPIV3Schema + ss, err := structuralschema.NewStructural(schema) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + f := NewRootObjectFunc().WithTypeMeta(metav1.TypeMeta{APIVersion: "validation/v1", Kind: "Validation"}) + + // cost budget is large enough to pass all validation rules + allErrs, err, _ := validate(ctx, field.NewPath("test"), ss, ss, f, false, false, 10) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + for _, err := range allErrs { + found := false + for _, expected := range tt.errors { + if strings.Contains(err.Error(), expected) { + found = true + break + } + } + if !found { + t.Errorf("unexpected error: %v", err) + } + } + + for _, expected := range tt.errors { + found := false + for _, err := range allErrs { + if strings.Contains(err.Error(), expected) { + found = true + break + } + } + if !found { + t.Errorf("expected error: %v", expected) + } + } + + }) + } +}