Return copy of annotations in grpcCatalogSourceDecorator (#2813)

* Update catalog source pod creation to not mutate parameters

Signed-off-by: perdasilva <[email protected]>

* Update pkg/controller/registry/reconciler/reconciler_test.go

Co-authored-by: Alexander Greene <[email protected]>
Signed-off-by: perdasilva <[email protected]>

Co-authored-by: Alexander Greene <[email protected]>

Author: perdasilva

pkg/controller/registry/reconciler/reconciler.go

@@ -113,14 +113,26 @@ func Pod(source *operatorsv1alpha1.CatalogSource, name string, image string, saN
 		pullPolicy = corev1.PullAlways
 	}
 
+	// make a copy of the labels and annotations to avoid mutating the input parameters
+	podLabels := make(map[string]string)
+	podAnnotations := make(map[string]string)
+
+	for key, value := range labels {
+		podLabels[key] = value
+	}
+
+	for key, value := range annotations {
+		podAnnotations[key] = value
+	}
+
 	readOnlyRootFilesystem := false
 
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: source.GetName() + "-",
 			Namespace:    source.GetNamespace(),
-			Labels:       labels,
-			Annotations:  annotations,
+			Labels:       podLabels,
+			Annotations:  podAnnotations,
 		},
 		Spec: corev1.PodSpec{
 			Containers: []corev1.Container{
@@ -207,25 +219,17 @@ func Pod(source *operatorsv1alpha1.CatalogSource, name string, image string, saN
 	}
 
 	// Set priorityclass if its annotation exists
-	if prio, ok := annotations[CatalogPriorityClassKey]; ok && prio != "" {
+	if prio, ok := podAnnotations[CatalogPriorityClassKey]; ok && prio != "" {
 		pod.Spec.PriorityClassName = prio
 	}
 
 	// Add PodSpec hash
 	// This hash info will be used to detect PodSpec changes
-	if labels == nil {
-		labels = make(map[string]string)
-	}
-	labels[PodHashLabelKey] = hashPodSpec(pod.Spec)
-	pod.SetLabels(labels)
+	podLabels[PodHashLabelKey] = hashPodSpec(pod.Spec)
 
 	// add eviction annotation to enable the cluster autoscaler to evict the pod in order to drain the node
 	// since catalog pods are not backed by a controller, they cannot be evicted by default
-	if annotations == nil {
-		annotations = make(map[string]string)
-	}
-	annotations[ClusterAutoscalingAnnotationKey] = "true"
-	pod.SetAnnotations(annotations)
+	podAnnotations[ClusterAutoscalingAnnotationKey] = "true"
 
 	return pod
 }

pkg/controller/registry/reconciler/reconciler_test.go

@@ -95,6 +95,37 @@ func TestPodContainerSecurityContext(t *testing.T) {
 	require.Equal(t, expectedContainerSecCtx, gotContainerSecCtx)
 }
 
+// TestPodAvoidsConcurrentWrite is a regression test for
+// https://bugzilla.redhat.com/show_bug.cgi?id=2101357
+// we were mutating the input annotations and labels parameters causing
+// concurrent write issues
+func TestPodAvoidsConcurrentWrite(t *testing.T) {
+	catsrc := &v1alpha1.CatalogSource{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "testns",
+		},
+	}
+
+	labels := map[string]string{
+		"label": "something",
+	}
+
+	annotations := map[string]string{
+		"annotation": "somethingelse",
+	}
+
+	gotPod := Pod(catsrc, "hello", "busybox", "", labels, annotations, int32(0), int32(0))
+
+	// check labels and annotations point to different addresses between parameters and what's in the pod
+	require.NotEqual(t, &labels, &gotPod.Labels)
+	require.NotEqual(t, &annotations, &gotPod.Annotations)
+
+	// check that labels and annotations from the parameters were copied down to the pod's
+	require.Equal(t, labels["label"], gotPod.Labels["label"])
+	require.Equal(t, annotations["annotation"], gotPod.Annotations["annotation"])
+}
+
 func TestPodSchedulingOverrides(t *testing.T) {
 	// This test ensures that any overriding pod scheduling configuration elements
 	// defined in spec.grpcPodConfig are applied to the catalog source pod created