diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index e5c851d650d..d584770ec1c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -787,8 +787,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch for property, jsonSchema := range schema.Properties { subSsv := ssv - // defensively assumes that a future map type is uncorrelatable - if schema.XMapType != nil && (*schema.XMapType != "granular" && *schema.XMapType != "atomic") { + if !cel.MapIsCorrelatable(schema.XMapType) { subSsv = subSsv.withForbidOldSelfValidations(fldPath) } @@ -968,10 +967,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch if cr.TransitionRule { if uncorrelatablePath := ssv.forbidOldSelfValidations(); uncorrelatablePath != nil { allErrs = append(allErrs, 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 { - // todo: remove when transition rule validation is implemented - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, "validation of rules containing oldSelf is not yet implemented")) - } } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index fdfb79c67b6..5ffc1f3b306 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -59,13 +59,6 @@ func immutable(path ...string) validationMatch { func forbidden(path ...string) validationMatch { return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden} } -func notImplemented(path ...string) validationMatch { - return validationMatch{ - path: field.NewPath(path[0], path[1:]...), - errorType: field.ErrorTypeInvalid, - contains: "not yet implemented", - } -} func (v validationMatch) matches(err *field.Error) bool { return err.Type == v.errorType && err.Field == v.path.String() && strings.Contains(err.Error(), v.contains) @@ -7668,9 +7661,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on list defaulting to type atomic", @@ -7692,9 +7682,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "forbid transition rule on element of list of type set", @@ -7742,9 +7729,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on element of list of type map", @@ -7772,9 +7756,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].items.x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on list of type map", @@ -7802,9 +7783,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on element of map of type granular", @@ -7827,9 +7805,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].properties[subfield].x-kubernetes-validations[0].rule"), - }, }, { name: "forbid transition rule on element of map of unrecognized type", @@ -7877,9 +7852,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].properties[subfield].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on map of type granular", @@ -7897,9 +7869,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on map defaulting to type granular", @@ -7916,9 +7885,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on element of map of type atomic", @@ -7941,9 +7907,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].properties[subfield].x-kubernetes-validations[0].rule"), - }, }, { name: "allow transition rule on map of type atomic", @@ -7961,9 +7924,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, }, }, - expectedErrors: []validationMatch{ - notImplemented("spec.validation.openAPIV3Schema.properties[value].x-kubernetes-validations[0].rule"), - }, }, } for _, tt := range tests { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go index f95a97e3203..30c65b967a8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/celcoststability_test.go @@ -1098,7 +1098,7 @@ func TestCelCostStability(t *testing.T) { t.Fatal("expected non nil validator") } ctx := context.TODO() - errs, remainingBudegt := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, RuntimeCELCostBudget) + errs, remainingBudegt := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, nil, RuntimeCELCostBudget) for _, err := range errs { t.Errorf("unexpected error: %v", err) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist.go new file mode 100644 index 00000000000..7e26ad956fc --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist.go @@ -0,0 +1,178 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cel + +import ( + "fmt" + "strings" + + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" +) + +// mapList provides a "lookup by key" operation for lists (arrays) with x-kubernetes-list-type=map. +type mapList interface { + // get returns the first element having given key, for all + // x-kubernetes-list-map-keys, to the provided object. If the provided object isn't itself a valid mapList element, + // get returns nil. + get(interface{}) interface{} +} + +type keyStrategy interface { + // CompositeKeyFor returns a composite key for the provided object, if possible, and a + // boolean that indicates whether or not a key could be generated for the provided object. + CompositeKeyFor(map[string]interface{}) (interface{}, bool) +} + +// singleKeyStrategy is a cheaper strategy for associative lists that have exactly one key. +type singleKeyStrategy struct { + key string +} + +// CompositeKeyFor directly returns the value of the single key to +// use as a composite key. +func (ks *singleKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interface{}, bool) { + v, ok := obj[ks.key] + if !ok { + return nil, false + } + + switch v.(type) { + case bool, float64, int64, string: + return v, true + default: + return nil, false // non-scalar + } +} + +// multiKeyStrategy computes a composite key of all key values. +type multiKeyStrategy struct { + sts *schema.Structural +} + +// CompositeKeyFor returns a composite key computed from the values of all +// keys. +func (ks *multiKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interface{}, bool) { + const keyDelimiter = "\x00" // 0 byte should never appear in the composite key except as delimiter + + var delimited strings.Builder + for _, key := range ks.sts.XListMapKeys { + v, ok := obj[key] + if !ok { + return nil, false + } + + switch v.(type) { + case bool: + fmt.Fprintf(&delimited, keyDelimiter+"%t", v) + case float64: + fmt.Fprintf(&delimited, keyDelimiter+"%f", v) + case int64: + fmt.Fprintf(&delimited, keyDelimiter+"%d", v) + case string: + fmt.Fprintf(&delimited, keyDelimiter+"%q", v) + default: + return nil, false // values must be scalars + } + } + return delimited.String(), true +} + +// emptyMapList is a mapList containing no elements. +type emptyMapList struct{} + +func (emptyMapList) get(interface{}) interface{} { + return nil +} + +type mapListImpl struct { + sts *schema.Structural + ks keyStrategy + // keyedItems contains all lazily keyed map items + keyedItems map[interface{}]interface{} + // unkeyedItems contains all map items that have not yet been keyed + unkeyedItems []interface{} +} + +func (a *mapListImpl) get(obj interface{}) interface{} { + mobj, ok := obj.(map[string]interface{}) + if !ok { + return nil + } + + key, ok := a.ks.CompositeKeyFor(mobj) + if !ok { + return nil + } + if match, ok := a.keyedItems[key]; ok { + return match + } + // keep keying items until we either find a match or run out of unkeyed items + for len(a.unkeyedItems) > 0 { + // dequeue an unkeyed item + item := a.unkeyedItems[0] + a.unkeyedItems = a.unkeyedItems[1:] + + // key the item + mitem, ok := item.(map[string]interface{}) + if !ok { + continue + } + itemKey, ok := a.ks.CompositeKeyFor(mitem) + if !ok { + continue + } + if _, exists := a.keyedItems[itemKey]; !exists { + a.keyedItems[itemKey] = mitem + } + + // if it matches, short-circuit + if itemKey == key { + return mitem + } + } + + return nil +} + +func makeKeyStrategy(sts *schema.Structural) keyStrategy { + if len(sts.XListMapKeys) == 1 { + key := sts.XListMapKeys[0] + return &singleKeyStrategy{ + key: key, + } + } + + return &multiKeyStrategy{ + sts: sts, + } +} + +// makeMapList returns a queryable interface over the provided x-kubernetes-list-type=map +// keyedItems. If the provided schema is _not_ an array with x-kubernetes-list-type=map, returns an +// empty mapList. +func makeMapList(sts *schema.Structural, items []interface{}) (rv mapList) { + if sts.Type != "array" || sts.XListType == nil || *sts.XListType != "map" || len(sts.XListMapKeys) == 0 || len(items) == 0 { + return emptyMapList{} + } + ks := makeKeyStrategy(sts) + return &mapListImpl{ + sts: sts, + ks: ks, + keyedItems: map[interface{}]interface{}{}, + unkeyedItems: items, + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist_test.go new file mode 100644 index 00000000000..97c2b80f76e --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist_test.go @@ -0,0 +1,334 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cel + +import ( + "reflect" + "testing" + + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" +) + +func TestMapList(t *testing.T) { + for _, tc := range []struct { + name string + sts schema.Structural + items []interface{} + warmUpQueries []interface{} + query interface{} + expected interface{} + }{ + { + name: "default list type", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + }, + query: map[string]interface{}{}, + expected: nil, + }, + { + name: "non list type", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "map", + }, + }, + query: map[string]interface{}{}, + expected: nil, + }, + { + name: "non-map list type", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeSet, + }, + }, + query: map[string]interface{}{}, + expected: nil, + }, + { + name: "no keys", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + }, + }, + query: map[string]interface{}{}, + expected: nil, + }, + { + name: "single key", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k": "a", + "v1": "a", + }, + map[string]interface{}{ + "k": "b", + "v1": "b", + }, + }, + query: map[string]interface{}{ + "k": "b", + "v1": "B", + }, + expected: map[string]interface{}{ + "k": "b", + "v1": "b", + }, + }, + { + name: "single key ignoring non-map query", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k": "a", + "v1": "a", + }, + }, + query: 42, + expected: nil, + }, + { + name: "single key ignoring unkeyable query", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k": "a", + "v1": "a", + }, + }, + query: map[string]interface{}{ + "k": map[string]interface{}{ + "keys": "must", + "be": "scalars", + }, + "v1": "A", + }, + expected: nil, + }, + { + name: "ignores item of invalid type", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k": "a", + "v1": "a", + }, + 5, + }, + query: map[string]interface{}{ + "k": "a", + "v1": "A", + }, + expected: map[string]interface{}{ + "k": "a", + "v1": "a", + }, + }, + { + name: "keep first entry when duplicated keys are encountered", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k": "a", + "v1": "a", + }, + map[string]interface{}{ + "k": "a", + "v1": "b", + }, + }, + query: map[string]interface{}{ + "k": "a", + "v1": "A", + }, + expected: map[string]interface{}{ + "k": "a", + "v1": "a", + }, + }, + { + name: "keep first entry when duplicated multi-keys are encountered", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k1", "k2"}, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "k1": "a", + "k2": "b", + "v1": "a", + }, + map[string]interface{}{ + "k1": "a", + "k2": "b", + "v1": "b", + }, + map[string]interface{}{ + "k1": "x", + "k2": "y", + "v1": "z", + }, + }, + warmUpQueries: []interface{}{ + map[string]interface{}{ + "k1": "x", + "k2": "y", + }, + }, + query: map[string]interface{}{ + "k1": "a", + "k2": "b", + }, + expected: map[string]interface{}{ + "k1": "a", + "k2": "b", + "v1": "a", + }, + }, + { + name: "multiple keys with defaults ignores item with nil value for key", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"kb", "kf", "ki", "ks"}, + }, + Properties: map[string]schema.Structural{ + "kb": { + Generic: schema.Generic{ + Default: schema.JSON{Object: true}, + }, + }, + "kf": { + Generic: schema.Generic{ + Default: schema.JSON{Object: float64(2.0)}, + }, + }, + "ki": { + Generic: schema.Generic{ + Default: schema.JSON{Object: int64(42)}, + }, + }, + "ks": { + Generic: schema.Generic{ + Default: schema.JSON{Object: "hello"}, + }, + }, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "kb": nil, + "kf": float64(2.0), + "ki": int64(42), + "ks": "hello", + "v1": "a", + }, + map[string]interface{}{ + "kb": false, + "kf": float64(2.0), + "ki": int64(42), + "ks": "hello", + "v1": "b", + }, + }, + query: map[string]interface{}{ + "kb": false, + "kf": float64(2.0), + "ki": int64(42), + "ks": "hello", + "v1": "B", + }, + expected: map[string]interface{}{ + "kb": false, + "kf": float64(2.0), + "ki": int64(42), + "ks": "hello", + "v1": "b", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + mapList := makeMapList(&tc.sts, tc.items) + for _, warmUp := range tc.warmUpQueries { + mapList.get(warmUp) + } + actual := mapList.get(tc.query) + if !reflect.DeepEqual(tc.expected, actual) { + t.Errorf("got: %v, expected %v", actual, tc.expected) + } + }) + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index cfe915aafdc..e3562b7c9ee 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "math" + "reflect" "strings" "github.com/google/cel-go/common/types" @@ -98,32 +99,45 @@ func validator(s *schema.Structural, isResourceRoot bool, perCallLimit uint64) * // If the validation rules exceed the costBudget, subsequent evaluations will be skipped, the list of errs returned will not be empty, and a negative remainingBudget will be returned. // Most callers can ignore the returned remainingBudget value unless another validate call is going to be made // context is passed for supporting context cancellation during cel validation -func (s *Validator) Validate(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) Validate(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { remainingBudget = costBudget if s == nil || obj == nil { return nil, remainingBudget } - errs, remainingBudget = s.validateExpressions(ctx, fldPath, sts, obj, remainingBudget) + errs, remainingBudget = s.validateExpressions(ctx, fldPath, sts, obj, oldObj, remainingBudget) if remainingBudget < 0 { return errs, remainingBudget } switch obj := obj.(type) { case []interface{}: + oldArray, _ := oldObj.([]interface{}) var arrayErrs field.ErrorList - arrayErrs, remainingBudget = s.validateArray(ctx, fldPath, sts, obj, remainingBudget) + arrayErrs, remainingBudget = s.validateArray(ctx, fldPath, sts, obj, oldArray, remainingBudget) errs = append(errs, arrayErrs...) return errs, remainingBudget case map[string]interface{}: + oldMap, _ := oldObj.(map[string]interface{}) var mapErrs field.ErrorList - mapErrs, remainingBudget = s.validateMap(ctx, fldPath, sts, obj, remainingBudget) + mapErrs, remainingBudget = s.validateMap(ctx, fldPath, sts, obj, oldMap, remainingBudget) errs = append(errs, mapErrs...) return errs, remainingBudget } return errs, remainingBudget } -func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { + // guard against oldObj being a non-nil interface with a nil value + if oldObj != nil { + v := reflect.ValueOf(oldObj) + switch v.Kind() { + case reflect.Map, reflect.Ptr, reflect.Interface, reflect.Slice: + if v.IsNil() { + oldObj = nil // +k8s:verify-mutation:reason=clone + } + } + } + remainingBudget = costBudget if obj == nil { // We only validate non-null values. Rules that need to check for the state of a nullable value or the presence of an optional @@ -145,7 +159,7 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path if s.isResourceRoot { sts = model.WithTypeAndObjectMeta(sts) } - activation := NewValidationActivation(obj, sts) + var activation interpreter.Activation = NewValidationActivation(obj, oldObj, sts) for i, compiled := range s.compiledRules { rule := sts.XValidations[i] if compiled.Error != nil { @@ -156,10 +170,9 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path // rule is empty continue } - if compiled.TransitionRule { + if compiled.TransitionRule && oldObj == nil { // transition rules are evaluated only if there is a comparable existing value - errs = append(errs, field.InternalError(fldPath, fmt.Errorf("oldSelf validation not implemented"))) - continue // todo: wire oldObj parameter + continue } evalResult, evalDetails, err := compiled.Program.ContextEval(ctx, activation) if evalDetails == nil { @@ -214,25 +227,37 @@ func ruleErrorString(rule apiextensions.ValidationRule) string { } type validationActivation struct { - self ref.Val + self, oldSelf ref.Val + hasOldSelf bool } -func NewValidationActivation(obj interface{}, structural *schema.Structural) *validationActivation { - return &validationActivation{self: UnstructuredToVal(obj, structural)} +func NewValidationActivation(obj, oldObj interface{}, structural *schema.Structural) *validationActivation { + va := &validationActivation{ + self: UnstructuredToVal(obj, structural), + } + if oldObj != nil { + va.oldSelf = UnstructuredToVal(oldObj, structural) // +k8s:verify-mutation:reason=clone + va.hasOldSelf = true // +k8s:verify-mutation:reason=clone + } + return va } func (a *validationActivation) ResolveName(name string) (interface{}, bool) { - if name == ScopedVarName { + switch name { + case ScopedVarName: return a.self, true + case OldScopedVarName: + return a.oldSelf, a.hasOldSelf + default: + return nil, false } - return nil, false } func (a *validationActivation) Parent() interpreter.Activation { return nil } -func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj map[string]interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj map[string]interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { remainingBudget = costBudget if remainingBudget < 0 { return errs, remainingBudget @@ -241,10 +266,17 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s return nil, remainingBudget } + correlatable := MapIsCorrelatable(sts.XMapType) + if s.AdditionalProperties != nil && sts.AdditionalProperties != nil && sts.AdditionalProperties.Structural != nil { for k, v := range obj { + var oldV interface{} + if correlatable { + oldV = oldObj[k] // +k8s:verify-mutation:reason=clone + } + var err field.ErrorList - err, remainingBudget = s.AdditionalProperties.Validate(ctx, fldPath.Key(k), sts.AdditionalProperties.Structural, v, remainingBudget) + err, remainingBudget = s.AdditionalProperties.Validate(ctx, fldPath.Key(k), sts.AdditionalProperties.Structural, v, oldV, remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget @@ -256,8 +288,13 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s stsProp, stsOk := sts.Properties[k] sub, ok := s.Properties[k] if ok && stsOk { + var oldV interface{} + if correlatable { + oldV = oldObj[k] // +k8s:verify-mutation:reason=clone + } + var err field.ErrorList - err, remainingBudget = sub.Validate(ctx, fldPath.Child(k), &stsProp, v, remainingBudget) + err, remainingBudget = sub.Validate(ctx, fldPath.Child(k), &stsProp, v, oldV, remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget @@ -269,15 +306,19 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s return errs, remainingBudget } -func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj []interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj []interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { remainingBudget = costBudget if remainingBudget < 0 { return errs, remainingBudget } + if s.Items != nil && sts.Items != nil { + // only map-type lists support self-oldSelf correlation for cel rules. if this isn't a + // map-type list, then makeMapList returns an implementation that always returns nil + correlatableOldItems := makeMapList(sts, oldObj) for i := range obj { var err field.ErrorList - err, remainingBudget = s.Items.Validate(ctx, fldPath.Index(i), sts.Items, obj[i], remainingBudget) + err, remainingBudget = s.Items.Validate(ctx, fldPath.Index(i), sts.Items, obj[i], correlatableOldItems.get(obj[i]), remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget @@ -287,3 +328,10 @@ func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts return errs, remainingBudget } + +// MapIsCorrelatable returns true if the mapType can be used to correlate the data elements of a map after an update +// with the data elements of the map from before the updated. +func MapIsCorrelatable(mapType *string) bool { + // if a third map type is introduced, assume it's not correlatable. granular is the default if unspecified. + return mapType == nil || *mapType == "granular" || *mapType == "atomic" +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 679876726f2..fd01f0b0fc8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -32,12 +32,15 @@ import ( // TestValidationExpressions tests CEL integration with custom resource values and OpenAPIv3. func TestValidationExpressions(t *testing.T) { tests := []struct { - name string - schema *schema.Structural - obj map[string]interface{} - valid []string - errors map[string]string // rule -> string that error message must contain - costBudget int64 + name string + schema *schema.Structural + obj interface{} + oldObj interface{} + valid []string + errors map[string]string // rule -> string that error message must contain + costBudget int64 + isRoot bool + expectSkipped bool }{ // tests where val1 and val2 are equal but val3 is different // equality, comparisons and type specific functions @@ -626,6 +629,7 @@ func TestValidationExpressions(t *testing.T) { }, }, {name: "typemeta and objectmeta access not specified", + isRoot: true, obj: map[string]interface{}{ "apiVersion": "v1", "kind": "Pod", @@ -1692,6 +1696,59 @@ func TestValidationExpressions(t *testing.T) { "isURL('../relative-path') == false", }, }, + {name: "transition rules", + obj: map[string]interface{}{ + "v": "new", + }, + oldObj: map[string]interface{}{ + "v": "old", + }, + schema: objectTypePtr(map[string]schema.Structural{ + "v": stringType, + }), + valid: []string{ + "oldSelf.v != self.v", + "oldSelf.v == 'old' && self.v == 'new'", + }, + }, + {name: "skipped transition rule for nil old primitive", + expectSkipped: true, + obj: "exists", + oldObj: nil, + schema: &stringType, + valid: []string{ + "oldSelf == self", + }, + }, + {name: "skipped transition rule for nil old array", + expectSkipped: true, + obj: []interface{}{}, + oldObj: nil, + schema: listTypePtr(&stringType), + valid: []string{ + "oldSelf == self", + }, + }, + {name: "skipped transition rule for nil old object", + expectSkipped: true, + obj: map[string]interface{}{"f": "exists"}, + oldObj: nil, + schema: objectTypePtr(map[string]schema.Structural{ + "f": stringType, + }), + valid: []string{ + "oldSelf.f == self.f", + }, + }, + {name: "skipped transition rule for old with non-nil interface but nil value", + expectSkipped: true, + obj: []interface{}{}, + oldObj: nilInterfaceOfStringSlice(), + schema: listTypePtr(&stringType), + valid: []string{ + "oldSelf == self", + }, + }, } for i := range tests { @@ -1706,17 +1763,24 @@ func TestValidationExpressions(t *testing.T) { t.Run(validRule, func(t *testing.T) { t.Parallel() s := withRule(*tt.schema, validRule) - celValidator := NewValidator(&s, PerCallLimit) + celValidator := validator(&s, tt.isRoot, PerCallLimit) if celValidator == nil { t.Fatal("expected non nil validator") } - errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.costBudget) + errs, remainingBudget := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.oldObj, tt.costBudget) for _, err := range errs { t.Errorf("unexpected error: %v", err) } + 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 { + t.Errorf("expected no cost expended for skipped validation, but got %d remaining from %d budget", remainingBudget, tt.costBudget) + } + return + } // test with cost budget exceeded - errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, 0) + errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.oldObj, 0) var found bool for _, err := range errs { if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Error(), "validation failed due to running out of cost budget, no further validation rules will be run") { @@ -1736,7 +1800,7 @@ func TestValidationExpressions(t *testing.T) { if celValidator == nil { t.Fatal("expected non nil validator") } - errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.costBudget) + errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.oldObj, tt.costBudget) for _, err := range errs { if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Error(), "no further validation rules will be run due to call cost exceeds limit for rule") { found = true @@ -1755,7 +1819,7 @@ func TestValidationExpressions(t *testing.T) { if celValidator == nil { t.Fatal("expected non nil validator") } - errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.costBudget) + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.oldObj, tt.costBudget) if len(errs) == 0 { t.Error("expected validation errors but got none") } @@ -1766,7 +1830,7 @@ func TestValidationExpressions(t *testing.T) { } // test with cost budget exceeded - errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, 0) + errs, _ = celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, tt.oldObj, 0) var found bool for _, err := range errs { if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Error(), "validation failed due to running out of cost budget, no further validation rules will be run") { @@ -1815,7 +1879,7 @@ func TestCELValidationContextCancellation(t *testing.T) { if celValidator == nil { t.Fatal("expected non nil validator") } - errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, RuntimeCELCostBudget) + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, nil, RuntimeCELCostBudget) for _, err := range errs { t.Errorf("unexpected error: %v", err) } @@ -1824,7 +1888,7 @@ func TestCELValidationContextCancellation(t *testing.T) { found := false evalCtx, cancel := context.WithTimeout(ctx, time.Microsecond) cancel() - errs, _ = celValidator.Validate(evalCtx, field.NewPath("root"), &s, tt.obj, RuntimeCELCostBudget) + errs, _ = celValidator.Validate(evalCtx, field.NewPath("root"), &s, tt.obj, nil, RuntimeCELCostBudget) for _, err := range errs { if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Error(), "operation interrupted") { found = true @@ -1869,7 +1933,7 @@ func BenchmarkCELValidationWithContext(b *testing.B) { b.Fatal("expected non nil validator") } for i := 0; i < b.N; i++ { - errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, RuntimeCELCostBudget) + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &s, tt.obj, nil, RuntimeCELCostBudget) for _, err := range errs { b.Fatalf("validation failed: %v", err) } @@ -1911,7 +1975,7 @@ func BenchmarkCELValidationWithCancelledContext(b *testing.B) { for i := 0; i < b.N; i++ { evalCtx, cancel := context.WithTimeout(ctx, time.Microsecond) cancel() - errs, _ := celValidator.Validate(evalCtx, field.NewPath("root"), &s, tt.obj, RuntimeCELCostBudget) + errs, _ := celValidator.Validate(evalCtx, field.NewPath("root"), &s, tt.obj, nil, RuntimeCELCostBudget) //found := false //for _, err := range errs { // if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Error(), "operation interrupted") { @@ -2091,3 +2155,8 @@ func withNullablePtr(nullable bool, s schema.Structural) *schema.Structural { s.Generic.Nullable = nullable return &s } + +func nilInterfaceOfStringSlice() []interface{} { + var slice []interface{} = nil + return slice +} 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 431708ee14c..40e1218c2d6 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 @@ -21,6 +21,9 @@ import ( "fmt" "reflect" + "k8s.io/kube-openapi/pkg/validation/strfmt" + kubeopenapivalidate "k8s.io/kube-openapi/pkg/validation/validate" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" schemaobjectmeta "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta" @@ -29,8 +32,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/kube-openapi/pkg/validation/strfmt" - kubeopenapivalidate "k8s.io/kube-openapi/pkg/validation/validate" ) // ValidateDefaults checks that default values validate and are properly pruned. @@ -89,7 +90,7 @@ func validate(ctx context.Context, pth *field.Path, s *structuralschema.Structur } else if errs := apiservervalidation.ValidateCustomResource(pth.Child("default"), s.Default.Object, validator); len(errs) > 0 { allErrs = append(allErrs, errs...) } else if celValidator := cel.NewValidator(s, cel.PerCallLimit); celValidator != nil { - celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, remainingCost) + celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) remainingCost = rmCost allErrs = append(allErrs, celErrs...) if remainingCost < 0 { @@ -114,7 +115,7 @@ func validate(ctx context.Context, pth *field.Path, s *structuralschema.Structur } else if errs := apiservervalidation.ValidateCustomResource(pth.Child("default"), s.Default.Object, validator); len(errs) > 0 { allErrs = append(allErrs, errs...) } else if celValidator := cel.NewValidator(s, cel.PerCallLimit); celValidator != nil { - celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, remainingCost) + celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) remainingCost = rmCost allErrs = append(allErrs, celErrs...) if remainingCost < 0 { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go index eadddc1113c..b27bd251c30 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go @@ -17,6 +17,7 @@ limitations under the License. package validation import ( + "context" "math/rand" "os" "strconv" @@ -27,16 +28,19 @@ import ( kjson "sigs.k8s.io/json" + kubeopenapispec "k8s.io/kube-openapi/pkg/validation/spec" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" "k8s.io/apimachinery/pkg/api/apitesting/fuzzer" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" - kubeopenapispec "k8s.io/kube-openapi/pkg/validation/spec" ) // TestRoundTrip checks the conversion to go-openapi types. @@ -145,6 +149,7 @@ func stripIntOrStringType(x interface{}) interface{} { type failingObject struct { object interface{} + oldObject interface{} expectErrs []string } @@ -153,6 +158,7 @@ func TestValidateCustomResource(t *testing.T) { name string schema apiextensions.JSONSchemaProps objects []interface{} + oldObjects []interface{} failingObjects []failingObject }{ {name: "!nullable", @@ -416,6 +422,119 @@ func TestValidateCustomResource(t *testing.T) { }}, }, }, + {name: "immutability transition rule", + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "string", + XValidations: []apiextensions.ValidationRule{ + { + Rule: "self == oldSelf", + }, + }, + }, + }, + }, + objects: []interface{}{ + map[string]interface{}{"field": "x"}, + }, + oldObjects: []interface{}{ + map[string]interface{}{"field": "x"}, + }, + failingObjects: []failingObject{ + { + object: map[string]interface{}{"field": "y"}, + oldObject: map[string]interface{}{"field": "x"}, + expectErrs: []string{ + `field: Invalid value: "string": failed rule: self == oldSelf`, + }}, + }, + }, + {name: "correlatable transition rule", + // Ensures a transition rule under a "listMap" is supported. + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "array", + XListType: &listMapType, + XListMapKeys: []string{"k1", "k2"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "k1": { + Type: "string", + }, + "k2": { + Type: "string", + }, + "v1": { + Type: "number", + XValidations: []apiextensions.ValidationRule{ + { + Rule: "self >= oldSelf", + }, + }, + }, + }, + }, + }, + }, + }, + }, + objects: []interface{}{ + map[string]interface{}{"field": []interface{}{map[string]interface{}{"k1": "a", "k2": "b", "v1": 1.2}}}, + }, + oldObjects: []interface{}{ + map[string]interface{}{"field": []interface{}{map[string]interface{}{"k1": "a", "k2": "b", "v1": 1.0}}}, + }, + failingObjects: []failingObject{ + { + object: map[string]interface{}{"field": []interface{}{map[string]interface{}{"k1": "a", "k2": "b", "v1": 0.9}}}, + oldObject: map[string]interface{}{"field": []interface{}{map[string]interface{}{"k1": "a", "k2": "b", "v1": 1.0}}}, + expectErrs: []string{ + `field[0].v1: Invalid value: "number": failed rule: self >= oldSelf`, + }}, + }, + }, + {name: "validation rule under non-correlatable field", + // The array makes the rule on the nested string non-correlatable + // for transition rule purposes. This test ensures that a rule that + // does NOT use oldSelf (is not a transition rule), still behaves + // as expected under a non-correlatable field. + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "field": { + Type: "array", + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "x": { + Type: "string", + XValidations: []apiextensions.ValidationRule{ + { + Rule: "self == 'x'", + }, + }, + }, + }, + }, + }, + }, + }, + }, + objects: []interface{}{ + map[string]interface{}{"field": []interface{}{map[string]interface{}{"x": "x"}}}, + }, + failingObjects: []failingObject{ + { + object: map[string]interface{}{"field": []interface{}{map[string]interface{}{"x": "y"}}}, + expectErrs: []string{ + `field[0].x: Invalid value: "string": failed rule: self == 'x'`, + }}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -423,13 +542,29 @@ func TestValidateCustomResource(t *testing.T) { if err != nil { t.Fatal(err) } - for _, obj := range tt.objects { + structural, err := structuralschema.NewStructural(&tt.schema) + if err != nil { + t.Fatal(err) + } + celValidator := cel.NewValidator(structural, cel.PerCallLimit) + for i, obj := range tt.objects { + var oldObject interface{} + if len(tt.oldObjects) == len(tt.objects) { + oldObject = tt.oldObjects[i] + } if errs := ValidateCustomResource(nil, obj, validator); len(errs) > 0 { t.Errorf("unexpected validation error for %v: %v", obj, errs) } + errs, _ := celValidator.Validate(context.TODO(), nil, structural, obj, oldObject, cel.RuntimeCELCostBudget) + if len(errs) > 0 { + t.Errorf(errs.ToAggregate().Error()) + } } for i, failingObject := range tt.failingObjects { - if errs := ValidateCustomResource(nil, failingObject.object, validator); len(errs) == 0 { + errs := ValidateCustomResource(nil, failingObject.object, validator) + celErrs, _ := celValidator.Validate(context.TODO(), nil, structural, failingObject.object, failingObject.oldObject, cel.RuntimeCELCostBudget) + errs = append(errs, celErrs...) + if len(errs) == 0 { t.Errorf("missing error for %v", failingObject.object) } else { sawErrors := sets.NewString() @@ -505,3 +640,5 @@ func TestItemsProperty(t *testing.T) { }) } } + +var listMapType = "map" diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go index d30c2ecd222..8d7b677d748 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go @@ -19,11 +19,12 @@ package customresource import ( "context" + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) type statusStrategy struct { @@ -85,15 +86,21 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj var errs field.ErrorList errs = append(errs, a.customResourceStrategy.validator.ValidateStatusUpdate(ctx, obj, old, a.scale)...) - // validate embedded resources - if u, ok := obj.(*unstructured.Unstructured); ok { - v := obj.GetObjectKind().GroupVersionKind().Version + uNew, ok := obj.(*unstructured.Unstructured) + if !ok { + return errs + } + uOld, ok := old.(*unstructured.Unstructured) + if !ok { + uOld = nil // as a safety precaution, continue with validation if uOld self cannot be cast + } - // validate x-kubernetes-validations rules - if celValidator, ok := a.customResourceStrategy.celValidators[v]; ok { - err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchemas[v], u.Object, cel.RuntimeCELCostBudget) - errs = append(errs, err...) - } + v := obj.GetObjectKind().GroupVersionKind().Version + + // validate x-kubernetes-validations rules + if celValidator, ok := a.customResourceStrategy.celValidators[v]; ok { + err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchemas[v], uNew.Object, uOld.Object, cel.RuntimeCELCostBudget) + errs = append(errs, err...) } return errs } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go index c59a6934fcc..af7a4b904df 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go @@ -174,7 +174,7 @@ func (a customResourceStrategy) Validate(ctx context.Context, obj runtime.Object // validate x-kubernetes-validations rules if celValidator, ok := a.celValidators[v]; ok { - err, _ := celValidator.Validate(ctx, nil, a.structuralSchemas[v], u.Object, cel.RuntimeCELCostBudget) + err, _ := celValidator.Validate(ctx, nil, a.structuralSchemas[v], u.Object, nil, cel.RuntimeCELCostBudget) errs = append(errs, err...) } } @@ -227,7 +227,7 @@ func (a customResourceStrategy) ValidateUpdate(ctx context.Context, obj, old run // validate x-kubernetes-validations rules if celValidator, ok := a.celValidators[v]; ok { - err, _ := celValidator.Validate(ctx, nil, a.structuralSchemas[v], uNew.Object, cel.RuntimeCELCostBudget) + err, _ := celValidator.Validate(ctx, nil, a.structuralSchemas[v], uNew.Object, uOld.Object, cel.RuntimeCELCostBudget) errs = append(errs, err...) } diff --git a/test/integration/apiserver/crd_validation_expressions_test.go b/test/integration/apiserver/crd_validation_expressions_test.go index 0de11b6a5b7..97f862aae1e 100644 --- a/test/integration/apiserver/crd_validation_expressions_test.go +++ b/test/integration/apiserver/crd_validation_expressions_test.go @@ -35,6 +35,7 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" featuregatetesting "k8s.io/component-base/featuregate/testing" + apiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" "k8s.io/kubernetes/test/integration/framework" ) @@ -300,6 +301,123 @@ func TestCustomResourceValidators(t *testing.T) { t.Error("Unexpected error creating custom resource but metadata validation rule") } }) + t.Run("Schema with valid transition rule", func(t *testing.T) { + structuralWithValidators := crdWithSchema(t, "ValidTransitionRule", structuralSchemaWithValidTransitionRule) + crd, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) + if err != nil { + t.Fatal(err) + } + gvr := schema.GroupVersionResource{ + Group: crd.Spec.Group, + Version: crd.Spec.Versions[0].Name, + Resource: crd.Spec.Names.Plural, + } + crClient := dynamicClient.Resource(gvr) + + t.Run("custom resource update MUST pass if a x-kubernetes-validations rule contains a valid transition rule", func(t *testing.T) { + name1 := names.SimpleNameGenerator.GenerateName("cr-1") + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": gvr.Group + "/" + gvr.Version, + "kind": crd.Spec.Names.Kind, + "metadata": map[string]interface{}{ + "name": name1, + }, + "spec": map[string]interface{}{ + "someImmutableThing": "original", + "somethingElse": "original", + }, + }} + cr, err = crClient.Create(context.TODO(), cr, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error creating custom resource: %v", err) + } + cr.Object["spec"].(map[string]interface{})["somethingElse"] = "new value" + _, err = crClient.Update(context.TODO(), cr, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("Unexpected error updating custom resource: %v", err) + } + }) + t.Run("custom resource update MUST fail if a x-kubernetes-validations rule contains an invalid transition rule", func(t *testing.T) { + name1 := names.SimpleNameGenerator.GenerateName("cr-1") + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": gvr.Group + "/" + gvr.Version, + "kind": crd.Spec.Names.Kind, + "metadata": map[string]interface{}{ + "name": name1, + }, + "spec": map[string]interface{}{ + "someImmutableThing": "original", + "somethingElse": "original", + }, + }} + cr, err = crClient.Create(context.TODO(), cr, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error creating custom resource: %v", err) + } + cr.Object["spec"].(map[string]interface{})["someImmutableThing"] = "new value" + _, err = crClient.Update(context.TODO(), cr, metav1.UpdateOptions{}) + if err == nil { + t.Fatalf("Expected error updating custom resource: %v", err) + } else if !strings.Contains(err.Error(), "failed rule: self.someImmutableThing == oldSelf.someImmutableThing") { + t.Errorf("Expected error to contain %s but got %v", "failed rule: self.someImmutableThing == oldSelf.someImmutableThing", err.Error()) + } + }) + }) + + t.Run("CRD creation MUST fail if a x-kubernetes-validations rule contains invalid transition rule", func(t *testing.T) { + structuralWithValidators := crdWithSchema(t, "InvalidTransitionRule", structuralSchemaWithInvalidTransitionRule) + _, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) + if err == nil { + t.Error("Expected error creating custom resource but got none") + } else if !strings.Contains(err.Error(), "oldSelf cannot be used on the uncorrelatable portion of the schema") { + t.Errorf("Expected error to contain %s but got %v", "oldSelf cannot be used on the uncorrelatable portion of the schema", err.Error()) + } + }) + t.Run("Schema with default map key transition rule", func(t *testing.T) { + structuralWithValidators := crdWithSchema(t, "DefaultMapKeyTransitionRule", structuralSchemaWithDefaultMapKeyTransitionRule) + crd, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) + if err != nil { + t.Fatal(err) + } + gvr := schema.GroupVersionResource{ + Group: crd.Spec.Group, + Version: crd.Spec.Versions[0].Name, + Resource: crd.Spec.Names.Plural, + } + crClient := dynamicClient.Resource(gvr) + + t.Run("custom resource update MUST fail if a x-kubernetes-validations if a transition rule contained in a mapList with default map keys fails validation", func(t *testing.T) { + name1 := names.SimpleNameGenerator.GenerateName("cr-1") + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": gvr.Group + "/" + gvr.Version, + "kind": crd.Spec.Names.Kind, + "metadata": map[string]interface{}{ + "name": name1, + }, + "spec": map[string]interface{}{ + "list": []interface{}{ + map[string]interface{}{ + "k1": "x", + "v": "value", + }, + }, + }, + }} + cr, err = crClient.Create(context.TODO(), cr, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error creating custom resource: %v", err) + } + item := cr.Object["spec"].(map[string]interface{})["list"].([]interface{})[0].(map[string]interface{}) + item["k2"] = "DEFAULT" + item["v"] = "new value" + _, err = crClient.Update(context.TODO(), cr, metav1.UpdateOptions{}) + if err == nil { + t.Fatalf("Expected error updating custom resource: %v", err) + } else if !strings.Contains(err.Error(), "failed rule: self.v == oldSelf.v") { + t.Errorf("Expected error to contain %s but got %v", "failed rule: self.v == oldSelf.v", err.Error()) + } + }) + }) } func nonStructuralCrdWithValidations() *apiextensionsv1beta1.CustomResourceDefinition { @@ -495,3 +613,100 @@ var structuralSchemaWithInvalidMetadataValidators = []byte(` } } }`) + +var structuralSchemaWithValidTransitionRule = []byte(` +{ + "openAPIV3Schema": { + "description": "CRD with CEL validators", + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "someImmutableThing": { "type": "string" }, + "somethingElse": { "type": "string" } + }, + "x-kubernetes-validations": [ + { + "rule": "self.someImmutableThing == oldSelf.someImmutableThing" + } + ] + }, + "status": { + "type": "object", + "properties": {} + } + } + } +}`) + +var structuralSchemaWithInvalidTransitionRule = []byte(` +{ + "openAPIV3Schema": { + "description": "CRD with CEL validators", + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "list": { + "type": "array", + "items": { + "type": "string", + "x-kubernetes-validations": [ + { + "rule": "self == oldSelf" + } + ] + } + } + } + }, + "status": { + "type": "object", + "properties": {} + } + } + } +}`) + +var structuralSchemaWithDefaultMapKeyTransitionRule = []byte(` +{ + "openAPIV3Schema": { + "description": "CRD with CEL validators", + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "list": { + "type": "array", + "x-kubernetes-list-map-keys": [ + "k1", + "k2" + ], + "x-kubernetes-list-type": "map", + "items": { + "type": "object", + "properties": { + "k1": { "type": "string" }, + "k2": { "type": "string", "default": "DEFAULT" }, + "v": { "type": "string" } + }, + "required": ["k1"], + "x-kubernetes-validations": [ + { + "rule": "self.v == oldSelf.v" + } + ] + } + } + } + }, + "status": { + "type": "object", + "properties": {} + } + } + } +}`)