diff --git a/controllers/rabbitmqcluster_controller_test.go b/controllers/rabbitmqcluster_controller_test.go index cfe00c1b9..e6d1cdf98 100644 --- a/controllers/rabbitmqcluster_controller_test.go +++ b/controllers/rabbitmqcluster_controller_test.go @@ -533,35 +533,6 @@ var _ = Describe("RabbitmqClusterController", func() { return roleBinding.Annotations }, 3).Should(HaveKeyWithValue(annotationKey, annotationValue)) }) - - It("updates annotations for config maps", func() { - Eventually(func() map[string]string { - roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("server-conf"), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - return roleBinding.Annotations - }, 3).Should(HaveKeyWithValue(annotationKey, annotationValue)) - - Eventually(func() map[string]string { - roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("plugins-conf"), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - return roleBinding.Annotations - }, 3).Should(HaveKeyWithValue(annotationKey, annotationValue)) - }) - - It("updates annotations for config maps", func() { - Eventually(func() map[string]string { - roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("server-conf"), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - return roleBinding.Annotations - }, 3).Should(HaveKeyWithValue(annotationKey, annotationValue)) - - Eventually(func() map[string]string { - roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("plugins-conf"), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - return roleBinding.Annotations - }, 3).Should(HaveKeyWithValue(annotationKey, annotationValue)) - }) - }) It("service type is updated", func() { diff --git a/internal/resource/configmap.go b/internal/resource/configmap.go index 91d4eb85e..8f568afd6 100644 --- a/internal/resource/configmap.go +++ b/internal/resource/configmap.go @@ -55,16 +55,16 @@ func (builder *RabbitmqResourceBuilder) ServerConfigMap() *ServerConfigMapBuilde func (builder *ServerConfigMapBuilder) Build() (runtime.Object, error) { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Name: builder.Instance.ChildResourceName(ServerConfigMapName), - Namespace: builder.Instance.Namespace, + Name: builder.Instance.ChildResourceName(ServerConfigMapName), + Namespace: builder.Instance.Namespace, + Labels: metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels), + Annotations: metadata.ReconcileAndFilterAnnotations(nil, builder.Instance.Annotations), }, }, nil } func (builder *ServerConfigMapBuilder) Update(object runtime.Object) error { configMap := object.(*corev1.ConfigMap) - configMap.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels) - configMap.Annotations = metadata.ReconcileAndFilterAnnotations(configMap.GetAnnotations(), builder.Instance.Annotations) ini.PrettySection = false // Remove trailing new line because rabbitmq.conf has only a default section. cfg, err := ini.Load([]byte(defaultRabbitmqConf)) diff --git a/internal/resource/configmap_test.go b/internal/resource/configmap_test.go index 7d3528286..467f41f0e 100644 --- a/internal/resource/configmap_test.go +++ b/internal/resource/configmap_test.go @@ -66,6 +66,22 @@ var _ = Describe("GenerateServerConfigMap", func() { var configMap *corev1.ConfigMap BeforeEach(func() { + instance.Labels = map[string]string{ + "app.kubernetes.io/foo": "bar", + "foo": "bar", + "rabbitmq": "is-great", + "foo/app.kubernetes.io": "edgecase", + } + instance.Annotations = map[string]string{ + "my-annotation": "i-like-this", + "kubernetes.io/name": "i-do-not-like-this", + "kubectl.kubernetes.io/name": "i-do-not-like-this", + "k8s.io/name": "i-do-not-like-this", + "kubernetes.io/other": "i-do-not-like-this", + "kubectl.kubernetes.io/other": "i-do-not-like-this", + "k8s.io/other": "i-do-not-like-this", + } + obj, err := configMapBuilder.Build() configMap = obj.(*corev1.ConfigMap) Expect(err).NotTo(HaveOccurred()) @@ -75,6 +91,32 @@ var _ = Describe("GenerateServerConfigMap", func() { Expect(configMap.Name).To(Equal(builder.Instance.ChildResourceName("server-conf"))) Expect(configMap.Namespace).To(Equal(builder.Instance.Namespace)) }) + + It("adds labels from the instance and default labels", func() { + Expect(configMap.Labels).To(SatisfyAll( + HaveLen(6), + HaveKeyWithValue("foo", "bar"), + HaveKeyWithValue("rabbitmq", "is-great"), + HaveKeyWithValue("foo/app.kubernetes.io", "edgecase"), + HaveKeyWithValue("app.kubernetes.io/name", instance.Name), + HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"), + HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"), + Not(HaveKey("app.kubernetes.io/foo")), + )) + }) + + It("adds annotations from the instance", func() { + Expect(configMap.Annotations).To(SatisfyAll( + HaveLen(1), + HaveKeyWithValue("my-annotation", "i-like-this"), + Not(HaveKey("kubernetes.io/name")), + Not(HaveKey("kubectl.kubernetes.io/name")), + Not(HaveKey("k8s.io/name")), + Not(HaveKey("kubernetes.io/other")), + Not(HaveKey("kubectl.kubernetes.io/other")), + Not(HaveKey("k8s.io/other")), + )) + }) }) Context("Update", func() { @@ -284,94 +326,33 @@ ssl_options.verify = verify_peer }) }) - Context("labels", func() { - BeforeEach(func() { - instance = rabbitmqv1beta1.RabbitmqCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rabbit-labelled", - }, - } - instance.Labels = map[string]string{ - "app.kubernetes.io/foo": "bar", - "foo": "bar", - "rabbitmq": "is-great", - "foo/app.kubernetes.io": "edgecase", - } - - configMap = &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/name": instance.Name, - "app.kubernetes.io/part-of": "rabbitmq", - "this-was-the-previous-label": "should-be-deleted", - }, - }, - } - err := configMapBuilder.Update(configMap) - Expect(err).NotTo(HaveOccurred()) - }) - - It("adds labels from the CR", func() { - testLabels(configMap.Labels) - }) - - It("restores the default labels", func() { - labels := configMap.Labels - Expect(labels["app.kubernetes.io/name"]).To(Equal(instance.Name)) - Expect(labels["app.kubernetes.io/component"]).To(Equal("rabbitmq")) - Expect(labels["app.kubernetes.io/part-of"]).To(Equal("rabbitmq")) - }) - - It("deletes the labels that are removed from the CR", func() { - Expect(configMap.Labels).NotTo(HaveKey("this-was-the-previous-label")) - }) + // this is to ensure that pods are not restarted when instance labels are updated + It("does not update labels on the config map", func() { + configMap.Labels = map[string]string{ + "app.kubernetes.io/name": instance.Name, + "app.kubernetes.io/component": "rabbitmq", + "app.kubernetes.io/part-of": "rabbitmq", + } + instance.Labels = map[string]string{ + "new-label": "test", + } + Expect(configMapBuilder.Update(configMap)).To(Succeed()) + Expect(configMap.Labels).To(SatisfyAll( + HaveLen(3), + HaveKeyWithValue("app.kubernetes.io/name", instance.Name), + HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"), + HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"), + Not(HaveKey("new-label")), + )) }) - Context("instance annotations", func() { - BeforeEach(func() { - instance = rabbitmqv1beta1.RabbitmqCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rabbit-labelled", - }, - } - instance.Annotations = map[string]string{ - "my-annotation": "i-like-this", - "kubernetes.io/name": "i-do-not-like-this", - "kubectl.kubernetes.io/name": "i-do-not-like-this", - "k8s.io/name": "i-do-not-like-this", - "kubernetes.io/other": "i-do-not-like-this", - "kubectl.kubernetes.io/other": "i-do-not-like-this", - "k8s.io/other": "i-do-not-like-this", - } - - configMap = &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "my-annotation": "i-will-not-stay", - "old-annotation": "old-value", - "im-here-to-stay.kubernetes.io": "for-a-while", - "kubernetes.io/name": "should-stay", - "kubectl.kubernetes.io/name": "should-stay", - "k8s.io/name": "should-stay", - }, - }, - } - err := configMapBuilder.Update(configMap) - Expect(err).NotTo(HaveOccurred()) - }) - - It("updates config map annotations", func() { - expectedAnnotations := map[string]string{ - "my-annotation": "i-like-this", - "old-annotation": "old-value", - "im-here-to-stay.kubernetes.io": "for-a-while", - "kubernetes.io/name": "should-stay", - "kubectl.kubernetes.io/name": "should-stay", - "k8s.io/name": "should-stay", - } - - Expect(configMap.Annotations).To(Equal(expectedAnnotations)) - }) + // this is to ensure that pods are not restarted when instance annotations are updated + It("does not update annotations on the config map", func() { + instance.Annotations = map[string]string{ + "new-annotation": "test", + } + Expect(configMapBuilder.Update(configMap)).To(Succeed()) + Expect(configMap.Annotations).To(BeEmpty()) }) }) }) diff --git a/internal/resource/rabbitmq_plugins.go b/internal/resource/rabbitmq_plugins.go index da2145577..3d3ac38be 100644 --- a/internal/resource/rabbitmq_plugins.go +++ b/internal/resource/rabbitmq_plugins.go @@ -36,8 +36,10 @@ func (builder *RabbitmqResourceBuilder) RabbitmqPluginsConfigMap() *RabbitmqPlug func (builder *RabbitmqPluginsConfigMapBuilder) Build() (runtime.Object, error) { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Name: builder.Instance.ChildResourceName(PluginsConfigName), - Namespace: builder.Instance.Namespace, + Name: builder.Instance.ChildResourceName(PluginsConfigName), + Namespace: builder.Instance.Namespace, + Labels: metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels), + Annotations: metadata.ReconcileAndFilterAnnotations(nil, builder.Instance.Annotations), }, Data: map[string]string{ "enabled_plugins": desiredPluginsAsString([]rabbitmqv1beta1.Plugin{}), @@ -47,8 +49,6 @@ func (builder *RabbitmqPluginsConfigMapBuilder) Build() (runtime.Object, error) func (builder *RabbitmqPluginsConfigMapBuilder) Update(object runtime.Object) error { configMap := object.(*corev1.ConfigMap) - configMap.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels) - configMap.Annotations = metadata.ReconcileAndFilterAnnotations(configMap.GetAnnotations(), builder.Instance.Annotations) if configMap.Data == nil { configMap.Data = make(map[string]string) diff --git a/internal/resource/rabbitmq_plugins_test.go b/internal/resource/rabbitmq_plugins_test.go index 6f5db0c22..7165af6a4 100644 --- a/internal/resource/rabbitmq_plugins_test.go +++ b/internal/resource/rabbitmq_plugins_test.go @@ -90,6 +90,22 @@ var _ = Describe("RabbitMQPlugins", func() { var configMap *corev1.ConfigMap BeforeEach(func() { + instance.Labels = map[string]string{ + "app.kubernetes.io/foo": "bar", + "foo": "bar", + "rabbitmq": "is-great", + "foo/app.kubernetes.io": "edgecase", + } + instance.Annotations = map[string]string{ + "my-annotation": "i-like-this", + "kubernetes.io/name": "i-do-not-like-this", + "kubectl.kubernetes.io/name": "i-do-not-like-this", + "k8s.io/name": "i-do-not-like-this", + "kubernetes.io/other": "i-do-not-like-this", + "kubectl.kubernetes.io/other": "i-do-not-like-this", + "k8s.io/other": "i-do-not-like-this", + } + obj, err := configMapBuilder.Build() configMap = obj.(*corev1.ConfigMap) Expect(err).NotTo(HaveOccurred()) @@ -112,6 +128,32 @@ var _ = Describe("RabbitMQPlugins", func() { configMap = obj.(*corev1.ConfigMap) Expect(configMap.Data).To(HaveKeyWithValue("enabled_plugins", expectedEnabledPlugins)) }) + + It("adds labels from the instance and default labels", func() { + Expect(configMap.Labels).To(SatisfyAll( + HaveLen(6), + HaveKeyWithValue("foo", "bar"), + HaveKeyWithValue("rabbitmq", "is-great"), + HaveKeyWithValue("foo/app.kubernetes.io", "edgecase"), + HaveKeyWithValue("app.kubernetes.io/name", instance.Name), + HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"), + HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"), + Not(HaveKey("app.kubernetes.io/foo")), + )) + }) + + It("adds annotations from the instance", func() { + Expect(configMap.Annotations).To(SatisfyAll( + HaveLen(1), + HaveKeyWithValue("my-annotation", "i-like-this"), + Not(HaveKey("kubernetes.io/name")), + Not(HaveKey("kubectl.kubernetes.io/name")), + Not(HaveKey("k8s.io/name")), + Not(HaveKey("kubernetes.io/other")), + Not(HaveKey("kubectl.kubernetes.io/other")), + Not(HaveKey("k8s.io/other")), + )) + }) }) Context("Update", func() { @@ -177,6 +219,34 @@ var _ = Describe("RabbitMQPlugins", func() { }) }) }) + + // ensures that we are not unnecessarily running `rabbitmq-plugins set` when CR labels are updated + It("does not update labels on the config map", func() { + configMap.Labels = map[string]string{ + "app.kubernetes.io/name": instance.Name, + "app.kubernetes.io/component": "rabbitmq", + "app.kubernetes.io/part-of": "rabbitmq", + } + instance.Labels = map[string]string{ + "new-label": "test", + } + Expect(configMapBuilder.Update(configMap)).To(Succeed()) + Expect(configMap.Labels).To(SatisfyAll( + HaveLen(3), + HaveKeyWithValue("app.kubernetes.io/name", instance.Name), + HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"), + HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"), + Not(HaveKey("new-label")), + )) + }) + // ensures that we are not unnecessarily running `rabbitmq-plugins set` when CR annotations are updated + It("does not update annotations on the config map", func() { + instance.Annotations = map[string]string{ + "new-annotation": "test", + } + Expect(configMapBuilder.Update(configMap)).To(Succeed()) + Expect(configMap.Annotations).To(BeEmpty()) + }) }) }) }) diff --git a/internal/resource/statefulset.go b/internal/resource/statefulset.go index dc0ab89f2..d1b1d94d5 100644 --- a/internal/resource/statefulset.go +++ b/internal/resource/statefulset.go @@ -114,17 +114,12 @@ func (builder *StatefulSetBuilder) Update(object runtime.Object) error { //Annotations sts.Annotations = metadata.ReconcileAndFilterAnnotations(sts.Annotations, builder.Instance.Annotations) - defaultPodAnnotations := map[string]string{ - "prometheus.io/scrape": "true", - "prometheus.io/port": "15692", - } - podAnnotations := metadata.ReconcileAnnotations(defaultPodAnnotations, metadata.ReconcileAndFilterAnnotations(sts.Spec.Template.Annotations, builder.Instance.Annotations)) //Labels - updatedLabels := metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels) - sts.Labels = updatedLabels + sts.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels) - sts.Spec.Template = builder.podTemplateSpec(podAnnotations, updatedLabels) + // pod template + sts.Spec.Template = builder.podTemplateSpec(sts.Spec.Template.Annotations) if !sts.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().Equal(*sts.Spec.Template.Spec.Containers[0].Resources.Requests.Memory()) { logger := ctrl.Log.WithName("statefulset").WithName("RabbitmqCluster") @@ -261,7 +256,13 @@ func sortEnvVar(envVar []corev1.EnvVar) { } } -func (builder *StatefulSetBuilder) podTemplateSpec(annotations, labels map[string]string) corev1.PodTemplateSpec { +func (builder *StatefulSetBuilder) podTemplateSpec(previousPodAnnotations map[string]string) corev1.PodTemplateSpec { + // default pod annotations used for prometheus metrics + defaultPodAnnotations := map[string]string{ + "prometheus.io/scrape": "true", + "prometheus.io/port": "15692", + } + //Init Container resources cpuRequest := k8sresource.MustParse(initContainerCPU) memoryRequest := k8sresource.MustParse(initContainerMemory) @@ -510,8 +511,8 @@ func (builder *StatefulSetBuilder) podTemplateSpec(annotations, labels map[strin return corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Annotations: annotations, - Labels: labels, + Annotations: metadata.ReconcileAnnotations(defaultPodAnnotations, previousPodAnnotations), + Labels: metadata.Label(builder.Instance.Name), }, Spec: corev1.PodSpec{ TopologySpreadConstraints: []corev1.TopologySpreadConstraint{ diff --git a/internal/resource/statefulset_test.go b/internal/resource/statefulset_test.go index 1038ba614..428241dff 100644 --- a/internal/resource/statefulset_test.go +++ b/internal/resource/statefulset_test.go @@ -401,11 +401,20 @@ var _ = Describe("StatefulSet", func() { testLabels(statefulSet.Labels) }) - It("has the labels from the instance on the pod", func() { + It("adds default labels to pods but does not populate labels from the instance onto pods", func() { stsBuilder := builder.StatefulSet() Expect(stsBuilder.Update(statefulSet)).To(Succeed()) - podTemplate := statefulSet.Spec.Template - testLabels(podTemplate.Labels) + + Expect(statefulSet.Spec.Template.ObjectMeta.Labels).To(SatisfyAll( + HaveLen(3), + HaveKeyWithValue("app.kubernetes.io/name", instance.Name), + HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"), + HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"), + Not(HaveKey("foo")), + Not(HaveKey("rabbitmq")), + Not(HaveKey("foo/app.kubernetes.io")), + Not(HaveKey("app.kubernetes.io/foo")), + )) }) It("adds the correct labels on the statefulset", func() { @@ -438,16 +447,6 @@ var _ = Describe("StatefulSet", func() { Expect(stsBuilder.Update(statefulSet)).To(Succeed()) Expect(statefulSet.Labels).NotTo(HaveKey("this-was-the-previous-label")) }) - - It("adds the correct labels on the rabbitmq pods", func() { - stsBuilder := builder.StatefulSet() - Expect(stsBuilder.Update(statefulSet)).To(Succeed()) - - labels := statefulSet.Spec.Template.ObjectMeta.Labels - Expect(labels["app.kubernetes.io/name"]).To(Equal(instance.Name)) - Expect(labels["app.kubernetes.io/component"]).To(Equal("rabbitmq")) - Expect(labels["app.kubernetes.io/part-of"]).To(Equal("rabbitmq")) - }) }) Context("annotations", func() { @@ -483,8 +482,6 @@ var _ = Describe("StatefulSet", func() { "prometheus.io/scrape": "true", "prometheus.io/port": "15692", "this-was-the-previous-pod-anno": "should-be-preserved", - "app.kubernetes.io/part-of": "rabbitmq-pod", - "app.k8s.io/something": "something-amazing-on-pod", } existingPvcTemplateAnnotations = map[string]string{ @@ -501,7 +498,7 @@ var _ = Describe("StatefulSet", func() { statefulSet.Spec.VolumeClaimTemplates[0].Annotations = existingPvcTemplateAnnotations }) - It("updates annotations", func() { + It("updates sts annotations", func() { stsBuilder.Instance.Annotations = map[string]string{ "kubernetes.io/name": "i-do-not-like-this", "kubectl.kubernetes.io/name": "i-do-not-like-this", @@ -523,28 +520,23 @@ var _ = Describe("StatefulSet", func() { Expect(statefulSet.Annotations).To(Equal(expectedAnnotations)) }) - It("update annotations from the instance to the pod", func() { + It("adds default annotations but does not populate annotations from the instance to the pod", func() { stsBuilder.Instance.Annotations = map[string]string{ - "kubernetes.io/name": "i-do-not-like-this", - "kubectl.kubernetes.io/name": "i-do-not-like-this", - "k8s.io/name": "i-do-not-like-this", - "kubernetes.io/other": "i-do-not-like-this", - "kubectl.kubernetes.io/other": "i-do-not-like-this", - "k8s.io/other": "i-do-not-like-this", - "my-annotation": "i-like-this", + "my-annotation": "my-annotation", + "k8s.io/other": "i-do-not-like-this", } Expect(stsBuilder.Update(statefulSet)).To(Succeed()) - expectedAnnotations := map[string]string{ - "prometheus.io/scrape": "true", - "prometheus.io/port": "15692", - "my-annotation": "i-like-this", - "app.kubernetes.io/part-of": "rabbitmq-pod", - "this-was-the-previous-pod-anno": "should-be-preserved", - "app.k8s.io/something": "something-amazing-on-pod", - } - - Expect(statefulSet.Spec.Template.Annotations).To(Equal(expectedAnnotations)) + Expect(statefulSet.Spec.Template.Annotations).To(SatisfyAll( + HaveLen(3), + HaveKeyWithValue("prometheus.io/scrape", "true"), + HaveKeyWithValue("prometheus.io/port", "15692"), + HaveKeyWithValue("this-was-the-previous-pod-anno", "should-be-preserved"), + Not(HaveKey("app.kubernetes.io/part-of")), + Not(HaveKey("app.k8s.io/something")), + Not(HaveKey("my-annotation")), + Not(HaveKey("k8s.io/other")), + )) }) It("does not update annotations from the instance to the pvc template", func() {