Skip to content

Commit

Permalink
ratcheting-cel: add optionalOldSelf field
Browse files Browse the repository at this point in the history 
Kubernetes-commit: 5edb27aa3820f15a235e7f4a5807eb20538cae94
  • Loading branch information
@k8s-publishing-bot
Alexander Zielenski authored and k8s-publishing-bot committed Oct 24, 2023
1 parent 2c1c704 commit 0889c57
Show file tree
Hide file tree
Showing 7 changed files with 829 additions and 0 deletions.
31 changes: 31 additions & 0 deletions pkg/apis/apiextensions/types_jsonschema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
31 changes: 31 additions & 0 deletions pkg/apis/apiextensions/v1/types_jsonschema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
31 changes: 31 additions & 0 deletions pkg/apis/apiextensions/v1beta1/types_jsonschema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/apiextensions/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
}
}
Expand Down
152 changes: 152 additions & 0 deletions pkg/apis/apiextensions/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
59 changes: 59 additions & 0 deletions pkg/registry/customresourcedefinition/strategy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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
})
}
Loading

0 comments on commit 0889c57

Please sign in to comment.