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..9b5a0499449 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 @@ -968,10 +968,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/maplist.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist.go new file mode 100644 index 00000000000..a60cf48f67a --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist.go @@ -0,0 +1,191 @@ +/* +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" + "hash/maphash" + + "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 unique element having identical values, for all + // x-kubernetes-list-map-keys, to the provided object. If no such unique element exists, or + // 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 + defawlt interface{} // default is a keyword +} + +// CompositeKeyFor directly returns the value of the single key (or its default value, if absent) to +// use as a composite key. +func (ks *singleKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interface{}, bool) { + v, ok := obj[ks.key] + if !ok { + v = ks.defawlt // substitute default value + } + + switch v.(type) { + case bool, float64, int64, string: + return v, true + default: + return nil, false // non-scalar + } +} + +// hashKeyStrategy computes a hash of all key values. +type hashKeyStrategy struct { + sts *schema.Structural + hasher maphash.Hash +} + +// CompositeKeyFor returns a hash computed from the values (or default values, if absent) of all +// keys. +func (ks *hashKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interface{}, bool) { + const keyDelimiter = "\x00" // 0 byte should never appear in the hash input except as delimiter + + ks.hasher.Reset() + for _, key := range ks.sts.XListMapKeys { + v, ok := obj[key] + if !ok { + v = ks.sts.Properties[key].Default.Object + } + + switch v.(type) { + case bool: + fmt.Fprintf(&ks.hasher, keyDelimiter+"%t", v) + case float64: + fmt.Fprintf(&ks.hasher, keyDelimiter+"%f", v) + case int64: + fmt.Fprintf(&ks.hasher, keyDelimiter+"%d", v) + case string: + fmt.Fprintf(&ks.hasher, keyDelimiter+"%q", v) + default: + return nil, false // values must be scalars + } + } + return ks.hasher.Sum64(), 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 + elements map[interface{}][]interface{} // composite key -> bucket +} + +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 + } + + // Scan bucket to handle key collisions and duplicate key sets: + var match interface{} + for _, element := range a.elements[key] { + all := true + for _, key := range a.sts.XListMapKeys { + va, ok := element.(map[string]interface{})[key] + if !ok { + va = a.sts.Properties[key].Default.Object + } + + vb, ok := mobj[key] + if !ok { + vb = a.sts.Properties[key].Default.Object + } + + all = all && (va == vb) + } + + if !all { + continue + } + + if match != nil { + // Duplicate key set / more than one element matches. This condition should + // have generated a validation error elsewhere. + return nil + } + match = element + } + return match // can be nil +} + +func makeKeyStrategy(sts *schema.Structural) keyStrategy { + if len(sts.XListMapKeys) == 1 { + key := sts.XListMapKeys[0] + return &singleKeyStrategy{ + key: key, + defawlt: sts.Properties[key].Default.Object, + } + } + + return &hashKeyStrategy{ + sts: sts, + } +} + +// makeMapList returns a queryable interface over the provided x-kubernetes-list-type=map +// elements. If the provided schema is _not_ an array with x-kubernetes-list-type=map, returns an +// empty mapList. +func makeMapList(sts *schema.Structural, ks keyStrategy, items []interface{}) (rv mapList) { + if sts.Type != "array" || sts.XListType == nil || *sts.XListType != "map" || len(sts.XListMapKeys) == 0 || len(items) == 0 { + return emptyMapList{} + } + + elements := make(map[interface{}][]interface{}, len(items)) + + for _, item := range items { + mitem, ok := item.(map[string]interface{}) + if !ok { + continue + } + if key, ok := ks.CompositeKeyFor(mitem); ok { + elements[key] = append(elements[key], mitem) + } + } + + return &mapListImpl{ + sts: sts, + ks: ks, + elements: elements, + } +} 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..50240bb628a --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/maplist_test.go @@ -0,0 +1,434 @@ +/* +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 + keyStrategy keyStrategy + items []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 with faked composite key collision", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + }, + keyStrategy: collisionfulKeyStrategy{}, + 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 with default", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + Properties: map[string]schema.Structural{ + "k": { + Generic: schema.Generic{ + Default: schema.JSON{Object: "a"}, + }, + }, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "v1": "a", + }, + map[string]interface{}{ + "k": "b", + "v1": "b", + }, + }, + query: map[string]interface{}{ + "k": "a", + "v1": "A", + }, + expected: map[string]interface{}{ + "v1": "a", + }, + }, + { + name: "single key with defaulted key missing from query", + sts: schema.Structural{ + Generic: schema.Generic{ + Type: "array", + }, + Extensions: schema.Extensions{ + XListType: &listTypeMap, + XListMapKeys: []string{"k"}, + }, + Properties: map[string]schema.Structural{ + "k": { + Generic: schema.Generic{ + Default: schema.JSON{Object: "a"}, + }, + }, + }, + }, + items: []interface{}{ + map[string]interface{}{ + "v1": "a", + }, + }, + query: map[string]interface{}{ + "v1": "A", + }, + expected: map[string]interface{}{ + "v1": "a", + }, + }, + { + 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: "ignores items with duplicated 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": "a", + "v1": "b", + }, + }, + query: map[string]interface{}{ + "k": "a", + "v1": "A", + }, + expected: nil, + }, + { + name: "multiple keys with defaults missing from query", + 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{}{ + "v1": "a", + }, + }, + query: map[string]interface{}{ + "v1": "A", + }, + expected: map[string]interface{}{ + "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) { + ks := tc.keyStrategy + if ks == nil { + ks = makeKeyStrategy(&tc.sts) + } + actual := makeMapList(&tc.sts, ks, tc.items).get(tc.query) + if !reflect.DeepEqual(tc.expected, actual) { + t.Errorf("got: %v, expected %v", actual, tc.expected) + } + }) + } +} + +type collisionfulKeyStrategy struct{} + +func (collisionfulKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interface{}, bool) { + return 7, true +} 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..da74006c6fb 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 @@ -98,32 +98,34 @@ 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) { 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 +147,10 @@ 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(ScopedVarName, obj, sts) + if oldObj != nil { + activation = interpreter.NewHierarchicalActivation(activation, NewValidationActivation(OldScopedVarName, oldObj, sts)) + } for i, compiled := range s.compiledRules { rule := sts.XValidations[i] if compiled.Error != nil { @@ -156,10 +161,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,16 +218,21 @@ func ruleErrorString(rule apiextensions.ValidationRule) string { } type validationActivation struct { - self ref.Val + name string + val ref.Val } -func NewValidationActivation(obj interface{}, structural *schema.Structural) *validationActivation { - return &validationActivation{self: UnstructuredToVal(obj, structural)} +func NewValidationActivation(name string, obj interface{}, structural *schema.Structural) *validationActivation { + va := &validationActivation{ + name: name, + val: UnstructuredToVal(obj, structural), + } + return va } func (a *validationActivation) ResolveName(name string) (interface{}, bool) { - if name == ScopedVarName { - return a.self, true + if name == a.name { + return a.val, true } return nil, false } @@ -232,7 +241,7 @@ 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 +250,18 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s return nil, remainingBudget } + // if a third map type is introduced, assume it's not correlatable. granular is the default if unspecified. + correlatable := sts.XMapType == nil || *sts.XMapType == "granular" || *sts.XMapType == "atomic" + if s.AdditionalProperties != nil && sts.AdditionalProperties != nil && sts.AdditionalProperties.Structural != nil { for k, v := range obj { + var oldV interface{} + if correlatable { + oldV = oldObj[k] + } + 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 +273,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] + } + 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 +291,20 @@ 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 } + + // 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, makeKeyStrategy(sts), oldObj) + if s.Items != nil && sts.Items != nil { 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 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..a3a1bf9a94d 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 @@ -35,6 +35,7 @@ func TestValidationExpressions(t *testing.T) { name string schema *schema.Structural obj map[string]interface{} + oldObj map[string]interface{} valid []string errors map[string]string // rule -> string that error message must contain costBudget int64 @@ -1692,6 +1693,21 @@ 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'", + }, + }, } for i := range tests { @@ -1710,13 +1726,13 @@ 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 { t.Errorf("unexpected error: %v", err) } // 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 +1752,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 +1771,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 +1782,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 +1831,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 +1840,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 +1885,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 +1927,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") { 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..62e3770493d 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, nil, 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, nil, remainingCost) remainingCost = rmCost allErrs = append(allErrs, celErrs...) if remainingCost < 0 { 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..e4c891b5cd5 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 @@ -85,15 +85,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 { + return errs + } - // 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..5c7388e6eae 100644 --- a/test/integration/apiserver/crd_validation_expressions_test.go +++ b/test/integration/apiserver/crd_validation_expressions_test.go @@ -300,6 +300,78 @@ 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, "Structural", 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, "InvalidStructuralMetadata", 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()) + } + }) } func nonStructuralCrdWithValidations() *apiextensionsv1beta1.CustomResourceDefinition { @@ -495,3 +567,59 @@ 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": {} + } + } + } +}`)