From 4fb5f1a61191f3eef73557bbf7da292d567eee69 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 09:48:28 -0700 Subject: [PATCH 01/19] refactor: remove unnecessary lambda (noop) --- .../pkg/apiserver/validation/ratcheting.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 6565d83eee5..ad5679e158f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -121,12 +121,8 @@ func newRatchetingValueValidator(newValue, oldValue interface{}, args schemaArgs // that injects a ratchetingValueValidator to be used for all subkeys and subindices func (r *ratchetingValueValidator) getValidateOption() validate.Option { return func(svo *validate.SchemaValidatorOptions) { - svo.NewValidatorForField = func(field string, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, opts ...validate.Option) validate.ValueValidator { - return r.SubPropertyValidator(field, schema, rootSchema, root, formats, opts...) - } - svo.NewValidatorForIndex = func(index int, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, opts ...validate.Option) validate.ValueValidator { - return r.SubIndexValidator(index, schema, rootSchema, root, formats, opts...) - } + svo.NewValidatorForField = r.SubPropertyValidator + svo.NewValidatorForIndex = r.SubIndexValidator } } From 30cf9ed56774687c1f1c01de1b5694449df629a0 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:00:31 -0700 Subject: [PATCH 02/19] refactor: directly implement ValueValidator inlineValidator unnecessary, since we already have access to the `new` object --- .../pkg/apiserver/validation/ratcheting.go | 76 ++++++++----------- 1 file changed, 32 insertions(+), 44 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index ad5679e158f..29be8b70532 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -60,7 +60,7 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result { } func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { - return newRatchetingValueValidator(new, old, r.schemaArgs).Validate() + return newRatchetingValueValidator(new, old, r.schemaArgs).Validate(new) } // ratchetingValueValidator represents an invocation of SchemaValidator.ValidateUpdate @@ -145,7 +145,7 @@ func (r *ratchetingValueValidator) getValidateOption() validate.Option { // // This call has a side-effect of populating it's `children` variable with // the explored nodes of the object tree. -func (r *ratchetingValueValidator) Validate() *validate.Result { +func (r *ratchetingValueValidator) Validate(new interface{}) *validate.Result { opts := append([]validate.Option{ r.getValidateOption(), }, r.options...) @@ -177,29 +177,27 @@ func (r *ratchetingValueValidator) Validate() *validate.Result { // If the old value cannot be correlated, then default validation is used. func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, options ...validate.Option) validate.ValueValidator { // Find correlated old value - asMap, ok := r.oldValue.(map[string]interface{}) - if !ok { + oldAsMap, okOld := r.oldValue.(map[string]interface{}) + newAsMap, okNew := r.value.(map[string]interface{}) + if !okOld || !okNew { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - oldValueForField, ok := asMap[field] - if !ok { + oldValueForField, okOld := oldAsMap[field] + newValueForField, okNew := newAsMap[field] + if !okOld || !okNew { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - return inlineValidator(func(new interface{}) *validate.Result { - childNode := newRatchetingValueValidator(new, oldValueForField, schemaArgs{ - schema: schema, - root: rootSchema, - path: root, - knownFormats: formats, - options: options, - }) - - r.children[field] = childNode - return childNode.Validate() + childNode := newRatchetingValueValidator(newValueForField, oldValueForField, schemaArgs{ + schema: schema, + root: rootSchema, + path: root, + knownFormats: formats, + options: options, }) - + r.children[field] = childNode + return childNode } // SubIndexValidator overrides the standard validator constructor for sub-indicies by @@ -216,18 +214,21 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - return inlineValidator(func(new interface{}) *validate.Result { - childNode := newRatchetingValueValidator(new, oldValueForIndex, schemaArgs{ - schema: schema, - root: rootSchema, - path: root, - knownFormats: formats, - options: options, - }) + asList, ok := r.value.([]interface{}) + if !ok || len(asList) <= index { + return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) + } - r.children[index] = childNode - return childNode.Validate() + childNode := newRatchetingValueValidator(asList[index], oldValueForIndex, schemaArgs{ + schema: schema, + root: rootSchema, + path: root, + knownFormats: formats, + options: options, }) + + r.children[index] = childNode + return childNode } // If oldValue is not a list, returns nil @@ -383,26 +384,13 @@ func (r *ratchetingValueValidator) CachedDeepEqual() (res bool) { return reflect.DeepEqual(r.oldValue, r.value) } -// A validator which just calls a validate function, and advertises that it -// validates anything -// -// In the future kube-openapi's ValueValidator interface can be simplified -// to be closer to `currentValidator.Options.NewValidator(value, ...).Validate()` -// so that a tree of "validation nodes" can be more formally encoded in the API. -// In that case this class would not be necessary. -type inlineValidator func(new interface{}) *validate.Result +var _ validate.ValueValidator = (&ratchetingValueValidator{}) -var _ validate.ValueValidator = inlineValidator(nil) - -func (f inlineValidator) Validate(new interface{}) *validate.Result { - return f(new) -} - -func (f inlineValidator) SetPath(path string) { +func (f ratchetingValueValidator) SetPath(path string) { // Do nothing // Unused by kube-openapi } -func (f inlineValidator) Applies(source interface{}, valueKind reflect.Kind) bool { +func (f ratchetingValueValidator) Applies(source interface{}, valueKind reflect.Kind) bool { return true } From 471e3ab8281a6c7acba4fda99fd35f8293251b2b Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:07:33 -0700 Subject: [PATCH 03/19] refactor: factor out object correlation so it is reusable by CEL validators --- .../pkg/apiserver/validation/ratcheting.go | 58 ++++++++++++------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 29be8b70532..91f486699ad 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -60,7 +60,11 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result { } func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { - return newRatchetingValueValidator(new, old, r.schemaArgs).Validate(new) + return newRatchetingValueValidator(&CorrelatedObject{ + oldValue: old, + value: new, + schema: r.schema, + }, r.schemaArgs).Validate(new) } // ratchetingValueValidator represents an invocation of SchemaValidator.ValidateUpdate @@ -80,13 +84,18 @@ type ratchetingValueValidator struct { // schemaArgs provides the arguments to use in the temporary SchemaValidator // that is created during a call to Validate. schemaArgs + correlation *CorrelatedObject +} +type CorrelatedObject struct { // Currently correlated old value during traversal of the schema/object oldValue interface{} // Value being validated value interface{} + schema *spec.Schema + // Scratch space below, may change during validation // Cached comparison result of DeepEqual of `value` and `thunk.oldValue` @@ -105,15 +114,13 @@ type ratchetingValueValidator struct { // // It should be expected to have an entry for either all of the children, or // none of them. - children map[interface{}]*ratchetingValueValidator + children map[interface{}]*CorrelatedObject } -func newRatchetingValueValidator(newValue, oldValue interface{}, args schemaArgs) *ratchetingValueValidator { +func newRatchetingValueValidator(correlation *CorrelatedObject, args schemaArgs) *ratchetingValueValidator { return &ratchetingValueValidator{ - oldValue: oldValue, - value: newValue, - schemaArgs: args, - children: map[interface{}]*ratchetingValueValidator{}, + schemaArgs: args, + correlation: correlation, } } @@ -152,14 +159,14 @@ func (r *ratchetingValueValidator) Validate(new interface{}) *validate.Result { s := validate.NewSchemaValidator(r.schema, r.root, r.path, r.knownFormats, opts...) - res := s.Validate(r.value) + res := s.Validate(r.correlation.value) if res.IsValid() { return res } // Current ratcheting rule is to ratchet errors if DeepEqual(old, new) is true. - if r.CachedDeepEqual() { + if r.correlation.CachedDeepEqual() { newRes := &validate.Result{} newRes.MergeAsWarnings(res) return newRes @@ -177,8 +184,8 @@ func (r *ratchetingValueValidator) Validate(new interface{}) *validate.Result { // If the old value cannot be correlated, then default validation is used. func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, options ...validate.Option) validate.ValueValidator { // Find correlated old value - oldAsMap, okOld := r.oldValue.(map[string]interface{}) - newAsMap, okNew := r.value.(map[string]interface{}) + oldAsMap, okOld := r.correlation.oldValue.(map[string]interface{}) + newAsMap, okNew := r.correlation.value.(map[string]interface{}) if !okOld || !okNew { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } @@ -189,15 +196,19 @@ func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *sp return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - childNode := newRatchetingValueValidator(newValueForField, oldValueForField, schemaArgs{ + childNode := &CorrelatedObject{ + oldValue: oldValueForField, + value: newValueForField, + schema: schema, + } + r.correlation.children[field] = childNode + return newRatchetingValueValidator(childNode, schemaArgs{ schema: schema, root: rootSchema, path: root, knownFormats: formats, options: options, }) - r.children[field] = childNode - return childNode } // SubIndexValidator overrides the standard validator constructor for sub-indicies by @@ -208,27 +219,30 @@ func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *sp // // If the old value cannot be correlated, then default validation is used. func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, options ...validate.Option) validate.ValueValidator { - oldValueForIndex := r.correlateOldValueForChildAtNewIndex(index) + oldValueForIndex := r.correlation.correlateOldValueForChildAtNewIndex(index) if oldValueForIndex == nil { // If correlation fails, default to non-ratcheting logic return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - asList, ok := r.value.([]interface{}) + asList, ok := r.correlation.value.([]interface{}) if !ok || len(asList) <= index { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - childNode := newRatchetingValueValidator(asList[index], oldValueForIndex, schemaArgs{ + childNode := &CorrelatedObject{ + oldValue: oldValueForIndex, + value: asList[index], + schema: schema, + } + r.correlation.children[index] = childNode + return newRatchetingValueValidator(childNode, schemaArgs{ schema: schema, root: rootSchema, path: root, knownFormats: formats, options: options, }) - - r.children[index] = childNode - return childNode } // If oldValue is not a list, returns nil @@ -237,7 +251,7 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch // // If listType is map, creates a map representation of the list using the designated // map-keys and caches it for future calls. -func (r *ratchetingValueValidator) correlateOldValueForChildAtNewIndex(index int) any { +func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { oldAsList, ok := r.oldValue.([]interface{}) if !ok { return nil @@ -295,7 +309,7 @@ func (r *ratchetingValueValidator) correlateOldValueForChildAtNewIndex(index int // If a lazy computation could not be found for all children possibly due // to validation logic short circuiting and skipping the children, then // this function simply defers to reflect.DeepEqual. -func (r *ratchetingValueValidator) CachedDeepEqual() (res bool) { +func (r *CorrelatedObject) CachedDeepEqual() (res bool) { if r.comparisonResult != nil { return *r.comparisonResult } From e73f3c8cdb389fd8c4e657000d49667c29e49eaa Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:48:21 -0700 Subject: [PATCH 04/19] refactor: add methods for Key and Index on CorrelatedObject going to be used from CEL validator too --- .../pkg/apiserver/validation/ratcheting.go | 118 +++++++++++++----- 1 file changed, 88 insertions(+), 30 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 91f486699ad..885e5ca7135 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -183,25 +183,11 @@ func (r *ratchetingValueValidator) Validate(new interface{}) *validate.Result { // // If the old value cannot be correlated, then default validation is used. func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, options ...validate.Option) validate.ValueValidator { - // Find correlated old value - oldAsMap, okOld := r.correlation.oldValue.(map[string]interface{}) - newAsMap, okNew := r.correlation.value.(map[string]interface{}) - if !okOld || !okNew { + childNode := r.correlation.Key(field) + if childNode == nil { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - oldValueForField, okOld := oldAsMap[field] - newValueForField, okNew := newAsMap[field] - if !okOld || !okNew { - return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) - } - - childNode := &CorrelatedObject{ - oldValue: oldValueForField, - value: newValueForField, - schema: schema, - } - r.correlation.children[field] = childNode return newRatchetingValueValidator(childNode, schemaArgs{ schema: schema, root: rootSchema, @@ -219,23 +205,11 @@ func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *sp // // If the old value cannot be correlated, then default validation is used. func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Schema, rootSchema interface{}, root string, formats strfmt.Registry, options ...validate.Option) validate.ValueValidator { - oldValueForIndex := r.correlation.correlateOldValueForChildAtNewIndex(index) - if oldValueForIndex == nil { - // If correlation fails, default to non-ratcheting logic + childNode := r.correlation.Index(index) + if childNode == nil { return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) } - asList, ok := r.correlation.value.([]interface{}) - if !ok || len(asList) <= index { - return validate.NewSchemaValidator(schema, rootSchema, root, formats, options...) - } - - childNode := &CorrelatedObject{ - oldValue: oldValueForIndex, - value: asList[index], - schema: schema, - } - r.correlation.children[index] = childNode return newRatchetingValueValidator(childNode, schemaArgs{ schema: schema, root: rootSchema, @@ -408,3 +382,87 @@ func (f ratchetingValueValidator) SetPath(path string) { func (f ratchetingValueValidator) Applies(source interface{}, valueKind reflect.Kind) bool { return true } + +// Key returns the child of the reciever with the given name. +// Returns nil if the given name is does not exist in the new object, or its +// value is not correlatable to an old value. +// If receiver is nil or if the new value is not an object/map, returns nil. +func (l *CorrelatedObject) Key(field string) *CorrelatedObject { + if l == nil || l.schema == nil { + return nil + } else if existing, exists := l.children[field]; exists { + return existing + } + + // Find correlated old value + oldAsMap, okOld := l.oldValue.(map[string]interface{}) + newAsMap, okNew := l.value.(map[string]interface{}) + if !okOld || !okNew { + return nil + } + + oldValueForField, okOld := oldAsMap[field] + newValueForField, okNew := newAsMap[field] + if !okOld || !okNew { + return nil + } + + var propertySchema *spec.Schema + if prop, exists := l.schema.Properties[field]; exists { + propertySchema = &prop + } else if addP := l.schema.AdditionalProperties; addP != nil && addP.Schema != nil { + propertySchema = addP.Schema + } else { + return nil + } + + res := &CorrelatedObject{ + oldValue: oldValueForField, + value: newValueForField, + schema: propertySchema, + } + if l.children == nil { + l.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) + } + l.children[field] = res + return res +} + +// Index returns the child of the reciever at the given index. +// Returns nil if the given index is out of bounds, or its value is not +// correlatable to an old value. +// If receiver is nil or if the new value is not an array, returns nil. +func (l *CorrelatedObject) Index(i int) *CorrelatedObject { + if l == nil || l.schema == nil { + return nil + } else if existing, exists := l.children[i]; exists { + return existing + } + + asList, ok := l.value.([]interface{}) + if !ok || len(asList) <= i { + return nil + } + + oldValueForIndex := l.correlateOldValueForChildAtNewIndex(i) + if oldValueForIndex == nil { + return nil + } + var itemSchema *spec.Schema + if i := l.schema.Items; i != nil && i.Schema != nil { + itemSchema = i.Schema + } else { + return nil + } + + res := &CorrelatedObject{ + oldValue: oldValueForIndex, + value: asList[i], + schema: itemSchema, + } + if l.children == nil { + l.children = make(map[interface{}]*CorrelatedObject, len(asList)) + } + l.children[i] = res + return res +} From 83a1dbf885dd5fce867fbb1525deed7c853b74d3 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:15:50 -0700 Subject: [PATCH 05/19] refactor: export correlated fields --- .../pkg/apiserver/validation/ratcheting.go | 74 +++++++++---------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 885e5ca7135..1eaa5768163 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -60,11 +60,7 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result { } func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { - return newRatchetingValueValidator(&CorrelatedObject{ - oldValue: old, - value: new, - schema: r.schema, - }, r.schemaArgs).Validate(new) + return newRatchetingValueValidator(NewCorrelatedObject(new, old, r.schema), r.schemaArgs).Validate(new) } // ratchetingValueValidator represents an invocation of SchemaValidator.ValidateUpdate @@ -89,12 +85,12 @@ type ratchetingValueValidator struct { type CorrelatedObject struct { // Currently correlated old value during traversal of the schema/object - oldValue interface{} + OldValue interface{} // Value being validated - value interface{} + Value interface{} - schema *spec.Schema + Schema *spec.Schema // Scratch space below, may change during validation @@ -117,6 +113,14 @@ type CorrelatedObject struct { children map[interface{}]*CorrelatedObject } +func NewCorrelatedObject(new, old interface{}, schema *spec.Schema) *CorrelatedObject { + return &CorrelatedObject{ + OldValue: old, + Value: new, + Schema: schema, + } +} + func newRatchetingValueValidator(correlation *CorrelatedObject, args schemaArgs) *ratchetingValueValidator { return &ratchetingValueValidator{ schemaArgs: args, @@ -159,7 +163,7 @@ func (r *ratchetingValueValidator) Validate(new interface{}) *validate.Result { s := validate.NewSchemaValidator(r.schema, r.root, r.path, r.knownFormats, opts...) - res := s.Validate(r.correlation.value) + res := s.Validate(r.correlation.Value) if res.IsValid() { return res @@ -226,12 +230,12 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch // If listType is map, creates a map representation of the list using the designated // map-keys and caches it for future calls. func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { - oldAsList, ok := r.oldValue.([]interface{}) + oldAsList, ok := r.OldValue.([]interface{}) if !ok { return nil } - asList, ok := r.value.([]interface{}) + asList, ok := r.Value.([]interface{}) if !ok { return nil } else if len(asList) <= index { @@ -239,7 +243,7 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { return nil } - listType, _ := r.schema.Extensions.GetString("x-kubernetes-list-type") + listType, _ := r.Schema.Extensions.GetString("x-kubernetes-list-type") switch listType { case "map": // Look up keys for this index in current object @@ -247,7 +251,7 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { oldList := r.mapList if oldList == nil { - oldList = celopenapi.MakeMapList(r.schema, oldAsList) + oldList = celopenapi.MakeMapList(r.Schema, oldAsList) r.mapList = oldList } return oldList.Get(currentElement) @@ -292,14 +296,14 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { r.comparisonResult = &res }() - if r.value == nil && r.oldValue == nil { + if r.Value == nil && r.OldValue == nil { return true - } else if r.value == nil || r.oldValue == nil { + } else if r.Value == nil || r.OldValue == nil { return false } - oldAsArray, oldIsArray := r.oldValue.([]interface{}) - newAsArray, newIsArray := r.value.([]interface{}) + oldAsArray, oldIsArray := r.OldValue.([]interface{}) + newAsArray, newIsArray := r.Value.([]interface{}) if oldIsArray != newIsArray { return false @@ -335,8 +339,8 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { return true } - oldAsMap, oldIsMap := r.oldValue.(map[string]interface{}) - newAsMap, newIsMap := r.value.(map[string]interface{}) + oldAsMap, oldIsMap := r.OldValue.(map[string]interface{}) + newAsMap, newIsMap := r.Value.(map[string]interface{}) if oldIsMap != newIsMap { return false @@ -369,7 +373,7 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { return true } - return reflect.DeepEqual(r.oldValue, r.value) + return reflect.DeepEqual(r.OldValue, r.Value) } var _ validate.ValueValidator = (&ratchetingValueValidator{}) @@ -388,15 +392,15 @@ func (f ratchetingValueValidator) Applies(source interface{}, valueKind reflect. // value is not correlatable to an old value. // If receiver is nil or if the new value is not an object/map, returns nil. func (l *CorrelatedObject) Key(field string) *CorrelatedObject { - if l == nil || l.schema == nil { + if l == nil || l.Schema == nil { return nil } else if existing, exists := l.children[field]; exists { return existing } // Find correlated old value - oldAsMap, okOld := l.oldValue.(map[string]interface{}) - newAsMap, okNew := l.value.(map[string]interface{}) + oldAsMap, okOld := l.OldValue.(map[string]interface{}) + newAsMap, okNew := l.Value.(map[string]interface{}) if !okOld || !okNew { return nil } @@ -408,22 +412,19 @@ func (l *CorrelatedObject) Key(field string) *CorrelatedObject { } var propertySchema *spec.Schema - if prop, exists := l.schema.Properties[field]; exists { + if prop, exists := l.Schema.Properties[field]; exists { propertySchema = &prop - } else if addP := l.schema.AdditionalProperties; addP != nil && addP.Schema != nil { + } else if addP := l.Schema.AdditionalProperties; addP != nil && addP.Schema != nil { propertySchema = addP.Schema } else { return nil } - res := &CorrelatedObject{ - oldValue: oldValueForField, - value: newValueForField, - schema: propertySchema, - } if l.children == nil { l.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) } + + res := NewCorrelatedObject(newValueForField, oldValueForField, propertySchema) l.children[field] = res return res } @@ -433,13 +434,13 @@ func (l *CorrelatedObject) Key(field string) *CorrelatedObject { // correlatable to an old value. // If receiver is nil or if the new value is not an array, returns nil. func (l *CorrelatedObject) Index(i int) *CorrelatedObject { - if l == nil || l.schema == nil { + if l == nil || l.Schema == nil { return nil } else if existing, exists := l.children[i]; exists { return existing } - asList, ok := l.value.([]interface{}) + asList, ok := l.Value.([]interface{}) if !ok || len(asList) <= i { return nil } @@ -449,20 +450,17 @@ func (l *CorrelatedObject) Index(i int) *CorrelatedObject { return nil } var itemSchema *spec.Schema - if i := l.schema.Items; i != nil && i.Schema != nil { + if i := l.Schema.Items; i != nil && i.Schema != nil { itemSchema = i.Schema } else { return nil } - res := &CorrelatedObject{ - oldValue: oldValueForIndex, - value: asList[i], - schema: itemSchema, - } if l.children == nil { l.children = make(map[interface{}]*CorrelatedObject, len(asList)) } + + res := NewCorrelatedObject(asList[i], oldValueForIndex, itemSchema) l.children[i] = res return res } From e76aad1813b380bf233681311488d3212bbe8eb0 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:23:06 -0700 Subject: [PATCH 06/19] refactor: use common.Schema in CorrelatedObject for reuseability with CEL structural schema impl --- .../pkg/apiserver/validation/ratcheting.go | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 1eaa5768163..45ab8003685 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -60,7 +60,10 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result { } func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { - return newRatchetingValueValidator(NewCorrelatedObject(new, old, r.schema), r.schemaArgs).Validate(new) + return newRatchetingValueValidator( + NewCorrelatedObject(new, old, &celopenapi.Schema{Schema: r.schemaArgs.schema}), + r.schemaArgs, + ).Validate(new) } // ratchetingValueValidator represents an invocation of SchemaValidator.ValidateUpdate @@ -90,7 +93,7 @@ type CorrelatedObject struct { // Value being validated Value interface{} - Schema *spec.Schema + Schema common.Schema // Scratch space below, may change during validation @@ -113,7 +116,7 @@ type CorrelatedObject struct { children map[interface{}]*CorrelatedObject } -func NewCorrelatedObject(new, old interface{}, schema *spec.Schema) *CorrelatedObject { +func NewCorrelatedObject(new, old interface{}, schema common.Schema) *CorrelatedObject { return &CorrelatedObject{ OldValue: old, Value: new, @@ -243,7 +246,7 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { return nil } - listType, _ := r.Schema.Extensions.GetString("x-kubernetes-list-type") + listType := r.Schema.XListType() switch listType { case "map": // Look up keys for this index in current object @@ -251,7 +254,7 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { oldList := r.mapList if oldList == nil { - oldList = celopenapi.MakeMapList(r.Schema, oldAsList) + oldList = common.MakeMapList(r.Schema, oldAsList) r.mapList = oldList } return oldList.Get(currentElement) @@ -411,11 +414,11 @@ func (l *CorrelatedObject) Key(field string) *CorrelatedObject { return nil } - var propertySchema *spec.Schema - if prop, exists := l.Schema.Properties[field]; exists { - propertySchema = &prop - } else if addP := l.Schema.AdditionalProperties; addP != nil && addP.Schema != nil { - propertySchema = addP.Schema + var propertySchema common.Schema + if prop, exists := l.Schema.Properties()[field]; exists { + propertySchema = prop + } else if addP := l.Schema.AdditionalProperties(); addP != nil && addP.Schema() != nil { + propertySchema = addP.Schema() } else { return nil } @@ -449,9 +452,9 @@ func (l *CorrelatedObject) Index(i int) *CorrelatedObject { if oldValueForIndex == nil { return nil } - var itemSchema *spec.Schema - if i := l.Schema.Items; i != nil && i.Schema != nil { - itemSchema = i.Schema + var itemSchema common.Schema + if i := l.Schema.Items(); i != nil { + itemSchema = i } else { return nil } From b321e8bf0db8c99a6757c3ab9cb219896904b497 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 11 Oct 2023 14:30:22 -0700 Subject: [PATCH 07/19] refactor: make CachedDeepEqual independent of validation before it required running validation first, now it builds the tree as needed --- .../pkg/apiserver/validation/ratcheting.go | 43 ++++++------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 45ab8003685..9c2e81cf52d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -313,26 +313,20 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { } else if oldIsArray { if len(oldAsArray) != len(newAsArray) { return false - } else if len(r.children) != len(oldAsArray) { - // kube-openapi validator is written to always visit all - // children of a slice, so this case is only possible if - // one of the children could not be correlated. In that case, - // we know the objects are not equal. - // - return false } - // Correctly considers map-type lists due to fact that index here - // is only used for numbering. The correlation is stored in the - // childInvocation itself - // - // NOTE: This does not consider sets, since we don't correlate them. for i := range newAsArray { - // Query for child - child, ok := r.children[i] - if !ok { - // This should not happen + child := r.Index(i) + if child == nil { + if r.mapList == nil { + // Treat non-correlatable array as a unit with reflect.DeepEqual + return reflect.DeepEqual(oldAsArray, newAsArray) + } + + // If array is correlatable, but old not found. Just short circuit + // comparison return false + } else if !child.CachedDeepEqual() { // If one child is not equal the entire object is not equal return false @@ -350,21 +344,12 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { } else if oldIsMap { if len(oldAsMap) != len(newAsMap) { return false - } else if len(oldAsMap) == 0 && len(newAsMap) == 0 { - // Both empty - return true - } else if len(r.children) != len(oldAsMap) { - // If we are missing a key it is because the old value could not - // be correlated to the new, so the objects are not equal. - // - return false } - for k := range oldAsMap { - // Check to see if this child was explored during validation - child, ok := r.children[k] - if !ok { - // Child from old missing in new due to key change + for k := range newAsMap { + child := r.Key(k) + if child == nil { + // Un-correlatable child due to key change. // Objects are not equal. return false } else if !child.CachedDeepEqual() { From 27cb869e5596525cec9884ecb9b02bfcfe5273e4 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 10 Oct 2023 10:53:12 -0700 Subject: [PATCH 08/19] refactor: move correlatedObject to its own file no changes except package naming --- .../pkg/apiserver/validation/ratcheting.go | 260 +---------------- .../apiserver/pkg/cel/common/equality.go | 275 ++++++++++++++++++ 2 files changed, 278 insertions(+), 257 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/cel/common/equality.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index 9c2e81cf52d..f91cf2cbed2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -61,7 +61,7 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result { func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { return newRatchetingValueValidator( - NewCorrelatedObject(new, old, &celopenapi.Schema{Schema: r.schemaArgs.schema}), + common.NewCorrelatedObject(new, old, &celopenapi.Schema{Schema: r.schemaArgs.schema}), r.schemaArgs, ).Validate(new) } @@ -83,48 +83,10 @@ type ratchetingValueValidator struct { // schemaArgs provides the arguments to use in the temporary SchemaValidator // that is created during a call to Validate. schemaArgs - correlation *CorrelatedObject + correlation *common.CorrelatedObject } -type CorrelatedObject struct { - // Currently correlated old value during traversal of the schema/object - OldValue interface{} - - // Value being validated - Value interface{} - - Schema common.Schema - - // Scratch space below, may change during validation - - // Cached comparison result of DeepEqual of `value` and `thunk.oldValue` - comparisonResult *bool - - // Cached map representation of a map-type list, or nil if not map-type list - mapList common.MapList - - // Children spawned by a call to `Validate` on this object - // key is either a string or an index, depending upon whether `value` is - // a map or a list, respectively. - // - // The list of children may be incomplete depending upon if the internal - // logic of kube-openapi's SchemaValidator short-circuited before - // reaching all of the children. - // - // It should be expected to have an entry for either all of the children, or - // none of them. - children map[interface{}]*CorrelatedObject -} - -func NewCorrelatedObject(new, old interface{}, schema common.Schema) *CorrelatedObject { - return &CorrelatedObject{ - OldValue: old, - Value: new, - Schema: schema, - } -} - -func newRatchetingValueValidator(correlation *CorrelatedObject, args schemaArgs) *ratchetingValueValidator { +func newRatchetingValueValidator(correlation *common.CorrelatedObject, args schemaArgs) *ratchetingValueValidator { return &ratchetingValueValidator{ schemaArgs: args, correlation: correlation, @@ -226,144 +188,6 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch }) } -// If oldValue is not a list, returns nil -// If oldValue is a list takes mapType into account and attempts to find the -// old value with the same index or key, depending upon the mapType. -// -// If listType is map, creates a map representation of the list using the designated -// map-keys and caches it for future calls. -func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { - oldAsList, ok := r.OldValue.([]interface{}) - if !ok { - return nil - } - - asList, ok := r.Value.([]interface{}) - if !ok { - return nil - } else if len(asList) <= index { - // Cannot correlate out of bounds index - return nil - } - - listType := r.Schema.XListType() - switch listType { - case "map": - // Look up keys for this index in current object - currentElement := asList[index] - - oldList := r.mapList - if oldList == nil { - oldList = common.MakeMapList(r.Schema, oldAsList) - r.mapList = oldList - } - return oldList.Get(currentElement) - - case "set": - // Are sets correlatable? Only if the old value equals the current value. - // We might be able to support this, but do not currently see a lot - // of value - // (would allow you to add/remove items from sets with ratcheting but not change them) - return nil - case "atomic": - // Atomic lists are not correlatable by item - // Ratcheting is not available on a per-index basis - return nil - default: - // Correlate by-index by default. - // - // Cannot correlate an out-of-bounds index - if len(oldAsList) <= index { - return nil - } - - return oldAsList[index] - } -} - -// CachedDeepEqual is equivalent to reflect.DeepEqual, but caches the -// results in the tree of ratchetInvocationScratch objects on the way: -// -// For objects and arrays, this function will make a best effort to make -// use of past DeepEqual checks performed by this Node's children, if available. -// -// If a lazy computation could not be found for all children possibly due -// to validation logic short circuiting and skipping the children, then -// this function simply defers to reflect.DeepEqual. -func (r *CorrelatedObject) CachedDeepEqual() (res bool) { - if r.comparisonResult != nil { - return *r.comparisonResult - } - - defer func() { - r.comparisonResult = &res - }() - - if r.Value == nil && r.OldValue == nil { - return true - } else if r.Value == nil || r.OldValue == nil { - return false - } - - oldAsArray, oldIsArray := r.OldValue.([]interface{}) - newAsArray, newIsArray := r.Value.([]interface{}) - - if oldIsArray != newIsArray { - return false - } else if oldIsArray { - if len(oldAsArray) != len(newAsArray) { - return false - } - - for i := range newAsArray { - child := r.Index(i) - if child == nil { - if r.mapList == nil { - // Treat non-correlatable array as a unit with reflect.DeepEqual - return reflect.DeepEqual(oldAsArray, newAsArray) - } - - // If array is correlatable, but old not found. Just short circuit - // comparison - return false - - } else if !child.CachedDeepEqual() { - // If one child is not equal the entire object is not equal - return false - } - } - - return true - } - - oldAsMap, oldIsMap := r.OldValue.(map[string]interface{}) - newAsMap, newIsMap := r.Value.(map[string]interface{}) - - if oldIsMap != newIsMap { - return false - } else if oldIsMap { - if len(oldAsMap) != len(newAsMap) { - return false - } - - for k := range newAsMap { - child := r.Key(k) - if child == nil { - // Un-correlatable child due to key change. - // Objects are not equal. - return false - } else if !child.CachedDeepEqual() { - // If one child is not equal the entire object is not equal - return false - } - } - - return true - } - - return reflect.DeepEqual(r.OldValue, r.Value) -} - var _ validate.ValueValidator = (&ratchetingValueValidator{}) func (f ratchetingValueValidator) SetPath(path string) { @@ -374,81 +198,3 @@ func (f ratchetingValueValidator) SetPath(path string) { func (f ratchetingValueValidator) Applies(source interface{}, valueKind reflect.Kind) bool { return true } - -// Key returns the child of the reciever with the given name. -// Returns nil if the given name is does not exist in the new object, or its -// value is not correlatable to an old value. -// If receiver is nil or if the new value is not an object/map, returns nil. -func (l *CorrelatedObject) Key(field string) *CorrelatedObject { - if l == nil || l.Schema == nil { - return nil - } else if existing, exists := l.children[field]; exists { - return existing - } - - // Find correlated old value - oldAsMap, okOld := l.OldValue.(map[string]interface{}) - newAsMap, okNew := l.Value.(map[string]interface{}) - if !okOld || !okNew { - return nil - } - - oldValueForField, okOld := oldAsMap[field] - newValueForField, okNew := newAsMap[field] - if !okOld || !okNew { - return nil - } - - var propertySchema common.Schema - if prop, exists := l.Schema.Properties()[field]; exists { - propertySchema = prop - } else if addP := l.Schema.AdditionalProperties(); addP != nil && addP.Schema() != nil { - propertySchema = addP.Schema() - } else { - return nil - } - - if l.children == nil { - l.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) - } - - res := NewCorrelatedObject(newValueForField, oldValueForField, propertySchema) - l.children[field] = res - return res -} - -// Index returns the child of the reciever at the given index. -// Returns nil if the given index is out of bounds, or its value is not -// correlatable to an old value. -// If receiver is nil or if the new value is not an array, returns nil. -func (l *CorrelatedObject) Index(i int) *CorrelatedObject { - if l == nil || l.Schema == nil { - return nil - } else if existing, exists := l.children[i]; exists { - return existing - } - - asList, ok := l.Value.([]interface{}) - if !ok || len(asList) <= i { - return nil - } - - oldValueForIndex := l.correlateOldValueForChildAtNewIndex(i) - if oldValueForIndex == nil { - return nil - } - var itemSchema common.Schema - if i := l.Schema.Items(); i != nil { - itemSchema = i - } else { - return nil - } - - if l.children == nil { - l.children = make(map[interface{}]*CorrelatedObject, len(asList)) - } - - res := NewCorrelatedObject(asList[i], oldValueForIndex, itemSchema) - l.children[i] = res - return res -} diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go new file mode 100644 index 00000000000..c3c86ea29ba --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -0,0 +1,275 @@ +/* +Copyright 2023 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 common + +import ( + "reflect" +) + +type CorrelatedObject struct { + // Currently correlated old value during traversal of the schema/object + OldValue interface{} + + // Value being validated + Value interface{} + + Schema Schema + + // Scratch space below, may change during validation + + // Cached comparison result of DeepEqual of `value` and `thunk.oldValue` + comparisonResult *bool + + // Cached map representation of a map-type list, or nil if not map-type list + mapList MapList + + // Children spawned by a call to `Validate` on this object + // key is either a string or an index, depending upon whether `value` is + // a map or a list, respectively. + // + // The list of children may be incomplete depending upon if the internal + // logic of kube-openapi's SchemaValidator short-circuited before + // reaching all of the children. + // + // It should be expected to have an entry for either all of the children, or + // none of them. + children map[interface{}]*CorrelatedObject +} + +func NewCorrelatedObject(new, old interface{}, schema Schema) *CorrelatedObject { + return &CorrelatedObject{ + OldValue: old, + Value: new, + Schema: schema, + } +} + +// If oldValue is not a list, returns nil +// If oldValue is a list takes mapType into account and attempts to find the +// old value with the same index or key, depending upon the mapType. +// +// If listType is map, creates a map representation of the list using the designated +// map-keys and caches it for future calls. +func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { + oldAsList, ok := r.OldValue.([]interface{}) + if !ok { + return nil + } + + asList, ok := r.Value.([]interface{}) + if !ok { + return nil + } else if len(asList) <= index { + // Cannot correlate out of bounds index + return nil + } + + listType := r.Schema.XListType() + switch listType { + case "map": + // Look up keys for this index in current object + currentElement := asList[index] + + oldList := r.mapList + if oldList == nil { + oldList = MakeMapList(r.Schema, oldAsList) + r.mapList = oldList + } + return oldList.Get(currentElement) + + case "set": + // Are sets correlatable? Only if the old value equals the current value. + // We might be able to support this, but do not currently see a lot + // of value + // (would allow you to add/remove items from sets with ratcheting but not change them) + return nil + case "atomic": + // Atomic lists are not correlatable by item + // Ratcheting is not available on a per-index basis + return nil + default: + // Correlate by-index by default. + // + // Cannot correlate an out-of-bounds index + if len(oldAsList) <= index { + return nil + } + + return oldAsList[index] + } +} + +// CachedDeepEqual is equivalent to reflect.DeepEqual, but caches the +// results in the tree of ratchetInvocationScratch objects on the way: +// +// For objects and arrays, this function will make a best effort to make +// use of past DeepEqual checks performed by this Node's children, if available. +// +// If a lazy computation could not be found for all children possibly due +// to validation logic short circuiting and skipping the children, then +// this function simply defers to reflect.DeepEqual. +func (r *CorrelatedObject) CachedDeepEqual() (res bool) { + if r.comparisonResult != nil { + return *r.comparisonResult + } + + defer func() { + r.comparisonResult = &res + }() + + if r.Value == nil && r.OldValue == nil { + return true + } else if r.Value == nil || r.OldValue == nil { + return false + } + + oldAsArray, oldIsArray := r.OldValue.([]interface{}) + newAsArray, newIsArray := r.Value.([]interface{}) + + if oldIsArray != newIsArray { + return false + } else if oldIsArray { + if len(oldAsArray) != len(newAsArray) { + return false + } + + for i := range newAsArray { + child := r.Index(i) + if child == nil { + if r.mapList == nil { + // Treat non-correlatable array as a unit with reflect.DeepEqual + return reflect.DeepEqual(oldAsArray, newAsArray) + } + + // If array is correlatable, but old not found. Just short circuit + // comparison + return false + + } else if !child.CachedDeepEqual() { + // If one child is not equal the entire object is not equal + return false + } + } + + return true + } + + oldAsMap, oldIsMap := r.OldValue.(map[string]interface{}) + newAsMap, newIsMap := r.Value.(map[string]interface{}) + + if oldIsMap != newIsMap { + return false + } else if oldIsMap { + if len(oldAsMap) != len(newAsMap) { + return false + } + + for k := range newAsMap { + child := r.Key(k) + if child == nil { + // Un-correlatable child due to key change. + // Objects are not equal. + return false + } else if !child.CachedDeepEqual() { + // If one child is not equal the entire object is not equal + return false + } + } + + return true + } + + return reflect.DeepEqual(r.OldValue, r.Value) +} + +// Key returns the child of the receiver with the given name. +// Returns nil if the given name is does not exist in the new object, or its +// value is not correlatable to an old value. +// If receiver is nil or if the new value is not an object/map, returns nil. +func (l *CorrelatedObject) Key(field string) *CorrelatedObject { + if l == nil || l.Schema == nil { + return nil + } else if existing, exists := l.children[field]; exists { + return existing + } + + // Find correlated old value + oldAsMap, okOld := l.OldValue.(map[string]interface{}) + newAsMap, okNew := l.Value.(map[string]interface{}) + if !okOld || !okNew { + return nil + } + + oldValueForField, okOld := oldAsMap[field] + newValueForField, okNew := newAsMap[field] + if !okOld || !okNew { + return nil + } + + var propertySchema Schema + if prop, exists := l.Schema.Properties()[field]; exists { + propertySchema = prop + } else if addP := l.Schema.AdditionalProperties(); addP != nil && addP.Schema() != nil { + propertySchema = addP.Schema() + } else { + return nil + } + + if l.children == nil { + l.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) + } + + res := NewCorrelatedObject(newValueForField, oldValueForField, propertySchema) + l.children[field] = res + return res +} + +// Index returns the child of the receiver at the given index. +// Returns nil if the given index is out of bounds, or its value is not +// correlatable to an old value. +// If receiver is nil or if the new value is not an array, returns nil. +func (l *CorrelatedObject) Index(i int) *CorrelatedObject { + if l == nil || l.Schema == nil { + return nil + } else if existing, exists := l.children[i]; exists { + return existing + } + + asList, ok := l.Value.([]interface{}) + if !ok || len(asList) <= i { + return nil + } + + oldValueForIndex := l.correlateOldValueForChildAtNewIndex(i) + if oldValueForIndex == nil { + return nil + } + var itemSchema Schema + if i := l.Schema.Items(); i != nil { + itemSchema = i + } else { + return nil + } + + if l.children == nil { + l.children = make(map[interface{}]*CorrelatedObject, len(asList)) + } + + res := NewCorrelatedObject(asList[i], oldValueForIndex, itemSchema) + l.children[i] = res + return res +} From c08a9321eed6a917a2fbc13b8e023d2f4122ee36 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 11 Oct 2023 13:51:49 -0700 Subject: [PATCH 09/19] cleanup: add header and fix spelling --- .../pkg/apiserver/validation/ratcheting.go | 4 +- .../apiserver/pkg/cel/common/equality.go | 38 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go index f91cf2cbed2..0609d8eb66e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go @@ -190,11 +190,11 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch var _ validate.ValueValidator = (&ratchetingValueValidator{}) -func (f ratchetingValueValidator) SetPath(path string) { +func (r ratchetingValueValidator) SetPath(path string) { // Do nothing // Unused by kube-openapi } -func (f ratchetingValueValidator) Applies(source interface{}, valueKind reflect.Kind) bool { +func (r ratchetingValueValidator) Applies(source interface{}, valueKind reflect.Kind) bool { return true } diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index c3c86ea29ba..10b03d01e1a 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -200,16 +200,16 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { // Returns nil if the given name is does not exist in the new object, or its // value is not correlatable to an old value. // If receiver is nil or if the new value is not an object/map, returns nil. -func (l *CorrelatedObject) Key(field string) *CorrelatedObject { - if l == nil || l.Schema == nil { +func (r *CorrelatedObject) Key(field string) *CorrelatedObject { + if r == nil || r.Schema == nil { return nil - } else if existing, exists := l.children[field]; exists { + } else if existing, exists := r.children[field]; exists { return existing } // Find correlated old value - oldAsMap, okOld := l.OldValue.(map[string]interface{}) - newAsMap, okNew := l.Value.(map[string]interface{}) + oldAsMap, okOld := r.OldValue.(map[string]interface{}) + newAsMap, okNew := r.Value.(map[string]interface{}) if !okOld || !okNew { return nil } @@ -221,20 +221,20 @@ func (l *CorrelatedObject) Key(field string) *CorrelatedObject { } var propertySchema Schema - if prop, exists := l.Schema.Properties()[field]; exists { + if prop, exists := r.Schema.Properties()[field]; exists { propertySchema = prop - } else if addP := l.Schema.AdditionalProperties(); addP != nil && addP.Schema() != nil { + } else if addP := r.Schema.AdditionalProperties(); addP != nil && addP.Schema() != nil { propertySchema = addP.Schema() } else { return nil } - if l.children == nil { - l.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) + if r.children == nil { + r.children = make(map[interface{}]*CorrelatedObject, len(newAsMap)) } res := NewCorrelatedObject(newValueForField, oldValueForField, propertySchema) - l.children[field] = res + r.children[field] = res return res } @@ -242,34 +242,34 @@ func (l *CorrelatedObject) Key(field string) *CorrelatedObject { // Returns nil if the given index is out of bounds, or its value is not // correlatable to an old value. // If receiver is nil or if the new value is not an array, returns nil. -func (l *CorrelatedObject) Index(i int) *CorrelatedObject { - if l == nil || l.Schema == nil { +func (r *CorrelatedObject) Index(i int) *CorrelatedObject { + if r == nil || r.Schema == nil { return nil - } else if existing, exists := l.children[i]; exists { + } else if existing, exists := r.children[i]; exists { return existing } - asList, ok := l.Value.([]interface{}) + asList, ok := r.Value.([]interface{}) if !ok || len(asList) <= i { return nil } - oldValueForIndex := l.correlateOldValueForChildAtNewIndex(i) + oldValueForIndex := r.correlateOldValueForChildAtNewIndex(i) if oldValueForIndex == nil { return nil } var itemSchema Schema - if i := l.Schema.Items(); i != nil { + if i := r.Schema.Items(); i != nil { itemSchema = i } else { return nil } - if l.children == nil { - l.children = make(map[interface{}]*CorrelatedObject, len(asList)) + if r.children == nil { + r.children = make(map[interface{}]*CorrelatedObject, len(asList)) } res := NewCorrelatedObject(asList[i], oldValueForIndex, itemSchema) - l.children[i] = res + r.children[i] = res return res } From ba9347230e6577140eaa0ac3d9ef99d0163a7934 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 11 Oct 2023 14:03:28 -0700 Subject: [PATCH 10/19] test: add correlatedobject test cases --- .../apiserver/pkg/cel/common/equality_test.go | 702 ++++++++++++++++++ 1 file changed, 702 insertions(+) create mode 100644 staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go new file mode 100644 index 00000000000..cf8eadcaaca --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go @@ -0,0 +1,702 @@ +/* +Copyright 2023 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 common_test + +import ( + "errors" + "fmt" + "reflect" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/apiserver/pkg/cel/common" + "k8s.io/apiserver/pkg/cel/openapi" + "k8s.io/kube-openapi/pkg/validation/spec" +) + +type TestCase struct { + Name string + + // Expected old value after traversal. If nil, then the traversal should fail. + OldValue interface{} + + // Expected value after traversal. If nil, then the traversal should fail. + NewValue interface{} + + // Whether OldValue and NewValue are considered to be equal. + // Defaults to reflect.DeepEqual comparison of the two. Can be overridden to + // true here if the two values are not DeepEqual, but are considered equal + // for instance due to map-list reordering. + ExpectEqual bool + + // Schema to provide to the correlated object + Schema common.Schema + + // Array of field names and indexes to traverse to get to the value + KeyPath []interface{} + + // Root object to traverse from + RootObject interface{} + RootOldObject interface{} +} + +func (c TestCase) Run() error { + // Create the correlated object + correlatedObject := common.NewCorrelatedObject(c.RootObject, c.RootOldObject, c.Schema) + + // Traverse the correlated object + var err error + for _, key := range c.KeyPath { + if correlatedObject == nil { + break + } + + switch k := key.(type) { + case string: + correlatedObject = correlatedObject.Key(k) + case int: + correlatedObject = correlatedObject.Index(k) + default: + return errors.New("key must be a string or int") + } + if err != nil { + return err + } + } + + if correlatedObject == nil { + if c.OldValue != nil || c.NewValue != nil { + return fmt.Errorf("expected non-nil value, got nil") + } + } else { + // Check that the correlated object has the expected values + if !reflect.DeepEqual(correlatedObject.Value, c.NewValue) { + return fmt.Errorf("expected value %v, got %v", c.NewValue, correlatedObject.Value) + } + if !reflect.DeepEqual(correlatedObject.OldValue, c.OldValue) { + return fmt.Errorf("expected old value %v, got %v", c.OldValue, correlatedObject.OldValue) + } + + // Check that the correlated object is considered equal to the expected value + if (c.ExpectEqual || reflect.DeepEqual(correlatedObject.Value, correlatedObject.OldValue)) != correlatedObject.CachedDeepEqual() { + return fmt.Errorf("expected equal, got not equal") + } + } + + return nil +} + +// Creates a *spec.Schema Schema by decoding the given YAML. Panics on error +func mustSchema(source string) *openapi.Schema { + d := yaml.NewYAMLOrJSONDecoder(strings.NewReader(source), 4096) + res := &spec.Schema{} + if err := d.Decode(res); err != nil { + panic(err) + } + return &openapi.Schema{Schema: res} +} + +// Creates an *unstructured by decoding the given YAML. Panics on error +func mustUnstructured(source string) interface{} { + d := yaml.NewYAMLOrJSONDecoder(strings.NewReader(source), 4096) + var res interface{} + if err := d.Decode(&res); err != nil { + panic(err) + } + return res +} + +func TestCorrelation(t *testing.T) { + // Tests ensure that the output of following keypath using the given + // schema and root objects yields the provided new value and old value. + // If new or old are nil, then ensures that the traversal failed due to + // uncorrelatable field path. + // Also confirms that CachedDeepEqual output is equal to expected result of + // reflect.DeepEqual of the new and old values. + cases := []TestCase{ + { + Name: "Basic Key", + RootObject: mustUnstructured(`a: b`), + RootOldObject: mustUnstructured(`a: b`), + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{"a"}, + NewValue: "b", + OldValue: "b", + }, + { + Name: "Basic Index", + RootObject: mustUnstructured(`[a, b]`), + RootOldObject: mustUnstructured(`[a, b]`), + Schema: mustSchema(` + items: + type: string + `), + KeyPath: []interface{}{1}, + NewValue: "b", + OldValue: "b", + }, + { + Name: "Added Key Not In Old Object", + RootObject: mustUnstructured(` + a: b + c: d + `), + RootOldObject: mustUnstructured(` + a: b + `), + Schema: mustSchema(` + properties: + a: { type: string } + c: { type: string } + `), + KeyPath: []interface{}{"c"}, + }, + { + Name: "Added Index Not In Old Object", + RootObject: mustUnstructured(` + - a + - b + - c + `), + RootOldObject: mustUnstructured(` + - a + - b + `), + Schema: mustSchema(` + items: + type: string + `), + KeyPath: []interface{}{2}, + }, + { + Name: "Changed Index In Old Object", + RootObject: []interface{}{ + "a", + "b", + }, + RootOldObject: []interface{}{ + "a", + "oldB", + }, + Schema: mustSchema(` + items: + type: string + `), + KeyPath: []interface{}{1}, + NewValue: "b", + OldValue: "oldB", + }, + { + Name: "Changed Index In Nested Old Object", + RootObject: []interface{}{ + "a", + "b", + }, + RootOldObject: []interface{}{ + "a", + "oldB", + }, + Schema: mustSchema(` + items: + type: string + `), + KeyPath: []interface{}{}, + NewValue: []interface{}{"a", "b"}, + OldValue: []interface{}{"a", "oldB"}, + }, + { + Name: "Changed Key In Old Object", + RootObject: map[string]interface{}{ + "a": "b", + }, + RootOldObject: map[string]interface{}{ + "a": "oldB", + }, + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{"a"}, + NewValue: "b", + OldValue: "oldB", + }, + { + Name: "Map list type", + RootObject: mustUnstructured(` + foo: + - bar: baz + val: newBazValue + `), + RootOldObject: mustUnstructured(` + foo: + - bar: fizz + val: fizzValue + - bar: baz + val: bazValue + `), + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + properties: + bar: + type: string + val: + type: string + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - bar + `), + KeyPath: []interface{}{"foo", 0, "val"}, + NewValue: "newBazValue", + OldValue: "bazValue", + }, + { + Name: "Atomic list item should not correlate", + RootObject: mustUnstructured(` + foo: + - bar: baz + val: newValue + `), + RootOldObject: mustUnstructured(` + foo: + - bar: fizz + val: fizzValue + - bar: baz + val: barValue + `), + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + properties: + bar: + type: string + val: + type: string + x-kubernetes-list-type: atomic + `), + KeyPath: []interface{}{"foo", 0, "val"}, + }, + { + Name: "Map used inside of map list type should correlate", + RootObject: mustUnstructured(` + foo: + - key: keyValue + bar: + baz: newValue + `), + RootOldObject: mustUnstructured(` + foo: + - key: otherKeyValue + bar: + baz: otherOldValue + - key: altKeyValue + bar: + baz: altOldValue + - key: keyValue + bar: + baz: oldValue + `), + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - key + `), + KeyPath: []interface{}{"foo", 0, "bar", "baz"}, + NewValue: "newValue", + OldValue: "oldValue", + }, + { + Name: "Map used inside another map should correlate", + RootObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: newValue + `), + RootOldObject: mustUnstructured(` + foo: + key: otherKeyValue + bar: + baz: otherOldValue + altFoo: + key: altKeyValue + bar: + baz: altOldValue + otherFoo: + key: keyValue + bar: + baz: oldValue + `), + Schema: mustSchema(` + properties: + foo: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + `), + KeyPath: []interface{}{"foo", "bar"}, + NewValue: map[string]interface{}{"baz": "newValue"}, + OldValue: map[string]interface{}{"baz": "otherOldValue"}, + }, + { + Name: "Nested map equal to old", + RootObject: mustUnstructured(` + foo: + key: newKeyValue + bar: + baz: value + `), + RootOldObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: value + `), + Schema: mustSchema(` + properties: + foo: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + `), + KeyPath: []interface{}{"foo", "bar"}, + NewValue: map[string]interface{}{"baz": "value"}, + OldValue: map[string]interface{}{"baz": "value"}, + }, + { + Name: "Re-ordered list considered equal to old value due to map keys", + RootObject: mustUnstructured(` + foo: + - key: keyValue + bar: + baz: value + - key: altKeyValue + bar: + baz: altValue + `), + RootOldObject: mustUnstructured(` + foo: + - key: altKeyValue + bar: + baz: altValue + - key: keyValue + bar: + baz: value + `), + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - key + `), + KeyPath: []interface{}{"foo"}, + NewValue: mustUnstructured(` + - key: keyValue + bar: + baz: value + - key: altKeyValue + bar: + baz: altValue + `), + OldValue: mustUnstructured(` + - key: altKeyValue + bar: + baz: altValue + - key: keyValue + bar: + baz: value + `), + ExpectEqual: true, + }, + { + Name: "Correlate unknown string key via additional properties", + RootObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: newValue + `), + RootOldObject: mustUnstructured(` + foo: + key: otherKeyValue + bar: + baz: otherOldValue + `), + Schema: mustSchema(` + properties: + foo: + type: object + additionalProperties: + properties: + baz: + type: string + `), + KeyPath: []interface{}{"foo", "bar", "baz"}, + NewValue: "newValue", + OldValue: "otherOldValue", + }, + { + Name: "Changed map value", + RootObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: newValue + `), + RootOldObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: oldValue + `), + Schema: mustSchema(` + properties: + foo: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + `), + KeyPath: []interface{}{"foo", "bar"}, + NewValue: mustUnstructured(` + baz: newValue + `), + OldValue: mustUnstructured(` + baz: oldValue + `), + }, + { + Name: "Changed nested map value", + RootObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: newValue + `), + RootOldObject: mustUnstructured(` + foo: + key: keyValue + bar: + baz: oldValue + `), + Schema: mustSchema(` + properties: + foo: + type: object + properties: + key: + type: string + bar: + type: object + properties: + baz: + type: string + `), + KeyPath: []interface{}{"foo"}, + NewValue: mustUnstructured(` + key: keyValue + bar: + baz: newValue + `), + OldValue: mustUnstructured(` + key: keyValue + bar: + baz: oldValue + `), + }, + { + Name: "unchanged list type set with atomic map values", + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + x-kubernetes-map-type: atomic + properties: + key: + type: string + bar: + type: string + x-kubernetes-list-type: set + `), + RootObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + RootOldObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + KeyPath: []interface{}{"foo"}, + NewValue: mustUnstructured(` + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + OldValue: mustUnstructured(` + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + }, + { + Name: "changed list type set with atomic map values", + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + x-kubernetes-map-type: atomic + properties: + key: + type: string + bar: + type: string + x-kubernetes-list-type: set + `), + RootObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: newValue2 + `), + RootOldObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + KeyPath: []interface{}{"foo"}, + NewValue: mustUnstructured(` + - key: key1 + bar: value1 + - key: key2 + bar: newValue2 + `), + OldValue: mustUnstructured(` + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + }, + { + Name: "elements of list type set with atomic map values are not correlated", + Schema: mustSchema(` + properties: + foo: + type: array + items: + type: object + x-kubernetes-map-type: atomic + properties: + key: + type: string + bar: + type: string + x-kubernetes-list-type: set + `), + RootObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: newValue2 + `), + RootOldObject: mustUnstructured(` + foo: + - key: key1 + bar: value1 + - key: key2 + bar: value2 + `), + KeyPath: []interface{}{"foo", 0, "key"}, + NewValue: nil, + }, + } + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + if err := c.Run(); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + } +} From 0149c1f8b315d704d6d80c00861526e2899001e5 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 11 Oct 2023 15:45:48 -0700 Subject: [PATCH 11/19] test: few more correlatedobject test cases --- .../apiserver/pkg/cel/common/equality_test.go | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go index cf8eadcaaca..c1129efa896 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go @@ -238,6 +238,64 @@ func TestCorrelation(t *testing.T) { NewValue: "b", OldValue: "oldB", }, + { + Name: "Replaced Key In Old Object", + RootObject: map[string]interface{}{ + "a": "b", + }, + RootOldObject: map[string]interface{}{ + "b": "a", + }, + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{}, + NewValue: map[string]interface{}{"a": "b"}, + OldValue: map[string]interface{}{"b": "a"}, + }, + { + Name: "Added Key In Old Object", + RootObject: map[string]interface{}{ + "a": "b", + }, + RootOldObject: map[string]interface{}{}, + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{}, + NewValue: map[string]interface{}{"a": "b"}, + OldValue: map[string]interface{}{}, + }, + { + Name: "Changed list to map", + RootObject: map[string]interface{}{ + "a": "b", + }, + RootOldObject: []interface{}{"a", "b"}, + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{}, + NewValue: map[string]interface{}{"a": "b"}, + OldValue: []interface{}{"a", "b"}, + }, + { + Name: "Changed string to map", + RootObject: map[string]interface{}{ + "a": "b", + }, + RootOldObject: "a string", + Schema: mustSchema(` + properties: + a: { type: string } + `), + KeyPath: []interface{}{}, + NewValue: map[string]interface{}{"a": "b"}, + OldValue: "a string", + }, { Name: "Map list type", RootObject: mustUnstructured(` From 4dedabf2a659ee702cbcd93a482c63296910d5c6 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Thu, 12 Oct 2023 15:51:25 -0700 Subject: [PATCH 12/19] test: fix boilerplate --- staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go index c1129efa896..8300d0a7f74 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go @@ -5,7 +5,7 @@ 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 + 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, From 0495616230a13dcc19c9da8ec7b8b2a38e2b6a33 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 13:50:19 -0700 Subject: [PATCH 13/19] cleanup: add godoc --- staging/src/k8s.io/apiserver/pkg/cel/common/equality.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index 10b03d01e1a..dd47341b597 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -20,6 +20,13 @@ import ( "reflect" ) +// CorrelatedObject represents a node in a tree of objects that are being +// validated. It is used to keep track of the old value of an object during +// traversal of the new value. It is also used to cache the results of +// DeepEqual comparisons between the old and new values of objects. +// +// CorrelatedObject is not thread-safe. It is the responsibility of the caller +// to handle concurrency, if any. type CorrelatedObject struct { // Currently correlated old value during traversal of the schema/object OldValue interface{} @@ -27,6 +34,8 @@ type CorrelatedObject struct { // Value being validated Value interface{} + // Schema used for validation of this value. The schema is also used + // to determine how to correlate the old object. Schema Schema // Scratch space below, may change during validation From e1fa1df3ae8414104f3710c064014e323e45aade Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 13:50:52 -0700 Subject: [PATCH 14/19] cleanup: consistent interface{} and any --- staging/src/k8s.io/apiserver/pkg/cel/common/equality.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index dd47341b597..952134e6d19 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -73,7 +73,7 @@ func NewCorrelatedObject(new, old interface{}, schema Schema) *CorrelatedObject // // If listType is map, creates a map representation of the list using the designated // map-keys and caches it for future calls. -func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) any { +func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) interface{} { oldAsList, ok := r.OldValue.([]interface{}) if !ok { return nil From abb68591afd30cf263b0d6bb2942f9693eb420d7 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 13:54:53 -0700 Subject: [PATCH 15/19] cleanup: clarify correlatedOldValueForChildAtNewIndex comment --- staging/src/k8s.io/apiserver/pkg/cel/common/equality.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index 952134e6d19..c6bba114a9a 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -68,11 +68,14 @@ func NewCorrelatedObject(new, old interface{}, schema Schema) *CorrelatedObject } // If oldValue is not a list, returns nil -// If oldValue is a list takes mapType into account and attempts to find the -// old value with the same index or key, depending upon the mapType. +// If oldValue is a list, this function takes mapType into account and attempts +// to find the old value with the same index or key // // If listType is map, creates a map representation of the list using the designated -// map-keys and caches it for future calls. +// map-keys, caches it for future calls, and returns the map value, or nil if +// the correlated key is not in the old map +// +// If the list type is not correlatable this funcion returns nil. func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) interface{} { oldAsList, ok := r.OldValue.([]interface{}) if !ok { From 60c90fc0854eb04b95e74d445d88f45c212900fe Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 13:57:55 -0700 Subject: [PATCH 16/19] cleanup: consistently support nil receiver and document --- staging/src/k8s.io/apiserver/pkg/cel/common/equality.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index c6bba114a9a..57e107db891 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -25,6 +25,10 @@ import ( // traversal of the new value. It is also used to cache the results of // DeepEqual comparisons between the old and new values of objects. // +// All receiver functions support being called on `nil` to support ergonomic +// recursive descent. The nil `CorrelatedObject` represents an uncorrelatable +// node in the tree. +// // CorrelatedObject is not thread-safe. It is the responsibility of the caller // to handle concurrency, if any. type CorrelatedObject struct { @@ -135,7 +139,10 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) interf // to validation logic short circuiting and skipping the children, then // this function simply defers to reflect.DeepEqual. func (r *CorrelatedObject) CachedDeepEqual() (res bool) { - if r.comparisonResult != nil { + if r == nil { + // Uncorrelatable node is not considered equal to its old value + return false + } else if r.comparisonResult != nil { return *r.comparisonResult } From 0ed67c9e41dcfc3eef6953ca63082454c189443b Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 14:05:47 -0700 Subject: [PATCH 17/19] cleanup: use swtich in CachedDeepEqual and add more comments --- .../apiserver/pkg/cel/common/equality.go | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index 57e107db891..e08103fc8a2 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -159,9 +159,18 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { oldAsArray, oldIsArray := r.OldValue.([]interface{}) newAsArray, newIsArray := r.Value.([]interface{}) - if oldIsArray != newIsArray { + oldAsMap, oldIsMap := r.OldValue.(map[string]interface{}) + newAsMap, newIsMap := r.Value.(map[string]interface{}) + + // If old and new are not the same type, they are not equal + if (oldIsArray != newIsArray) || oldIsMap != newIsMap { return false - } else if oldIsArray { + } + + // Objects are known to be same type of (map, slice, or primitive) + switch { + case oldIsArray: + // Both arrays case. oldIsArray == newIsArray if len(oldAsArray) != len(newAsArray) { return false } @@ -185,14 +194,8 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { } return true - } - - oldAsMap, oldIsMap := r.OldValue.(map[string]interface{}) - newAsMap, newIsMap := r.Value.(map[string]interface{}) - - if oldIsMap != newIsMap { - return false - } else if oldIsMap { + case oldIsMap: + // Both maps case. oldIsMap == newIsMap if len(oldAsMap) != len(newAsMap) { return false } @@ -210,9 +213,11 @@ func (r *CorrelatedObject) CachedDeepEqual() (res bool) { } return true - } - return reflect.DeepEqual(r.OldValue, r.Value) + default: + // Primitive: use reflect.DeepEqual + return reflect.DeepEqual(r.OldValue, r.Value) + } } // Key returns the child of the receiver with the given name. From d991ed56c29e646c0c5c51ce1ebd2376f34fce28 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 14:11:02 -0700 Subject: [PATCH 18/19] comments: clear up correlateOldValueForChildAtNewIndex godoc --- staging/src/k8s.io/apiserver/pkg/cel/common/equality.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index e08103fc8a2..3046880bb5b 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -71,15 +71,16 @@ func NewCorrelatedObject(new, old interface{}, schema Schema) *CorrelatedObject } } -// If oldValue is not a list, returns nil -// If oldValue is a list, this function takes mapType into account and attempts -// to find the old value with the same index or key +// If OldValue or Value is not a list, or the index is out of bounds of the +// Value list, returns nil +// If oldValue is a list, this considers the x-list-type to decide how to +// correlate old values: // // If listType is map, creates a map representation of the list using the designated // map-keys, caches it for future calls, and returns the map value, or nil if // the correlated key is not in the old map // -// If the list type is not correlatable this funcion returns nil. +// Otherwise, if the list type is not correlatable this funcion returns nil. func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) interface{} { oldAsList, ok := r.OldValue.([]interface{}) if !ok { From fb1fc8b4a72758688d1251278579b2b0ac666fc7 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 13 Oct 2023 14:36:46 -0700 Subject: [PATCH 19/19] ratcheting: disable correlation by index discussion: https://github.com/kubernetes/kubernetes/pull/121118#discussion_r1358865893 --- .../test/integration/ratcheting_test.go | 10 +++++----- .../k8s.io/apiserver/pkg/cel/common/equality.go | 14 +++++--------- .../apiserver/pkg/cel/common/equality_test.go | 12 ++++-------- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index cc88e1d8114..d0f850e97cc 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -1175,7 +1175,7 @@ func TestRatchetingFunctionality(t *testing.T) { }, }, { - Name: "ArrayItems correlate by index", + Name: "ArrayItems do not correlate by index", Operations: []ratchetingTestOperation{ updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ Type: "object", @@ -1246,9 +1246,9 @@ func TestRatchetingFunctionality(t *testing.T) { }, "otherField": "hello world", }}, - // (This test shows an array can be correlated by index with its old value) - applyPatchOperation{ - "add new, valid fields to elements of the array, ratcheting unchanged old fields within the array elements by correlating by index", + // (This test shows an array cannpt be correlated by index with its old value) + expectError{applyPatchOperation{ + "add new, valid fields to elements of the array, failing to ratchet unchanged old fields within the array elements by correlating by index due to atomic list", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "values": []interface{}{ map[string]interface{}{ @@ -1261,7 +1261,7 @@ func TestRatchetingFunctionality(t *testing.T) { "key2": "valid value", }, }, - }}, + }}}, expectError{ applyPatchOperation{ "reorder the array, preventing index correlation", diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go index 3046880bb5b..a271cae795e 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality.go @@ -114,19 +114,15 @@ func (r *CorrelatedObject) correlateOldValueForChildAtNewIndex(index int) interf // of value // (would allow you to add/remove items from sets with ratcheting but not change them) return nil + case "": + fallthrough case "atomic": - // Atomic lists are not correlatable by item + // Atomic lists are the default are not correlatable by item // Ratcheting is not available on a per-index basis return nil default: - // Correlate by-index by default. - // - // Cannot correlate an out-of-bounds index - if len(oldAsList) <= index { - return nil - } - - return oldAsList[index] + // Unrecognized list type. Assume non-correlatable. + return nil } } diff --git a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go index 8300d0a7f74..50c5146ae45 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/common/equality_test.go @@ -142,16 +142,14 @@ func TestCorrelation(t *testing.T) { OldValue: "b", }, { - Name: "Basic Index", + Name: "Atomic Array not correlatable", RootObject: mustUnstructured(`[a, b]`), RootOldObject: mustUnstructured(`[a, b]`), Schema: mustSchema(` items: type: string `), - KeyPath: []interface{}{1}, - NewValue: "b", - OldValue: "b", + KeyPath: []interface{}{1}, }, { Name: "Added Key Not In Old Object", @@ -187,7 +185,7 @@ func TestCorrelation(t *testing.T) { KeyPath: []interface{}{2}, }, { - Name: "Changed Index In Old Object", + Name: "Changed Index In Old Object not correlatable", RootObject: []interface{}{ "a", "b", @@ -200,9 +198,7 @@ func TestCorrelation(t *testing.T) { items: type: string `), - KeyPath: []interface{}{1}, - NewValue: "b", - OldValue: "oldB", + KeyPath: []interface{}{1}, }, { Name: "Changed Index In Nested Old Object",