Fixes placement of pod annotations for opamp bridge (#4264)

* fix test

* use pod annotations and normal annotations

* fix test

* fix tests

Author: jaronoff97

.chloggen/fix-bridge-rollout-issue.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: opamp
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: fixes a bug where the bridge deployment wouldn't rollout on a config change.
+
+# One or more tracking issues related to the change
+issues: [4020]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:

internal/controllers/builder_test.go

@@ -1021,9 +1021,7 @@ func TestBuildAll_OpAMPBridge(t *testing.T) {
 							"app.kubernetes.io/part-of":    "opentelemetry",
 							"app.kubernetes.io/version":    "latest",
 						},
-						Annotations: map[string]string{
-							"opentelemetry-opampbridge-config/hash": "05e1dc681267a9bc28fc2877ab464a98b9bd043843f14ffc0b4a394b5c86ba9f",
-						},
+						Annotations: map[string]string{},
 					},
 					Spec: appsv1.DeploymentSpec{
 						Replicas: &one,
@@ -1040,6 +1038,9 @@ func TestBuildAll_OpAMPBridge(t *testing.T) {
 									"app.kubernetes.io/part-of":    "opentelemetry",
 									"app.kubernetes.io/version":    "latest",
 								},
+								Annotations: map[string]string{
+									"opentelemetry-opampbridge-config/hash": "05e1dc681267a9bc28fc2877ab464a98b9bd043843f14ffc0b4a394b5c86ba9f",
+								},
 							},
 							Spec: corev1.PodSpec{
 								Volumes: []corev1.Volume{

internal/manifests/opampbridge/annotations.go

@@ -6,6 +6,7 @@ package opampbridge
 import (
 	"crypto/sha256"
 	"fmt"
+	"maps"
 
 	v1 "k8s.io/api/core/v1"
 
@@ -15,13 +16,27 @@ import (
 
 const configMapHashAnnotationKey = "opentelemetry-opampbridge-config/hash"
 
-// Annotations returns the annotations for the OPAmpBridge Pod.
-func Annotations(instance v1alpha1.OpAMPBridge, configMap *v1.ConfigMap, filterAnnotations []string) map[string]string {
+// Annotations return the annotations for OpenTelemetryCollector resources.
+func Annotations(instance v1alpha1.OpAMPBridge, filterAnnotations []string) map[string]string {
+	// new map every time, so that we don't touch the instance's annotations
+	annotations := map[string]string{}
+
+	if instance.ObjectMeta.Annotations != nil {
+		for k, v := range instance.ObjectMeta.Annotations {
+			if !manifestutils.IsFilteredSet(k, filterAnnotations) {
+				annotations[k] = v
+			}
+		}
+	}
+
+	return annotations
+}
+
+// PodAnnotations returns the annotations for the OPAmpBridge Pod.
+func PodAnnotations(instance v1alpha1.OpAMPBridge, configMap *v1.ConfigMap, filterAnnotations []string) map[string]string {
 	// Make a copy of PodAnnotations to be safe
 	annotations := make(map[string]string, len(instance.Spec.PodAnnotations))
-	for key, value := range instance.Spec.PodAnnotations {
-		annotations[key] = value
-	}
+	maps.Copy(annotations, instance.Spec.PodAnnotations)
 	if instance.ObjectMeta.Annotations != nil {
 		for k, v := range instance.ObjectMeta.Annotations {
 			if !manifestutils.IsFilteredSet(k, filterAnnotations) {

internal/manifests/opampbridge/annotations_test.go

@@ -42,7 +42,7 @@ func TestConfigMapHash(t *testing.T) {
 	expectedConfig := expectedConfigMap.Data[OpAMPBridgeFilename]
 	require.NotEmpty(t, expectedConfig)
 	expectedHash := sha256.Sum256([]byte(expectedConfig))
-	annotations := Annotations(opampBridge, expectedConfigMap, []string{".*\\.bar\\.io"})
+	annotations := PodAnnotations(opampBridge, expectedConfigMap, []string{".*\\.bar\\.io"})
 	require.Contains(t, annotations, configMapHashAnnotationKey)
 	cmHash := annotations[configMapHashAnnotationKey]
 	assert.Equal(t, fmt.Sprintf("%x", expectedHash), cmHash)

internal/manifests/opampbridge/deployment.go

@@ -22,7 +22,8 @@ func Deployment(params manifests.Params) *appsv1.Deployment {
 		params.Log.Info("failed to construct OpAMPBridge ConfigMap for annotations")
 		configMap = nil
 	}
-	annotations := Annotations(params.OpAMPBridge, configMap, params.Config.AnnotationsFilter)
+	podAnnotations := PodAnnotations(params.OpAMPBridge, configMap, params.Config.AnnotationsFilter)
+	annotations := Annotations(params.OpAMPBridge, params.Config.AnnotationsFilter)
 	return &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        name,
@@ -38,7 +39,7 @@ func Deployment(params manifests.Params) *appsv1.Deployment {
 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels:      labels,
-					Annotations: params.OpAMPBridge.Spec.PodAnnotations,
+					Annotations: podAnnotations,
 				},
 				Spec: corev1.PodSpec{
 					ServiceAccountName:        ServiceAccountName(params.OpAMPBridge),

internal/manifests/opampbridge/deployment_test.go

@@ -109,7 +109,10 @@ func TestDeploymentNewDefault(t *testing.T) {
 
 func TestDeploymentPodAnnotations(t *testing.T) {
 	// prepare
-	testPodAnnotationValues := map[string]string{"annotation-key": "annotation-value"}
+	testPodAnnotationValues := map[string]string{
+		"annotation-key":                        "annotation-value",
+		"opentelemetry-opampbridge-config/hash": "ca3d163bab055381827226140568f3bef7eaac187cebd76878e0b63e9e442356",
+	}
 	opampBridge := v1alpha1.OpAMPBridge{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "my-instance",
@@ -130,7 +133,7 @@ func TestDeploymentPodAnnotations(t *testing.T) {
 	d := Deployment(params)
 
 	// verify
-	assert.Len(t, d.Spec.Template.Annotations, 1)
+	assert.Len(t, d.Spec.Template.Annotations, 2)
 	assert.Equal(t, "my-instance-opamp-bridge", d.Name)
 	assert.Equal(t, testPodAnnotationValues, d.Spec.Template.Annotations)
 }
@@ -271,9 +274,12 @@ func TestDeploymentFilterAnnotations(t *testing.T) {
 
 	d := Deployment(params)
 
-	assert.Len(t, d.ObjectMeta.Annotations, 2)
+	assert.Len(t, d.ObjectMeta.Annotations, 1)
 	assert.NotContains(t, d.ObjectMeta.Annotations, "foo")
 	assert.NotContains(t, d.ObjectMeta.Annotations, "app.foo.bar")
+	assert.Len(t, d.Spec.Template.ObjectMeta.Annotations, 2)
+	assert.NotContains(t, d.Spec.Template.ObjectMeta.Annotations, "foo")
+	assert.NotContains(t, d.Spec.Template.ObjectMeta.Annotations, "app.foo.bar")
 }
 
 func TestDeploymentNodeSelector(t *testing.T) {