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 diff --git a/merge/leaf_test.go b/merge/leaf_test.go index 344bef11..57b6dad3 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,43 @@ func TestUpdateLeaf(t *testing.T) { ), }, }, - "apply_twice_dangling_different_version": { + "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{ Manager: "default", @@ -400,9 +434,7 @@ func TestUpdateLeaf(t *testing.T) { }, }, Object: ` - numeric: 1 string: "new string" - bool: false `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ @@ -462,7 +494,6 @@ func TestUpdateLeaf(t *testing.T) { }, }, Object: ` - string: "string" `, APIVersion: "v1", Managed: fieldpath.ManagedFields{}, diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go index 0571ae50..c879f302 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,397 @@ 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_with_applier": { + 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_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{ + 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(` + struct: + name: a + `), + APIVersion: v3, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("struct", "name"), + ), + v2, + false, + ), + }, + }, + "unset_complex_shared_with_applier": { + 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": { @@ -679,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"), @@ -892,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"), @@ -912,7 +1318,18 @@ func TestMultipleAppliersRealConversion(t *testing.T) { }, }, "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: ` @@ -951,10 +1368,67 @@ func TestMultipleAppliersRealConversion(t *testing.T) { `, APIVersion: "v3", Managed: fieldpath.ManagedFields{ - "controller": fieldpath.NewVersionedSet( + "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: + bbb: + ccc: + ccc: + `, + APIVersion: "v3", + Managed: fieldpath.ManagedFields{ + "updater": fieldpath.NewVersionedSet( _NS( _P("mapOfMapsRecursive"), _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "a", "b", "c"), ), "v1", false, @@ -980,6 +1454,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 @@ -1041,6 +1586,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/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 { diff --git a/merge/update.go b/merge/update.go index 3c2a4690..f4fa5ed8 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 @@ -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) @@ -225,18 +226,16 @@ 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, -// but other appliers (or the current applier's new config) claim to own. +// addBackOwnedItems adds back any fields, list and map items that were removed by prune, +// 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) @@ -261,14 +260,15 @@ 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 } -// 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 { @@ -285,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/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) 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) diff --git a/typed/typed.go b/typed/typed.go index 4aa9a238..ae4d83f6 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -61,11 +61,21 @@ 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 } +// 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()