From c7239f71a8666570a920c59b280b080402b3dd0a Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Mon, 22 Jun 2020 12:03:49 -0700 Subject: [PATCH 01/10] Remove fields when unset from applied configuration if there are no other owners --- merge/leaf_test.go | 9 ++------- merge/update.go | 10 +++++----- typed/remove.go | 7 +++---- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/merge/leaf_test.go b/merge/leaf_test.go index 344bef11..c38aa990 100644 --- a/merge/leaf_test.go +++ b/merge/leaf_test.go @@ -345,7 +345,7 @@ func TestUpdateLeaf(t *testing.T) { ), }, }, - "apply_twice_dangling": { + "apply_twice_remove": { Ops: []Operation{ Apply{ Manager: "default", @@ -365,9 +365,7 @@ func TestUpdateLeaf(t *testing.T) { }, }, Object: ` - numeric: 1 string: "new string" - bool: false `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ @@ -380,7 +378,7 @@ func TestUpdateLeaf(t *testing.T) { ), }, }, - "apply_twice_dangling_different_version": { + "apply_twice_remove_different_version": { Ops: []Operation{ Apply{ Manager: "default", @@ -400,9 +398,7 @@ func TestUpdateLeaf(t *testing.T) { }, }, Object: ` - numeric: 1 string: "new string" - bool: false `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ @@ -462,7 +458,6 @@ func TestUpdateLeaf(t *testing.T) { }, }, Object: ` - string: "string" `, APIVersion: "v1", Managed: fieldpath.ManagedFields{}, diff --git a/merge/update.go b/merge/update.go index 3c2a4690..281ed8d0 100644 --- a/merge/update.go +++ b/merge/update.go @@ -197,7 +197,7 @@ func shallowCopyManagers(managers fieldpath.ManagedFields) fieldpath.ManagedFiel return newManagers } -// prune will remove a list or map item, iff: +// prune will remove a field, list or map item, iff: // * applyingManager applied it last time // * applyingManager didn't apply it this time // * no other applier claims to manage it @@ -225,7 +225,7 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel return s.Converter.Convert(pruned, managers[applyingManager].APIVersion()) } -// addBackOwnedItems adds back any list and map items that were removed by prune, +// addBackOwnedItems adds back any fields, list and map items that were removed by prune, // but other appliers (or the current applier's new config) claim to own. func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFields fieldpath.ManagedFields, applyingManager string) (*typed.TypedValue, error) { var err error @@ -266,9 +266,9 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie return pruned, nil } -// addBackDanglingItems makes sure that the only items removed by prune are items that were -// previously owned by the currently applying manager. This will add back unowned items and items -// which are owned by Updaters that shouldn't be removed. +// addBackDanglingItems makes sure that the fields list and map items removed by prune were +// previously owned by the currently applying manager. This will add back fields list and map items +// that are unowned or that are owned by Updaters and shouldn't be removed. func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet fieldpath.VersionedSet) (*typed.TypedValue, error) { convertedPruned, err := s.Converter.Convert(pruned, lastSet.APIVersion()) if err != nil { diff --git a/typed/remove.go b/typed/remove.go index f30e02a6..cfa6365a 100644 --- a/typed/remove.go +++ b/typed/remove.go @@ -95,10 +95,9 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { fieldType := t.ElementType if ft, ok := fieldTypes[k]; ok { fieldType = ft - } else { - if w.toRemove.Has(path) { - return true - } + } + if w.toRemove.Has(path) { + return true } if subset := w.toRemove.WithPrefix(pe); !subset.Empty() { val = removeItemsWithSchema(val, subset, w.schema, fieldType) From 173a95eb58d6893b7bb44f17fe784307322e35f8 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Thu, 25 Jun 2020 23:37:57 -0700 Subject: [PATCH 02/10] Add field unsetting tests --- merge/multiple_appliers_test.go | 342 ++++++++++++++++++++++++++++++++ typed/typed.go | 5 + 2 files changed, 347 insertions(+) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 0571ae50..c44e70d5 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -27,6 +27,7 @@ import ( . "sigs.k8s.io/structured-merge-diff/v3/internal/fixture" "sigs.k8s.io/structured-merge-diff/v3/merge" "sigs.k8s.io/structured-merge-diff/v3/typed" + "sigs.k8s.io/structured-merge-diff/v3/value" ) func TestMultipleAppliersSet(t *testing.T) { @@ -243,6 +244,300 @@ func TestMultipleAppliersSet(t *testing.T) { } } +var structMultiversionParser = func() Parser { + parser, err := typed.NewParser(`types: +- name: v1 + map: + fields: + - name: struct + type: + namedType: struct + - name: version + type: + scalar: string +- name: struct + map: + fields: + - name: name + type: + scalar: string + - name: scalarField_v1 + type: + scalar: string + - name: complexField_v1 + type: + namedType: complex +- name: complex + map: + fields: + - name: name + type: + scalar: string +- name: v2 + map: + fields: + - name: struct + type: + namedType: struct_v2 + - name: version + type: + scalar: string +- name: struct_v2 + map: + fields: + - name: name + type: + scalar: string + - name: scalarField_v2 + type: + scalar: string + - name: complexField_v2 + type: + namedType: complex_v2 +- name: complex_v2 + map: + fields: + - name: name + type: + scalar: string +- name: v3 + map: + fields: + - name: struct + type: + namedType: struct_v3 + - name: version + type: + scalar: string +- name: struct_v3 + map: + fields: + - name: name + type: + scalar: string + - name: scalarField_v3 + type: + scalar: string + - name: complexField_v3 + type: + namedType: complex_v3 +- name: complex_v3 + map: + fields: + - name: name + type: + scalar: string +`) + if err != nil { + panic(err) + } + return parser +}() + +func TestMultipleAppliersFieldUnsetting(t *testing.T) { + versions := []fieldpath.APIVersion{"v1", "v2", "v3"} + for _, v1 := range versions { + for _, v2 := range versions { + for _, v3 := range versions { + t.Run(fmt.Sprintf("%s-%s-%s", v1, v2, v3), func(t *testing.T) { + testMultipleAppliersFieldUnsetting(t, v1, v2, v3) + }) + } + } + } +} + +func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVersion) { + tests := map[string]TestCase{ + "unset_scalar_sole_owner": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v1)), + }, + Apply{ + Manager: "apply-one", + APIVersion: v2, + Object: ` + struct: + name: a + `, + }, + }, + Object: ` + struct: + name: a + `, + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v2, + false, + ), + }, + }, + "unset_scalar_shared_owner": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v1)), + }, + Apply{ + Manager: "apply-two", + APIVersion: v2, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + scalarField_%s: a + `, v2)), + }, + Apply{ + Manager: "apply-one", + APIVersion: v3, + Object: ` + struct: + name: a + `, + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v3, + true, + ), + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("struct", fmt.Sprintf("scalarField_%s", v2)), + ), + v2, + false, + ), + }, + }, + "unset_complex_sole_owner": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + complexField_%s: + name: b + `, v1)), + }, + Apply{ + Manager: "apply-one", + APIVersion: v2, + Object: ` + struct: + name: a + `, + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + complexField_%s: null + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v2, + false, + ), + }, + }, + "unset_complex_shared_owner": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + complexField_%s: + name: b + `, v1)), + }, + Apply{ + Manager: "apply-two", + APIVersion: v2, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + complexField_%s: + name: b + `, v2)), + }, + Apply{ + Manager: "apply-one", + APIVersion: v3, + Object: ` + struct: + name: a + `, + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + complexField_%s: + name: b + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v3, + false, + ), + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("struct", fmt.Sprintf("complexField_%s", v2), "name"), + ), + v2, + false, + ), + }, + }, + } + + converter := renamingConverter{structMultiversionParser} + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.TestWithConverter(structMultiversionParser, converter); err != nil { + t.Fatal(err) + } + }) + } +} + func TestMultipleAppliersNestedType(t *testing.T) { tests := map[string]TestCase{ "remove_one_keep_one_with_two_sub_items": { @@ -1041,6 +1336,53 @@ func (r repeatingConverter) IsMissingVersionError(err error) bool { return err == missingVersionError } +// renamingConverter renames fields by substituting the version suffix of the field name. E.g. +// converting a map with a field named "name_v1" from v1 to v2 renames the field to "name_v2". +// Fields without a version suffix are not converted; they are the same in all versions. +// When parsing, this converter will look for the type by using the APIVersion of the +// object it's trying to parse. If trying to parse a "v1" object, a corresponding "v1" type +// should exist in the schema of the provided parser. +type renamingConverter struct { + parser Parser +} + +// Convert implements merge.Converter +func (r renamingConverter) Convert(v *typed.TypedValue, version fieldpath.APIVersion) (*typed.TypedValue, error) { + inVersion := fieldpath.APIVersion(*v.TypeRef().NamedType) + outType := r.parser.Type(string(version)) + return outType.FromUnstructured(renameFields(v.AsValue(), string(inVersion), string(version))) +} + +func renameFields(v value.Value, oldSuffix, newSuffix string) interface{} { + if v.IsMap() { + out := map[string]interface{}{} + v.AsMap().Iterate(func(key string, value value.Value) bool { + if strings.HasSuffix(key, oldSuffix) { + out[strings.TrimSuffix(key, oldSuffix)+newSuffix] = renameFields(value, oldSuffix, newSuffix) + } else { + out[key] = renameFields(value, oldSuffix, newSuffix) + } + return true + }) + return out + } + if v.IsList() { + var out []interface{} + ri := v.AsList().Range() + for ri.Next() { + _, v := ri.Item() + out = append(out, renameFields(v, oldSuffix, newSuffix)) + } + return out + } + return v.Unstructured() +} + +// Convert implements merge.Converter +func (r renamingConverter) IsMissingVersionError(err error) bool { + return err == missingVersionError +} + func BenchmarkMultipleApplierRecursiveRealConversion(b *testing.B) { test := TestCase{ Ops: []Operation{ diff --git a/typed/typed.go b/typed/typed.go index 4aa9a238..b8fea9e7 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -61,6 +61,11 @@ type TypedValue struct { schema *schema.Schema } +// TypeRef is the type of the value. +func (tv TypedValue) TypeRef() schema.TypeRef { + return tv.typeRef +} + // AsValue removes the type from the TypedValue and only keeps the value. func (tv TypedValue) AsValue() value.Value { return tv.value From e96f1eba498dc73d5323e0ecad976318f829bd09 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Tue, 30 Jun 2020 13:43:02 -0400 Subject: [PATCH 03/10] Apply suggestion Co-authored-by: Antoine Pelisse --- merge/leaf_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/merge/leaf_test.go b/merge/leaf_test.go index c38aa990..57b6dad3 100644 --- a/merge/leaf_test.go +++ b/merge/leaf_test.go @@ -378,6 +378,42 @@ func TestUpdateLeaf(t *testing.T) { ), }, }, + "update_apply_omits": { + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 2 + `, + }, + Update{ + Manager: "controller", + APIVersion: "v1", + Object: ` + numeric: 1 + `, + }, + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ``, + }, + }, + Object: ` + numeric: 1 + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "controller": fieldpath.NewVersionedSet( + _NS( + _P("numeric"), + ), + "v1", + false, + ), + }, + }, "apply_twice_remove_different_version": { Ops: []Operation{ Apply{ From 95a763d7d11de32eab94edd01839614066ebe668 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Tue, 30 Jun 2020 14:58:55 -0700 Subject: [PATCH 04/10] Change merge rules so that updaters to share ownership with appliers --- merge/multiple_appliers_test.go | 179 +++++++++++++++++++++++++++++++- merge/update.go | 10 +- 2 files changed, 180 insertions(+), 9 deletions(-) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index c44e70d5..4b0f88b5 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -384,7 +384,7 @@ func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVe ), }, }, - "unset_scalar_shared_owner": { + "unset_scalar_shared_with_applier": { Ops: []Operation{ Apply{ Manager: "apply-one", @@ -435,6 +435,104 @@ func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVe ), }, }, + "unset_scalar_shared_with_updater": { + Ops: []Operation{ + Update{ + Manager: "updater", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v1)), + }, + Apply{ + Manager: "applier", + APIVersion: v2, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v2)), + }, + Apply{ + Manager: "applier", + APIVersion: v3, + Object: ` + struct: + name: a + `, + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "updater": fieldpath.NewVersionedSet( + _NS( + _P("struct"), + _P("struct", "name"), + _P("struct", fmt.Sprintf("scalarField_%s", v1)), + ), + v1, + false, + ), + "applier": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v3, + true, + ), + }, + }, + "updater_claims_field": { + Ops: []Operation{ + Apply{ + Manager: "applier", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v1)), + }, + Update{ + Manager: "updater", + APIVersion: v2, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: b + `, v2)), + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: b + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "updater": fieldpath.NewVersionedSet( + _NS( + _P("struct", fmt.Sprintf("scalarField_%s", v2)), + ), + v2, + false, + ), + "applier": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v1, + true, + ), + }, + }, "unset_complex_sole_owner": { Ops: []Operation{ Apply{ @@ -472,7 +570,7 @@ func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVe ), }, }, - "unset_complex_shared_owner": { + "unset_complex_shared_with_applier": { Ops: []Operation{ Apply{ Manager: "apply-one", @@ -1206,7 +1304,7 @@ func TestMultipleAppliersRealConversion(t *testing.T) { ), }, }, - "appliers_remove_from_controller_real_conversion": { + "applier_updater_shared_ownership_real_conversion": { Ops: []Operation{ Update{ Manager: "controller", @@ -1242,6 +1340,8 @@ func TestMultipleAppliersRealConversion(t *testing.T) { Object: ` mapOfMapsRecursive: aaa: + bbb: + ccc: ccc: `, APIVersion: "v3", @@ -1250,6 +1350,8 @@ func TestMultipleAppliersRealConversion(t *testing.T) { _NS( _P("mapOfMapsRecursive"), _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "a", "b", "c"), ), "v1", false, @@ -1275,6 +1377,77 @@ func TestMultipleAppliersRealConversion(t *testing.T) { } } +func TestMultipleAppliersFieldRenameConversions(t *testing.T) { + versions := []fieldpath.APIVersion{"v1", "v2", "v3"} + for _, v1 := range versions { + for _, v2 := range versions { + for _, v3 := range versions { + t.Run(fmt.Sprintf("%s-%s-%s", v1, v2, v3), func(t *testing.T) { + testMultipleAppliersFieldRenameConversions(t, v1, v2, v3) + }) + } + } + } +} + +func testMultipleAppliersFieldRenameConversions(t *testing.T, v1, v2, v3 fieldpath.APIVersion) { + tests := map[string]TestCase{ + "updater_claims_field": { + Ops: []Operation{ + Apply{ + Manager: "applier", + APIVersion: v1, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: a + `, v1)), + }, + Update{ + Manager: "updater", + APIVersion: v2, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: b + `, v2)), + }, + }, + Object: typed.YAMLObject(fmt.Sprintf(` + struct: + name: a + scalarField_%s: b + `, v3)), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "updater": fieldpath.NewVersionedSet( + _NS( + _P("struct", fmt.Sprintf("scalarField_%s", v2)), + ), + v2, + false, + ), + "applier": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v1, + true, + ), + }, + }, + } + + converter := renamingConverter{structMultiversionParser} + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.TestWithConverter(structMultiversionParser, converter); err != nil { + t.Fatal(err) + } + }) + } +} + // repeatingConverter repeats a single letterkey v times, where v is the version. type repeatingConverter struct { parser Parser diff --git a/merge/update.go b/merge/update.go index 281ed8d0..c410a9ab 100644 --- a/merge/update.go +++ b/merge/update.go @@ -226,17 +226,15 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel } // addBackOwnedItems adds back any fields, list and map items that were removed by prune, -// but other appliers (or the current applier's new config) claim to own. +// but other appliers or updaters (or the current applier's new config) claim to own. func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFields fieldpath.ManagedFields, applyingManager string) (*typed.TypedValue, error) { var err error managedAtVersion := map[fieldpath.APIVersion]*fieldpath.Set{} for _, managerSet := range managedFields { - if managerSet.Applied() { - if _, ok := managedAtVersion[managerSet.APIVersion()]; !ok { - managedAtVersion[managerSet.APIVersion()] = fieldpath.NewSet() - } - managedAtVersion[managerSet.APIVersion()] = managedAtVersion[managerSet.APIVersion()].Union(managerSet.Set()) + if _, ok := managedAtVersion[managerSet.APIVersion()]; !ok { + managedAtVersion[managerSet.APIVersion()] = fieldpath.NewSet() } + managedAtVersion[managerSet.APIVersion()] = managedAtVersion[managerSet.APIVersion()].Union(managerSet.Set()) } for version, managed := range managedAtVersion { merged, err = s.Converter.Convert(merged, version) From 660a7a31ceba35533f86863bab1ec7d3baa65b9e Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 1 Jul 2020 11:31:08 -0700 Subject: [PATCH 05/10] Add and fix tests to match improved update merging --- merge/multiple_appliers_test.go | 68 ++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 4b0f88b5..fcd1e8a9 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -1304,8 +1304,19 @@ func TestMultipleAppliersRealConversion(t *testing.T) { ), }, }, - "applier_updater_shared_ownership_real_conversion": { + "appliers_remove_from_controller_real_conversion": { + // Ensures that an applier can delete associative map items it created after a controller + // modifies them. Ops: []Operation{ + Apply{ + Manager: "apply", + Object: ` + mapOfMapsRecursive: + aaa: + bbb: + `, + APIVersion: "v3", + }, Update{ Manager: "controller", Object: ` @@ -1337,6 +1348,59 @@ func TestMultipleAppliersRealConversion(t *testing.T) { APIVersion: "v3", }, }, + Object: ` + mapOfMapsRecursive: + aaa: + ccc: + `, + APIVersion: "v3", + Managed: fieldpath.ManagedFields{ + "apply": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "aaa"), + _P("mapOfMapsRecursive", "ccc"), + ), + "v3", + false, + ), + }, + }, + "applier_updater_shared_ownership_real_conversion": { + // Ensures that when an updater creates maps that they are not deleted when + // an applier shares ownership in them and then later removes them from its applied + // configuration + Ops: []Operation{ + Update{ + Manager: "updater", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + `, + APIVersion: "v1", + }, + Apply{ + Manager: "apply", + Object: ` + mapOfMapsRecursive: + aa: + bb: + cc: + dd: + `, + APIVersion: "v2", + }, + Apply{ + Manager: "apply", + Object: ` + mapOfMapsRecursive: + aaa: + ccc: + `, + APIVersion: "v3", + }, + }, Object: ` mapOfMapsRecursive: aaa: @@ -1346,7 +1410,7 @@ func TestMultipleAppliersRealConversion(t *testing.T) { `, APIVersion: "v3", Managed: fieldpath.ManagedFields{ - "controller": fieldpath.NewVersionedSet( + "updater": fieldpath.NewVersionedSet( _NS( _P("mapOfMapsRecursive"), _P("mapOfMapsRecursive", "a"), From 80350483d1ff4281c6524b77389bd2d42cf97542 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 25 Mar 2021 14:50:02 -0700 Subject: [PATCH 06/10] Add missing TypedValue.Schema function --- typed/typed.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/typed/typed.go b/typed/typed.go index b8fea9e7..ae4d83f6 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 35bfbd98c1eb37b845641123ca770ed7d1ddd6c4 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Fri, 22 Jan 2021 16:35:20 -0800 Subject: [PATCH 07/10] Create NestedSet to populate set branches along with the leaves --- fieldpath/set.go | 51 ++++++++++++++++++++++++++ fieldpath/set_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/fieldpath/set.go b/fieldpath/set.go index f280a2fc..873b6b24 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -19,6 +19,8 @@ package fieldpath import ( "sort" "strings" + + "sigs.k8s.io/structured-merge-diff/v3/schema" ) // Set identifies a set of fields. @@ -94,6 +96,30 @@ func (s *Set) Difference(s2 *Set) *Set { } } +// EnsureNamedFieldsAreMembers returns a Set that contains all the +// fields in s, as well as all the named fields that are typically not +// included. For example, a set made of "a.b.c" will end-up also owning +// "a" if it's a named fields but not "a.b" if it's a map. +func (s *Set) EnsureNamedFieldsAreMembers(sc *schema.Schema, tr schema.TypeRef) *Set { + members := PathElementSet{ + members: make(sortedPathElements, 0, s.Members.Size()+len(s.Children.members)), + } + atom, _ := sc.Resolve(tr) + members.members = append(members.members, s.Members.members...) + for _, node := range s.Children.members { + // Only insert named fields. + if node.pathElement.FieldName != nil && atom.Map != nil { + if _, has := atom.Map.FindField(*node.pathElement.FieldName); has { + members.Insert(node.pathElement) + } + } + } + return &Set{ + Members: members, + Children: *s.Children.EnsureNamedFieldsAreMembers(sc, tr), + } +} + // Size returns the number of members of the set. func (s *Set) Size() int { return s.Members.Size() + s.Children.Size() @@ -333,6 +359,31 @@ func (s *SetNodeMap) Difference(s2 *Set) *SetNodeMap { return out } +// EnsureNamedFieldsAreMembers returns a set that contains all the named fields along with the leaves. +func (s *SetNodeMap) EnsureNamedFieldsAreMembers(sc *schema.Schema, tr schema.TypeRef) *SetNodeMap { + out := make(sortedSetNode, 0, s.Size()) + atom, _ := sc.Resolve(tr) + for _, member := range s.members { + tr := schema.TypeRef{} + if member.pathElement.FieldName != nil && atom.Map != nil { + tr = atom.Map.ElementType + if sf, ok := atom.Map.FindField(*member.pathElement.FieldName); ok { + tr = sf.Type + } + } else if member.pathElement.Key != nil && atom.List != nil { + tr = atom.List.ElementType + } + out = append(out, setNode{ + pathElement: member.pathElement, + set: member.set.EnsureNamedFieldsAreMembers(sc, tr), + }) + } + + return &SetNodeMap{ + members: 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..97bd8edd 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -21,6 +21,9 @@ import ( "fmt" "math/rand" "testing" + + "gopkg.in/yaml.v2" + "sigs.k8s.io/structured-merge-diff/v3/schema" ) type randomPathAlphabet []PathElement @@ -447,6 +450,86 @@ func TestSetIntersectionDifference(t *testing.T) { }) } +var nestedSchema = func() (*schema.Schema, schema.TypeRef) { + sc := &schema.Schema{} + name := "type" + err := yaml.Unmarshal([]byte(`types: +- name: type + map: + elementType: + namedType: type + fields: + - name: named + type: + namedType: type + - name: list + type: + list: + elementRelationShip: associative + keys: ["name"] + elementType: + namedType: type + - name: value + type: + scalar: numeric +`), &sc) + if err != nil { + panic(err) + } + return sc, schema.TypeRef{NamedType: &name} +} + +var _P = MakePathOrDie + +func TestEnsureNamedFieldsAreMembers(t *testing.T) { + table := []struct { + set, expected *Set + }{ + { + set: NewSet(_P("named", "named", "value")), + expected: NewSet( + _P("named", "named", "value"), + _P("named", "named"), + _P("named"), + ), + }, + { + set: NewSet(_P("named", "a", "named", "value"), _P("a", "named", "value"), _P("a", "b", "value")), + expected: NewSet( + _P("named", "a", "named", "value"), + _P("named", "a", "named"), + _P("named"), + _P("a", "named", "value"), + _P("a", "named"), + _P("a", "b", "value"), + ), + }, + { + set: NewSet(_P("named", "list", KeyByFields("name", "a"), "named", "a", "value")), + expected: NewSet( + _P("named", "list", KeyByFields("name", "a"), "named", "a", "value"), + _P("named", "list", KeyByFields("name", "a"), "named"), + _P("named", "list"), + _P("named"), + ), + }, + } + + for _, test := range table { + t.Run(fmt.Sprintf("%v", test.set), func(t *testing.T) { + got := test.set.EnsureNamedFieldsAreMembers(nestedSchema()) + if !got.Equals(test.expected) { + t.Errorf("expected %v, got %v (missing: %v/superfluous: %v)", + test.expected, + got, + test.expected.Difference(got), + got.Difference(test.expected), + ) + } + }) + } +} + func TestSetNodeMapIterate(t *testing.T) { set := &SetNodeMap{} toAdd := 5 From 6611ec93ea137d498749b0a8e514e951e4b01ded Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Tue, 2 Feb 2021 16:21:23 -0800 Subject: [PATCH 08/10] Use NestedSet to also remove structs owned by no-one --- merge/update.go | 12 +++++++++--- typed/tofieldset.go | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/merge/update.go b/merge/update.go index c410a9ab..f4fa5ed8 100644 --- a/merge/update.go +++ b/merge/update.go @@ -213,7 +213,8 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel return nil, fmt.Errorf("failed to convert merged object to last applied version: %v", err) } - pruned := convertedMerged.RemoveItems(lastSet.Set()) + sc, tr := convertedMerged.Schema(), convertedMerged.TypeRef() + pruned := convertedMerged.RemoveItems(lastSet.Set().EnsureNamedFieldsAreMembers(sc, tr)) pruned, err = s.addBackOwnedItems(convertedMerged, pruned, managers, applyingManager) if err != nil { return nil, fmt.Errorf("failed add back owned items: %v", err) @@ -259,7 +260,8 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie if err != nil { return nil, fmt.Errorf("failed to create field set from pruned object at version %v: %v", version, err) } - pruned = merged.RemoveItems(mergedSet.Difference(prunedSet.Union(managed))) + sc, tr := merged.Schema(), merged.TypeRef() + pruned = merged.RemoveItems(mergedSet.EnsureNamedFieldsAreMembers(sc, tr).Difference(prunedSet.EnsureNamedFieldsAreMembers(sc, tr).Union(managed.EnsureNamedFieldsAreMembers(sc, tr)))) } return pruned, nil } @@ -283,5 +285,9 @@ func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet if err != nil { return nil, fmt.Errorf("failed to create field set from merged object in last applied version: %v", err) } - return merged.RemoveItems(mergedSet.Difference(prunedSet).Intersection(lastSet.Set())), nil + sc, tr := merged.Schema(), merged.TypeRef() + prunedSet = prunedSet.EnsureNamedFieldsAreMembers(sc, tr) + mergedSet = mergedSet.EnsureNamedFieldsAreMembers(sc, tr) + last := lastSet.Set().EnsureNamedFieldsAreMembers(sc, tr) + return merged.RemoveItems(mergedSet.Difference(prunedSet).Intersection(last)), nil } diff --git a/typed/tofieldset.go b/typed/tofieldset.go index b3c4ff00..0800a9d0 100644 --- a/typed/tofieldset.go +++ b/typed/tofieldset.go @@ -137,7 +137,9 @@ func (v *toFieldSetWalker) visitMapItems(t *schema.Map, m value.Map) (errs Valid v2 := v.prepareDescent(pe, tr) v2.value = val errs = append(errs, v2.toFieldSet()...) - if _, ok := t.FindField(key); !ok { + if val.IsNull() || (val.IsMap() && val.AsMap().Length() == 0) { + v2.set.Insert(v2.path) + } else if _, ok := t.FindField(key); !ok { v2.set.Insert(v2.path) } v.finishDescent(v2) From 928ab00c59d8b10b897386f9f25d3aaba33a676b Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Tue, 2 Feb 2021 16:19:31 -0800 Subject: [PATCH 09/10] Fix tests broken by new mechanism --- merge/multiple_appliers_test.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index fcd1e8a9..c879f302 100644 --- a/merge/multiple_appliers_test.go +++ b/merge/multiple_appliers_test.go @@ -554,11 +554,10 @@ func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVe `, }, }, - Object: typed.YAMLObject(fmt.Sprintf(` + Object: typed.YAMLObject(` struct: name: a - complexField_%s: null - `, v3)), + `), APIVersion: v3, Managed: fieldpath.ManagedFields{ "apply-one": fieldpath.NewVersionedSet( @@ -1072,6 +1071,13 @@ func TestMultipleAppliersNestedType(t *testing.T) { `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive"), + ), + "v4", + false, + ), "apply-two": fieldpath.NewVersionedSet( _NS( _P("mapOfMapsRecursive", "a"), @@ -1285,6 +1291,13 @@ func TestMultipleAppliersRealConversion(t *testing.T) { `, APIVersion: "v4", Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive"), + ), + "v4", + false, + ), "apply-two": fieldpath.NewVersionedSet( _NS( _P("mapOfMapsRecursive", "aa"), From 65b8000c1924563af26786e2e76fac5265142aa1 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Tue, 22 Sep 2020 15:11:03 -0700 Subject: [PATCH 10/10] Show how null is inserted when struct is removed --- merge/nested_test.go | 199 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) diff --git a/merge/nested_test.go b/merge/nested_test.go index 0bb1ef5a..ce7308e8 100644 --- a/merge/nested_test.go +++ b/merge/nested_test.go @@ -44,6 +44,18 @@ var nestedTypeParser = func() Parser { - name: mapOfMapsRecursive type: namedType: mapOfMapsRecursive + - name: struct + type: + namedType: struct +- name: struct + map: + fields: + - name: name + type: + scalar: string + - name: value + type: + scalar: number - name: listOfLists list: elementType: @@ -500,6 +512,193 @@ func TestUpdateNestedType(t *testing.T) { ), }, }, + "struct_apply_remove_all": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + value: 1 + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + `, + APIVersion: "v1", + }, + }, + Object: ` + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{}, + }, + "struct_apply_remove_dangling": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + struct: + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + `, + APIVersion: "v1", + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + _NS( + _P("struct"), + ), + "v1", + true, + ), + }, + }, + "struct_apply_update_remove_all": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + `, + APIVersion: "v1", + }, + Update{ + Manager: "controller", + Object: ` + struct: + name: a + value: 1 + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + value: 1 + `, + APIVersion: "v1", + }, + "struct_apply_update_dict_dangling": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + `, + APIVersion: "v1", + }, + Update{ + Manager: "controller", + Object: ` + struct: + name: a + value: 1 + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + struct: {} + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + value: 1 + `, + APIVersion: "v1", + }, + "struct_apply_update_dict_null": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + `, + APIVersion: "v1", + }, + Update{ + Manager: "controller", + Object: ` + struct: + name: a + value: 1 + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + struct: + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + value: 1 + `, + APIVersion: "v1", + }, + "struct_apply_update_took_over": { + Ops: []Operation{ + Apply{ + Manager: "default", + Object: ` + struct: + name: a + `, + APIVersion: "v1", + }, + Update{ + Manager: "controller", + Object: ` + struct: + name: b + value: 1 + `, + APIVersion: "v1", + }, + Apply{ + Manager: "default", + Object: ` + struct: + `, + APIVersion: "v1", + }, + }, + Object: ` + struct: + name: b + value: 1 + `, + APIVersion: "v1", + }, } for name, test := range tests {