Skip to content

Commit 1042d0a

Browse files
jpbetzjpbetz
committed
Add atomic<->granular schema change fixup
1 parent b84068c commit 1042d0a

File tree

6 files changed

+939
-15
lines changed

6 files changed

+939
-15
lines changed

‎internal/fixture/state.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,33 @@ func (f UpdateObject) preprocess(parser Parser) (Operation, error) {
385385
return f, nil
386386
}
387387

388+
// ChangeParser is a type of operation. It simulates making changes a schema without versioning
389+
// the schema. This can be used to test the behavior of making backward compatible schema changes,
390+
// e.g. setting "elementRelationship: atomic" on an existing struct. It also may be used to ensure
391+
// that backward incompatible changes are detected appropriately.
392+
type ChangeParser struct {
393+
Parser *typed.Parser
394+
}
395+
396+
var _ Operation = &ChangeParser{}
397+
398+
func (cs ChangeParser) run(state *State) error {
399+
state.Parser = cs.Parser
400+
// Swap the schema in for use with the live object so it merges.
401+
// If the schema is incompatible, this will fail validation.
402+
403+
liveWithNewSchema, err := typed.AsTyped(state.Live.AsValue(), &cs.Parser.Schema, state.Live.TypeRef())
404+
if err != nil {
405+
return err
406+
}
407+
state.Live = liveWithNewSchema
408+
return nil
409+
}
410+
411+
func (cs ChangeParser) preprocess(_ Parser) (Operation, error) {
412+
return cs, nil
413+
}
414+
388415
// TestCase is the list of operations that need to be run, as well as
389416
// the object/managedfields as they are supposed to look like after all
390417
// the operations have been successfully performed. If Object/Managed is

‎merge/schema_change_test.go‎

