diff --git a/docs/component.md b/docs/component.md index b37ca6d5..6c4fdb56 100644 --- a/docs/component.md +++ b/docs/component.md @@ -388,12 +388,24 @@ 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`. 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/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..1dbef7fc --- /dev/null +++ b/pkg/component/applied_change.go @@ -0,0 +1,264 @@ +package component + +import ( + "fmt" + "maps" + "slices" + + "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 write that sets +// a field inside an empty object that manager owns, such as emptyDir: {}, does +// count. +// +// 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) { + 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 := &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 { + 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 + owned.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) + }) + 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 +} + +// 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 == "" { + return entry, true + } + } + 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 +// whether the path exists there. +func lookupFieldPath(content map[string]any, path fieldpath.Path) (any, bool) { + var current any = content + for _, element := range path { + switch { + case element.FieldName != nil: + fields, ok := current.(map[string]any) + if !ok { + return nil, false + } + 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 = items[*element.Index] + default: + items, _ := current.([]any) + index, ok := listItemIndex(items, element) + if !ok { + return nil, false + } + current = items[index] + } + } + return current, true +} + +// 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 + } + + 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 order +} + +// 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..735e0dac --- /dev/null +++ b/pkg/component/applied_change_integration_test.go @@ -0,0 +1,171 @@ +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")) + }) + + 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 new file mode 100644 index 00000000..7ab690f2 --- /dev/null +++ b/pkg/component/applied_change_test.go @@ -0,0 +1,378 @@ +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) + }) + } +} + +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 315bb55f..f7611667 100644 --- a/pkg/component/concepts/converging.go +++ b/pkg/component/concepts/converging.go @@ -13,21 +13,30 @@ 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, 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: its labels, annotations, owner references, spec or data differ - // from what was observed before the apply. + // 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/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..1f8e24c0 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -389,12 +389,24 @@ 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`. 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/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