SSA: ignore diff of other managers

kubernetes-sigs · Jul 8, 2022 · 9dd1cf9 · 9dd1cf9
1 parent 26943ea
commit 9dd1cf9
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 21 deletions.
diff --git a/internal/controllers/topology/cluster/structuredmerge/dryrun.go b/internal/controllers/topology/cluster/structuredmerge/dryrun.go
@@ -66,12 +66,12 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
 	// 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 {
+	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 {
+	if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.originalUnstructured); err != nil {
 		return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on originalUnstructured")
 	}
 
@@ -106,11 +106,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 +120,16 @@ func cleanupTopologyDryRunAnnotation(obj *unstructured.Unstructured) error {
 		}),
 	})
 
-	// Adjust the managed field for Manager=TopologyManagerName, Subresource="", Operation="Apply"
-	managedFields := obj.GetManagedFields()
-	for i, managedField := range managedFields {
+	// We're done if there are no managed fields.
+	oldManagedFields := obj.GetManagedFields()
+	if len(oldManagedFields) == 0 {
+		return nil
+	}
+
+	// Adjust the managed field for Manager=TopologyManagerName, Subresource="", Operation="Apply" and
+	// drop managed fields of other controllers.
+	newManagedFields := []metav1.ManagedFieldsEntry{}
+	for _, managedField := range oldManagedFields {
 		if managedField.Manager != TopologyManagerName {
 			continue
 		}
@@ -157,9 +164,14 @@ 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)
+	}
+
+	if len(newManagedFields) == 0 {
+		// Setting the managedFields to nil will remove the managed fields field entirely.
+		obj.SetManagedFields(nil)
+	} else {
+		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
@@ -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 {