From 9430c0387b7aee5d766c5d07c9d0d0cda9d2e243 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Wed, 3 Nov 2021 20:29:28 +0100 Subject: [PATCH] topology should not generate empty metadata --- controllers/topology/desired_state.go | 15 +++- controllers/topology/desired_state_test.go | 25 +++++++ .../topology/internal/contract/metadata.go | 23 +++--- .../internal/contract/metadata_test.go | 72 +++++++++++++++++++ 4 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 controllers/topology/internal/contract/metadata_test.go diff --git a/controllers/topology/desired_state.go b/controllers/topology/desired_state.go index 4541f5e06555..ceaeb072aef3 100644 --- a/controllers/topology/desired_state.go +++ b/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 } diff --git a/controllers/topology/desired_state_test.go b/controllers/topology/desired_state_test.go index ab5f56def733..f57df5f05e3d 100644 --- a/controllers/topology/desired_state_test.go +++ b/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()) + }) +} diff --git a/controllers/topology/internal/contract/metadata.go b/controllers/topology/internal/contract/metadata.go index eed2c9915912..801a148cd646 100644 --- a/controllers/topology/internal/contract/metadata.go +++ b/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 } diff --git a/controllers/topology/internal/contract/metadata_test.go b/controllers/topology/internal/contract/metadata_test.go new file mode 100644 index 000000000000..9921a4382b2e --- /dev/null +++ b/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)) + }) +}