Merge pull request #5580 from fabriziopandini/topology-should-not-gen…

…erate-empty-metadata

🐛 topology should not generate empty patch for metadata

controllers/topology/desired_state.go

@@ -162,6 +162,9 @@ func computeControlPlane(_ context.Context, s *scope.Scope, infrastructureMachin
 		clusterClassMetadata := s.Blueprint.ClusterClass.Spec.ControlPlane.Metadata
 
 		machineLabels := mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels)
+		if machineLabels == nil {
+			machineLabels = map[string]string{}
+		}
 		machineLabels[clusterv1.ClusterLabelName] = cluster.Name
 		machineLabels[clusterv1.ClusterTopologyOwnedLabel] = ""
 		if err := contract.ControlPlane().MachineTemplate().Metadata().Set(controlPlane,
@@ -401,7 +404,7 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 	labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name
 	desiredMachineDeploymentObj.SetLabels(labels)
 
-	// Set the select with the subset of labels identifying controlled machines.
+	// Set the selector with the subset of labels identifying controlled machines.
 	// NOTE: this prevents the web hook to add cluster.x-k8s.io/deployment-name label, that is
 	// redundant for managed MachineDeployments given that we already have topology.cluster.x-k8s.io/deployment-name.
 	desiredMachineDeploymentObj.Spec.Selector.MatchLabels = map[string]string{}
@@ -412,6 +415,9 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 	// Also set the labels in .spec.template.labels so that they are propagated to
 	// MachineSet.labels and MachineSet.spec.template.labels and thus to Machine.labels.
 	// Note: the labels in MachineSet are used to properly cleanup templates when the MachineSet is deleted.
+	if desiredMachineDeploymentObj.Spec.Template.Labels == nil {
+		desiredMachineDeploymentObj.Spec.Template.Labels = map[string]string{}
+	}
 	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name
 	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyOwnedLabel] = ""
 	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name
@@ -618,5 +624,12 @@ func mergeMap(a, b map[string]string) map[string]string {
 	for k, v := range a {
 		m[k] = v
 	}
+
+	// Nil the result if the map is empty, thus avoiding triggering infinite reconcile
+	// given that at json level label: {} or annotation: {} is different from no field, which is the
+	// corresponding value stored in etcd given that those fields are defined as omitempty.
+	if len(m) == 0 {
+		return nil
+	}
 	return m
 }

controllers/topology/desired_state_test.go

@@ -1325,3 +1325,28 @@ func duplicateMachineDeploymentsState(s scope.MachineDeploymentsStateMap) scope.
 	}
 	return n
 }
+
+func TestMergeMap(t *testing.T) {
+	t.Run("Merge maps", func(t *testing.T) {
+		g := NewWithT(t)
+
+		m := mergeMap(
+			map[string]string{
+				"a": "a",
+				"b": "b",
+			}, map[string]string{
+				"a": "ax",
+				"c": "c",
+			},
+		)
+		g.Expect(m).To(HaveKeyWithValue("a", "a"))
+		g.Expect(m).To(HaveKeyWithValue("b", "b"))
+		g.Expect(m).To(HaveKeyWithValue("c", "c"))
+	})
+	t.Run("Nils empty maps", func(t *testing.T) {
+		g := NewWithT(t)
+
+		m := mergeMap(map[string]string{}, map[string]string{})
+		g.Expect(m).To(BeNil())
+	})
+}

controllers/topology/internal/contract/metadata.go

@@ -17,8 +17,6 @@ limitations under the License.
 package contract
 
 import (
-	"strings"
-
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -42,7 +40,7 @@ func (m *Metadata) Get(obj *unstructured.Unstructured) (*clusterv1.ObjectMeta, e
 		return nil, errors.Wrap(err, "failed to retrieve control plane metadata.labels")
 	}
 	if !ok {
-		return nil, errors.Errorf("%s not found", "."+strings.Join(labelsPath, "."))
+		labelsValue = map[string]string{}
 	}
 
 	annotationsPath := append(m.path, "annotations")
@@ -51,7 +49,7 @@ func (m *Metadata) Get(obj *unstructured.Unstructured) (*clusterv1.ObjectMeta, e
 		return nil, errors.Wrap(err, "failed to retrieve control plane metadata.annotations")
 	}
 	if !ok {
-		return nil, errors.Errorf("%s not found", "."+strings.Join(annotationsPath, "."))
+		annotationsValue = map[string]string{}
 	}
 
 	return &clusterv1.ObjectMeta{
@@ -61,15 +59,24 @@ func (m *Metadata) Get(obj *unstructured.Unstructured) (*clusterv1.ObjectMeta, e
 }
 
 // Set sets the metadata value.
+// Note: We are blanking out empty label annotations, thus avoiding triggering infinite reconcile
+// given that at json level label: {} or annotation: {} is different from no field, which is the
+// corresponding value stored in etcd given that those fields are defined as omitempty.
 func (m *Metadata) Set(obj *unstructured.Unstructured, metadata *clusterv1.ObjectMeta) error {
 	labelsPath := append(m.path, "labels")
-	if err := unstructured.SetNestedStringMap(obj.UnstructuredContent(), metadata.Labels, labelsPath...); err != nil {
-		return errors.Wrap(err, "failed to set control plane metadata.labels")
+	unstructured.RemoveNestedField(obj.UnstructuredContent(), labelsPath...)
+	if len(metadata.Labels) > 0 {
+		if err := unstructured.SetNestedStringMap(obj.UnstructuredContent(), metadata.Labels, labelsPath...); err != nil {
+			return errors.Wrap(err, "failed to set control plane metadata.labels")
+		}
 	}
 
 	annotationsPath := append(m.path, "annotations")
-	if err := unstructured.SetNestedStringMap(obj.UnstructuredContent(), metadata.Annotations, annotationsPath...); err != nil {
-		return errors.Wrap(err, "failed to set control plane metadata.annotations")
+	unstructured.RemoveNestedField(obj.UnstructuredContent(), annotationsPath...)
+	if len(metadata.Annotations) > 0 {
+		if err := unstructured.SetNestedStringMap(obj.UnstructuredContent(), metadata.Annotations, annotationsPath...); err != nil {
+			return errors.Wrap(err, "failed to set control plane metadata.annotations")
+		}
 	}
 	return nil
 }

controllers/topology/internal/contract/metadata_test.go

@@ -0,0 +1,72 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package contract
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+)
+
+func TestMetadata(t *testing.T) {
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+
+	t.Run("Manages metadata", func(t *testing.T) {
+		g := NewWithT(t)
+
+		metadata := &clusterv1.ObjectMeta{
+			Labels: map[string]string{
+				"label1": "labelValue1",
+			},
+			Annotations: map[string]string{
+				"annotation1": "annotationValue1",
+			},
+		}
+
+		m := Metadata{path: Path{"foo"}}
+		g.Expect(m.Path()).To(Equal(Path{"foo"}))
+
+		err := m.Set(obj, metadata)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		got, err := m.Get(obj)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).ToNot(BeNil())
+		g.Expect(got).To(Equal(metadata))
+	})
+	t.Run("Manages empty metadata", func(t *testing.T) {
+		g := NewWithT(t)
+
+		metadata := &clusterv1.ObjectMeta{
+			Labels:      map[string]string{},
+			Annotations: map[string]string{},
+		}
+
+		m := Metadata{path: Path{"foo"}}
+		g.Expect(m.Path()).To(Equal(Path{"foo"}))
+
+		err := m.Set(obj, metadata)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		got, err := m.Get(obj)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).ToNot(BeNil())
+		g.Expect(got).To(Equal(metadata))
+	})
+}