From 4ffcef48154442245507d82b9147f45f572e561b Mon Sep 17 00:00:00 2001 From: DWonMtl Date: Thu, 19 May 2022 07:08:25 -0400 Subject: [PATCH] avoid non static labels in workload objects selector (#849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Gagné --- pkg/collector/daemonset.go | 2 +- pkg/collector/daemonset_test.go | 29 ++++++++++++++++++++++---- pkg/collector/deployment.go | 2 +- pkg/collector/deployment_test.go | 29 ++++++++++++++++++++++---- pkg/collector/labels.go | 20 ++++++++++++++---- pkg/collector/labels_test.go | 19 +++++++++++++++++ pkg/collector/reconcile/daemonset.go | 3 +++ pkg/collector/reconcile/deployment.go | 3 +++ pkg/collector/reconcile/statefulset.go | 3 +++ pkg/collector/serviceaccount.go | 2 +- pkg/collector/statefulset.go | 2 +- pkg/collector/statefulset_test.go | 29 ++++++++++++++++++++++---- 12 files changed, 123 insertions(+), 20 deletions(-) diff --git a/pkg/collector/daemonset.go b/pkg/collector/daemonset.go index 5af932957d..dec920a094 100644 --- a/pkg/collector/daemonset.go +++ b/pkg/collector/daemonset.go @@ -41,7 +41,7 @@ func DaemonSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem }, Spec: appsv1.DaemonSetSpec{ Selector: &metav1.LabelSelector{ - MatchLabels: labels, + MatchLabels: SelectorLabels(otelcol), }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/collector/daemonset_test.go b/pkg/collector/daemonset_test.go index eab9cd9d6a..f1705adc39 100644 --- a/pkg/collector/daemonset_test.go +++ b/pkg/collector/daemonset_test.go @@ -30,7 +30,8 @@ func TestDaemonSetNewDefault(t *testing.T) { // prepare otelcol := v1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-instance", + Name: "my-instance", + Namespace: "my-namespace", }, Spec: v1alpha1.OpenTelemetryCollectorSpec{ Tolerations: testTolerationValues, @@ -57,8 +58,28 @@ func TestDaemonSetNewDefault(t *testing.T) { } assert.Equal(t, expectedAnnotations, d.Spec.Template.Annotations) - // the pod selector should match the pod spec's labels - assert.Equal(t, d.Spec.Selector.MatchLabels, d.Spec.Template.Labels) + expectedLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/name": "my-instance-collector", + "app.kubernetes.io/part-of": "opentelemetry", + "app.kubernetes.io/version": "latest", + } + assert.Equal(t, expectedLabels, d.Spec.Template.Labels) + + expectedSelectorLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/part-of": "opentelemetry", + } + assert.Equal(t, expectedSelectorLabels, d.Spec.Selector.MatchLabels) + + // the pod selector must be contained within pod spec's labels + for k, v := range d.Spec.Selector.MatchLabels { + assert.Equal(t, v, d.Spec.Template.Labels[k]) + } } func TestDaemonsetHostNetwork(t *testing.T) { @@ -95,7 +116,7 @@ func TestDaemonsetPodAnnotations(t *testing.T) { // test ds := DaemonSet(cfg, logger, otelcol) - //Add sha256 podAnnotation + // Add sha256 podAnnotation testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" // verify diff --git a/pkg/collector/deployment.go b/pkg/collector/deployment.go index f69f6203fa..bc0e4ce7c7 100644 --- a/pkg/collector/deployment.go +++ b/pkg/collector/deployment.go @@ -43,7 +43,7 @@ func Deployment(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTele Spec: appsv1.DeploymentSpec{ Replicas: otelcol.Spec.Replicas, Selector: &metav1.LabelSelector{ - MatchLabels: labels, + MatchLabels: SelectorLabels(otelcol), }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/collector/deployment_test.go b/pkg/collector/deployment_test.go index 145ede5329..f9e1be9353 100644 --- a/pkg/collector/deployment_test.go +++ b/pkg/collector/deployment_test.go @@ -38,7 +38,8 @@ func TestDeploymentNewDefault(t *testing.T) { // prepare otelcol := v1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-instance", + Name: "my-instance", + Namespace: "my-namespace", }, Spec: v1alpha1.OpenTelemetryCollectorSpec{ Tolerations: testTolerationValues, @@ -65,8 +66,28 @@ func TestDeploymentNewDefault(t *testing.T) { } assert.Equal(t, expectedAnnotations, d.Spec.Template.Annotations) - // the pod selector should match the pod spec's labels - assert.Equal(t, d.Spec.Template.Labels, d.Spec.Selector.MatchLabels) + expectedLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/name": "my-instance-collector", + "app.kubernetes.io/part-of": "opentelemetry", + "app.kubernetes.io/version": "latest", + } + assert.Equal(t, expectedLabels, d.Spec.Template.Labels) + + expectedSelectorLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/part-of": "opentelemetry", + } + assert.Equal(t, expectedSelectorLabels, d.Spec.Selector.MatchLabels) + + // the pod selector must be contained within pod spec's labels + for k, v := range d.Spec.Selector.MatchLabels { + assert.Equal(t, v, d.Spec.Template.Labels[k]) + } } func TestDeploymentPodAnnotations(t *testing.T) { @@ -85,7 +106,7 @@ func TestDeploymentPodAnnotations(t *testing.T) { // test d := Deployment(cfg, logger, otelcol) - //Add sha256 podAnnotation + // Add sha256 podAnnotation testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" // verify diff --git a/pkg/collector/labels.go b/pkg/collector/labels.go index 5f1baba832..311ba6f7e9 100644 --- a/pkg/collector/labels.go +++ b/pkg/collector/labels.go @@ -43,10 +43,10 @@ func Labels(instance v1alpha1.OpenTelemetryCollector, filterLabels []string) map } } - base["app.kubernetes.io/managed-by"] = "opentelemetry-operator" - base["app.kubernetes.io/instance"] = naming.Truncate("%s.%s", 63, instance.Namespace, instance.Name) - base["app.kubernetes.io/part-of"] = "opentelemetry" - base["app.kubernetes.io/component"] = "opentelemetry-collector" + for k, v := range SelectorLabels(instance) { + base[k] = v + } + version := strings.Split(instance.Spec.Image, ":") if len(version) > 1 { base["app.kubernetes.io/version"] = version[len(version)-1] @@ -56,3 +56,15 @@ func Labels(instance v1alpha1.OpenTelemetryCollector, filterLabels []string) map return base } + +// SelectorLabels return the common labels to all objects that are part of a managed OpenTelemetryCollector to use as selector. +// Selector labels are immutable for Deployment, StatefulSet and DaemonSet, therefore, no labels in selector should be +// expected to be modified for the lifetime of the object. +func SelectorLabels(instance v1alpha1.OpenTelemetryCollector) map[string]string { + return map[string]string{ + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/instance": naming.Truncate("%s.%s", 63, instance.Namespace, instance.Name), + "app.kubernetes.io/part-of": "opentelemetry", + "app.kubernetes.io/component": "opentelemetry-collector", + } +} diff --git a/pkg/collector/labels_test.go b/pkg/collector/labels_test.go index 9e9be95685..2116b9f192 100644 --- a/pkg/collector/labels_test.go +++ b/pkg/collector/labels_test.go @@ -97,3 +97,22 @@ func TestLabelsFilter(t *testing.T) { assert.NotContains(t, labels, "test.bar.io") assert.Equal(t, "bar", labels["test.foo.io"]) } + +func TestSelectorLabels(t *testing.T) { + // prepare + expected := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-opentelemetry-collector", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/part-of": "opentelemetry", + } + otelcol := v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{Name: "my-opentelemetry-collector", Namespace: "my-namespace"}, + } + + // test + result := SelectorLabels(otelcol) + + // verify + assert.Equal(t, expected, result) +} diff --git a/pkg/collector/reconcile/daemonset.go b/pkg/collector/reconcile/daemonset.go index ff4f4a134e..be597cd7e3 100644 --- a/pkg/collector/reconcile/daemonset.go +++ b/pkg/collector/reconcile/daemonset.go @@ -89,6 +89,9 @@ func expectedDaemonSets(ctx context.Context, params Params, expected []appsv1.Da updated.ObjectMeta.Labels[k] = v } + // Selector is an immutable field, if set, we cannot modify it otherwise we will face reconciliation error. + updated.Spec.Selector = existing.Spec.Selector.DeepCopy() + patch := client.MergeFrom(existing) if err := params.Client.Patch(ctx, updated, patch); err != nil { return fmt.Errorf("failed to apply changes: %w", err) diff --git a/pkg/collector/reconcile/deployment.go b/pkg/collector/reconcile/deployment.go index d8bb01d54e..b57a030c9f 100644 --- a/pkg/collector/reconcile/deployment.go +++ b/pkg/collector/reconcile/deployment.go @@ -104,6 +104,9 @@ func expectedDeployments(ctx context.Context, params Params, expected []appsv1.D updated.Spec.Replicas = ¤tReplicas } + // Selector is an immutable field, if set, we cannot modify it otherwise we will face reconciliation error. + updated.Spec.Selector = existing.Spec.Selector.DeepCopy() + patch := client.MergeFrom(existing) if err := params.Client.Patch(ctx, updated, patch); err != nil { diff --git a/pkg/collector/reconcile/statefulset.go b/pkg/collector/reconcile/statefulset.go index 0f8baecdf3..372b28d3b0 100644 --- a/pkg/collector/reconcile/statefulset.go +++ b/pkg/collector/reconcile/statefulset.go @@ -90,6 +90,9 @@ func expectedStatefulSets(ctx context.Context, params Params, expected []appsv1. updated.ObjectMeta.Labels[k] = v } + // Selector is an immutable field, if set, we cannot modify it otherwise we will face reconciliation error. + updated.Spec.Selector = existing.Spec.Selector.DeepCopy() + patch := client.MergeFrom(existing) if err := params.Client.Patch(ctx, updated, patch); err != nil { return fmt.Errorf("failed to apply changes: %w", err) diff --git a/pkg/collector/serviceaccount.go b/pkg/collector/serviceaccount.go index ac7e61ca4f..4804b83b3a 100644 --- a/pkg/collector/serviceaccount.go +++ b/pkg/collector/serviceaccount.go @@ -31,7 +31,7 @@ func ServiceAccountName(instance v1alpha1.OpenTelemetryCollector) string { return instance.Spec.ServiceAccount } -//ServiceAccount returns the service account for the given instance. +// ServiceAccount returns the service account for the given instance. func ServiceAccount(otelcol v1alpha1.OpenTelemetryCollector) corev1.ServiceAccount { labels := Labels(otelcol, []string{}) labels["app.kubernetes.io/name"] = naming.ServiceAccount(otelcol) diff --git a/pkg/collector/statefulset.go b/pkg/collector/statefulset.go index 04fe18b334..cfe20fcee6 100644 --- a/pkg/collector/statefulset.go +++ b/pkg/collector/statefulset.go @@ -43,7 +43,7 @@ func StatefulSet(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTel Spec: appsv1.StatefulSetSpec{ ServiceName: naming.Service(otelcol), Selector: &metav1.LabelSelector{ - MatchLabels: labels, + MatchLabels: SelectorLabels(otelcol), }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/collector/statefulset_test.go b/pkg/collector/statefulset_test.go index bfdc6e26bd..add60afba6 100644 --- a/pkg/collector/statefulset_test.go +++ b/pkg/collector/statefulset_test.go @@ -33,7 +33,8 @@ func TestStatefulSetNewDefault(t *testing.T) { // prepare otelcol := v1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-instance", + Name: "my-instance", + Namespace: "my-namespace", }, Spec: v1alpha1.OpenTelemetryCollectorSpec{ Mode: "statefulset", @@ -61,8 +62,28 @@ func TestStatefulSetNewDefault(t *testing.T) { } assert.Equal(t, expectedAnnotations, ss.Spec.Template.Annotations) - // the pod selector should match the pod spec's labels - assert.Equal(t, ss.Spec.Selector.MatchLabels, ss.Spec.Template.Labels) + expectedLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/name": "my-instance-collector", + "app.kubernetes.io/part-of": "opentelemetry", + "app.kubernetes.io/version": "latest", + } + assert.Equal(t, expectedLabels, ss.Spec.Template.Labels) + + expectedSelectorLabels := map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "my-namespace.my-instance", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/part-of": "opentelemetry", + } + assert.Equal(t, expectedSelectorLabels, ss.Spec.Selector.MatchLabels) + + // the pod selector must be contained within pod spec's labels + for k, v := range ss.Spec.Selector.MatchLabels { + assert.Equal(t, v, ss.Spec.Template.Labels[k]) + } // assert correct service name assert.Equal(t, "my-instance-collector", ss.Spec.ServiceName) @@ -144,7 +165,7 @@ func TestStatefulSetPodAnnotations(t *testing.T) { // test ss := StatefulSet(cfg, logger, otelcol) - //Add sha256 podAnnotation + // Add sha256 podAnnotation testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" // verify