From 0889c5738ed8a9bc2bdd7a6149b324f5c73391cd Mon Sep 17 00:00:00 2001 From: Alexander Zielenski <zielenski@google.com> Date: Tue, 24 Oct 2023 11:40:59 -0700 Subject: [PATCH] ratcheting-cel: add optionalOldSelf field Kubernetes-commit: 5edb27aa3820f15a235e7f4a5807eb20538cae94 --- pkg/apis/apiextensions/types_jsonschema.go | 31 ++ pkg/apis/apiextensions/v1/types_jsonschema.go | 31 ++ .../apiextensions/v1beta1/types_jsonschema.go | 31 ++ .../apiextensions/validation/validation.go | 2 + .../validation/validation_test.go | 152 +++++ .../customresourcedefinition/strategy.go | 59 ++ .../customresourcedefinition/strategy_test.go | 523 ++++++++++++++++++ 7 files changed, 829 insertions(+) diff --git a/pkg/apis/apiextensions/types_jsonschema.go b/pkg/apis/apiextensions/types_jsonschema.go index cc1c7437f..8c4e147f0 100644 --- a/pkg/apis/apiextensions/types_jsonschema.go +++ b/pkg/apis/apiextensions/types_jsonschema.go @@ -210,6 +210,19 @@ type ValidationRule struct { // - 'map': `X + Y` performs a merge where the array positions of all keys in `X` are preserved but the values // are overwritten by values in `Y` when the key sets of `X` and `Y` intersect. Elements in `Y` with // non-intersecting keys are appended, retaining their partial order. + // + // If `rule` makes use of the `oldSelf` variable it is implicitly a + // `transition rule`. + // + // By default, the `oldSelf` variable is the same type as `self`. + // When `optionalOldSelf` is true, the `oldSelf` variable is a CEL optional + // variable whose value() is the same type as `self`. + // See the documentation for the `optionalOldSelf` field for details. + // + // Transition rules by default are applied only on UPDATE requests and are + // skipped if an old value could not be found. You can opt a transition + // rule into unconditional evaluation by setting `optionalOldSelf` to true. + // Rule string // Message represents the message displayed when validation fails. The message is required if the Rule contains // line breaks. The message must not contain line breaks. @@ -246,6 +259,24 @@ type ValidationRule struct { // e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` // +optional FieldPath string + + // optionalOldSelf is used to opt a transition rule into evaluation + // even when the object is first created, or if the old object is + // missing the value. + // + // When enabled `oldSelf` will be a CEL optional whose value will be + // `None` if there is no old value, or when the object is initially created. + // + // You may check for presence of oldSelf using `oldSelf.hasValue()` and + // unwrap it after checking using `oldSelf.value()`. Check the CEL + // documentation for Optional types for more information: + // https://pkg.go.dev/github.com/google/cel-go/cel#OptionalTypes + // + // May not be set unless `oldSelf` is used in `rule`. + // + // +featureGate=CRDValidationRatcheting + // +optional + OptionalOldSelf *bool } // JSON represents any valid JSON value. diff --git a/pkg/apis/apiextensions/v1/types_jsonschema.go b/pkg/apis/apiextensions/v1/types_jsonschema.go index 1c90d464a..a81451ad6 100644 --- a/pkg/apis/apiextensions/v1/types_jsonschema.go +++ b/pkg/apis/apiextensions/v1/types_jsonschema.go @@ -249,6 +249,19 @@ type ValidationRule struct { // - 'map': `X + Y` performs a merge where the array positions of all keys in `X` are preserved but the values // are overwritten by values in `Y` when the key sets of `X` and `Y` intersect. Elements in `Y` with // non-intersecting keys are appended, retaining their partial order. + // + // If `rule` makes use of the `oldSelf` variable it is implicitly a + // `transition rule`. + // + // By default, the `oldSelf` variable is the same type as `self`. + // When `optionalOldSelf` is true, the `oldSelf` variable is a CEL optional + // variable whose value() is the same type as `self`. + // See the documentation for the `optionalOldSelf` field for details. + // + // Transition rules by default are applied only on UPDATE requests and are + // skipped if an old value could not be found. You can opt a transition + // rule into unconditional evaluation by setting `optionalOldSelf` to true. + // Rule string `json:"rule" protobuf:"bytes,1,opt,name=rule"` // Message represents the message displayed when validation fails. The message is required if the Rule contains // line breaks. The message must not contain line breaks. @@ -285,6 +298,24 @@ type ValidationRule struct { // e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` // +optional FieldPath string `json:"fieldPath,omitempty" protobuf:"bytes,5,opt,name=fieldPath"` + + // optionalOldSelf is used to opt a transition rule into evaluation + // even when the object is first created, or if the old object is + // missing the value. + // + // When enabled `oldSelf` will be a CEL optional whose value will be + // `None` if there is no old value, or when the object is initially created. + // + // You may check for presence of oldSelf using `oldSelf.hasValue()` and + // unwrap it after checking using `oldSelf.value()`. Check the CEL + // documentation for Optional types for more information: + // https://pkg.go.dev/github.com/google/cel-go/cel#OptionalTypes + // + // May not be set unless `oldSelf` is used in `rule`. + // + // +featureGate=CRDValidationRatcheting + // +optional + OptionalOldSelf *bool `json:"optionalOldSelf,omitempty" protobuf:"bytes,6,opt,name=optionalOldSelf"` } // JSON represents any valid JSON value. diff --git a/pkg/apis/apiextensions/v1beta1/types_jsonschema.go b/pkg/apis/apiextensions/v1beta1/types_jsonschema.go index 59a093227..24c45bb04 100644 --- a/pkg/apis/apiextensions/v1beta1/types_jsonschema.go +++ b/pkg/apis/apiextensions/v1beta1/types_jsonschema.go @@ -249,6 +249,19 @@ type ValidationRule struct { // - 'map': `X + Y` performs a merge where the array positions of all keys in `X` are preserved but the values // are overwritten by values in `Y` when the key sets of `X` and `Y` intersect. Elements in `Y` with // non-intersecting keys are appended, retaining their partial order. + // + // If `rule` makes use of the `oldSelf` variable it is implicitly a + // `transition rule`. + // + // By default, the `oldSelf` variable is the same type as `self`. + // When `optionalOldSelf` is true, the `oldSelf` variable is a CEL optional + // variable whose value() is the same type as `self`. + // See the documentation for the `optionalOldSelf` field for details. + // + // Transition rules by default are applied only on UPDATE requests and are + // skipped if an old value could not be found. You can opt a transition + // rule into unconditional evaluation by setting `optionalOldSelf` to true. + // Rule string `json:"rule" protobuf:"bytes,1,opt,name=rule"` // Message represents the message displayed when validation fails. The message is required if the Rule contains // line breaks. The message must not contain line breaks. @@ -285,6 +298,24 @@ type ValidationRule struct { // e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` // +optional FieldPath string `json:"fieldPath,omitempty" protobuf:"bytes,5,opt,name=fieldPath"` + + // optionalOldSelf is used to opt a transition rule into evaluation + // even when the object is first created, or if the old object is + // missing the value. + // + // When enabled `oldSelf` will be a CEL optional whose value will be + // `None` if there is no old value, or when the object is initially created. + // + // You may check for presence of oldSelf using `oldSelf.hasValue()` and + // unwrap it after checking using `oldSelf.value()`. Check the CEL + // documentation for Optional types for more information: + // https://pkg.go.dev/github.com/google/cel-go/cel#OptionalTypes + // + // May not be set unless `oldSelf` is used in `rule`. + // + // +featureGate=CRDValidationRatcheting + // +optional + OptionalOldSelf *bool `json:"optionalOldSelf,omitempty" protobuf:"bytes,6,opt,name=optionalOldSelf"` } // JSON represents any valid JSON value. diff --git a/pkg/apis/apiextensions/validation/validation.go b/pkg/apis/apiextensions/validation/validation.go index 3df476c03..69ac8d96d 100644 --- a/pkg/apis/apiextensions/validation/validation.go +++ b/pkg/apis/apiextensions/validation/validation.go @@ -1186,6 +1186,8 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch if uncorrelatablePath := ssv.forbidOldSelfValidations(); uncorrelatablePath != nil { allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, fmt.Sprintf("oldSelf cannot be used on the uncorrelatable portion of the schema within %v", uncorrelatablePath))) } + } else if schema.XValidations[i].OptionalOldSelf != nil { + allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("optionalOldSelf"), *schema.XValidations[i].OptionalOldSelf, "may not be set if oldSelf is not used in rule")) } } } diff --git a/pkg/apis/apiextensions/validation/validation_test.go b/pkg/apis/apiextensions/validation/validation_test.go index 4b7363b60..43f10f99f 100644 --- a/pkg/apis/apiextensions/validation/validation_test.go +++ b/pkg/apis/apiextensions/validation/validation_test.go @@ -41,6 +41,7 @@ import ( "k8s.io/apiserver/pkg/cel/environment" "k8s.io/apiserver/pkg/cel/library" "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) type validationMatch struct { @@ -9650,6 +9651,157 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { required("spec.validation.openAPIV3Schema.properties[f].x-kubernetes-validations[0].messageExpression"), }, }, + { + name: "forbid transition rule on element of list of type atomic when optionalOldSelf is set", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "array", + XListType: strPtr("atomic"), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == oldSelf.orValue("")`, OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].items.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "forbid transition rule on element of list defaulting to type atomic when optionalOldSelf is set", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "array", + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == oldSelf.orValue("")`, OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].items.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "forbid transition rule on element of list of type set when optionalOldSelf is set", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "array", + MaxItems: int64ptr(10), + XListType: strPtr("set"), + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == oldSelf.orValue("")`, OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].items.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "forbid transition rule on element of map of unrecognized type when optionalOldSelf is set", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "object", + XMapType: strPtr("future"), + Properties: map[string]apiextensions.JSONSchemaProps{ + "subfield": { + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == oldSelf.orValue("")`, OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].properties[subfield].x-kubernetes-validations[0].rule"), + unsupported("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-map-type"), + }, + }, + { + name: "forbid setting optionalOldSelf to true if oldSelf is not used", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == "foo"`, OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].optionalOldSelf"), + }, + }, + { + name: "forbid setting optionalOldSelf to false if oldSelf is not used", + opts: validationOptions{requireStructuralSchema: true}, + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "value": { + Type: "string", + MaxLength: int64ptr(10), + XValidations: apiextensions.ValidationRules{ + {Rule: `self == "foo"`, OptionalOldSelf: ptr.To(false)}, + }, + }, + }, + }, + }, + expectedErrors: []validationMatch{ + invalid("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].optionalOldSelf"), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/registry/customresourcedefinition/strategy.go b/pkg/registry/customresourcedefinition/strategy.go index 1144d4225..2d2767f76 100644 --- a/pkg/registry/customresourcedefinition/strategy.go +++ b/pkg/registry/customresourcedefinition/strategy.go @@ -24,6 +24,7 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -33,6 +34,7 @@ import ( "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" ) // strategy implements behavior for CustomResources. @@ -223,3 +225,60 @@ func MatchCustomResourceDefinition(label labels.Selector, field fields.Selector) func CustomResourceDefinitionToSelectableFields(obj *apiextensions.CustomResourceDefinition) fields.Set { return generic.ObjectMetaFieldsSet(&obj.ObjectMeta, true) } + +// dropDisabledFields drops disabled fields that are not used if their associated feature gates +// are not enabled. +func dropDisabledFields(newCRD *apiextensions.CustomResourceDefinition, oldCRD *apiextensions.CustomResourceDefinition) { + if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) && (oldCRD == nil || (oldCRD != nil && !specHasOptionalOldSelf(&oldCRD.Spec))) { + if newCRD.Spec.Validation != nil { + dropOptionalOldSelfField(newCRD.Spec.Validation.OpenAPIV3Schema) + } + + for _, v := range newCRD.Spec.Versions { + if v.Schema != nil { + dropOptionalOldSelfField(v.Schema.OpenAPIV3Schema) + } + } + } +} + +// dropOptionalOldSelfField drops field optionalOldSelf from CRD schema +func dropOptionalOldSelfField(schema *apiextensions.JSONSchemaProps) { + if schema == nil { + return + } + for i := range schema.XValidations { + schema.XValidations[i].OptionalOldSelf = nil + } + + if schema.AdditionalProperties != nil { + dropOptionalOldSelfField(schema.AdditionalProperties.Schema) + } + for def, jsonSchema := range schema.Properties { + dropOptionalOldSelfField(&jsonSchema) + schema.Properties[def] = jsonSchema + } + if schema.Items != nil { + dropOptionalOldSelfField(schema.Items.Schema) + for i, jsonSchema := range schema.Items.JSONSchemas { + dropOptionalOldSelfField(&jsonSchema) + schema.Items.JSONSchemas[i] = jsonSchema + } + } +} + +func specHasOptionalOldSelf(spec *apiextensions.CustomResourceDefinitionSpec) bool { + return validation.HasSchemaWith(spec, schemaHasOptionalOldSelf) +} + +func schemaHasOptionalOldSelf(s *apiextensions.JSONSchemaProps) bool { + return validation.SchemaHas(s, func(s *apiextensions.JSONSchemaProps) bool { + for _, v := range s.XValidations { + if v.OptionalOldSelf != nil { + return true + } + + } + return false + }) +} diff --git a/pkg/registry/customresourcedefinition/strategy_test.go b/pkg/registry/customresourcedefinition/strategy_test.go index 54362768c..2aa22f238 100644 --- a/pkg/registry/customresourcedefinition/strategy_test.go +++ b/pkg/registry/customresourcedefinition/strategy_test.go @@ -20,12 +20,17 @@ import ( "context" "testing" + "github.com/google/go-cmp/cmp" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" + 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" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) func strPtr(in string) *string { @@ -189,3 +194,521 @@ func TestValidateAPIApproval(t *testing.T) { }) } } + +// TestDropDisabledFields tests if the drop functionality is working fine or not with feature gate switch +func TestDropDisabledFields(t *testing.T) { + testCases := []struct { + name string + enableRatcheting bool + crd *apiextensions.CustomResourceDefinition + oldCRD *apiextensions.CustomResourceDefinition + expectedCRD *apiextensions.CustomResourceDefinition + }{ + { + name: "Ratcheting, For creation, FG disabled, no OptionalOldSelf, no field drop", + enableRatcheting: false, + crd: &apiextensions.CustomResourceDefinition{}, + oldCRD: nil, + expectedCRD: &apiextensions.CustomResourceDefinition{}, + }, + { + name: "Ratcheting, For creation, FG disabled, set OptionalOldSelf, drop OptionalOldSelf", + enableRatcheting: false, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + oldCRD: nil, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For creation, FG enabled, set OptionalOldSelf, update with OptionalOldSelf", + enableRatcheting: true, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + oldCRD: nil, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For update, FG disabled, oldCRD OptionalOldSelf in use, don't drop OptionalOldSelfs", + enableRatcheting: false, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + oldCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "otherRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "self.isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + }, + }, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For update, FG disabled, oldCRD OptionalOldSelf in use, but different from new, don't drop OptionalOldSelfs", + enableRatcheting: false, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + oldCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + }, + }, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "subRule": { + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "isTest == true", + Message: "isTest should be true.", + OptionalOldSelf: ptr.To(true), + }, + }, + Properties: map[string]apiextensions.JSONSchemaProps{ + "isTest": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For update, FG disabled, oldCRD has no OptionalOldSelf, drop OptionalOldSelf", + enableRatcheting: false, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + oldCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For update, FG enabled, oldCRD has optionalOldSelf, updated to newCRD", + enableRatcheting: true, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + oldCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "old data", + Message: "old data", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + }, + { + name: "Ratcheting, For update, FG enabled, oldCRD has no OptionalOldSelf, updated to newCRD", + enableRatcheting: true, + crd: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + oldCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + expectedCRD: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "foos.sigs.k8s.io", Annotations: map[string]string{v1beta1.KubeAPIApprovedAnnotation: "valid"}, ResourceVersion: "1"}, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Validation: &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + XValidations: apiextensions.ValidationRules{ + { + Rule: "size(self) > 0", + Message: "openAPIV3Schema should contain more than 0 element.", + OptionalOldSelf: ptr.To(true), + }, + }, + }, + }, + }, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, tc.enableRatcheting)() + old := tc.oldCRD.DeepCopy() + + dropDisabledFields(tc.crd, tc.oldCRD) + + // old crd should never be changed + if diff := cmp.Diff(tc.oldCRD, old); diff != "" { + t.Fatalf("old crd changed from %v to %v\n%v", tc.oldCRD, old, diff) + } + + if diff := cmp.Diff(tc.expectedCRD, tc.crd); diff != "" { + t.Fatalf("unexpected crd: %v\n%v", tc.crd, diff) + } + }) + } +}