Skip to content

Commit c2e0bc4

Browse files
sourcehawkclaude
andauthored
fix(component): stop reporting writes by other controllers as updates (#249)
* fix(component): stop reporting writes by other controllers as updates A component compared the whole object before and after its apply. When another controller added a finalizer, a label or an annotation between the read and the apply, the component recorded an Updated<Kind> event for a change it did not make. The comparison now covers only the fields that the field manager of the component owns before or after the apply, as recorded in managedFields. A changed, added or removed owned field still counts as an update. When the read carries no managedFields, the whole object is compared as before. Fixes #248 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(component): count reordered owned list items as an update A lookup by key finds a list item wherever it sits, so the owned-field comparison reported None when the component only changed the order of its containers. The order of some lists, such as init containers, changes what the object does. The comparison now also compares the relative order of the owned items of each list. Items that only other managers own do not count. The paths of a managedFields entry are expressed in the API version of that entry. When the entries of the component before and after the apply have different versions, a conversion can rename a field, and a path from the old version finds nothing. The comparison now falls back to the whole object in that case. The docs now state both rules, and that a write which sets a field inside an empty object the component applies, such as emptyDir: {}, still counts as a change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 81f613c commit c2e0bc4

11 files changed

Lines changed: 901 additions & 95 deletions

File tree

‎docs/component.md‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -388,12 +388,24 @@ except where the owner is namespace-scoped and the resource is cluster-scoped (s
388388
[Cluster-scoped resources](#cluster-scoped-resources)).
389389

390390
Each apply is classified as `Created`, `Updated` or `None` by comparing the object observed before the patch (read
391-
through the client, usually the informer cache) with the API server's response to it, ignoring `status`,
392-
`managedFields`, `resourceVersion` and `generation`. The classification is passed to the resource's status handler as
393-
its `concepts.ConvergingOperation` and drives the `Created<Kind>` and `Updated<Kind>` events recorded on the owner;
394-
`None` records no event. Because it compares observed cluster state rather than the desired object before and after
395-
mutation, a steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every
396-
reconcile or keeps them across reconciles.
391+
through the client, usually the informer cache) with the API server's response to it. The comparison covers only the
392+
fields that the component's field manager owns before or after the apply, as recorded in `managedFields`. An owned field
393+
that gets a new value, is added, or is removed makes the apply `Updated`. So do owned list items, such as containers,
394+
that change their order. A write by another field manager does not, so a finalizer, label or annotation that another
395+
controller adds between the read and the apply is not reported as an update of the component. The classification is
396+
passed to the resource's status handler as its `concepts.ConvergingOperation` and drives the `Created<Kind>` and
397+
`Updated<Kind>` events recorded on the owner; `None` records no event. Because it compares observed cluster state rather
398+
than the desired object before and after mutation, a steady-state reconcile reports `None` regardless of whether the
399+
operator rebuilds its components on every reconcile or keeps them across reconciles.
400+
401+
The owned fields are not known in three cases: the read returns no `managedFields`, for example because the manager's
402+
cache strips them with `cache.TransformStripManagedFields`, the apply response holds no apply entry for the component's
403+
field manager, or the entries of the component before and after the apply have different API versions, as after the
404+
operator moves to a new version of a custom resource. The comparison then covers the whole object except `status`,
405+
`managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is reported as
406+
`Updated`. The same false `Updated` occurs when the component applies an empty object, such as `emptyDir: {}`, and
407+
another controller sets a field inside it in that window. The component owns such an object as one value, so a field
408+
that another controller adds inside it counts as a change to that value.
397409

398410
### Previewing desired state
399411

‎docs/custom-resource.md‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -360,12 +360,13 @@ seen the spec just applied.
360360
The converge and operational status handlers also receive a `concepts.ConvergingOperation` that says what the apply did:
361361
`Created` when the object did not exist before, `Updated` when the apply changed an existing object, and `None` when it
362362
left the object as it was. The framework decides this by comparing the object observed before the Server-Side Apply
363-
(read through the client, usually the informer cache) with the API server's response, ignoring `status`,
364-
`managedFields`, `resourceVersion` and `generation`. It does not depend on how the desired object was built, so a
365-
wrapper reports `None` on a steady-state reconcile whether the operator keeps its resources across reconciles or
366-
rebuilds them each pass. Use the operation to distinguish `Creating` from `Updating` while `status.observedGeneration`
367-
lags, as the example below does; do not use it as a change detector for anything the apply cannot see, such as external
368-
state.
363+
(read through the client, usually the informer cache) with the API server's response, on the fields that the component's
364+
field manager owns. A write by another controller, such as a finalizer it adds, is not an update. See
365+
[Reconciliation Lifecycle](component.md#reconciliation-lifecycle) for the case where the read carries no
366+
`managedFields`. It does not depend on how the desired object was built, so a wrapper reports `None` on a steady-state
367+
reconcile whether the operator keeps its resources across reconciles or rebuilds them each pass. Use the operation to
368+
distinguish `Creating` from `Updating` while `status.observedGeneration` lags, as the example below does; do not use it
369+
as a change detector for anything the apply cannot see, such as external state.
369370

370371
Three handlers are outside that guarantee. The **guard** runs before the resource is applied, which is its whole
371372
purpose, so it sees the desired object rather than the server's response. The suspension **mutation** handler takes the

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ require (
1818
k8s.io/client-go v0.37.1
1919
k8s.io/utils v0.0.0-20261023123148-ca04ba58ecab
2020
sigs.k8s.io/controller-runtime v0.25.2
21+
sigs.k8s.io/structured-merge-diff/v6 v6.4.2
2122
sigs.k8s.io/yaml v1.6.0
2223
)
2324

@@ -86,5 +87,4 @@ require (
8687
k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad // indirect
8788
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect
8889
sigs.k8s.io/randfill v1.0.0 // indirect
89-
sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect
9090
)

‎pkg/component/applied_change.go‎

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
1+
package component
2+
3+
import (
4+
"fmt"
5+
"maps"
6+
"slices"
7+
8+
"k8s.io/apimachinery/pkg/api/equality"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"k8s.io/apimachinery/pkg/runtime"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
12+
"sigs.k8s.io/structured-merge-diff/v6/fieldpath"
13+
"sigs.k8s.io/structured-merge-diff/v6/value"
14+
)
15+
16+
// appliedObjectChanged reports whether a Server-Side Apply by manager changed
17+
// the object, by comparing the object observed before the patch (read through
18+
// the client, usually the informer cache) with the API server's response to it.
19+
//
20+
// Only the fields that manager owns before or after the apply are compared, so
21+
// a write by another manager between the read and the apply, such as a
22+
// finalizer, label or annotation, does not count as a change. A write that sets
23+
// a field inside an empty object that manager owns, such as emptyDir: {}, does
24+
// count.
25+
//
26+
// When the owned fields cannot be known, the whole object is compared instead,
27+
// apart from status, managedFields, resourceVersion, generation and TypeMeta,
28+
// and a concurrent write by another manager does count as a change. That is the
29+
// case when before carries no managedFields, as when the cache strips them, when
30+
// the response holds no apply entry for manager, and when the two entries were
31+
// recorded at different API versions.
32+
func appliedObjectChanged(before, after client.Object, manager string) (bool, error) {
33+
afterEntry, found := appliedEntry(after, manager)
34+
if !found || len(before.GetManagedFields()) == 0 {
35+
return wholeObjectChanged(before, after)
36+
}
37+
38+
// No entry for manager before the apply means it owned nothing yet, as when
39+
// it adopts an object another client created.
40+
beforeOwned := &fieldpath.Set{}
41+
if beforeEntry, found := appliedEntry(before, manager); found {
42+
// Paths are expressed in the version of their entry. A conversion
43+
// between versions can rename a field, and a path from the other version
44+
// would then find nothing in either object.
45+
if beforeEntry.APIVersion != afterEntry.APIVersion {
46+
return wholeObjectChanged(before, after)
47+
}
48+
var err error
49+
if beforeOwned, err = decodeFieldSet(beforeEntry); err != nil {
50+
return false, err
51+
}
52+
}
53+
afterOwned, err := decodeFieldSet(afterEntry)
54+
if err != nil {
55+
return false, err
56+
}
57+
owned := afterOwned.Union(beforeOwned)
58+
59+
beforeContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(before)
60+
if err != nil {
61+
return false, err
62+
}
63+
afterContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(after)
64+
if err != nil {
65+
return false, err
66+
}
67+
68+
// A path that has children is compared through them. Comparing the parent
69+
// would bring back the fields other managers added beside the owned ones.
70+
// Iterate rather than a range over Set.All: All ignores the result of yield,
71+
// so leaving the loop early panics.
72+
changed := false
73+
owned.Leaves().Iterate(func(path fieldpath.Path) {
74+
if changed {
75+
return
76+
}
77+
beforeValue, beforeFound := lookupFieldPath(beforeContent, path)
78+
afterValue, afterFound := lookupFieldPath(afterContent, path)
79+
changed = beforeFound != afterFound || !equality.Semantic.DeepEqual(beforeValue, afterValue)
80+
})
81+
if changed {
82+
return true, nil
83+
}
84+
85+
// A lookup by key finds a list item wherever it sits, so the comparison of
86+
// values above does not see a reorder. The order of some lists, such as
87+
// init containers, changes what the object does.
88+
return ownedListOrderChanged(owned, nil, beforeContent, afterContent), nil
89+
}
90+
91+
// appliedEntry returns the managedFields entry of manager's Server-Side Apply
92+
// to the main resource of obj.
93+
func appliedEntry(obj client.Object, manager string) (metav1.ManagedFieldsEntry, bool) {
94+
for _, entry := range obj.GetManagedFields() {
95+
if entry.Manager == manager && entry.Operation == metav1.ManagedFieldsOperationApply && entry.Subresource == "" {
96+
return entry, true
97+
}
98+
}
99+
return metav1.ManagedFieldsEntry{}, false
100+
}
101+
102+
func decodeFieldSet(entry metav1.ManagedFieldsEntry) (*fieldpath.Set, error) {
103+
set := &fieldpath.Set{}
104+
if entry.FieldsV1 == nil {
105+
return set, nil
106+
}
107+
if err := set.FromJSON(entry.FieldsV1.GetRawReader()); err != nil {
108+
return nil, fmt.Errorf("failed to decode managed fields of %q: %w", entry.Manager, err)
109+
}
110+
return set, nil
111+
}
112+
113+
// lookupFieldPath returns the value at path in the unstructured content, and
114+
// whether the path exists there.
115+
func lookupFieldPath(content map[string]any, path fieldpath.Path) (any, bool) {
116+
var current any = content
117+
for _, element := range path {
118+
switch {
119+
case element.FieldName != nil:
120+
fields, ok := current.(map[string]any)
121+
if !ok {
122+
return nil, false
123+
}
124+
if current, ok = fields[*element.FieldName]; !ok {
125+
return nil, false
126+
}
127+
case element.Index != nil:
128+
items, ok := current.([]any)
129+
if !ok || *element.Index < 0 || *element.Index >= len(items) {
130+
return nil, false
131+
}
132+
current = items[*element.Index]
133+
default:
134+
items, _ := current.([]any)
135+
index, ok := listItemIndex(items, element)
136+
if !ok {
137+
return nil, false
138+
}
139+
current = items[index]
140+
}
141+
}
142+
return current, true
143+
}
144+
145+
// listItemIndex returns the position in items of the item that a Key or Value
146+
// path element selects.
147+
func listItemIndex(items []any, element fieldpath.PathElement) (int, bool) {
148+
index := slices.IndexFunc(items, func(item any) bool {
149+
if element.Value != nil {
150+
return value.Equals(value.NewValueInterface(item), *element.Value)
151+
}
152+
if element.Key == nil {
153+
return false
154+
}
155+
fields, ok := item.(map[string]any)
156+
if !ok {
157+
return false
158+
}
159+
for _, key := range *element.Key {
160+
field, ok := fields[key.Name]
161+
if !ok || !value.Equals(value.NewValueInterface(field), key.Value) {
162+
return false
163+
}
164+
}
165+
return true
166+
})
167+
return index, index >= 0
168+
}
169+
170+
// ownedListOrderChanged reports whether the owned items of any list in owned,
171+
// below prefix, are in a different relative order before and after. Items that
172+
// only other managers own are left out, so another manager inserting an item
173+
// does not count.
174+
func ownedListOrderChanged(owned *fieldpath.Set, prefix fieldpath.Path, before, after map[string]any) bool {
175+
var items fieldpath.PathElementSet
176+
collect := func(element fieldpath.PathElement) {
177+
if element.Key != nil || element.Value != nil {
178+
items.Insert(element)
179+
}
180+
}
181+
owned.Members.Iterate(collect)
182+
owned.Children.Iterate(collect)
183+
if items.Size() > 1 && !slices.Equal(ownedItemOrder(&items, prefix, before), ownedItemOrder(&items, prefix, after)) {
184+
return true
185+
}
186+
187+
changed := false
188+
owned.Children.Iterate(func(element fieldpath.PathElement) {
189+
if changed {
190+
return
191+
}
192+
child, _ := owned.Children.Get(element)
193+
changed = ownedListOrderChanged(child, append(prefix.Copy(), element), before, after)
194+
})
195+
return changed
196+
}
197+
198+
// ownedItemOrder returns the items of the list at prefix that are in owned, in
199+
// the order the list holds them. Owned items the list does not hold are left
200+
// out: the comparison of values already reports them.
201+
func ownedItemOrder(owned *fieldpath.PathElementSet, prefix fieldpath.Path, content map[string]any) []string {
202+
list, _ := lookupFieldPath(content, prefix)
203+
items, _ := list.([]any)
204+
positions := map[int]string{}
205+
owned.Iterate(func(element fieldpath.PathElement) {
206+
if index, ok := listItemIndex(items, element); ok {
207+
positions[index] = element.String()
208+
}
209+
})
210+
order := make([]string, 0, len(positions))
211+
for _, index := range slices.Sorted(maps.Keys(positions)) {
212+
order = append(order, positions[index])
213+
}
214+
return order
215+
}
216+
217+
// wholeObjectChanged compares the two objects on everything apart from status,
218+
// managedFields, resourceVersion, generation and TypeMeta. Those change without
219+
// the desired state changing: other controllers write status, the server bumps
220+
// the bookkeeping fields on any write, fake clients bump resourceVersion even on
221+
// a no-op apply, and TypeMeta depends on how the object was decoded.
222+
func wholeObjectChanged(before, after client.Object) (bool, error) {
223+
beforeContent, err := comparableContent(before)
224+
if err != nil {
225+
return false, err
226+
}
227+
afterContent, err := comparableContent(after)
228+
if err != nil {
229+
return false, err
230+
}
231+
return !equality.Semantic.DeepEqual(beforeContent, afterContent), nil
232+
}
233+
234+
// comparableContent renders an object as an unstructured map with the fields
235+
// wholeObjectChanged ignores removed. It copies the top-level and metadata maps
236+
// before deleting keys so the object itself is never mutated (for
237+
// *unstructured.Unstructured, ToUnstructured returns the object's own content).
238+
func comparableContent(obj client.Object) (map[string]any, error) {
239+
converted, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
240+
if err != nil {
241+
return nil, err
242+
}
243+
244+
content := make(map[string]any, len(converted))
245+
for k, v := range converted {
246+
content[k] = v
247+
}
248+
delete(content, "apiVersion")
249+
delete(content, "kind")
250+
delete(content, "status")
251+
252+
if rawMetadata, ok := content["metadata"].(map[string]any); ok {
253+
metadata := make(map[string]any, len(rawMetadata))
254+
for k, v := range rawMetadata {
255+
metadata[k] = v
256+
}
257+
delete(metadata, "managedFields")
258+
delete(metadata, "resourceVersion")
259+
delete(metadata, "generation")
260+
content["metadata"] = metadata
261+
}
262+
263+
return content, nil
264+
}

0 commit comments

Comments
 (0)