From 0ee5985d52277715ab78dcaf79e77d030c757170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Thu, 12 Nov 2026 22:21:17 +0000 Subject: [PATCH 1/2] 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 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 --- docs/component.md | 20 +- docs/custom-resource.md | 13 +- go.mod | 2 +- pkg/component/applied_change.go | 194 ++++++++++++ .../applied_change_integration_test.go | 131 ++++++++ pkg/component/applied_change_test.go | 291 ++++++++++++++++++ pkg/component/concepts/converging.go | 24 +- pkg/component/create.go | 61 +--- pkg/component/create_test.go | 19 +- .../references/component.md | 20 +- .../references/custom-resource.md | 13 +- 11 files changed, 694 insertions(+), 94 deletions(-) create mode 100644 pkg/component/applied_change.go create mode 100644 pkg/component/applied_change_integration_test.go create mode 100644 pkg/component/applied_change_test.go diff --git a/docs/component.md b/docs/component.md index b37ca6d5..f3224f7b 100644 --- a/docs/component.md +++ b/docs/component.md @@ -388,12 +388,20 @@ except where the owner is namespace-scoped and the resource is cluster-scoped (s [Cluster-scoped resources](#cluster-scoped-resources)). Each apply is classified as `Created`, `Updated` or `None` by comparing the object observed before the patch (read -through the client, usually the informer cache) with the API server's response to it, ignoring `status`, -`managedFields`, `resourceVersion` and `generation`. The classification is passed to the resource's status handler as -its `concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; -`None` records no event. Because it compares observed cluster state rather than the desired object before and after -mutation, a steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every -reconcile or keeps them across reconciles. +through the client, usually the informer cache) with the API server's response to it. The comparison covers only the +fields that the component's field manager owns before or after the apply, as recorded in `managedFields`. An owned field +that gets a new value, is added, or is removed makes the apply `Updated`. A write by another field manager does not, so +a finalizer, label or annotation that another controller adds between the read and the apply is not reported as an +update of the component. The classification is passed to the resource's status handler as its +`concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; `None` +records no event. Because it compares observed cluster state rather than the desired object before and after mutation, a +steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every reconcile or +keeps them across reconciles. + +If the read returns no `managedFields`, for example because the manager's cache strips them with +`cache.TransformStripManagedFields`, the owned fields are not known. The comparison then covers the whole object except +`status`, `managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is +reported as `Updated`. ### Previewing desired state diff --git a/docs/custom-resource.md b/docs/custom-resource.md index 8e1a6d8f..dc7efd80 100644 --- a/docs/custom-resource.md +++ b/docs/custom-resource.md @@ -360,12 +360,13 @@ seen the spec just applied. The converge and operational status handlers also receive a `concepts.ConvergingOperation` that says what the apply did: `Created` when the object did not exist before, `Updated` when the apply changed an existing object, and `None` when it left the object as it was. The framework decides this by comparing the object observed before the Server-Side Apply -(read through the client, usually the informer cache) with the API server's response, ignoring `status`, -`managedFields`, `resourceVersion` and `generation`. It does not depend on how the desired object was built, so a -wrapper reports `None` on a steady-state reconcile whether the operator keeps its resources across reconciles or -rebuilds them each pass. Use the operation to distinguish `Creating` from `Updating` while `status.observedGeneration` -lags, as the example below does; do not use it as a change detector for anything the apply cannot see, such as external -state. +(read through the client, usually the informer cache) with the API server's response, on the fields that the component's +field manager owns. A write by another controller, such as a finalizer it adds, is not an update. See +[Reconciliation Lifecycle](component.md#reconciliation-lifecycle) for the case where the read carries no +`managedFields`. It does not depend on how the desired object was built, so a wrapper reports `None` on a steady-state +reconcile whether the operator keeps its resources across reconciles or rebuilds them each pass. Use the operation to +distinguish `Creating` from `Updating` while `status.observedGeneration` lags, as the example below does; do not use it +as a change detector for anything the apply cannot see, such as external state. Three handlers are outside that guarantee. The **guard** runs before the resource is applied, which is its whole purpose, so it sees the desired object rather than the server's response. The suspension **mutation** handler takes the diff --git a/go.mod b/go.mod index 60204e8a..17fa6fb1 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( k8s.io/client-go v0.37.1 k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 sigs.k8s.io/controller-runtime v0.25.2 + sigs.k8s.io/structured-merge-diff/v6 v6.4.2 sigs.k8s.io/yaml v1.6.0 ) @@ -86,5 +87,4 @@ require ( k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect - sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect ) diff --git a/pkg/component/applied_change.go b/pkg/component/applied_change.go new file mode 100644 index 00000000..42fac6bf --- /dev/null +++ b/pkg/component/applied_change.go @@ -0,0 +1,194 @@ +package component + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/value" +) + +// appliedObjectChanged reports whether a Server-Side Apply by manager changed +// the object, by comparing the object observed before the patch (read through +// the client, usually the informer cache) with the API server's response to it. +// +// Only the fields that manager owns before or after the apply are compared, so +// a write by another manager between the read and the apply, such as a +// finalizer, label or annotation, does not count as a change. A changed owned +// field and an owned field the apply removed both count. +// +// When before carries no managedFields, as when the cache strips them, or the +// response holds no apply entry for manager, the owned fields cannot be known. +// The whole object is then compared instead, apart from status, managedFields, +// resourceVersion, generation and TypeMeta, and a concurrent write by another +// manager does count as a change. +func appliedObjectChanged(before, after client.Object, manager string) (bool, error) { + afterOwned, found, err := appliedFieldSet(after, manager) + if err != nil { + return false, err + } + if !found || len(before.GetManagedFields()) == 0 { + return wholeObjectChanged(before, after) + } + + // No entry for manager before the apply means it owned nothing yet, as when + // it adopts an object another client created. + beforeOwned, _, err := appliedFieldSet(before, manager) + if err != nil { + return false, err + } + + beforeContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(before) + if err != nil { + return false, err + } + afterContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(after) + if err != nil { + return false, err + } + + // A path that has children is compared through them. Comparing the parent + // would bring back the fields other managers added beside the owned ones. + // Iterate rather than a range over Set.All: All ignores the result of yield, + // so leaving the loop early panics. + changed := false + afterOwned.Union(beforeOwned).Leaves().Iterate(func(path fieldpath.Path) { + if changed { + return + } + beforeValue, beforeFound := lookupFieldPath(beforeContent, path) + afterValue, afterFound := lookupFieldPath(afterContent, path) + changed = beforeFound != afterFound || !equality.Semantic.DeepEqual(beforeValue, afterValue) + }) + return changed, nil +} + +// appliedFieldSet returns the fields that manager owns on obj through Server-Side +// Apply to the main resource. found is false when obj has no such entry. +func appliedFieldSet(obj client.Object, manager string) (set *fieldpath.Set, found bool, err error) { + set = &fieldpath.Set{} + for _, entry := range obj.GetManagedFields() { + if entry.Manager != manager || entry.Operation != metav1.ManagedFieldsOperationApply || entry.Subresource != "" { + continue + } + if entry.FieldsV1 == nil { + return set, true, nil + } + if err := set.FromJSON(entry.FieldsV1.GetRawReader()); err != nil { + return nil, false, fmt.Errorf("failed to decode managed fields of %q: %w", manager, err) + } + return set, true, nil + } + return set, false, nil +} + +// lookupFieldPath returns the value at path in the unstructured content, and +// whether the path exists there. +func lookupFieldPath(content map[string]any, path fieldpath.Path) (any, bool) { + var current any = content + for _, element := range path { + var found bool + switch { + case element.FieldName != nil: + fields, ok := current.(map[string]any) + if !ok { + return nil, false + } + current, found = fields[*element.FieldName] + case element.Key != nil: + current, found = findListItem(current, func(item any) bool { + fields, ok := item.(map[string]any) + if !ok { + return false + } + for _, key := range *element.Key { + field, ok := fields[key.Name] + if !ok || !value.Equals(value.NewValueInterface(field), key.Value) { + return false + } + } + return true + }) + case element.Value != nil: + current, found = findListItem(current, func(item any) bool { + return value.Equals(value.NewValueInterface(item), *element.Value) + }) + case element.Index != nil: + items, ok := current.([]any) + if !ok || *element.Index < 0 || *element.Index >= len(items) { + return nil, false + } + current, found = items[*element.Index], true + } + if !found { + return nil, false + } + } + return current, true +} + +// findListItem returns the first item of the list for which match is true. +func findListItem(list any, match func(any) bool) (any, bool) { + items, ok := list.([]any) + if !ok { + return nil, false + } + for _, item := range items { + if match(item) { + return item, true + } + } + return nil, false +} + +// wholeObjectChanged compares the two objects on everything apart from status, +// managedFields, resourceVersion, generation and TypeMeta. Those change without +// the desired state changing: other controllers write status, the server bumps +// the bookkeeping fields on any write, fake clients bump resourceVersion even on +// a no-op apply, and TypeMeta depends on how the object was decoded. +func wholeObjectChanged(before, after client.Object) (bool, error) { + beforeContent, err := comparableContent(before) + if err != nil { + return false, err + } + afterContent, err := comparableContent(after) + if err != nil { + return false, err + } + return !equality.Semantic.DeepEqual(beforeContent, afterContent), nil +} + +// comparableContent renders an object as an unstructured map with the fields +// wholeObjectChanged ignores removed. It copies the top-level and metadata maps +// before deleting keys so the object itself is never mutated (for +// *unstructured.Unstructured, ToUnstructured returns the object's own content). +func comparableContent(obj client.Object) (map[string]any, error) { + converted, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return nil, err + } + + content := make(map[string]any, len(converted)) + for k, v := range converted { + content[k] = v + } + delete(content, "apiVersion") + delete(content, "kind") + delete(content, "status") + + if rawMetadata, ok := content["metadata"].(map[string]any); ok { + metadata := make(map[string]any, len(rawMetadata)) + for k, v := range rawMetadata { + metadata[k] = v + } + delete(metadata, "managedFields") + delete(metadata, "resourceVersion") + delete(metadata, "generation") + content["metadata"] = metadata + } + + return content, nil +} diff --git a/pkg/component/applied_change_integration_test.go b/pkg/component/applied_change_integration_test.go new file mode 100644 index 00000000..a2a7368d --- /dev/null +++ b/pkg/component/applied_change_integration_test.go @@ -0,0 +1,131 @@ +package component + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// betweenReadAndApplyClient runs between once, right after the next read of a +// Deployment returns. That is the window between the read of the component and +// its apply, where a write by another controller lands. +type betweenReadAndApplyClient struct { + client.Client + between func(ctx context.Context) error +} + +func (c *betweenReadAndApplyClient) Get( + ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption, +) error { + if err := c.Client.Get(ctx, key, obj, opts...); err != nil { + return err + } + if _, ok := obj.(*appsv1.Deployment); !ok || c.between == nil { + return nil + } + write := c.between + c.between = nil + return write(ctx) +} + +// The Updated event of a component reports a change to the fields its +// field manager owns. A write by another controller that lands between the read +// and the apply of the component is not one +// (sourcehawk/operator-component-framework#248). +var _ = Describe("Apply classification with a concurrent writer", func() { + var ( + ctx = context.Background() + namespace string + owner *MockOperatorCRD + ) + + const deploymentName = "web" + + buildDeployment := func(image string, port int32) func() client.Object { + return func() client.Object { + labels := map[string]string{"app": deploymentName} + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](1), + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "app", + Image: image, + Ports: []corev1.ContainerPort{{Name: "http", ContainerPort: port}}, + }}, + }, + }, + }, + } + } + } + + addFinalizer := func(ctx context.Context) error { + deployment := &appsv1.Deployment{} + key := client.ObjectKey{Name: deploymentName, Namespace: namespace} + if err := k8sClient.Get(ctx, key, deployment); err != nil { + return err + } + deployment.Finalizers = append(deployment.Finalizers, "other.example.com/cleanup") + return k8sClient.Update(ctx, deployment, client.FieldOwner("other-controller")) + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "concurrent-writer-test-") + owner = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: namespace}} + Expect(k8sClient.Create(ctx, owner)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(ctx, owner)).To(Succeed()) + }) + + It("records no Updated event for a finalizer another controller adds, and one for each owned change", func() { + res := &objectOperationRecordingResource{build: buildDeployment("app:1", 8080)} + comp := &Component{ + name: "web", + conditionType: "WebReady", + reconcileResources: []reconcileEntry{{Resource: res}}, + } + racing := &betweenReadAndApplyClient{Client: k8sClient} + rec := newTestReconcileContext(owner) + rec.Client = racing + + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + racing.between = addFinalizer + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(racing.between).To(BeNil(), "the concurrent write must have run") + + res.build = buildDeployment("app:2", 8080) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + res.build = buildDeployment("app:2", 9090) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + Expect(res.operations).To(Equal([]concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationNone, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationNone, + })) + Expect(rec.EventRecorder.(*spyRecorder).recordedWithReason("UpdatedDeployment")).To(HaveLen(2)) + + live := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: namespace}, live)).To(Succeed()) + Expect(live.Finalizers).To(ContainElement("other.example.com/cleanup")) + }) +}) diff --git a/pkg/component/applied_change_test.go b/pkg/component/applied_change_test.go new file mode 100644 index 00000000..18b7dc41 --- /dev/null +++ b/pkg/component/applied_change_test.go @@ -0,0 +1,291 @@ +package component + +import ( + "context" + "maps" + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" + "sigs.k8s.io/structured-merge-diff/v6/value" +) + +// concurrentWriteEnv is a reconcile context whose client returns managedFields, +// as the API server does, and runs between once right after the next read of a +// ConfigMap. That is the window between the read of the component and its apply. +type concurrentWriteEnv struct { + rec ReconcileContext + client client.Client + between func(ctx context.Context, c client.WithWatch) error +} + +func newConcurrentWriteEnv(t *testing.T, stripReadManagedFields bool) *concurrentWriteEnv { + t.Helper() + env := &concurrentWriteEnv{} + scheme := setupScheme() + owner := &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "test-namespace", UID: "owner-uid"}, + } + env.client = fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(owner). + WithStatusSubresource(owner). + WithReturnManagedFields(). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func( + ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption, + ) error { + if err := c.Get(ctx, key, obj, opts...); err != nil { + return err + } + if _, ok := obj.(*corev1.ConfigMap); !ok { + return nil + } + if stripReadManagedFields { + obj.SetManagedFields(nil) + } + if env.between == nil { + return nil + } + write := env.between + env.between = nil + return write(ctx, c) + }, + }). + Build() + env.rec = setupReconcileContext(scheme, owner, env.client) + return env +} + +func (e *concurrentWriteEnv) apply(t *testing.T, res Resource) { + t.Helper() + _, err := applyResources(t.Context(), e.rec, []reconcileEntry{{Resource: res}}, "test-component", createTestRESTMapper()) + require.NoError(t, err) + require.Nil(t, e.between, "the concurrent write must have run") +} + +func (e *concurrentWriteEnv) updatedEvents() []recordedEvent { + return e.rec.EventRecorder.(*spyRecorder).recordedWithReason("UpdatedConfigMap") +} + +const concurrentWriteConfigMap = "concurrent-write-cm" + +// writeByOtherManager returns a write that another controller makes to the +// ConfigMap with an Update, under its own field manager. +func writeByOtherManager(edit func(*corev1.ConfigMap)) func(context.Context, client.WithWatch) error { + return func(ctx context.Context, c client.WithWatch) error { + cm := &corev1.ConfigMap{} + key := client.ObjectKey{Name: concurrentWriteConfigMap, Namespace: "test-namespace"} + if err := c.Get(ctx, key, cm); err != nil { + return err + } + edit(cm) + return c.Update(ctx, cm, client.FieldOwner("other-controller")) + } +} + +func buildConcurrentWriteConfigMap(data map[string]string) func() *corev1.ConfigMap { + return func() *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: concurrentWriteConfigMap, Namespace: "test-namespace"}, + Data: maps.Clone(data), + } + } +} + +func TestApplyResource_ClassifiesOnOwnedFields(t *testing.T) { + otherWrites := map[string]func(*corev1.ConfigMap){ + "a finalizer": func(cm *corev1.ConfigMap) { + cm.Finalizers = append(cm.Finalizers, "other.example.com/cleanup") + }, + "a label": func(cm *corev1.ConfigMap) { + cm.Labels = map[string]string{"other.example.com/seen": "true"} + }, + "an annotation": func(cm *corev1.ConfigMap) { + cm.Annotations = map[string]string{"other.example.com/seen": "true"} + }, + } + for name, edit := range otherWrites { + t.Run("reports None when another manager adds "+name+" between the read and the apply", func(t *testing.T) { + env := newConcurrentWriteEnv(t, false) + res := &operationRecordingResource{build: buildConcurrentWriteConfigMap(map[string]string{"foo": "bar"})} + env.apply(t, res) + + env.between = writeByOtherManager(edit) + env.apply(t, res) + + assert.Equal(t, []concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationNone, + }, res.operations) + assert.Empty(t, env.updatedEvents()) + }) + } + + t.Run("reports Updated once when an owned field changes during a concurrent write", func(t *testing.T) { + env := newConcurrentWriteEnv(t, false) + res := &operationRecordingResource{build: buildConcurrentWriteConfigMap(map[string]string{"foo": "bar"})} + env.apply(t, res) + + res.build = buildConcurrentWriteConfigMap(map[string]string{"foo": "baz"}) + env.between = writeByOtherManager(otherWrites["a finalizer"]) + env.apply(t, res) + env.apply(t, res) + + assert.Equal(t, []concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationNone, + }, res.operations) + assert.Len(t, env.updatedEvents(), 1) + }) + + t.Run("reports Updated once when the apply removes an owned field", func(t *testing.T) { + env := newConcurrentWriteEnv(t, false) + res := &operationRecordingResource{ + build: buildConcurrentWriteConfigMap(map[string]string{"foo": "bar", "removed": "soon"}), + } + env.apply(t, res) + + res.build = buildConcurrentWriteConfigMap(map[string]string{"foo": "bar"}) + env.apply(t, res) + env.apply(t, res) + + assert.Equal(t, []concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationNone, + }, res.operations) + assert.Len(t, env.updatedEvents(), 1) + + live := &corev1.ConfigMap{} + require.NoError(t, env.client.Get( + t.Context(), client.ObjectKey{Name: concurrentWriteConfigMap, Namespace: "test-namespace"}, live, + )) + assert.Equal(t, map[string]string{"foo": "bar"}, live.Data) + }) + + t.Run("reports Updated when the apply removes an owned field and the read carries no managedFields", func(t *testing.T) { + env := newConcurrentWriteEnv(t, true) + res := &operationRecordingResource{ + build: buildConcurrentWriteConfigMap(map[string]string{"foo": "bar", "removed": "soon"}), + } + env.apply(t, res) + + res.build = buildConcurrentWriteConfigMap(map[string]string{"foo": "bar"}) + env.apply(t, res) + env.apply(t, res) + + assert.Equal(t, []concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationNone, + }, res.operations) + }) +} + +func TestLookupFieldPath(t *testing.T) { + content := map[string]any{ + "spec": map[string]any{ + "replicas": int64(2), + "containers": []any{ + map[string]any{ + "name": "app", + "image": "app:1", + "ports": []any{ + map[string]any{"containerPort": int64(8080), "protocol": "TCP", "name": "http"}, + map[string]any{"containerPort": int64(8080), "protocol": "UDP", "name": "quic"}, + }, + }, + }, + "args": []any{"--verbose", "--port=8080"}, + "finalizers": []any{"a", "b"}, + }, + } + + tests := []struct { + name string + path fieldpath.Path + wantValue any + wantFound bool + }{ + { + name: "field of a map", + path: fieldpath.MakePathOrDie("spec", "replicas"), + wantValue: int64(2), + wantFound: true, + }, + { + name: "absent field", + path: fieldpath.MakePathOrDie("spec", "paused"), + wantFound: false, + }, + { + name: "item of an associative list by its key", + path: fieldpath.MakePathOrDie("spec", "containers", fieldpath.KeyByFields("name", "app"), "image"), + wantValue: "app:1", + wantFound: true, + }, + { + name: "item of an associative list by a key of several fields", + path: fieldpath.MakePathOrDie( + "spec", "containers", fieldpath.KeyByFields("name", "app"), + "ports", fieldpath.KeyByFields("containerPort", 8080, "protocol", "UDP"), "name", + ), + wantValue: "quic", + wantFound: true, + }, + { + name: "absent item of an associative list", + path: fieldpath.MakePathOrDie("spec", "containers", fieldpath.KeyByFields("name", "sidecar")), + wantFound: false, + }, + { + name: "item of a set by its value", + path: fieldpath.MakePathOrDie("spec", "finalizers", value.NewValueInterface("b")), + wantValue: "b", + wantFound: true, + }, + { + name: "absent item of a set", + path: fieldpath.MakePathOrDie("spec", "finalizers", value.NewValueInterface("c")), + wantFound: false, + }, + { + name: "item of an atomic list by its index", + path: fieldpath.MakePathOrDie("spec", "args", 1), + wantValue: "--port=8080", + wantFound: true, + }, + { + name: "index past the end of a list", + path: fieldpath.MakePathOrDie("spec", "args", 2), + wantFound: false, + }, + { + name: "field name on a value that is not a map", + path: fieldpath.MakePathOrDie("spec", "replicas", "value"), + wantFound: false, + }, + { + name: "key on a value that is not a list", + path: fieldpath.MakePathOrDie("spec", fieldpath.KeyByFields("name", "app")), + wantFound: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, found := lookupFieldPath(content, tt.path) + assert.Equal(t, tt.wantFound, found) + assert.Equal(t, tt.wantValue, got) + }) + } +} diff --git a/pkg/component/concepts/converging.go b/pkg/component/concepts/converging.go index 315bb55f..9126a092 100644 --- a/pkg/component/concepts/converging.go +++ b/pkg/component/concepts/converging.go @@ -13,12 +13,19 @@ import ( // // The framework derives it by comparing the object observed before the // Server-Side Apply (read through the client, usually the informer cache) with -// the API server's response to it, ignoring the status -// subresource, managedFields, resourceVersion and generation. The classification -// therefore reflects whether the applied desired state changed the object, not -// how the desired object was constructed: operators that rebuild their desired -// objects on every reconcile and operators that keep them across reconciles get -// the same result. +// the API server's response to it, on the fields the component's field manager +// owns before or after the apply. The classification therefore reflects whether +// the applied desired state changed the object, not how the desired object was +// constructed: operators that rebuild their desired objects on every reconcile +// and operators that keep them across reconciles get the same result. A write +// by another field manager, such as a finalizer or label another controller +// adds between the read and the apply, is not a change. +// +// The owned fields come from the object's managedFields. When the read returns +// none, as with a cache that strips them, the framework compares the whole +// object apart from the status subresource, managedFields, resourceVersion and +// generation, and a write by another field manager in that window does count +// as a change. type ConvergingOperation string const ( @@ -26,8 +33,9 @@ const ( // the apply and was created by it. ConvergingOperationCreated ConvergingOperation = "Created" // ConvergingOperationUpdated indicates that the resource existed and the apply - // changed it: its labels, annotations, owner references, spec or data differ - // from what was observed before the apply. + // changed it: a field the component's field manager owns, such as a label, + // an annotation, an owner reference, or a field of the spec or data, was set + // to a new value, added, or removed. ConvergingOperationUpdated ConvergingOperation = "Updated" // ConvergingOperationNone indicates that the apply left the existing resource // unchanged, or that the resource was only read (read-only resources). diff --git a/pkg/component/create.go b/pkg/component/create.go index 0b305403..68cb6626 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -11,7 +11,6 @@ import ( "github.com/sourcehawk/operator-component-framework/internal/scope" "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" "github.com/sourcehawk/operator-component-framework/pkg/recording" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -132,7 +131,7 @@ func applyResource( // feature mutations are always added relative to the freshly built object. convergingOperation := concepts.ConvergingOperationCreated if objectExists { - changed, err := appliedObjectChanged(existing, obj) + changed, err := appliedObjectChanged(existing, obj, string(fieldOwner)) if err != nil { return nil, applyFailed(rec, labels, fmt.Errorf( "failed to compare applied state of resource %s: %w", resource.Identity(), err, @@ -516,64 +515,6 @@ func newEmptyObjectLike(obj client.Object) (client.Object, error) { return empty, nil } -// appliedObjectChanged reports whether a Server-Side Apply changed the object, -// by comparing the object observed before the patch (read through the client, -// usually the informer cache) with the API server's response to it. -// -// The comparison ignores fields that change without the desired state changing: -// the status subresource (written by other controllers), managedFields and -// resourceVersion (bookkeeping the server touches on any write), generation -// (derived from the compared content) and TypeMeta (present or absent depending -// on how the object was decoded). Everything else, including labels, -// annotations, owner references and the object's spec or data, counts. -// -// It does not rely on resourceVersion because concurrent status writes bump it -// without touching the desired state, and because fake clients bump it on -// every apply, even a no-op one. -func appliedObjectChanged(before, after client.Object) (bool, error) { - beforeContent, err := comparableContent(before) - if err != nil { - return false, err - } - afterContent, err := comparableContent(after) - if err != nil { - return false, err - } - return !equality.Semantic.DeepEqual(beforeContent, afterContent), nil -} - -// comparableContent renders an object as an unstructured map with the fields -// appliedObjectChanged ignores removed. It copies the top-level and metadata -// maps before deleting keys so the object itself is never mutated (for -// *unstructured.Unstructured, ToUnstructured returns the object's own content). -func comparableContent(obj client.Object) (map[string]any, error) { - converted, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) - if err != nil { - return nil, err - } - - content := make(map[string]any, len(converted)) - for k, v := range converted { - content[k] = v - } - delete(content, "apiVersion") - delete(content, "kind") - delete(content, "status") - - if rawMetadata, ok := content["metadata"].(map[string]any); ok { - metadata := make(map[string]any, len(rawMetadata)) - for k, v := range rawMetadata { - metadata[k] = v - } - delete(metadata, "managedFields") - delete(metadata, "resourceVersion") - delete(metadata, "generation") - content["metadata"] = metadata - } - - return content, nil -} - // clearServerFields removes metadata fields that the API server populates on // responses but must not be present in an SSA patch object. After the first // apply, the resource's internal DesiredObject retains these from the server diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index 60266a8f..d21f2fb7 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_test.go @@ -395,7 +395,20 @@ func (r *objectOperationRecordingResource) GraceStatus() (concepts.GraceStatusWi return concepts.GraceStatusWithReason{Status: concepts.GraceStatusHealthy}, nil } +// TestApplyResource_ConvergingOperation runs every case twice: once where the +// client returns managedFields, so only the fields the component owns are +// compared, and once where it strips them, as a cache with a managedFields +// transform does, so the whole object is compared. func TestApplyResource_ConvergingOperation(t *testing.T) { + t.Run("with managedFields", func(t *testing.T) { + testApplyResourceConvergingOperation(t, true) + }) + t.Run("without managedFields", func(t *testing.T) { + testApplyResourceConvergingOperation(t, false) + }) +} + +func testApplyResourceConvergingOperation(t *testing.T, returnManagedFields bool) { const namespace = "test-namespace" newEnv := func(t *testing.T) (ReconcileContext, client.Client) { @@ -404,7 +417,11 @@ func TestApplyResource_ConvergingOperation(t *testing.T) { owner := &MockOperatorCRD{ ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: namespace, UID: "owner-uid"}, } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner).WithStatusSubresource(owner).Build() + builder := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner).WithStatusSubresource(owner) + if returnManagedFields { + builder = builder.WithReturnManagedFields() + } + fakeClient := builder.Build() return setupReconcileContext(scheme, owner, fakeClient), fakeClient } diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 8700e729..391d6626 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -389,12 +389,20 @@ except where the owner is namespace-scoped and the resource is cluster-scoped (s [Cluster-scoped resources](#cluster-scoped-resources)). Each apply is classified as `Created`, `Updated` or `None` by comparing the object observed before the patch (read -through the client, usually the informer cache) with the API server's response to it, ignoring `status`, -`managedFields`, `resourceVersion` and `generation`. The classification is passed to the resource's status handler as -its `concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; -`None` records no event. Because it compares observed cluster state rather than the desired object before and after -mutation, a steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every -reconcile or keeps them across reconciles. +through the client, usually the informer cache) with the API server's response to it. The comparison covers only the +fields that the component's field manager owns before or after the apply, as recorded in `managedFields`. An owned field +that gets a new value, is added, or is removed makes the apply `Updated`. A write by another field manager does not, so +a finalizer, label or annotation that another controller adds between the read and the apply is not reported as an +update of the component. The classification is passed to the resource's status handler as its +`concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; `None` +records no event. Because it compares observed cluster state rather than the desired object before and after mutation, a +steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every reconcile or +keeps them across reconciles. + +If the read returns no `managedFields`, for example because the manager's cache strips them with +`cache.TransformStripManagedFields`, the owned fields are not known. The comparison then covers the whole object except +`status`, `managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is +reported as `Updated`. ### Previewing desired state diff --git a/plugin/skills/custom-resource-wrappers/references/custom-resource.md b/plugin/skills/custom-resource-wrappers/references/custom-resource.md index bcb41e90..fdfa7b4c 100644 --- a/plugin/skills/custom-resource-wrappers/references/custom-resource.md +++ b/plugin/skills/custom-resource-wrappers/references/custom-resource.md @@ -363,12 +363,13 @@ seen the spec just applied. The converge and operational status handlers also receive a `concepts.ConvergingOperation` that says what the apply did: `Created` when the object did not exist before, `Updated` when the apply changed an existing object, and `None` when it left the object as it was. The framework decides this by comparing the object observed before the Server-Side Apply -(read through the client, usually the informer cache) with the API server's response, ignoring `status`, -`managedFields`, `resourceVersion` and `generation`. It does not depend on how the desired object was built, so a -wrapper reports `None` on a steady-state reconcile whether the operator keeps its resources across reconciles or -rebuilds them each pass. Use the operation to distinguish `Creating` from `Updating` while `status.observedGeneration` -lags, as the example below does; do not use it as a change detector for anything the apply cannot see, such as external -state. +(read through the client, usually the informer cache) with the API server's response, on the fields that the component's +field manager owns. A write by another controller, such as a finalizer it adds, is not an update. See +[Reconciliation Lifecycle](../../building-components/references/component.md#reconciliation-lifecycle) for the case +where the read carries no `managedFields`. It does not depend on how the desired object was built, so a wrapper reports +`None` on a steady-state reconcile whether the operator keeps its resources across reconciles or rebuilds them each +pass. Use the operation to distinguish `Creating` from `Updating` while `status.observedGeneration` lags, as the example +below does; do not use it as a change detector for anything the apply cannot see, such as external state. Three handlers are outside that guarantee. The **guard** runs before the resource is applied, which is its whole purpose, so it sees the desired object rather than the server's response. The suspension **mutation** handler takes the From 5755f4c860cd93f2c949f58357cd5534e0f8e9c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Thu, 12 Nov 2026 22:45:47 +0000 Subject: [PATCH 2/2] 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 --- docs/component.md | 28 +-- pkg/component/applied_change.go | 192 ++++++++++++------ .../applied_change_integration_test.go | 40 ++++ pkg/component/applied_change_test.go | 87 ++++++++ pkg/component/concepts/converging.go | 17 +- .../references/component.md | 28 +-- 6 files changed, 299 insertions(+), 93 deletions(-) diff --git a/docs/component.md b/docs/component.md index f3224f7b..6c4fdb56 100644 --- a/docs/component.md +++ b/docs/component.md @@ -390,18 +390,22 @@ except where the owner is namespace-scoped and the resource is cluster-scoped (s Each apply is classified as `Created`, `Updated` or `None` by comparing the object observed before the patch (read through the client, usually the informer cache) with the API server's response to it. The comparison covers only the fields that the component's field manager owns before or after the apply, as recorded in `managedFields`. An owned field -that gets a new value, is added, or is removed makes the apply `Updated`. A write by another field manager does not, so -a finalizer, label or annotation that another controller adds between the read and the apply is not reported as an -update of the component. The classification is passed to the resource's status handler as its -`concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; `None` -records no event. Because it compares observed cluster state rather than the desired object before and after mutation, a -steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every reconcile or -keeps them across reconciles. - -If the read returns no `managedFields`, for example because the manager's cache strips them with -`cache.TransformStripManagedFields`, the owned fields are not known. The comparison then covers the whole object except -`status`, `managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is -reported as `Updated`. +that gets a new value, is added, or is removed makes the apply `Updated`. So do owned list items, such as containers, +that change their order. A write by another field manager does not, so a finalizer, label or annotation that another +controller adds between the read and the apply is not reported as an update of the component. The classification is +passed to the resource's status handler as its `concepts.ConvergingOperation` and drives the `Created` and +`Updated` events recorded on the owner; `None` records no event. Because it compares observed cluster state rather +than the desired object before and after mutation, a steady-state reconcile reports `None` regardless of whether the +operator rebuilds its components on every reconcile or keeps them across reconciles. + +The owned fields are not known in three cases: the read returns no `managedFields`, for example because the manager's +cache strips them with `cache.TransformStripManagedFields`, the apply response holds no apply entry for the component's +field manager, or the entries of the component before and after the apply have different API versions, as after the +operator moves to a new version of a custom resource. The comparison then covers the whole object except `status`, +`managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is reported as +`Updated`. The same false `Updated` occurs when the component applies an empty object, such as `emptyDir: {}`, and +another controller sets a field inside it in that window. The component owns such an object as one value, so a field +that another controller adds inside it counts as a change to that value. ### Previewing desired state diff --git a/pkg/component/applied_change.go b/pkg/component/applied_change.go index 42fac6bf..1dbef7fc 100644 --- a/pkg/component/applied_change.go +++ b/pkg/component/applied_change.go @@ -2,6 +2,8 @@ package component import ( "fmt" + "maps" + "slices" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,29 +19,42 @@ import ( // // Only the fields that manager owns before or after the apply are compared, so // a write by another manager between the read and the apply, such as a -// finalizer, label or annotation, does not count as a change. A changed owned -// field and an owned field the apply removed both count. +// finalizer, label or annotation, does not count as a change. A write that sets +// a field inside an empty object that manager owns, such as emptyDir: {}, does +// count. // -// When before carries no managedFields, as when the cache strips them, or the -// response holds no apply entry for manager, the owned fields cannot be known. -// The whole object is then compared instead, apart from status, managedFields, -// resourceVersion, generation and TypeMeta, and a concurrent write by another -// manager does count as a change. +// When the owned fields cannot be known, the whole object is compared instead, +// apart from status, managedFields, resourceVersion, generation and TypeMeta, +// and a concurrent write by another manager does count as a change. That is the +// case when before carries no managedFields, as when the cache strips them, when +// the response holds no apply entry for manager, and when the two entries were +// recorded at different API versions. func appliedObjectChanged(before, after client.Object, manager string) (bool, error) { - afterOwned, found, err := appliedFieldSet(after, manager) - if err != nil { - return false, err - } + afterEntry, found := appliedEntry(after, manager) if !found || len(before.GetManagedFields()) == 0 { return wholeObjectChanged(before, after) } // No entry for manager before the apply means it owned nothing yet, as when // it adopts an object another client created. - beforeOwned, _, err := appliedFieldSet(before, manager) + beforeOwned := &fieldpath.Set{} + if beforeEntry, found := appliedEntry(before, manager); found { + // Paths are expressed in the version of their entry. A conversion + // between versions can rename a field, and a path from the other version + // would then find nothing in either object. + if beforeEntry.APIVersion != afterEntry.APIVersion { + return wholeObjectChanged(before, after) + } + var err error + if beforeOwned, err = decodeFieldSet(beforeEntry); err != nil { + return false, err + } + } + afterOwned, err := decodeFieldSet(afterEntry) if err != nil { return false, err } + owned := afterOwned.Union(beforeOwned) beforeContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(before) if err != nil { @@ -55,7 +70,7 @@ func appliedObjectChanged(before, after client.Object, manager string) (bool, er // Iterate rather than a range over Set.All: All ignores the result of yield, // so leaving the loop early panics. changed := false - afterOwned.Union(beforeOwned).Leaves().Iterate(func(path fieldpath.Path) { + owned.Leaves().Iterate(func(path fieldpath.Path) { if changed { return } @@ -63,26 +78,36 @@ func appliedObjectChanged(before, after client.Object, manager string) (bool, er afterValue, afterFound := lookupFieldPath(afterContent, path) changed = beforeFound != afterFound || !equality.Semantic.DeepEqual(beforeValue, afterValue) }) - return changed, nil + if changed { + return true, nil + } + + // A lookup by key finds a list item wherever it sits, so the comparison of + // values above does not see a reorder. The order of some lists, such as + // init containers, changes what the object does. + return ownedListOrderChanged(owned, nil, beforeContent, afterContent), nil } -// appliedFieldSet returns the fields that manager owns on obj through Server-Side -// Apply to the main resource. found is false when obj has no such entry. -func appliedFieldSet(obj client.Object, manager string) (set *fieldpath.Set, found bool, err error) { - set = &fieldpath.Set{} +// appliedEntry returns the managedFields entry of manager's Server-Side Apply +// to the main resource of obj. +func appliedEntry(obj client.Object, manager string) (metav1.ManagedFieldsEntry, bool) { for _, entry := range obj.GetManagedFields() { - if entry.Manager != manager || entry.Operation != metav1.ManagedFieldsOperationApply || entry.Subresource != "" { - continue - } - if entry.FieldsV1 == nil { - return set, true, nil - } - if err := set.FromJSON(entry.FieldsV1.GetRawReader()); err != nil { - return nil, false, fmt.Errorf("failed to decode managed fields of %q: %w", manager, err) + if entry.Manager == manager && entry.Operation == metav1.ManagedFieldsOperationApply && entry.Subresource == "" { + return entry, true } - return set, true, nil } - return set, false, nil + return metav1.ManagedFieldsEntry{}, false +} + +func decodeFieldSet(entry metav1.ManagedFieldsEntry) (*fieldpath.Set, error) { + set := &fieldpath.Set{} + if entry.FieldsV1 == nil { + return set, nil + } + if err := set.FromJSON(entry.FieldsV1.GetRawReader()); err != nil { + return nil, fmt.Errorf("failed to decode managed fields of %q: %w", entry.Manager, err) + } + return set, nil } // lookupFieldPath returns the value at path in the unstructured content, and @@ -90,58 +115,103 @@ func appliedFieldSet(obj client.Object, manager string) (set *fieldpath.Set, fou func lookupFieldPath(content map[string]any, path fieldpath.Path) (any, bool) { var current any = content for _, element := range path { - var found bool switch { case element.FieldName != nil: fields, ok := current.(map[string]any) if !ok { return nil, false } - current, found = fields[*element.FieldName] - case element.Key != nil: - current, found = findListItem(current, func(item any) bool { - fields, ok := item.(map[string]any) - if !ok { - return false - } - for _, key := range *element.Key { - field, ok := fields[key.Name] - if !ok || !value.Equals(value.NewValueInterface(field), key.Value) { - return false - } - } - return true - }) - case element.Value != nil: - current, found = findListItem(current, func(item any) bool { - return value.Equals(value.NewValueInterface(item), *element.Value) - }) + if current, ok = fields[*element.FieldName]; !ok { + return nil, false + } case element.Index != nil: items, ok := current.([]any) if !ok || *element.Index < 0 || *element.Index >= len(items) { return nil, false } - current, found = items[*element.Index], true - } - if !found { - return nil, false + current = items[*element.Index] + default: + items, _ := current.([]any) + index, ok := listItemIndex(items, element) + if !ok { + return nil, false + } + current = items[index] } } return current, true } -// findListItem returns the first item of the list for which match is true. -func findListItem(list any, match func(any) bool) (any, bool) { - items, ok := list.([]any) - if !ok { - return nil, false +// listItemIndex returns the position in items of the item that a Key or Value +// path element selects. +func listItemIndex(items []any, element fieldpath.PathElement) (int, bool) { + index := slices.IndexFunc(items, func(item any) bool { + if element.Value != nil { + return value.Equals(value.NewValueInterface(item), *element.Value) + } + if element.Key == nil { + return false + } + fields, ok := item.(map[string]any) + if !ok { + return false + } + for _, key := range *element.Key { + field, ok := fields[key.Name] + if !ok || !value.Equals(value.NewValueInterface(field), key.Value) { + return false + } + } + return true + }) + return index, index >= 0 +} + +// ownedListOrderChanged reports whether the owned items of any list in owned, +// below prefix, are in a different relative order before and after. Items that +// only other managers own are left out, so another manager inserting an item +// does not count. +func ownedListOrderChanged(owned *fieldpath.Set, prefix fieldpath.Path, before, after map[string]any) bool { + var items fieldpath.PathElementSet + collect := func(element fieldpath.PathElement) { + if element.Key != nil || element.Value != nil { + items.Insert(element) + } + } + owned.Members.Iterate(collect) + owned.Children.Iterate(collect) + if items.Size() > 1 && !slices.Equal(ownedItemOrder(&items, prefix, before), ownedItemOrder(&items, prefix, after)) { + return true } - for _, item := range items { - if match(item) { - return item, true + + changed := false + owned.Children.Iterate(func(element fieldpath.PathElement) { + if changed { + return } + child, _ := owned.Children.Get(element) + changed = ownedListOrderChanged(child, append(prefix.Copy(), element), before, after) + }) + return changed +} + +// ownedItemOrder returns the items of the list at prefix that are in owned, in +// the order the list holds them. Owned items the list does not hold are left +// out: the comparison of values already reports them. +func ownedItemOrder(owned *fieldpath.PathElementSet, prefix fieldpath.Path, content map[string]any) []string { + list, _ := lookupFieldPath(content, prefix) + items, _ := list.([]any) + positions := map[int]string{} + owned.Iterate(func(element fieldpath.PathElement) { + if index, ok := listItemIndex(items, element); ok { + positions[index] = element.String() + } + }) + order := make([]string, 0, len(positions)) + for _, index := range slices.Sorted(maps.Keys(positions)) { + order = append(order, positions[index]) } - return nil, false + return order } // wholeObjectChanged compares the two objects on everything apart from status, diff --git a/pkg/component/applied_change_integration_test.go b/pkg/component/applied_change_integration_test.go index a2a7368d..735e0dac 100644 --- a/pkg/component/applied_change_integration_test.go +++ b/pkg/component/applied_change_integration_test.go @@ -128,4 +128,44 @@ var _ = Describe("Apply classification with a concurrent writer", func() { Expect(k8sClient.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: namespace}, live)).To(Succeed()) Expect(live.Finalizers).To(ContainElement("other.example.com/cleanup")) }) + + It("records an Updated event when only the order of owned containers changes", func() { + withContainers := func(names ...string) func() client.Object { + return func() client.Object { + deployment := buildDeployment("app:1", 8080)().(*appsv1.Deployment) + deployment.Spec.Template.Spec.Containers = nil + for _, name := range names { + deployment.Spec.Template.Spec.Containers = append( + deployment.Spec.Template.Spec.Containers, + corev1.Container{Name: name, Image: name + ":1"}, + ) + } + return deployment + } + } + res := &objectOperationRecordingResource{build: withContainers("first", "second")} + comp := &Component{ + name: "web", + conditionType: "WebReady", + reconcileResources: []reconcileEntry{{Resource: res}}, + } + rec := newTestReconcileContext(owner) + + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + res.build = withContainers("second", "first") + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + Expect(res.operations).To(Equal([]concepts.ConvergingOperation{ + concepts.ConvergingOperationCreated, + concepts.ConvergingOperationUpdated, + concepts.ConvergingOperationNone, + })) + + live := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: namespace}, live)).To(Succeed()) + Expect(live.Spec.Template.Spec.Containers).To(HaveLen(2)) + Expect(live.Spec.Template.Spec.Containers[0].Name).To(Equal("second")) + }) }) diff --git a/pkg/component/applied_change_test.go b/pkg/component/applied_change_test.go index 18b7dc41..7ab690f2 100644 --- a/pkg/component/applied_change_test.go +++ b/pkg/component/applied_change_test.go @@ -289,3 +289,90 @@ func TestLookupFieldPath(t *testing.T) { }) } } + +func TestOwnedListOrderChanged(t *testing.T) { + containers := func(names ...string) map[string]any { + items := make([]any, 0, len(names)) + for _, name := range names { + items = append(items, map[string]any{"name": name}) + } + return map[string]any{"spec": map[string]any{"containers": items}} + } + owned := fieldpath.NewSet( + fieldpath.MakePathOrDie("spec", "containers", fieldpath.KeyByFields("name", "first")), + fieldpath.MakePathOrDie("spec", "containers", fieldpath.KeyByFields("name", "second")), + ) + + tests := []struct { + name string + before map[string]any + after map[string]any + want bool + }{ + { + name: "same order", + before: containers("first", "second"), + after: containers("first", "second"), + want: false, + }, + { + name: "owned items swapped", + before: containers("first", "second"), + after: containers("second", "first"), + want: true, + }, + { + name: "item of another manager inserted between owned items", + before: containers("first", "second"), + after: containers("first", "sidecar", "second"), + want: false, + }, + { + name: "item of another manager moved ahead of owned items", + before: containers("first", "sidecar", "second"), + after: containers("sidecar", "first", "second"), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, ownedListOrderChanged(owned, nil, tt.before, tt.after)) + }) + } +} + +func TestAppliedObjectChanged_ComparesWholeObjectWhenEntryVersionsDiffer(t *testing.T) { + const manager = "test-manager" + entry := func(apiVersion, fields string) metav1.ManagedFieldsEntry { + raw := &metav1.FieldsV1{} + raw.SetRawString(fields) + return metav1.ManagedFieldsEntry{ + Manager: manager, + Operation: metav1.ManagedFieldsOperationApply, + APIVersion: apiVersion, + FieldsType: "FieldsV1", + FieldsV1: raw, + } + } + + // The entry before the apply was recorded at an older version, where the + // field had another name. The apply removes the field. + before := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "converted", + ManagedFields: []metav1.ManagedFieldsEntry{entry("v1beta1", `{"f:data":{"f:oldName":{}}}`)}, + }, + Data: map[string]string{"newName": "value"}, + } + after := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "converted", + ManagedFields: []metav1.ManagedFieldsEntry{entry("v1", `{"f:metadata":{"f:name":{}}}`)}, + }, + } + + changed, err := appliedObjectChanged(before, after, manager) + require.NoError(t, err) + assert.True(t, changed) +} diff --git a/pkg/component/concepts/converging.go b/pkg/component/concepts/converging.go index 9126a092..f7611667 100644 --- a/pkg/component/concepts/converging.go +++ b/pkg/component/concepts/converging.go @@ -22,20 +22,21 @@ import ( // adds between the read and the apply, is not a change. // // The owned fields come from the object's managedFields. When the read returns -// none, as with a cache that strips them, the framework compares the whole -// object apart from the status subresource, managedFields, resourceVersion and -// generation, and a write by another field manager in that window does count -// as a change. +// none, as with a cache that strips them, when the apply response holds no +// apply entry for the component's field manager, or when the component's +// entries before and after the apply have different API versions, the framework +// compares the whole object apart from the status subresource, managedFields, +// resourceVersion and generation. A write by another field manager in that +// window then does count as a change. It also counts when it sets a field inside +// an empty object that the component applies, such as emptyDir: {}. type ConvergingOperation string const ( // ConvergingOperationCreated indicates that the resource did not exist before // the apply and was created by it. ConvergingOperationCreated ConvergingOperation = "Created" - // ConvergingOperationUpdated indicates that the resource existed and the apply - // changed it: a field the component's field manager owns, such as a label, - // an annotation, an owner reference, or a field of the spec or data, was set - // to a new value, added, or removed. + // ConvergingOperationUpdated indicates that the resource existed and the + // comparison described on ConvergingOperation found a change. ConvergingOperationUpdated ConvergingOperation = "Updated" // ConvergingOperationNone indicates that the apply left the existing resource // unchanged, or that the resource was only read (read-only resources). diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 391d6626..1f8e24c0 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -391,18 +391,22 @@ except where the owner is namespace-scoped and the resource is cluster-scoped (s Each apply is classified as `Created`, `Updated` or `None` by comparing the object observed before the patch (read through the client, usually the informer cache) with the API server's response to it. The comparison covers only the fields that the component's field manager owns before or after the apply, as recorded in `managedFields`. An owned field -that gets a new value, is added, or is removed makes the apply `Updated`. A write by another field manager does not, so -a finalizer, label or annotation that another controller adds between the read and the apply is not reported as an -update of the component. The classification is passed to the resource's status handler as its -`concepts.ConvergingOperation` and drives the `Created` and `Updated` events recorded on the owner; `None` -records no event. Because it compares observed cluster state rather than the desired object before and after mutation, a -steady-state reconcile reports `None` regardless of whether the operator rebuilds its components on every reconcile or -keeps them across reconciles. - -If the read returns no `managedFields`, for example because the manager's cache strips them with -`cache.TransformStripManagedFields`, the owned fields are not known. The comparison then covers the whole object except -`status`, `managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is -reported as `Updated`. +that gets a new value, is added, or is removed makes the apply `Updated`. So do owned list items, such as containers, +that change their order. A write by another field manager does not, so a finalizer, label or annotation that another +controller adds between the read and the apply is not reported as an update of the component. The classification is +passed to the resource's status handler as its `concepts.ConvergingOperation` and drives the `Created` and +`Updated` events recorded on the owner; `None` records no event. Because it compares observed cluster state rather +than the desired object before and after mutation, a steady-state reconcile reports `None` regardless of whether the +operator rebuilds its components on every reconcile or keeps them across reconciles. + +The owned fields are not known in three cases: the read returns no `managedFields`, for example because the manager's +cache strips them with `cache.TransformStripManagedFields`, the apply response holds no apply entry for the component's +field manager, or the entries of the component before and after the apply have different API versions, as after the +operator moves to a new version of a custom resource. The comparison then covers the whole object except `status`, +`managedFields`, `resourceVersion` and `generation`, and a write by another controller in that window is reported as +`Updated`. The same false `Updated` occurs when the component applies an empty object, such as `emptyDir: {}`, and +another controller sets a field inside it in that window. The component owns such an object as one value, so a field +that another controller adds inside it counts as a change to that value. ### Previewing desired state