diff --git a/fieldpath/set.go b/fieldpath/set.go index f280a2fc..029c2b60 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -94,6 +94,22 @@ func (s *Set) Difference(s2 *Set) *Set { } } +// RecursiveDifference returns a Set containing elements which: +// * appear in s +// * do not appear in s2 +// +// Compared to a regular difference, +// this removes every field **and its children** from s that is contained in s2. +// +// For example, with s containing `a.b.c` and s2 containing `a.b`, +// a RecursiveDifference will result in `a`, as the entire node `a.b` gets removed. +func (s *Set) RecursiveDifference(s2 *Set) *Set { + return &Set{ + Members: *s.Members.Difference(&s2.Members), + Children: *s.Children.RecursiveDifference(s2), + } +} + // Size returns the number of members of the set. func (s *Set) Size() int { return s.Members.Size() + s.Children.Size() @@ -333,6 +349,48 @@ func (s *SetNodeMap) Difference(s2 *Set) *SetNodeMap { return out } +// RecursiveDifference returns a SetNodeMap with members that appear in s but not in s2. +// +// Compared to a regular difference, +// this removes every field **and its children** from s that is contained in s2. +// +// For example, with s containing `a.b.c` and s2 containing `a.b`, +// a RecursiveDifference will result in `a`, as the entire node `a.b` gets removed. +func (s *SetNodeMap) RecursiveDifference(s2 *Set) *SetNodeMap { + out := &SetNodeMap{} + + i, j := 0, 0 + for i < len(s.members) && j < len(s2.Children.members) { + if s.members[i].pathElement.Less(s2.Children.members[j].pathElement) { + if !s2.Members.Has(s.members[i].pathElement) { + out.members = append(out.members, setNode{pathElement: s.members[i].pathElement, set: s.members[i].set}) + } + i++ + } else { + if !s2.Children.members[j].pathElement.Less(s.members[i].pathElement) { + if !s2.Members.Has(s.members[i].pathElement) { + diff := s.members[i].set.RecursiveDifference(s2.Children.members[j].set) + if !diff.Empty() { + out.members = append(out.members, setNode{pathElement: s.members[i].pathElement, set: diff}) + } + } + i++ + } + j++ + } + } + + if i < len(s.members) { + for _, c := range s.members[i:] { + if !s2.Members.Has(c.pathElement) { + out.members = append(out.members, c) + } + } + } + + return out +} + // Iterate calls f for each PathElement in the set. func (s *SetNodeMap) Iterate(f func(PathElement)) { for _, n := range s.members { diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index fb06dd05..8eee60c1 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -131,6 +131,12 @@ func BenchmarkFieldSet(b *testing.B) { randOperand().Difference(randOperand()) } }) + b.Run(fmt.Sprintf("recursive-difference-%v", here.size), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + randOperand().RecursiveDifference(randOperand()) + } + }) } } @@ -447,6 +453,94 @@ func TestSetIntersectionDifference(t *testing.T) { }) } +func TestSetDifference(t *testing.T) { + table := []struct { + name string + a *Set + b *Set + expectDifference *Set + expectRecursiveDifference *Set + }{ + { + name: "removes simple path", + a: NewSet(MakePathOrDie("a")), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet(), + expectRecursiveDifference: NewSet(), + }, + { + name: "removes direct path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a", "b", "c")), + expectDifference: NewSet(), + expectRecursiveDifference: NewSet(), + }, + { + name: "only removes matching child", + a: NewSet( + MakePathOrDie("a", "b", "c"), + MakePathOrDie("b", "b", "c"), + ), + b: NewSet(MakePathOrDie("a", "b", "c")), + expectDifference: NewSet(MakePathOrDie("b", "b", "c")), + expectRecursiveDifference: NewSet(MakePathOrDie("b", "b", "c")), + }, + { + name: "does not remove parent of specific path", + a: NewSet( + MakePathOrDie("a"), + ), + b: NewSet(MakePathOrDie("a", "aa")), + expectDifference: NewSet(MakePathOrDie("a")), + expectRecursiveDifference: NewSet(MakePathOrDie("a")), + }, + { + name: "RecursiveDifference removes nested path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet(MakePathOrDie("a", "b", "c")), + expectRecursiveDifference: NewSet(), + }, + { + name: "RecursiveDifference only removes nested path for matching children", + a: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + b: NewSet(MakePathOrDie("a", "aa")), + expectDifference: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + expectRecursiveDifference: NewSet(MakePathOrDie("a", "ab", "aba")), + }, + { + name: "RecursiveDifference removes all matching children", + a: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + expectRecursiveDifference: NewSet(), + }, + } + + for _, c := range table { + t.Run(c.name, func(t *testing.T) { + if result := c.a.Difference(c.b); !result.Equals(c.expectDifference) { + t.Fatalf("Difference expected: \n%v\n, got: \n%v\n", c.expectDifference, result) + } + if result := c.a.RecursiveDifference(c.b); !result.Equals(c.expectRecursiveDifference) { + t.Fatalf("RecursiveDifference expected: \n%v\n, got: \n%v\n", c.expectRecursiveDifference, result) + } + }) + } +} + func TestSetNodeMapIterate(t *testing.T) { set := &SetNodeMap{} toAdd := 5 diff --git a/go.mod b/go.mod index fae371bf..29e44aeb 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/json-iterator/go v1.1.6 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.1 // indirect + github.com/stretchr/testify v1.3.0 // indirect ) go 1.13 diff --git a/go.sum b/go.sum index e0f34a25..bde0aebc 100644 --- a/go.sum +++ b/go.sum @@ -17,5 +17,4 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -sigs.k8s.io/structured-merge-diff v1.0.1 h1:LOs1LZWMsz1xs77Phr/pkB4LFaavH7IVq/3+WTN9XTA= -sigs.k8s.io/structured-merge-diff v1.0.1/go.mod h1:IIgPezJWb76P0hotTxzDbWsMYB8APh18qZnxkomBpxA= +sigs.k8s.io/structured-merge-diff v1.0.2 h1:WiMoyniAVAYm03w+ImfF9IE2G23GLR/SwDnQyaNZvPk= diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 7d93ee1e..c275ce8d 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -405,6 +405,8 @@ type TestCase struct { Managed fieldpath.ManagedFields // Set to true if the test case needs the union behavior enabled. RequiresUnions bool + // IgnoredFields containing the set to ignore for every version + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } // Test runs the test-case using the given parser and a dummy converter. @@ -436,7 +438,7 @@ func (tc TestCase) PreprocessOperations(parser Parser) error { // actually passes.. func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) error { state := State{ - Updater: &merge.Updater{Converter: converter}, + Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields}, Parser: parser, } if tc.RequiresUnions { @@ -456,7 +458,7 @@ func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) // TestWithConverter runs the test-case using the given parser and converter. func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) error { state := State{ - Updater: &merge.Updater{Converter: converter}, + Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields}, Parser: parser, } if tc.RequiresUnions { diff --git a/merge/ignore_test.go b/merge/ignore_test.go new file mode 100644 index 00000000..1853e137 --- /dev/null +++ b/merge/ignore_test.go @@ -0,0 +1,343 @@ +/* +Copyright 2020 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/v3/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v3/internal/fixture" +) + +func TestIgnoredFields(t *testing.T) { + tests := map[string]TestCase{ + "update_does_not_own_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + }, + Object: ` + numeric: 1 + string: "some string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("numeric"), + ), + "v1", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("string"), + ), + }, + }, + "update_does_not_own_deep_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "default", + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + }, + }, + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("numeric"), + ), + "v1", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("obj"), + ), + }, + }, + "apply_does_not_own_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + }, + Object: ` + numeric: 1 + string: "some string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("numeric"), + ), + "v1", + true, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("string"), + ), + }, + }, + "apply_does_not_own_deep_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + }, + }, + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("numeric"), + ), + "v1", + true, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("obj"), + ), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(DeducedParser); err != nil { + t.Fatal("Should fail:", err) + } + }) + } +} + +func TestIgnoredFieldsUsesVersions(t *testing.T) { + tests := map[string]TestCase{ + "does_use_ignored_fields_versions": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + }, + Apply{ + Manager: "apply-two", + Object: ` + mapOfMapsRecursive: + aa: + cc: + dd: + `, + APIVersion: "v2", + }, + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + `, + APIVersion: "v4", + }, + }, + // note that this still contains cccc due to ignored fields not being removed from the update result + Object: ` + mapOfMapsRecursive: + aaaa: + cccc: + dddd: + `, + APIVersion: "v4", + Managed: fieldpath.ManagedFields{ + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "aa"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("mapOfMapsRecursive", "c"), + ), + "v2": _NS( + _P("mapOfMapsRecursive", "cc"), + ), + "v3": _NS( + _P("mapOfMapsRecursive", "ccc"), + ), + "v4": _NS( + _P("mapOfMapsRecursive", "cccc"), + ), + }, + }, + "update_does_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "update-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + }, + Update{ + Manager: "update-two", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + APIVersion: "v2", + }, + }, + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + Managed: fieldpath.ManagedFields{ + "update-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive"), + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "c"), + ), + "v1", + false, + ), + "update-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v2": _NS( + _P("mapOfMapsRecursive", "c"), + ), + }, + }, + "apply_does_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + }, + Apply{ + Manager: "apply-two", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + APIVersion: "v2", + }, + }, + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "c"), + _P("mapOfMapsRecursive", "c", "d"), + ), + "v1", + false, + ), + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v2": _NS( + _P("mapOfMapsRecursive", "c"), + ), + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.TestWithConverter(nestedTypeParser, repeatingConverter{nestedTypeParser}); err != nil { + t.Fatal(err) + } + }) + } +} diff --git a/merge/update.go b/merge/update.go index 3c2a4690..edf8eee3 100644 --- a/merge/update.go +++ b/merge/update.go @@ -30,7 +30,8 @@ type Converter interface { // Updater is the object used to compute updated FieldSets and also // merge the object on Apply. type Updater struct { - Converter Converter + Converter Converter + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set enableUnions bool } @@ -50,7 +51,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa } versions := map[fieldpath.APIVersion]*typed.Comparison{ - version: compare, + version: compare.ExcludeFields(s.IgnoredFields[version]), } for manager, managerSet := range managers { @@ -80,7 +81,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa if err != nil { return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } - versions[managerSet.APIVersion()] = compare + versions[managerSet.APIVersion()] = compare.ExcludeFields(s.IgnoredFields[managerSet.APIVersion()]) } conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added)) @@ -135,8 +136,13 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp if _, ok := managers[manager]; !ok { managers[manager] = fieldpath.NewVersionedSet(fieldpath.NewSet(), version, false) } + + ignored := s.IgnoredFields[version] + if ignored == nil { + ignored = fieldpath.NewSet() + } managers[manager] = fieldpath.NewVersionedSet( - managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed), + managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed).RecursiveDifference(ignored), version, false, ) @@ -174,6 +180,15 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err) } + + ignored := s.IgnoredFields[version] + if ignored != nil { + set = set.RecursiveDifference(ignored) + // TODO: is this correct. If we don't remove from lastSet pruning might remove the fields? + if lastSet != nil { + lastSet.Set().RecursiveDifference(ignored) + } + } managers[manager] = fieldpath.NewVersionedSet(set, version, true) newObject, err = s.prune(newObject, managers, manager, lastSet) if err != nil { diff --git a/typed/comparison_test.go b/typed/comparison_test.go new file mode 100644 index 00000000..ef2934d6 --- /dev/null +++ b/typed/comparison_test.go @@ -0,0 +1,76 @@ +package typed_test + +import ( + "testing" + + "sigs.k8s.io/structured-merge-diff/v3/fieldpath" + "sigs.k8s.io/structured-merge-diff/v3/typed" +) + +func TestComparisonExcludeFields(t *testing.T) { + cases := []struct { + name string + Comparison *typed.Comparison + Remove *fieldpath.Set + Expect *typed.Comparison + Fails bool + }{ + { + name: "works on nil set", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + Remove: nil, + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + }, + { + name: "works on empty set", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + Remove: fieldpath.NewSet(), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + }, + { + name: "removes from entire object", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a", "aa")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "ba")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c", "ca")), + }, + Remove: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + fieldpath.MakePathOrDie("b"), + fieldpath.MakePathOrDie("c"), + ), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + c.Comparison.ExcludeFields(c.Remove) + if (!c.Comparison.Added.Equals(c.Expect.Added) || + !c.Comparison.Modified.Equals(c.Expect.Modified) || + !c.Comparison.Removed.Equals(c.Expect.Removed)) != c.Fails { + t.Fatalf("remove expected: \n%v\nremoved:\n%v\ngot:\n%v\n", c.Expect, c.Remove, c.Comparison) + } + }) + } +} diff --git a/typed/typed.go b/typed/typed.go index 4aa9a238..9a7ef9be 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -291,3 +291,15 @@ func (c *Comparison) String() string { } return bld.String() } + +// ExcludeFields fields from the compare recursively removes the fields +// from the entire comparison +func (c *Comparison) ExcludeFields(fields *fieldpath.Set) *Comparison { + if fields == nil || fields.Empty() { + return c + } + c.Removed = c.Removed.RecursiveDifference(fields) + c.Modified = c.Modified.RecursiveDifference(fields) + c.Added = c.Added.RecursiveDifference(fields) + return c +}