Lines changed: 252 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,252 @@
1+
/*
2+
Copyright 2018 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package merge_test
18+
19+
import (
20+
"testing"
21+
22+
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
23+
. "sigs.k8s.io/structured-merge-diff/v4/internal/fixture"
24+
"sigs.k8s.io/structured-merge-diff/v4/merge"
25+
"sigs.k8s.io/structured-merge-diff/v4/typed"
26+
)
27+
28+
var structParser = func() *typed.Parser {
29+
oldParser, err := typed.NewParser(`types:
30+
- name: v1
31+
map:
32+
fields:
33+
- name: struct
34+
type:
35+
namedType: struct
36+
- name: struct
37+
map:
38+
fields:
39+
- name: numeric
40+
type:
41+
scalar: numeric
42+
- name: string
43+
type:
44+
scalar: string`)
45+
if err != nil {
46+
panic(err)
47+
}
48+
return oldParser
49+
}()
50+
51+
var structWithAtomicParser = func() *typed.Parser {
52+
newParser, err := typed.NewParser(`types:
53+
- name: v1
54+
map:
55+
fields:
56+
- name: struct
57+
type:
58+
namedType: struct
59+
- name: struct
60+
map:
61+
fields:
62+
- name: numeric
63+
type:
64+
scalar: numeric
65+
- name: string
66+
type:
67+
scalar: string
68+
elementRelationship: atomic`)
69+
if err != nil {
70+
panic(err)
71+
}
72+
return newParser
73+
}()
74+
75+
func TestGranularToAtomicSchemaChanges(t *testing.T) {
76+
tests := map[string]TestCase{
77+
"to-atomic": {
78+
Ops: []Operation{
79+
Apply{
80+
Manager: "one",
81+
Object: `
82+
struct:
83+
numeric: 1
84+
`,
85+
APIVersion: "v1",
86+
},
87+
ChangeParser{Parser: structWithAtomicParser},
88+
Apply{
89+
Manager: "two",
90+
Object: `
91+
struct:
92+
string: "string"
93+
`,
94+
APIVersion: "v1",
95+
Conflicts: merge.Conflicts{
96+
merge.Conflict{Manager: "one", Path: _P("struct")},
97+
},
98+
},
99+
ForceApply{
100+
Manager: "two",
101+
Object: `
102+
struct:
103+
string: "string"
104+
`,
105+
APIVersion: "v1",
106+
},
107+
},
108+
Object: `
109+
struct:
110+
string: "string"
111+
`,
112+
APIVersion: "v1",
113+
Managed: fieldpath.ManagedFields{
114+
"two": fieldpath.NewVersionedSet(_NS(
115+
_P("struct"),
116+
), "v1", true),
117+
},
118+
},
119+
"to-atomic-owner-with-no-child-fields": {
120+
Ops: []Operation{
121+
Apply{
122+
Manager: "one",
123+
Object: `
124+
struct:
125+
numeric: 1
126+
`,
127+
APIVersion: "v1",
128+
},
129+
ForceApply{ // take the only child field from manager "one"
130+
Manager: "two",
131+
Object: `
132+
struct:
133+
numeric: 2
134+
`,
135+
APIVersion: "v1",
136+
},
137+
ChangeParser{Parser: structWithAtomicParser},
138+
Apply{
139+
Manager: "three",
140+
Object: `
141+
struct:
142+
string: "string"
143+
`,
144+
APIVersion: "v1",
145+
Conflicts: merge.Conflicts{
146+
// We expect no conflict with "one" because we do not allow a manager
147+
// to own a map without owning any of the children.
148+
merge.Conflict{Manager: "two", Path: _P("struct")},
149+
},
150+
},
151+
ForceApply{
152+
Manager: "two",
153+
Object: `
154+
struct:
155+
string: "string"
156+
`,
157+
APIVersion: "v1",
158+
},
159+
},
160+
Object: `
161+
struct:
162+
string: "string"
163+
`,
164+
APIVersion: "v1",
165+
Managed: fieldpath.ManagedFields{
166+
"two": fieldpath.NewVersionedSet(_NS(
167+
_P("struct"),
168+
), "v1", true),
169+
},
170+
},
171+
}
172+
173+
for name, test := range tests {
174+
t.Run(name, func(t *testing.T) {
175+
if err := test.Test(structParser); err != nil {
176+
t.Fatal(err)
177+
}
178+
})
179+
}
180+
}
181+
182+
func TestAtomicToGranularSchemaChanges(t *testing.T) {
183+
tests := map[string]TestCase{
184+
"to-granular": {
185+
Ops: []Operation{
186+
Apply{
187+
Manager: "one",
188+
Object: `
189+
struct:
190+
numeric: 1
191+
string: "a"
192+
`,
193+
APIVersion: "v1",
194+
},
195+
Apply{
196+
Manager: "two",
197+
Object: `
198+
struct:
199+
string: "b"
200+
`,
201+
APIVersion: "v1",
202+
Conflicts: merge.Conflicts{
203+
merge.Conflict{Manager: "one", Path: _P("struct")},
204+
},
205+
},
206+
ChangeParser{Parser: structParser},
207+
Apply{
208+
Manager: "two",
209+
Object: `
210+
struct:
211+
string: "b"
212+
`,
213+
APIVersion: "v1",
214+
Conflicts: merge.Conflicts{
215+
merge.Conflict{Manager: "one", Path: _P("struct", "string")},
216+
},
217+
},
218+
ForceApply{
219+
Manager: "two",
220+
Object: `
221+
struct:
222+
string: "b"
223+
`,
224+
APIVersion: "v1",
225+
},
226+
},
227+
Object: `
228+
struct:
229+
numeric: 1
230+
string: "b"
231+
`,
232+
APIVersion: "v1",
233+
Managed: fieldpath.ManagedFields{
234+
"one": fieldpath.NewVersionedSet(_NS(
235+
_P("struct"),
236+
_P("struct", "numeric"),
237+
), "v1", true),
238+
"two": fieldpath.NewVersionedSet(_NS(
239+
_P("struct", "string"),
240+
), "v1", true),
241+
},
242+
},
243+
}
244+
245+
for name, test := range tests {
246+
t.Run(name, func(t *testing.T) {
247+
if err := test.Test(structWithAtomicParser); err != nil {
248+
t.Fatal(err)
249+
}
250+
})
251+
}
252+
}

‎merge/update.go‎

Lines changed: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,16 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
122122
// this is a CREATE call).
123123
func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) {
124124
var err error
125+
managers, err = s.reconcileManagedFields(liveObject, managers)
126+
if err != nil {
127+
return nil, fieldpath.ManagedFields{}, err
128+
}
125129
if s.enableUnions {
126130
newObject, err = liveObject.NormalizeUnions(newObject)
127131
if err != nil {
128132
return nil, fieldpath.ManagedFields{}, err
129133
}
130134
}
131-
managers = shallowCopyManagers(managers)
132135
managers, compare, err := s.update(liveObject, newObject, version, managers, manager, true)
133136
if err != nil {
134137
return nil, fieldpath.ManagedFields{}, err
@@ -141,24 +144,63 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
141144
if ignored == nil {
142145
ignored = fieldpath.NewSet()
143146
}
144-
managers[manager] = fieldpath.NewVersionedSet(
145-
managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed).RecursiveDifference(ignored),
146-
version,
147-
false,
148-
)
147+
managers[manager] = fieldpath.NewVersionedSet(managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed).RecursiveDifference(ignored), version, false)
149148
if managers[manager].Set().Empty() {
150149
delete(managers, manager)
151150
}
152151
return newObject, managers, nil
153152
}
154153

154+
// reconcileManagedFields attempts to fix up the managed fields to be compatible with the live state of
155+
// the schema.
156+
// This is needed when an "unversioned" change are made to a schema that impact the managedFields.
157+
// By "unversionsed" we mean that a specific version of a schema was directly modified (introduction
158+
// of new versions does not require fixup).
159+
//
160+
// Supported schema changes:
161+
// - Changing a type to "atomic": All field manager that managed any granular fields become managers
162+
// of the atomic to ensure a conflict if any of the fields they previously managed are changed.
163+
// Granular managed fields of the struct are removed.
164+
//
165+
// TODO: Changes to add support for:
166+
// - Change a type from "atomic" to "granular", "set" or "map". All field manager that managed the
167+
// atomic become managers of all child fields to ensure a conflict if any child field is changed.
168+
//
169+
// Potential future optimization: Write a SHA-256 hash to managedFields that can be compared
170+
// with the current hash of the live schemas to determine if any fixup is needed.
171+
func (s *Updater) reconcileManagedFields(liveObject *typed.TypedValue, managers fieldpath.ManagedFields) (fieldpath.ManagedFields, error) {
172+
result := fieldpath.ManagedFields{}
173+
for manager, versionedSet := range managers {
174+
tv, err := s.Converter.Convert(liveObject, versionedSet.APIVersion())
175+
if s.Converter.IsMissingVersionError(err) { // okay to skip, obsolete versions will be deleted automatically anyway
176+
continue
177+
}
178+
if err != nil {
179+
return nil, err
180+
}
181+
fixedFieldSet, err := typed.ReconcileFieldSetWithSchema(versionedSet.Set(), tv)
182+
if err != nil {
183+
return nil, err
184+
}
185+
if fixedFieldSet != nil {
186+
result[manager] = fieldpath.NewVersionedSet(fixedFieldSet, versionedSet.APIVersion(), versionedSet.Applied())
187+
} else {
188+
result[manager] = versionedSet
189+
}
190+
}
191+
return result, nil
192+
}
193+
155194
// Apply should be called when Apply is run, given the current object as
156195
// well as the configuration that is applied. This will merge the object
157196
// and return it. If the object hasn't changed, nil is returned (the
158197
// managers can still have changed though).
159198
func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) {
160-
managers = shallowCopyManagers(managers)
161199
var err error
200+
managers, err = s.reconcileManagedFields(liveObject, managers)
201+
if err != nil {
202+
return nil, fieldpath.ManagedFields{}, err
203+
}
162204
if s.enableUnions {
163205
configObject, err = configObject.NormalizeUnionsApply(configObject)
164206
if err != nil {
@@ -204,14 +246,6 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel
204246
return newObject, managers, nil
205247
}
206248

207-
func shallowCopyManagers(managers fieldpath.ManagedFields) fieldpath.ManagedFields {
208-
newManagers := fieldpath.ManagedFields{}
209-
for manager, set := range managers {
210-
newManagers[manager] = set
211-
}
212-
return newManagers
213-
}
214-
215249
// prune will remove a field, list or map item, iff:
216250
// * applyingManager applied it last time
217251
// * applyingManager didn't apply it this time

0 commit comments

Comments
 (0)