From f71c4d4cf46580e6bb34497cc787995da797c892 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 16 Mar 2022 00:10:08 -0400 Subject: [PATCH] Add validation rule tests for transition rules --- .../apiextensions/validation/validation.go | 3 +- .../schema/cel/celcoststability_test.go | 2 +- .../pkg/apiserver/schema/cel/maplist.go | 131 ++++++------- .../pkg/apiserver/schema/cel/maplist_test.go | 178 ++++-------------- .../pkg/apiserver/schema/cel/validation.go | 61 ++++-- .../apiserver/schema/cel/validation_test.go | 71 ++++++- .../apiserver/schema/defaulting/validation.go | 4 +- .../apiserver/validation/validation_test.go | 143 +++++++++++++- .../customresource/status_strategy.go | 5 +- .../crd_validation_expressions_test.go | 91 ++++++++- 10 files changed, 437 insertions(+), 252 deletions(-) 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 9b5a0499449..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) } 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 index a60cf48f67a..7e26ad956fc 100644 --- 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 @@ -18,16 +18,16 @@ package cel import ( "fmt" - "hash/maphash" + "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 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 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{} } @@ -39,16 +39,15 @@ type keyStrategy interface { // singleKeyStrategy is a cheaper strategy for associative lists that have exactly one key. type singleKeyStrategy struct { - key string - defawlt interface{} // default is a keyword + key string } -// CompositeKeyFor directly returns the value of the single key (or its default value, if absent) to +// 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 { - v = ks.defawlt // substitute default value + return nil, false } switch v.(type) { @@ -59,38 +58,37 @@ func (ks *singleKeyStrategy) CompositeKeyFor(obj map[string]interface{}) (interf } } -// hashKeyStrategy computes a hash of all key values. -type hashKeyStrategy struct { - sts *schema.Structural - hasher maphash.Hash +// multiKeyStrategy computes a composite key of all key values. +type multiKeyStrategy struct { + sts *schema.Structural } -// CompositeKeyFor returns a hash computed from the values (or default values, if absent) of all +// CompositeKeyFor returns a composite key computed from the values 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 +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 - ks.hasher.Reset() + var delimited strings.Builder for _, key := range ks.sts.XListMapKeys { v, ok := obj[key] if !ok { - v = ks.sts.Properties[key].Default.Object + return nil, false } switch v.(type) { case bool: - fmt.Fprintf(&ks.hasher, keyDelimiter+"%t", v) + fmt.Fprintf(&delimited, keyDelimiter+"%t", v) case float64: - fmt.Fprintf(&ks.hasher, keyDelimiter+"%f", v) + fmt.Fprintf(&delimited, keyDelimiter+"%f", v) case int64: - fmt.Fprintf(&ks.hasher, keyDelimiter+"%d", v) + fmt.Fprintf(&delimited, keyDelimiter+"%d", v) case string: - fmt.Fprintf(&ks.hasher, keyDelimiter+"%q", v) + fmt.Fprintf(&delimited, keyDelimiter+"%q", v) default: return nil, false // values must be scalars } } - return ks.hasher.Sum64(), true + return delimited.String(), true } // emptyMapList is a mapList containing no elements. @@ -101,9 +99,12 @@ func (emptyMapList) get(interface{}) interface{} { } type mapListImpl struct { - sts *schema.Structural - ks keyStrategy - elements map[interface{}][]interface{} // composite key -> bucket + 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{} { @@ -116,76 +117,62 @@ func (a *mapListImpl) get(obj interface{}) interface{} { 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:] - // 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 { + // key the item + mitem, ok := item.(map[string]interface{}) + if !ok { continue } - - if match != nil { - // Duplicate key set / more than one element matches. This condition should - // have generated a validation error elsewhere. - return nil + 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 } - match = element } - return match // can be nil + + return 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, + key: key, } } - return &hashKeyStrategy{ + return &multiKeyStrategy{ 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 +// keyedItems. 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) { +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{} } - - 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) - } - } - + ks := makeKeyStrategy(sts) return &mapListImpl{ - sts: sts, - ks: ks, - elements: elements, + 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 index 50240bb628a..97c2b80f76e 100644 --- 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 @@ -25,12 +25,12 @@ import ( func TestMapList(t *testing.T) { for _, tc := range []struct { - name string - sts schema.Structural - keyStrategy keyStrategy - items []interface{} - query interface{} - expected interface{} + name string + sts schema.Structural + items []interface{} + warmUpQueries []interface{} + query interface{} + expected interface{} }{ { name: "default list type", @@ -108,102 +108,6 @@ func TestMapList(t *testing.T) { "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{ @@ -278,7 +182,7 @@ func TestMapList(t *testing.T) { }, }, { - name: "ignores items with duplicated key", + name: "keep first entry when duplicated keys are encountered", sts: schema.Structural{ Generic: schema.Generic{ Type: "array", @@ -302,50 +206,52 @@ func TestMapList(t *testing.T) { "k": "a", "v1": "A", }, - expected: nil, + expected: map[string]interface{}{ + "k": "a", + "v1": "a", + }, }, { - name: "multiple keys with defaults missing from query", + 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{"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"}, - }, - }, + 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{}{ - "v1": "A", + "k1": "a", + "k2": "b", }, expected: map[string]interface{}{ + "k1": "a", + "k2": "b", "v1": "a", }, }, @@ -415,20 +321,14 @@ func TestMapList(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - ks := tc.keyStrategy - if ks == nil { - ks = makeKeyStrategy(&tc.sts) + mapList := makeMapList(&tc.sts, tc.items) + for _, warmUp := range tc.warmUpQueries { + mapList.get(warmUp) } - actual := makeMapList(&tc.sts, ks, tc.items).get(tc.query) + actual := mapList.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 da74006c6fb..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" @@ -126,6 +127,17 @@ func (s *Validator) Validate(ctx context.Context, fldPath *field.Path, sts *sche } 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 @@ -147,10 +159,7 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path if s.isResourceRoot { sts = model.WithTypeAndObjectMeta(sts) } - var activation interpreter.Activation = NewValidationActivation(ScopedVarName, obj, sts) - if oldObj != nil { - activation = interpreter.NewHierarchicalActivation(activation, NewValidationActivation(OldScopedVarName, oldObj, sts)) - } + var activation interpreter.Activation = NewValidationActivation(obj, oldObj, sts) for i, compiled := range s.compiledRules { rule := sts.XValidations[i] if compiled.Error != nil { @@ -218,23 +227,30 @@ func ruleErrorString(rule apiextensions.ValidationRule) string { } type validationActivation struct { - name string - val ref.Val + self, oldSelf ref.Val + hasOldSelf bool } -func NewValidationActivation(name string, obj interface{}, structural *schema.Structural) *validationActivation { +func NewValidationActivation(obj, oldObj interface{}, structural *schema.Structural) *validationActivation { va := &validationActivation{ - name: name, - val: UnstructuredToVal(obj, structural), + 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 == a.name { - return a.val, true + 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 { @@ -250,14 +266,13 @@ 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" + 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] + oldV = oldObj[k] // +k8s:verify-mutation:reason=clone } var err field.ErrorList @@ -275,7 +290,7 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s if ok && stsOk { var oldV interface{} if correlatable { - oldV = oldObj[k] + oldV = oldObj[k] // +k8s:verify-mutation:reason=clone } var err field.ErrorList @@ -297,11 +312,10 @@ func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts 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 { + // 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], correlatableOldItems.get(obj[i]), remainingBudget) @@ -314,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 a3a1bf9a94d..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,13 +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{} - oldObj 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 @@ -627,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", @@ -1708,6 +1711,44 @@ func TestValidationExpressions(t *testing.T) { "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 { @@ -1722,14 +1763,21 @@ 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.oldObj, 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, tt.oldObj, 0) @@ -2107,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 62e3770493d..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 @@ -90,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, nil, 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 { @@ -115,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, nil, 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 e4c891b5cd5..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 { @@ -91,7 +92,7 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj } uOld, ok := old.(*unstructured.Unstructured) if !ok { - return errs + uOld = nil // as a safety precaution, continue with validation if uOld self cannot be cast } v := obj.GetObjectKind().GroupVersionKind().Version diff --git a/test/integration/apiserver/crd_validation_expressions_test.go b/test/integration/apiserver/crd_validation_expressions_test.go index 5c7388e6eae..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" ) @@ -301,7 +302,7 @@ func TestCustomResourceValidators(t *testing.T) { } }) t.Run("Schema with valid transition rule", func(t *testing.T) { - structuralWithValidators := crdWithSchema(t, "Structural", structuralSchemaWithValidTransitionRule) + structuralWithValidators := crdWithSchema(t, "ValidTransitionRule", structuralSchemaWithValidTransitionRule) crd, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) if err != nil { t.Fatal(err) @@ -364,7 +365,7 @@ func TestCustomResourceValidators(t *testing.T) { }) 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) + structuralWithValidators := crdWithSchema(t, "InvalidTransitionRule", structuralSchemaWithInvalidTransitionRule) _, err := fixtures.CreateNewV1CustomResourceDefinition(structuralWithValidators, apiExtensionClient, dynamicClient) if err == nil { t.Error("Expected error creating custom resource but got none") @@ -372,6 +373,51 @@ func TestCustomResourceValidators(t *testing.T) { 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 { @@ -623,3 +669,44 @@ var structuralSchemaWithInvalidTransitionRule = []byte(` } } }`) + +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": {} + } + } + } +}`)