From 90b911a03faec6b2e482a2ee3dc4d5517ab804ea Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 18 Sep 2020 21:51:38 -0700 Subject: [PATCH 1/2] Add atomic<->granular schema change fixup --- internal/fixture/state.go | 27 +++ merge/schema_change_test.go | 252 ++++++++++++++++++++++++++ merge/update.go | 47 +++-- typed/reconcile_schema.go | 297 +++++++++++++++++++++++++++++++ typed/reconcile_schema_test.go | 311 +++++++++++++++++++++++++++++++++ typed/typed.go | 5 + 6 files changed, 929 insertions(+), 10 deletions(-) create mode 100644 merge/schema_change_test.go create mode 100644 typed/reconcile_schema.go create mode 100644 typed/reconcile_schema_test.go diff --git a/internal/fixture/state.go b/internal/fixture/state.go index b7c09946..b91c069e 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -385,6 +385,33 @@ func (f UpdateObject) preprocess(parser Parser) (Operation, error) { return f, nil } +// ChangeParser is a type of operation. It simulates making changes a schema without versioning +// the schema. This can be used to test the behavior of making backward compatible schema changes, +// e.g. setting "elementRelationship: atomic" on an existing struct. It also may be used to ensure +// that backward incompatible changes are detected appropriately. +type ChangeParser struct { + Parser *typed.Parser +} + +var _ Operation = &ChangeParser{} + +func (cs ChangeParser) run(state *State) error { + state.Parser = cs.Parser + // Swap the schema in for use with the live object so it merges. + // If the schema is incompatible, this will fail validation. + + liveWithNewSchema, err := typed.AsTyped(state.Live.AsValue(), &cs.Parser.Schema, state.Live.TypeRef()) + if err != nil { + return err + } + state.Live = liveWithNewSchema + return nil +} + +func (cs ChangeParser) preprocess(_ Parser) (Operation, error) { + return cs, nil +} + // TestCase is the list of operations that need to be run, as well as // the object/managedfields as they are supposed to look like after all // the operations have been successfully performed. If Object/Managed is diff --git a/merge/schema_change_test.go b/merge/schema_change_test.go new file mode 100644 index 00000000..82dd5b76 --- /dev/null +++ b/merge/schema_change_test.go @@ -0,0 +1,252 @@ +/* +Copyright 2018 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 merge_test + +import ( + "testing" + + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v4/internal/fixture" + "sigs.k8s.io/structured-merge-diff/v4/merge" + "sigs.k8s.io/structured-merge-diff/v4/typed" +) + +var structParser = func() *typed.Parser { + oldParser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: struct + type: + namedType: struct +- name: struct + map: + fields: + - name: numeric + type: + scalar: numeric + - name: string + type: + scalar: string`) + if err != nil { + panic(err) + } + return oldParser +}() + +var structWithAtomicParser = func() *typed.Parser { + newParser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: struct + type: + namedType: struct +- name: struct + map: + fields: + - name: numeric + type: + scalar: numeric + - name: string + type: + scalar: string + elementRelationship: atomic`) + if err != nil { + panic(err) + } + return newParser +}() + +func TestGranularToAtomicSchemaChanges(t *testing.T) { + tests := map[string]TestCase{ + "to-atomic": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + struct: + numeric: 1 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: structWithAtomicParser}, + Apply{ + Manager: "two", + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + Conflicts: merge.Conflicts{ + merge.Conflict{Manager: "one", Path: _P("struct")}, + }, + }, + ForceApply{ + Manager: "two", + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "two": fieldpath.NewVersionedSet(_NS( + _P("struct"), + ), "v1", true), + }, + }, + "to-atomic-owner-with-no-child-fields": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + struct: + numeric: 1 + `, + APIVersion: "v1", + }, + ForceApply{ // take the only child field from manager "one" + Manager: "two", + Object: ` + struct: + numeric: 2 + `, + APIVersion: "v1", + }, + ChangeParser{Parser: structWithAtomicParser}, + Apply{ + Manager: "three", + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + Conflicts: merge.Conflicts{ + // We expect no conflict with "one" because we do not allow a manager + // to own a map without owning any of the children. + merge.Conflict{Manager: "two", Path: _P("struct")}, + }, + }, + ForceApply{ + Manager: "two", + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + string: "string" + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "two": fieldpath.NewVersionedSet(_NS( + _P("struct"), + ), "v1", true), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(structParser); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestAtomicToGranularSchemaChanges(t *testing.T) { + tests := map[string]TestCase{ + "to-granular": { + Ops: []Operation{ + Apply{ + Manager: "one", + Object: ` + struct: + numeric: 1 + string: "a" + `, + APIVersion: "v1", + }, + Apply{ + Manager: "two", + Object: ` + struct: + string: "b" + `, + APIVersion: "v1", + Conflicts: merge.Conflicts{ + merge.Conflict{Manager: "one", Path: _P("struct")}, + }, + }, + ChangeParser{Parser: structParser}, + Apply{ + Manager: "two", + Object: ` + struct: + string: "b" + `, + APIVersion: "v1", + Conflicts: merge.Conflicts{ + merge.Conflict{Manager: "one", Path: _P("struct", "string")}, + }, + }, + ForceApply{ + Manager: "two", + Object: ` + struct: + string: "b" + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + numeric: 1 + string: "b" + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "one": fieldpath.NewVersionedSet(_NS( + _P("struct"), + _P("struct", "numeric"), + ), "v1", true), + "two": fieldpath.NewVersionedSet(_NS( + _P("struct", "string"), + ), "v1", true), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(structWithAtomicParser); err != nil { + t.Fatal(err) + } + }) + } +} diff --git a/merge/update.go b/merge/update.go index e648d8a2..cddeadb3 100644 --- a/merge/update.go +++ b/merge/update.go @@ -122,13 +122,16 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa // this is a CREATE call). func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) { var err error + managers, err = s.reconcileManagedFieldsWithSchemaChanges(liveObject, managers) + if err != nil { + return nil, fieldpath.ManagedFields{}, err + } if s.enableUnions { newObject, err = liveObject.NormalizeUnions(newObject) if err != nil { return nil, fieldpath.ManagedFields{}, err } } - managers = shallowCopyManagers(managers) managers, compare, err := s.update(liveObject, newObject, version, managers, manager, true) if err != nil { return nil, fieldpath.ManagedFields{}, err @@ -152,13 +155,45 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp return newObject, managers, nil } +// reconcileManagedFieldsWithSchemaChanges reconciles the managed fields with any changes to the +// object's schema since the managed fields were written. +// +// Supports: +// - changing types from atomic to granular +// - changing types from granular to atomic +func (s *Updater) reconcileManagedFieldsWithSchemaChanges(liveObject *typed.TypedValue, managers fieldpath.ManagedFields) (fieldpath.ManagedFields, error) { + result := fieldpath.ManagedFields{} + for manager, versionedSet := range managers { + tv, err := s.Converter.Convert(liveObject, versionedSet.APIVersion()) + if s.Converter.IsMissingVersionError(err) { // okay to skip, obsolete versions will be deleted automatically anyway + continue + } + if err != nil { + return nil, err + } + reconciled, err := typed.ReconcileFieldSetWithSchema(versionedSet.Set(), tv) + if err != nil { + return nil, err + } + if reconciled != nil { + result[manager] = fieldpath.NewVersionedSet(reconciled, versionedSet.APIVersion(), versionedSet.Applied()) + } else { + result[manager] = versionedSet + } + } + return result, nil +} + // Apply should be called when Apply is run, given the current object as // well as the configuration that is applied. This will merge the object // and return it. If the object hasn't changed, nil is returned (the // managers can still have changed though). func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) { - managers = shallowCopyManagers(managers) var err error + managers, err = s.reconcileManagedFieldsWithSchemaChanges(liveObject, managers) + if err != nil { + return nil, fieldpath.ManagedFields{}, err + } if s.enableUnions { configObject, err = configObject.NormalizeUnionsApply(configObject) if err != nil { @@ -204,14 +239,6 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel return newObject, managers, nil } -func shallowCopyManagers(managers fieldpath.ManagedFields) fieldpath.ManagedFields { - newManagers := fieldpath.ManagedFields{} - for manager, set := range managers { - newManagers[manager] = set - } - return newManagers -} - // prune will remove a field, list or map item, iff: // * applyingManager applied it last time // * applyingManager didn't apply it this time diff --git a/typed/reconcile_schema.go b/typed/reconcile_schema.go new file mode 100644 index 00000000..2e71ff87 --- /dev/null +++ b/typed/reconcile_schema.go @@ -0,0 +1,297 @@ +/* +Copyright 2018 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 typed + +import ( + "fmt" + "sync" + + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "sigs.k8s.io/structured-merge-diff/v4/schema" +) + +var fmPool = sync.Pool{ + New: func() interface{} { return &reconcileWithSchemaWalker{} }, +} + +func (v *reconcileWithSchemaWalker) finished() { + v.fieldSet = nil + v.schema = nil + v.value = nil + v.typeRef = schema.TypeRef{} + v.path = nil + v.toRemove = nil + v.toAdd = nil + fmPool.Put(v) +} + +type reconcileWithSchemaWalker struct { + value *TypedValue // root of the live object + schema *schema.Schema // root of the live schema + + // state of node being visited by walker + fieldSet *fieldpath.Set + typeRef schema.TypeRef + path fieldpath.Path + isAtomic bool + + // the accumulated diff to perform to apply reconciliation + toRemove *fieldpath.Set // paths to remove recursively + toAdd *fieldpath.Set // paths to add after any removals + + // Allocate only as many walkers as needed for the depth by storing them here. + spareWalkers *[]*reconcileWithSchemaWalker +} + +func (v *reconcileWithSchemaWalker) prepareDescent(pe fieldpath.PathElement, tr schema.TypeRef) *reconcileWithSchemaWalker { + if v.spareWalkers == nil { + // first descent. + v.spareWalkers = &[]*reconcileWithSchemaWalker{} + } + var v2 *reconcileWithSchemaWalker + if n := len(*v.spareWalkers); n > 0 { + v2, *v.spareWalkers = (*v.spareWalkers)[n-1], (*v.spareWalkers)[:n-1] + } else { + v2 = &reconcileWithSchemaWalker{} + } + *v2 = *v + v2.typeRef = tr + v2.path = append(v.path, pe) + v2.value = v.value + return v2 +} + +func (v *reconcileWithSchemaWalker) finishDescent(v2 *reconcileWithSchemaWalker) { + v2.fieldSet = nil + v2.schema = nil + v2.value = nil + v2.typeRef = schema.TypeRef{} + if cap(v2.path) < 20 { // recycle slices that do not have unexpectedly high capacity + v2.path = v2.path[:0] + } else { + v2.path = nil + } + + // merge any accumulated changes into parent walker + if v2.toRemove != nil { + if v.toRemove == nil { + v.toRemove = v2.toRemove + } else { + v.toRemove = v.toRemove.Union(v2.toRemove) + } + } + if v2.toAdd != nil { + if v.toAdd == nil { + v.toAdd = v2.toAdd + } else { + v.toAdd = v.toAdd.Union(v2.toAdd) + } + } + v2.toRemove = nil + v2.toAdd = nil + + // if the descent caused a realloc, ensure that we reuse the buffer + // for the next sibling. + *v.spareWalkers = append(*v.spareWalkers, v2) +} + +// ReconcileFieldSetWithSchema reconciles the a field set with any changes to the +//// object's schema since the field set was written. Returns the reconciled field set, or nil of +// no changes were made to the field set. +// +// Supports: +// - changing types from atomic to granular +// - changing types from granular to atomic +func ReconcileFieldSetWithSchema(fieldset *fieldpath.Set, tv *TypedValue) (*fieldpath.Set, error) { + v := fmPool.Get().(*reconcileWithSchemaWalker) + v.fieldSet = fieldset + v.value = tv + + v.schema = tv.schema + v.typeRef = tv.typeRef + + // We don't reconcile deduced types, which are primarily for use by unstructured CRDs. Deduced + // types do not support atomic or granular tags. Nor does the dynamic schema deduction + // interact well with the reconcile logic. + if v.schema == DeducedParseableType.Schema { + return nil, nil + } + + defer v.finished() + errs := v.reconcile() + + if len(errs) > 0 { + return nil, fmt.Errorf("errors reconciling field set with schema: %s", errs.Error()) + } + + // If there are any accumulated changes, apply them + if v.toAdd != nil || v.toRemove != nil { + out := v.fieldSet + if v.toRemove != nil { + out = out.RecursiveDifference(v.toRemove) + } + if v.toAdd != nil { + out = out.Union(v.toAdd) + } + return out, nil + } + return nil, nil +} + +func (v *reconcileWithSchemaWalker) reconcile() (errs ValidationErrors) { + a, ok := v.schema.Resolve(v.typeRef) + if !ok { + errs = append(errs, errorf("could not resolve %v", v.typeRef)...) + return + } + return handleAtom(a, v.typeRef, v) +} + +func (v *reconcileWithSchemaWalker) doScalar(_ *schema.Scalar) (errs ValidationErrors) { + return errs +} + +func (v *reconcileWithSchemaWalker) visitListItems(t *schema.List, element *fieldpath.Set) (errs ValidationErrors) { + handleElement := func(pe fieldpath.PathElement, isMember bool) { + var hasChildren bool + v2 := v.prepareDescent(pe, t.ElementType) + v2.fieldSet, hasChildren = element.Children.Get(pe) + v2.isAtomic = isMember && !hasChildren + errs = append(errs, v2.reconcile()...) + v.finishDescent(v2) + } + element.Children.Iterate(func(pe fieldpath.PathElement) { + if element.Members.Has(pe) { + return + } + handleElement(pe, false) + }) + element.Members.Iterate(func(pe fieldpath.PathElement) { + handleElement(pe, true) + }) + return nil +} + +func (v *reconcileWithSchemaWalker) doList(t *schema.List) (errs ValidationErrors) { + // reconcile lists changed from granular to atomic + if !v.isAtomic && t.ElementRelationship == schema.Atomic { + v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields + v.toAdd = fieldpath.NewSet(v.path) // add the root of the atomic + return errs + } + // reconcile lists changed from atomic to granular + if v.isAtomic && t.ElementRelationship == schema.Associative { + v.toAdd, errs = buildGranularFieldSet(v.path, v.value) + if errs != nil { + return errs + } + } + if v.fieldSet != nil { + errs = v.visitListItems(t, v.fieldSet) + } + return errs +} + +func (v *reconcileWithSchemaWalker) visitMapItems(t *schema.Map, element *fieldpath.Set) (errs ValidationErrors) { + handleElement := func(pe fieldpath.PathElement, isMember bool) { + var hasChildren bool + tr, lookupErrs := typeRefAtPath(t, pe) + errs = append(errs, lookupErrs...) + v2 := v.prepareDescent(pe, tr) + v2.fieldSet, hasChildren = element.Children.Get(pe) + v2.isAtomic = isMember && !hasChildren + errs = append(errs, v2.reconcile()...) + v.finishDescent(v2) + } + element.Children.Iterate(func(pe fieldpath.PathElement) { + if element.Members.Has(pe) { + return + } + handleElement(pe, false) + }) + element.Members.Iterate(func(pe fieldpath.PathElement) { + handleElement(pe, true) + }) + + return nil +} + +func (v *reconcileWithSchemaWalker) doMap(t *schema.Map) (errs ValidationErrors) { + // reconcile maps and structs changed from granular to atomic + if !v.isAtomic && t.ElementRelationship == schema.Atomic { + if v.fieldSet != nil && v.fieldSet.Size() > 0 { + v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields + v.toAdd = fieldpath.NewSet(v.path) // add the root of the atomic + } + return errs + } + // reconcile maps changed from atomic to granular + if v.isAtomic && (t.ElementRelationship == schema.Separable || t.ElementRelationship == "") { + v.toAdd, errs = buildGranularFieldSet(v.path, v.value) + if errs != nil { + return errs + } + } + if v.fieldSet != nil { + errs = v.visitMapItems(t, v.fieldSet) + } + return errs +} + +func buildGranularFieldSet(path fieldpath.Path, value *TypedValue) (*fieldpath.Set, ValidationErrors) { + result := fieldpath.NewSet(path) + valueFieldSet, err := value.ToFieldSet() + if err != nil { + return nil, errorf("toFieldSet: %v", err) + } + if listValueFieldSet, ok := fieldSetAtPath(valueFieldSet, path); ok { + valueFieldSet = prefixWithPath(path, listValueFieldSet) + result = result.Union(valueFieldSet) + } + return result, nil +} + +func prefixWithPath(prefix fieldpath.Path, set *fieldpath.Set) *fieldpath.Set { + result := fieldpath.NewSet() + set.Iterate(func(path fieldpath.Path) { + result.Insert(append(prefix.Copy(), path...)) + }) + return result +} + +func fieldSetAtPath(node *fieldpath.Set, path fieldpath.Path) (*fieldpath.Set, bool) { + ok := true + for _, pe := range path { + if node, ok = node.Children.Get(pe); !ok { + break + } + } + return node, ok +} + +func typeRefAtPath(t *schema.Map, pe fieldpath.PathElement) (tr schema.TypeRef, errs ValidationErrors) { + tr = t.ElementType + if pe.FieldName != nil { + if sf, ok := t.FindField(*pe.FieldName); ok { + tr = sf.Type + } + } + if (tr == schema.TypeRef{}) { + errs = append(errs, errorf("field not declared in schema").WithPrefix(pe.String())...) + } + return tr, errs +} diff --git a/typed/reconcile_schema_test.go b/typed/reconcile_schema_test.go new file mode 100644 index 00000000..29d30719 --- /dev/null +++ b/typed/reconcile_schema_test.go @@ -0,0 +1,311 @@ +/* +Copyright 2018 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 typed_test + +import ( + "fmt" + "testing" + + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "sigs.k8s.io/structured-merge-diff/v4/typed" +) + +type reconcileTestCase struct { + name string + rootTypeName string + oldSchema typed.YAMLObject + newSchema typed.YAMLObject + liveObject typed.YAMLObject + oldFields *fieldpath.Set + fixedFields *fieldpath.Set +} + +func granularSchema(version string) typed.YAMLObject { + return typed.YAMLObject(fmt.Sprintf(`types: +- name: %s + map: + fields: + - name: struct + type: + namedType: struct + - name: list + type: + namedType: list + - name: objectList + type: + namedType: objectList + - name: stringMap + type: + namedType: stringMap + - name: unchanged + type: + namedType: unchanged +- name: struct + map: + fields: + - name: numeric + type: + scalar: numeric + - name: string + type: + scalar: string +- name: list + list: + elementType: + scalar: string + elementRelationship: associative +- name: objectList + list: + elementType: + namedType: listItem + elementRelationship: associative + keys: + - keyA + - keyB +- name: listItem + map: + fields: + - name: keyA + type: + scalar: string + - name: keyB + type: + scalar: string + - name: value + type: + scalar: string +- name: stringMap + map: + elementType: + scalar: string +- name: unchanged + map: + fields: + - name: numeric + type: + scalar: numeric +`, version)) +} + +func atomicSchema(version string) typed.YAMLObject { + return typed.YAMLObject(fmt.Sprintf(`types: +- name: %s + map: + fields: + - name: struct + type: + namedType: struct + - name: list + type: + namedType: list + - name: objectList + type: + namedType: objectList + - name: stringMap + type: + namedType: stringMap + - name: unchanged + type: + namedType: unchanged +- name: struct + map: + fields: + - name: numeric + type: + scalar: numeric + - name: string + type: + scalar: string + elementRelationship: atomic +- name: list + list: + elementType: + scalar: string + elementRelationship: atomic +- name: objectList + list: + elementType: + namedType: listItem + elementRelationship: atomic +- name: listItem + map: + fields: + - name: keyA + type: + scalar: string + - name: keyB + type: + scalar: string + - name: value + type: + scalar: string +- name: stringMap + map: + elementType: + scalar: string + elementRelationship: atomic +- name: unchanged + map: + fields: + - name: numeric + type: + scalar: numeric +`, version)) +} + +const basicLiveObject = typed.YAMLObject(` +struct: + numeric: 1 + string: "two" +list: + - one + - two +objectList: + - keyA: a1 + keyB: b1 + value: v1 + - keyA: a2 + keyB: b2 + value: v2 +stringMap: + key1: value1 +unchanged: + numeric: 10 +`) + +var reconcileCases = []reconcileTestCase{{ + name: "granular-to-atomic", + rootTypeName: "v1", + oldSchema: granularSchema("v1"), + newSchema: atomicSchema("v1"), + liveObject: basicLiveObject, + oldFields: _NS( + _P("struct", "numeric"), + _P("list", _V("one")), + _P("stringMap", "key1"), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "value"), + _P("unchanged", "numeric"), + ), + fixedFields: _NS( + _P("struct"), + _P("list"), + _P("objectList"), + _P("stringMap"), + _P("unchanged", "numeric"), + ), +}, { + name: "atomic-to-granular", + rootTypeName: "v1", + oldSchema: atomicSchema("v1"), + newSchema: granularSchema("v1"), + liveObject: basicLiveObject, + oldFields: _NS( + _P("struct"), + _P("list"), + _P("objectList"), + _P("stringMap"), + _P("unchanged", "numeric"), + ), + fixedFields: _NS( + _P("struct"), + _P("struct", "numeric"), + _P("struct", "string"), + _P("list"), + _P("list", _V("one")), + _P("list", _V("two")), + _P("objectList"), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1")), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "value"), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyA"), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyB"), + _P("objectList", _KBF("keyA", "a2", "keyB", "b2")), + _P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "value"), + _P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyA"), + _P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyB"), + _P("stringMap"), + _P("stringMap", "key1"), + _P("unchanged", "numeric"), + ), +}, { + name: "no-change-granular", + rootTypeName: "v1", + oldSchema: granularSchema("v1"), + newSchema: granularSchema("v1"), + liveObject: basicLiveObject, + oldFields: _NS( + _P("struct", "numeric"), + _P("list", _V("one")), + _P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "value"), + _P("stringMap", "key1"), + _P("unchanged", "numeric"), + ), + fixedFields: nil, // indicates no change +}, { + name: "no-change-atomic", + rootTypeName: "v1", + oldSchema: atomicSchema("v1"), + newSchema: atomicSchema("v1"), + liveObject: basicLiveObject, + oldFields: _NS( + _P("struct"), + _P("list"), + _P("objectList"), + _P("unchanged", "numeric"), + ), + fixedFields: nil, // indicates no change +}} + +func TestReconcileFieldSetWithSchema(t *testing.T) { + for _, tt := range reconcileCases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + tt.testReconcileCase(t) + }) + } +} + +func (tt reconcileTestCase) testReconcileCase(t *testing.T) { + parser, err := typed.NewParser(tt.newSchema) + if err != nil { + t.Fatalf("failed to create schema: %v", err) + } + pt := parser.Type(tt.rootTypeName) + liveObject, err := pt.FromYAML(tt.liveObject) + if err != nil { + t.Fatalf("failed to parse/validate yaml: %v\n%v", err, tt.liveObject) + } + + fixed, err := typed.ReconcileFieldSetWithSchema(tt.oldFields, liveObject) + if err != nil { + t.Fatalf("fixup errors: %v", err) + } + if tt.fixedFields == nil { + if fixed != nil { + t.Fatalf("expected fieldset to be null but got\n:%s", fixed.String()) + } + return + } + + if fixed == nil { + t.Fatalf("expected fieldset to be\n:%s\n:but got null", tt.fixedFields.String()) + } + + if !fixed.Equals(tt.fixedFields) { + t.Errorf("expected fieldset:\n%s\n:but got\n:%s", tt.fixedFields.String(), fixed.String()) + } +} diff --git a/typed/typed.go b/typed/typed.go index 1a99159a..54766f6e 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -71,6 +71,11 @@ func (tv TypedValue) AsValue() value.Value { return tv.value } +// Schema gets the schema from the TypedValue. +func (tv TypedValue) Schema() *schema.Schema { + return tv.schema +} + // Validate returns an error with a list of every spec violation. func (tv TypedValue) Validate() error { w := tv.walker() From 9a22ec66c022a6acb6fe516a82f2fbb76f3fdfc8 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Tue, 29 Sep 2020 12:15:37 -0700 Subject: [PATCH 2/2] Apply feedback --- merge/update.go | 58 +++++++++++++++++++-------------------- typed/reconcile_schema.go | 28 +++++++++---------- 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/merge/update.go b/merge/update.go index cddeadb3..73e0ec73 100644 --- a/merge/update.go +++ b/merge/update.go @@ -155,35 +155,6 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp return newObject, managers, nil } -// reconcileManagedFieldsWithSchemaChanges reconciles the managed fields with any changes to the -// object's schema since the managed fields were written. -// -// Supports: -// - changing types from atomic to granular -// - changing types from granular to atomic -func (s *Updater) reconcileManagedFieldsWithSchemaChanges(liveObject *typed.TypedValue, managers fieldpath.ManagedFields) (fieldpath.ManagedFields, error) { - result := fieldpath.ManagedFields{} - for manager, versionedSet := range managers { - tv, err := s.Converter.Convert(liveObject, versionedSet.APIVersion()) - if s.Converter.IsMissingVersionError(err) { // okay to skip, obsolete versions will be deleted automatically anyway - continue - } - if err != nil { - return nil, err - } - reconciled, err := typed.ReconcileFieldSetWithSchema(versionedSet.Set(), tv) - if err != nil { - return nil, err - } - if reconciled != nil { - result[manager] = fieldpath.NewVersionedSet(reconciled, versionedSet.APIVersion(), versionedSet.Applied()) - } else { - result[manager] = versionedSet - } - } - return result, nil -} - // Apply should be called when Apply is run, given the current object as // well as the configuration that is applied. This will merge the object // and return it. If the object hasn't changed, nil is returned (the @@ -327,3 +298,32 @@ func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet } return merged.RemoveItems(mergedSet.Difference(prunedSet).Intersection(lastSet.Set())), nil } + +// reconcileManagedFieldsWithSchemaChanges reconciles the managed fields with any changes to the +// object's schema since the managed fields were written. +// +// Supports: +// - changing types from atomic to granular +// - changing types from granular to atomic +func (s *Updater) reconcileManagedFieldsWithSchemaChanges(liveObject *typed.TypedValue, managers fieldpath.ManagedFields) (fieldpath.ManagedFields, error) { + result := fieldpath.ManagedFields{} + for manager, versionedSet := range managers { + tv, err := s.Converter.Convert(liveObject, versionedSet.APIVersion()) + if s.Converter.IsMissingVersionError(err) { // okay to skip, obsolete versions will be deleted automatically anyway + continue + } + if err != nil { + return nil, err + } + reconciled, err := typed.ReconcileFieldSetWithSchema(versionedSet.Set(), tv) + if err != nil { + return nil, err + } + if reconciled != nil { + result[manager] = fieldpath.NewVersionedSet(reconciled, versionedSet.APIVersion(), versionedSet.Applied()) + } else { + result[manager] = versionedSet + } + } + return result, nil +} diff --git a/typed/reconcile_schema.go b/typed/reconcile_schema.go index 2e71ff87..506b037c 100644 --- a/typed/reconcile_schema.go +++ b/typed/reconcile_schema.go @@ -183,7 +183,7 @@ func (v *reconcileWithSchemaWalker) visitListItems(t *schema.List, element *fiel element.Members.Iterate(func(pe fieldpath.PathElement) { handleElement(pe, true) }) - return nil + return errs } func (v *reconcileWithSchemaWalker) doList(t *schema.List) (errs ValidationErrors) { @@ -227,7 +227,7 @@ func (v *reconcileWithSchemaWalker) visitMapItems(t *schema.Map, element *fieldp handleElement(pe, true) }) - return nil + return errs } func (v *reconcileWithSchemaWalker) doMap(t *schema.Map) (errs ValidationErrors) { @@ -253,26 +253,19 @@ func (v *reconcileWithSchemaWalker) doMap(t *schema.Map) (errs ValidationErrors) } func buildGranularFieldSet(path fieldpath.Path, value *TypedValue) (*fieldpath.Set, ValidationErrors) { - result := fieldpath.NewSet(path) + valueFieldSet, err := value.ToFieldSet() if err != nil { return nil, errorf("toFieldSet: %v", err) } - if listValueFieldSet, ok := fieldSetAtPath(valueFieldSet, path); ok { - valueFieldSet = prefixWithPath(path, listValueFieldSet) - result = result.Union(valueFieldSet) + result := fieldpath.NewSet(path) + resultAtPath := descendToPath(result, path) + if valueFieldSetAtPath, ok := fieldSetAtPath(valueFieldSet, path); ok { + *resultAtPath = *valueFieldSetAtPath } return result, nil } -func prefixWithPath(prefix fieldpath.Path, set *fieldpath.Set) *fieldpath.Set { - result := fieldpath.NewSet() - set.Iterate(func(path fieldpath.Path) { - result.Insert(append(prefix.Copy(), path...)) - }) - return result -} - func fieldSetAtPath(node *fieldpath.Set, path fieldpath.Path) (*fieldpath.Set, bool) { ok := true for _, pe := range path { @@ -283,6 +276,13 @@ func fieldSetAtPath(node *fieldpath.Set, path fieldpath.Path) (*fieldpath.Set, b return node, ok } +func descendToPath(node *fieldpath.Set, path fieldpath.Path) *fieldpath.Set { + for _, pe := range path { + node = node.Children.Descend(pe) + } + return node +} + func typeRefAtPath(t *schema.Map, pe fieldpath.PathElement) (tr schema.TypeRef, errs ValidationErrors) { tr = t.ElementType if pe.FieldName != nil {