validate defaults across an update from nil to ensure create ratcheting rules work

Kubernetes-commit: bba0c9a81e9b598a67005d343832e52bb5fbfcc2

Author: Alexander Zielenski

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
 				}

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)
+				}
+			}
+
+		})
+	}
+}