Merge pull request #6880 from sbueringer/pr-optimize-ssa-dryrun

🐛 SSA: ignore diff of other managers

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
 }

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,14 +129,26 @@ 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 {
 	return objectBuilder{b.u.DeepCopy()}
 }
 
 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()
 }
 

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) {