Refactor E2E script, reduce excess reconciling of resources

Signed-off-by: Jonathan West <jonwest@redhat.com>

controllers/configmap.go

@@ -146,12 +146,12 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro
 		actualConfigMap.Data[TrafficRouterPluginConfigMapKey] = string(desiredTrafficRouterPluginString)
 		actualConfigMap.Data[MetricPluginConfigMapKey] = string(desiredMetricPluginString)
 
+		log.Info("Updating Rollouts ConfigMap due to detected difference")
+
 		// Update the ConfigMap in the cluster
 		if err := r.Client.Update(ctx, actualConfigMap); err != nil {
 			return fmt.Errorf("failed to update ConfigMap: %v", err)
 		}
-		log.Info("ConfigMap updated successfully")
-
 		// Restarting rollouts pod only if configMap is updated
 		if err := r.restartRolloutsPod(ctx, cr.Namespace); err != nil {
 			return err
@@ -183,8 +183,8 @@ func (r *RolloutManagerReconciler) restartRolloutsPod(ctx context.Context, names
 
 	for i := range podList.Items {
 		pod := podList.Items[i]
-		log.Info("Deleting Rollouts Pod", "podName", pod.Name)
 		if pod.ObjectMeta.DeletionTimestamp == nil {
+			log.Info("Deleting Rollouts Pod", "podName", pod.Name)
 			if err := r.Client.Delete(ctx, &pod); err != nil {
 				if errors.IsNotFound(err) {
 					log.Info(fmt.Sprintf("Pod %s already deleted", pod.Name))

controllers/configmap_test.go

@@ -9,7 +9,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	appsv1 "k8s.io/api/apps/v1"
-	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -19,7 +18,7 @@ var _ = Describe("ConfigMap Test", func() {
 	var a v1alpha1.RolloutManager
 	var r *RolloutManagerReconciler
 	var sa *corev1.ServiceAccount
-	var existingDeployment *v1.Deployment
+	var existingDeployment *appsv1.Deployment
 	const trafficrouterPluginLocation = "https://custom-traffic-plugin-location"
 	const metricPluginLocation = "https://custom-metric-plugin-location"
 

controllers/resources.go

@@ -46,7 +46,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsServiceAccount(ctx context.C
 	normalizedLiveServiceAccount := liveServiceAccount.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveServiceAccount.ObjectMeta, cr)
 
-	if !reflect.DeepEqual(normalizedLiveServiceAccount.Labels, expectedServiceAccount.Labels) || !reflect.DeepEqual(normalizedLiveServiceAccount.Annotations, expectedServiceAccount.Annotations) {
+	if !areStringMapsEqual(normalizedLiveServiceAccount.Labels, expectedServiceAccount.Labels) || !areStringMapsEqual(normalizedLiveServiceAccount.Annotations, expectedServiceAccount.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of ServiceAccount %s do not match the expected state, hence updating it", liveServiceAccount.Name))
 
@@ -111,7 +111,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsRole(ctx context.Context, cr
 
 	removeUserLabelsAndAnnotations(&normalizedLiveRole.ObjectMeta, cr)
 
-	if !reflect.DeepEqual(normalizedLiveRole.Labels, expectedRole.Labels) || !reflect.DeepEqual(normalizedLiveRole.Annotations, expectedRole.Annotations) {
+	if !areStringMapsEqual(normalizedLiveRole.Labels, expectedRole.Labels) || !areStringMapsEqual(normalizedLiveRole.Annotations, expectedRole.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of Role %s do not match the expected state, hence updating it", liveRole.Name))
 
@@ -165,7 +165,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsClusterRole(ctx context.Cont
 	normalizedLiveClusterRole := liveClusterRole.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
 
-	if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
+	if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of Role %s do not match the expected state, hence updating it", liveClusterRole.Name))
 
@@ -248,7 +248,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsRoleBinding(ctx context.Cont
 
 	normalizedLiveRoleBinding := liveRoleBinding.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveRoleBinding.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveRoleBinding.Labels, expectedRoleBinding.Labels) || !reflect.DeepEqual(normalizedLiveRoleBinding.Annotations, expectedRoleBinding.Annotations) {
+	if !areStringMapsEqual(normalizedLiveRoleBinding.Labels, expectedRoleBinding.Labels) || !areStringMapsEqual(normalizedLiveRoleBinding.Annotations, expectedRoleBinding.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of RoleBinding %s do not match the expected state, hence updating it", liveRoleBinding.Name))
 
@@ -327,7 +327,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsClusterRoleBinding(ctx conte
 
 	normalizedLiveClusterRoleBinding := liveClusterRoleBinding.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveClusterRoleBinding.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveClusterRoleBinding.Labels, expectedClusterRoleBinding.Labels) || !reflect.DeepEqual(normalizedLiveClusterRoleBinding.Annotations, expectedClusterRoleBinding.Annotations) {
+	if !areStringMapsEqual(normalizedLiveClusterRoleBinding.Labels, expectedClusterRoleBinding.Labels) || !areStringMapsEqual(normalizedLiveClusterRoleBinding.Annotations, expectedClusterRoleBinding.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of ClusterRoleBinding %s do not match the expected state, hence updating it", liveClusterRoleBinding.Name))
 
@@ -460,7 +460,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToAdminClusterRole(
 
 	normalizedLiveClusterRole := liveClusterRole.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
+	if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))
 
@@ -512,7 +512,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToEditClusterRole(c
 
 	normalizedLiveClusterRole := liveClusterRole.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
+	if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))
 
@@ -564,7 +564,8 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToViewClusterRole(c
 
 	normalizedLiveClusterRole := liveClusterRole.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
+	if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
+
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))
 
@@ -702,7 +703,8 @@ func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.C
 
 	normalizedLiveService := liveService.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveService.ObjectMeta, cr)
-	if !reflect.DeepEqual(normalizedLiveService.Labels, expectedSvc.Labels) || !reflect.DeepEqual(normalizedLiveService.Annotations, expectedSvc.Annotations) {
+	if !areStringMapsEqual(normalizedLiveService.Labels, expectedSvc.Labels) || !areStringMapsEqual(normalizedLiveService.Annotations, expectedSvc.Annotations) {
+
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of metrics Service %s do not match the expected state, hence updating it", liveService.Name))
 
@@ -778,7 +780,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsSecrets(ctx context.Context,
 	normalizedLiveSecret := liveSecret.DeepCopy()
 	removeUserLabelsAndAnnotations(&normalizedLiveSecret.ObjectMeta, cr)
 
-	if !reflect.DeepEqual(normalizedLiveSecret.Labels, expectedSecret.Labels) || !reflect.DeepEqual(normalizedLiveSecret.Annotations, expectedSecret.Annotations) {
+	if !areStringMapsEqual(normalizedLiveSecret.Labels, expectedSecret.Labels) || !areStringMapsEqual(normalizedLiveSecret.Annotations, expectedSecret.Annotations) {
 		updateNeeded = true
 		log.Info(fmt.Sprintf("Labels/Annotations of Secret %s do not match the expected state, hence updating it", liveSecret.Name))
 

controllers/utils.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"reflect"
 	"sort"
 	"strings"
 
@@ -86,6 +87,30 @@ func appendStringMap(src map[string]string, add map[string]string) map[string]st
 	return res
 }
 
+// In go, reflect.DeepEqual(a, b), on maps a and b, will return false even when len(a) == 0 and len(b) == 0. This is due to a nil map being considered different from an empty map.
+// This function handles this case better: if maps both have a length of 0, we consider them always equal.
+func areStringMapsEqual(one map[string]string, two map[string]string) bool {
+
+	isOneEmpty := false
+
+	isTwoEmpty := false
+
+	if len(one) == 0 {
+		isOneEmpty = true
+	}
+
+	if len(two) == 0 {
+		isTwoEmpty = true
+	}
+
+	if isOneEmpty && isTwoEmpty {
+		return true
+	}
+
+	return reflect.DeepEqual(one, two)
+
+}
+
 // combineStringMaps will combine multiple maps: maps defined earlier in the 'maps' slice may have their values overriden by maps defined later in the 'maps' slice.
 func combineStringMaps(maps ...map[string]string) map[string]string {
 
@@ -448,6 +473,11 @@ func removeUserLabelsAndAnnotations(obj *metav1.ObjectMeta, cr rolloutsmanagerv1
 
 	for objectLabelKey := range obj.Labels {
 
+		// We add this label to the aggregated cluster roles we generate, so we should not process it as a user label.
+		if strings.Contains(objectLabelKey, "rbac.authorization.k8s.io/") && strings.Contains(objectLabelKey, "aggregate") {
+			continue
+		}
+
 		existsInDefault := false
 
 		for defaultLabelKey := range defaultLabelsAndAnnotations.Labels {

controllers/utils_test.go

@@ -452,42 +452,36 @@ var _ = Describe("removeUserLabelsAndAnnotations tests", func() {
 			}
 
 			Expect(k8sClient.Create(ctx, &cr)).To(Succeed())
-			setRolloutsLabelsAndAnnotations(&obj)
 
 			removeUserLabelsAndAnnotations(&obj, cr)
 
 			Expect(obj.Labels).To(Equal(expectedLabels))
 			Expect(obj.Annotations).To(Equal(expectedAnnotations))
 		},
 		Entry("when no user-defined labels or annotations exist",
-			map[string]string{}, map[string]string{},
-			map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/part-of":   DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
+			map[string]string{},
+			map[string]string{},
+			map[string]string{},
 			map[string]string{},
 		),
 		Entry("when user-defined labels and annotations are present",
-			map[string]string{"user-label": "value"}, map[string]string{"user-annotation": "value"},
-			map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/part-of":   DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
+			map[string]string{"user-label": "value"},
+			map[string]string{"user-annotation": "value"},
+			map[string]string{},
 			map[string]string{},
 		),
 		Entry("when user-defined labels are present and annotations are empty",
-			map[string]string{"user-label": "value"}, map[string]string{},
-			map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/part-of":   DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
+			map[string]string{"user-label": "value"},
+			map[string]string{},
+			map[string]string{},
 			map[string]string{},
 		),
 		Entry("when user-defined labels are empty and annotations are present",
-			map[string]string{}, map[string]string{"user-annotation": "value"},
-			map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/part-of":   DefaultArgoRolloutsResourceName,
-				"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
+			map[string]string{},
+			map[string]string{"user-annotation": "value"},
+			map[string]string{},
 			map[string]string{},
 		),
-
 		Entry("when both user and non-user-defined labels are present, and annotations are empty",
 			map[string]string{"user-label": "value",
 				"app.kubernetes.io/name":      DefaultArgoRolloutsResourceName,
@@ -499,6 +493,20 @@ var _ = Describe("removeUserLabelsAndAnnotations tests", func() {
 				"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
 			map[string]string{},
 		),
+		Entry("rbac aggregate-to- labels should not be removed, as we add these to ClusterRoles",
+			map[string]string{
+				"rbac.authorization.k8s.io/aggregate-to-admin": "true",
+				"app.kubernetes.io/name":                       DefaultArgoRolloutsResourceName,
+				"app.kubernetes.io/part-of":                    DefaultArgoRolloutsResourceName,
+				"app.kubernetes.io/component":                  DefaultArgoRolloutsResourceName},
+			map[string]string{},
+			map[string]string{
+				"rbac.authorization.k8s.io/aggregate-to-admin": "true",
+				"app.kubernetes.io/name":                       DefaultArgoRolloutsResourceName,
+				"app.kubernetes.io/part-of":                    DefaultArgoRolloutsResourceName,
+				"app.kubernetes.io/component":                  DefaultArgoRolloutsResourceName},
+			map[string]string{},
+		),
 	)
 })
 
@@ -683,6 +691,23 @@ var _ = Describe("setAdditionalRolloutsLabelsAndAnnotationsToObject tests", func
 	})
 })
 
+var _ = Describe("areStringMapsEqual tests", func() {
+	DescribeTable("areStringMapsEqual should match reflect.DeepEqual(), except when comparing nil with empty map", func(one map[string]string, two map[string]string, expectedRes bool) {
+
+		res := areStringMapsEqual(one, two)
+		Expect(res).To(Equal(expectedRes))
+	},
+		Entry("both nil", nil, nil, true),
+		Entry("one nil, one empty", map[string]string{}, nil, true),
+		Entry("one empty, one nil", nil, map[string]string{}, true),
+		Entry("both empty", map[string]string{}, map[string]string{}, true),
+		Entry("one empty, one has value", map[string]string{}, map[string]string{"key": "value"}, false),
+		Entry("one has value, one nil", map[string]string{"key": "value"}, nil, false),
+		Entry("equal values", map[string]string{"key": "value"}, map[string]string{"key": "value"}, true),
+	)
+
+})
+
 var _ = Describe("envMerge tests", func() {
 	DescribeTable("merges two slices of EnvVar entries",
 		func(existing, merge, expected []corev1.EnvVar, override bool) {