avoid non static labels in workload objects selector (#849)

Signed-off-by: Daniel Gagné <daniel.gagne.2@ens.etsmtl.ca>
open-telemetry · May 19, 2022 · 4ffcef4 · 4ffcef4
1 parent 8e502ff
commit 4ffcef4
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -104,6 +104,9 @@ func expectedDeployments(ctx context.Context, params Params, expected []appsv1.D
 			updated.Spec.Replicas = &currentReplicas
 		}
 
+		// 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
@@ -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
@@ -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
@@ -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
@@ -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