diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun.go b/internal/controllers/topology/cluster/structuredmerge/dryrun.go index 9f17792e4c0e..bc4998532e96 100644 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun.go +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun.go @@ -63,15 +63,26 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, } // Cleanup the dryRunUnstructured object to remove the added TopologyDryRunAnnotation - // and remove the affected managedFields for `manager=capi-topology` which would + // and remove the affected managedFields for `manager=capi-topology` which would // otherwise show the additional field ownership for the annotation we added and // the changed managedField timestamp. - if err := cleanupTopologyDryRunAnnotation(dryRunCtx.dryRunUnstructured); err != nil { + // We also drop managedFields of other managers as we don't care about if other managers + // made changes to the object. We only want to trigger a ServerSideApply if we would make + // changes to the object. + // Please note that if other managers made changes to fields that we care about and thus ownership changed, + // this would affect our managed fields as well and we would still detect it by diffing our managed fields. + if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.dryRunUnstructured); err != nil { return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on dryRunUnstructured") } + // Also run the function for the originalUnstructured to remove the managedField // timestamp for `manager=capi-topology`. - if err := cleanupTopologyDryRunAnnotation(dryRunCtx.originalUnstructured); err != nil { + // We also drop managedFields of other managers as we don't care about if other managers + // made changes to the object. We only want to trigger a ServerSideApply if we would make + // changes to the object. + // Please note that if other managers made changes to fields that we care about and thus ownership changed, + // this would affect our managed fields as well and we would still detect it by diffing our managed fields. + if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.originalUnstructured); err != nil { return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on originalUnstructured") } @@ -106,11 +117,11 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, return hasChanges, hasSpecChanges, nil } -// cleanupTopologyDryRunAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run +// cleanupManagedFieldsAndAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run // annotation as well as the field ownership reference in managedFields. It does // also remove the timestamp of the managedField for `manager=capi-topology` because // it is expected to change due to the additional annotation. -func cleanupTopologyDryRunAnnotation(obj *unstructured.Unstructured) error { +func cleanupManagedFieldsAndAnnotation(obj *unstructured.Unstructured) error { // Filter the topology.cluster.x-k8s.io/dry-run annotation as well as leftover empty maps. filterIntent(&filterIntentInput{ path: contract.Path{}, @@ -120,9 +131,11 @@ func cleanupTopologyDryRunAnnotation(obj *unstructured.Unstructured) error { }), }) - // Adjust the managed field for Manager=TopologyManagerName, Subresource="", Operation="Apply" - managedFields := obj.GetManagedFields() - for i, managedField := range managedFields { + // Adjust the managed field for Manager=TopologyManagerName, Subresource="", Operation="Apply" and + // drop managed fields of other controllers. + oldManagedFields := obj.GetManagedFields() + newManagedFields := []metav1.ManagedFieldsEntry{} + for _, managedField := range oldManagedFields { if managedField.Manager != TopologyManagerName { continue } @@ -157,10 +170,10 @@ func cleanupTopologyDryRunAnnotation(obj *unstructured.Unstructured) error { } managedField.FieldsV1.Raw = fieldsV1Raw - // Replace the modified managedField entry at the slice. - managedFields[i] = managedField - obj.SetManagedFields(managedFields) + newManagedFields = append(newManagedFields, managedField) } + obj.SetManagedFields(newManagedFields) + return nil } diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go index 76ca70e1cff4..ebe2eb39af54 100644 --- a/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/dryrun_test.go @@ -27,7 +27,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -func Test_cleanupTopologyDryRunAnnotation(t *testing.T) { +func Test_cleanupManagedFieldsAndAnnotation(t *testing.T) { rawManagedFieldWithAnnotation := `{"f:metadata":{"f:annotations":{"f:topology.cluster.x-k8s.io/dry-run":{}}}}` rawManagedFieldWithAnnotationSpecLabels := `{"f:metadata":{"f:annotations":{"f:topology.cluster.x-k8s.io/dry-run":{}},"f:labels":{}},"f:spec":{"f:foo":{}}}` rawManagedFieldWithSpecLabels := `{"f:metadata":{"f:labels":{}},"f:spec":{"f:foo":{}}}` @@ -53,33 +53,30 @@ func Test_cleanupTopologyDryRunAnnotation(t *testing.T) { Build(), }, { - name: "managedFields: manager does not match", + name: "managedFields: should drop managed fields of other manager", obj: newObjectBuilder(). WithManagedFieldsEntry("other", "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). Build(), wantErr: false, want: newObjectBuilder(). - WithManagedFieldsEntry("other", "", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). Build(), }, { - name: "managedFields: subresource does not match", + name: "managedFields: should drop managed fields of a subresource", obj: newObjectBuilder(). WithManagedFieldsEntry(TopologyManagerName, "status", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). Build(), wantErr: false, want: newObjectBuilder(). - WithManagedFieldsEntry(TopologyManagerName, "status", metav1.ManagedFieldsOperationApply, []byte(`{}`), nil). Build(), }, { - name: "managedFields: operation does not match", + name: "managedFields: should drop managed fields of another operation", obj: newObjectBuilder(). WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationUpdate, []byte(`{}`), nil). Build(), wantErr: false, want: newObjectBuilder(). - WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationUpdate, []byte(`{}`), nil). Build(), }, { @@ -93,7 +90,7 @@ func Test_cleanupTopologyDryRunAnnotation(t *testing.T) { Build(), }, { - name: "managedFields: cleanup the managed field entry but preserve other ownership", + name: "managedFields: cleanup the managed field entry and preserve other ownership", obj: newObjectBuilder(). WithManagedFieldsEntry(TopologyManagerName, "", metav1.ManagedFieldsOperationApply, []byte(rawManagedFieldWithAnnotationSpecLabels), nil). Build(), @@ -117,8 +114,8 @@ func Test_cleanupTopologyDryRunAnnotation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - if err := cleanupTopologyDryRunAnnotation(tt.obj); (err != nil) != tt.wantErr { - t.Errorf("cleanupTopologyDryRunAnnotation() error = %v, wantErr %v", err, tt.wantErr) + if err := cleanupManagedFieldsAndAnnotation(tt.obj); (err != nil) != tt.wantErr { + t.Errorf("cleanupManagedFieldsAndAnnotation() error = %v, wantErr %v", err, tt.wantErr) } if tt.want != nil { g.Expect(tt.obj).To(BeEquivalentTo(tt.want)) @@ -132,7 +129,13 @@ type objectBuilder struct { } func newObjectBuilder() objectBuilder { - return objectBuilder{&unstructured.Unstructured{Object: map[string]interface{}{}}} + u := &unstructured.Unstructured{Object: map[string]interface{}{}} + u.SetName("test") + u.SetNamespace("default") + + return objectBuilder{ + u: u, + } } func (b objectBuilder) DeepCopy() objectBuilder { @@ -140,6 +143,12 @@ func (b objectBuilder) DeepCopy() objectBuilder { } func (b objectBuilder) Build() *unstructured.Unstructured { + // Setting an empty managed field array if no managed field is set so there is + // no difference between an object which never had a managed field and one from + // which all managed field entries were removed. + if b.u.GetManagedFields() == nil { + b.u.SetManagedFields([]metav1.ManagedFieldsEntry{}) + } return b.u.DeepCopy() } diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 8336059216a4..71b254908832 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go @@ -73,7 +73,6 @@ func TestServerSideApply(t *testing.T) { g.Expect(p0.HasChanges()).To(BeTrue()) g.Expect(p0.HasSpecChanges()).To(BeTrue()) }) - t.Run("When creating an object using server side apply, it should track managed fields for the topology controller", func(t *testing.T) { g := NewWithT(t) @@ -104,7 +103,6 @@ func TestServerSideApply(t *testing.T) { g.Expect(controlPlaneEndpointFieldV1).To(HaveKey("f:host")) // topology controller should express opinions on spec.controlPlaneEndpoint.host. g.Expect(controlPlaneEndpointFieldV1).To(HaveKey("f:port")) // topology controller should express opinions on spec.controlPlaneEndpoint.port. }) - t.Run("Server side apply patch helper detects no changes", func(t *testing.T) { g := NewWithT(t) @@ -236,6 +234,9 @@ func TestServerSideApply(t *testing.T) { obj := obj.DeepCopy() g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + // Store object before another controller applies changes. + original := obj.DeepCopy() + // Create a patch helper like we do/recommend doing in the controllers and use it to apply some changes. p, err := patch.NewHelper(obj, env.Client) g.Expect(err).ToNot(HaveOccurred()) @@ -246,6 +247,17 @@ func TestServerSideApply(t *testing.T) { g.Expect(unstructured.SetNestedField(obj.Object, true, "status", "ready")).To(Succeed()) // Required field g.Expect(p.Patch(ctx, obj)).To(Succeed()) + + // Verify that the topology controller detects no changes after another controller changed fields. + // Note: We verify here that the ServerSidePatchHelper ignores changes in managed fields of other controllers. + // There's also a change in .spec.bar that is intentionally not ignored by the controller, we have to ignore + // it here to be able to verify that managed field changes are ignored. This is the same situation as when + // other controllers update .status (that is ignored) and the ServerSidePatchHelper then ignores the corresponding + // managed field changes. + p0, err := NewServerSidePatchHelper(ctx, original, original, env.GetClient(), IgnorePaths{{"spec", "foo"}, {"spec", "bar"}}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p0.HasChanges()).To(BeFalse()) + g.Expect(p0.HasSpecChanges()).To(BeFalse()) }) t.Run("Topology controller reconcile again with no changes on topology managed fields", func(t *testing.T) {