Refactor controller tests

Controller tests are flaking way too often. One of the test flakes was
around the "retain" feature. These tests were buggy and should have been
failing consistently; they were flaking to green. The problem was a
create-delete right away, without awaiting for validation on creation.
Sometimes, the create was registered in etcd, and it was causing a
legitimate failure (due to test setup, production code was fine). When
the create did not complete and, in parallel, the deletion executed, it
skipped the deletion (because the object did not exist in etcd yet),
therefore, it skipped the HTTP calls, passing the test assertion
(incorrectly).

The other refactor is to provide better test isolation. Some flakes were
happening, rarely, due to test polution, where the manager for one test
was reconciling the object for another test. After the refactor, each
manager is constrained to its own test suite using label selectors and
namespace isolation. This adds a layer of complexity to writing tests
for controllers, however, the setup can be copy-pasted for new tests.
Hopefully that will alleviate the complexity of writing these tests.

Author: Zerpet

controllers/binding_controller_test.go

@@ -4,8 +4,11 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
+	"github.com/rabbitmq/cluster-operator/v2/api/v1beta1"
 	"github.com/rabbitmq/messaging-topology-operator/controllers"
 	"io"
+	"k8s.io/apimachinery/pkg/labels"
 	"net/http"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -34,14 +37,28 @@ var _ = Describe("bindingController", func() {
 		k8sClient     runtimeClient.Client
 	)
 
-	BeforeEach(func() {
+	initialiseManager := func(keyValPair ...string) {
+		var sel labels.Selector
+		if len(keyValPair) == 2 {
+			var err error
+			sel, err = labels.Parse(fmt.Sprintf("%s == %s", keyValPair[0], keyValPair[1]))
+			Expect(err).NotTo(HaveOccurred())
+		}
+
 		var err error
 		bindingMgr, err = ctrl.NewManager(testEnv.Config, ctrl.Options{
 			Metrics: server.Options{
 				BindAddress: "0", // To avoid MacOS firewall pop-up every time you run this suite
 			},
 			Cache: cache.Options{
-				DefaultNamespaces: map[string]cache.Config{bindingNamespace: {}},
+				DefaultNamespaces: map[string]cache.Config{bindingNamespace: {
+					LabelSelector: sel,
+				}},
+				ByObject: map[runtimeClient.Object]cache.ByObject{
+					&v1beta1.RabbitmqCluster{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+					&corev1.Secret{}:           {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+					&corev1.Service{}:          {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+				},
 			},
 			Logger: GinkgoLogr,
 			Controller: config.Controller{
@@ -66,21 +83,9 @@ var _ = Describe("bindingController", func() {
 			RabbitmqClientFactory: fakeRabbitMQClientFactory,
 			ReconcileFunc:         &controllers.BindingReconciler{},
 		}).SetupWithManager(bindingMgr)).To(Succeed())
-	})
+	}
 
-	AfterEach(func() {
-		managerCancel()
-		// Sad workaround to avoid controllers racing for the reconciliation of other's
-		// test cases. Without this wait, the last run test consistently fails because
-		// the previous cancelled manager is just in time to reconcile the Queue of the
-		// new/last test, and use the wrong/unexpected arguments in the queue declare call
-		//
-		// Eventual consistency is nice when you have good means of awaiting. That's not the
-		// case with testenv and kubernetes controllers.
-		<-time.After(time.Second)
-	})
-
-	JustBeforeEach(func() {
+	initialiseBinding := func() {
 		binding = topology.Binding{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      bindingName,
@@ -92,6 +97,18 @@ var _ = Describe("bindingController", func() {
 				},
 			},
 		}
+	}
+
+	AfterEach(func() {
+		managerCancel()
+		// Sad workaround to avoid controllers racing for the reconciliation of other's
+		// test cases. Without this wait, the last run test consistently fails because
+		// the previous cancelled manager is just in time to reconcile the Queue of the
+		// new/last test, and use the wrong/unexpected arguments in the queue declare call
+		//
+		// Eventual consistency is nice when you have good means of awaiting. That's not the
+		// case with testenv and kubernetes controllers.
+		<-time.After(time.Second)
 	})
 
 	When("creating a binding", func() {
@@ -106,6 +123,9 @@ var _ = Describe("bindingController", func() {
 					Status:     "418 I'm a teapot",
 					StatusCode: 418,
 				}, errors.New("some HTTP error"))
+				initialiseBinding()
+				binding.Labels = map[string]string{"test": "test-binding-http-error"}
+				initialiseManager("test", "test-binding-http-error")
 			})
 
 			It("sets the status condition to indicate a failure to reconcile", func() {
@@ -131,6 +151,9 @@ var _ = Describe("bindingController", func() {
 			BeforeEach(func() {
 				bindingName = "test-binding-go-error"
 				fakeRabbitMQClient.DeclareBindingReturns(nil, errors.New("hit a exception"))
+				initialiseBinding()
+				binding.Labels = map[string]string{"test": "test-binding-go-error"}
+				initialiseManager("test", "test-binding-go-error")
 			})
 
 			It("sets the status condition to indicate a failure to reconcile", func() {
@@ -155,6 +178,8 @@ var _ = Describe("bindingController", func() {
 
 	When("Deleting a binding", func() {
 		JustBeforeEach(func() {
+			// Must use a JustBeforeEach to extract this common behaviour
+			// JustBeforeEach runs AFTER all BeforeEach have completed
 			fakeRabbitMQClient.DeclareBindingReturns(&http.Response{
 				Status:     "201 Created",
 				StatusCode: http.StatusCreated,
@@ -183,6 +208,9 @@ var _ = Describe("bindingController", func() {
 					StatusCode: http.StatusBadGateway,
 					Body:       io.NopCloser(bytes.NewBufferString("Hello World")),
 				}, nil)
+				initialiseBinding()
+				binding.Labels = map[string]string{"test": "delete-binding-http-error"}
+				initialiseManager("test", "delete-binding-http-error")
 			})
 
 			It("raises an event to indicate a failure to delete", func() {
@@ -199,6 +227,9 @@ var _ = Describe("bindingController", func() {
 			BeforeEach(func() {
 				bindingName = "delete-binding-go-error"
 				fakeRabbitMQClient.DeleteBindingReturns(nil, errors.New("some error"))
+				initialiseBinding()
+				binding.Labels = map[string]string{"test": "delete-binding-go-error"}
+				initialiseManager("test", "delete-binding-go-error")
 			})
 
 			It("raises an event to indicate a failure to delete", func() {

controllers/exchange_controller_test.go

@@ -4,7 +4,10 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
+	"github.com/rabbitmq/cluster-operator/v2/api/v1beta1"
 	"io"
+	"k8s.io/apimachinery/pkg/labels"
 	"net/http"
 	"time"
 
@@ -35,14 +38,28 @@ var _ = Describe("exchange-controller", func() {
 		k8sClient     runtimeClient.Client
 	)
 
-	BeforeEach(func() {
+	initialiseManager := func(keyValPair ...string) {
+		var sel labels.Selector
+		if len(keyValPair) == 2 {
+			var err error
+			sel, err = labels.Parse(fmt.Sprintf("%s == %s", keyValPair[0], keyValPair[1]))
+			Expect(err).NotTo(HaveOccurred())
+		}
+
 		var err error
 		exchangeMgr, err = ctrl.NewManager(testEnv.Config, ctrl.Options{
 			Metrics: server.Options{
 				BindAddress: "0", // To avoid MacOS firewall pop-up every time you run this suite
 			},
 			Cache: cache.Options{
-				DefaultNamespaces: map[string]cache.Config{exchangeNamespace: {}},
+				DefaultNamespaces: map[string]cache.Config{exchangeNamespace: {
+					LabelSelector: sel,
+				}},
+				ByObject: map[runtimeClient.Object]cache.ByObject{
+					&v1beta1.RabbitmqCluster{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+					&corev1.Secret{}:           {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+					&corev1.Service{}:          {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
+				},
 			},
 			Logger: GinkgoLogr,
 			Controller: config.Controller{
@@ -67,22 +84,9 @@ var _ = Describe("exchange-controller", func() {
 			RabbitmqClientFactory: fakeRabbitMQClientFactory,
 			ReconcileFunc:         &controllers.ExchangeReconciler{},
 		}).SetupWithManager(exchangeMgr)).To(Succeed())
-	})
+	}
 
-	AfterEach(func() {
-		managerCancel()
-		// Sad workaround to avoid controllers racing for the reconciliation of other's
-		// test cases. Without this wait, the last run test consistently fails because
-		// the previous cancelled manager is just in time to reconcile the Queue of the
-		// new/last test, and use the wrong/unexpected arguments in the queue declare call
-		//
-		// Eventual consistency is nice when you have good means of awaiting. That's not the
-		// case with testenv and kubernetes controllers.
-		<-time.After(time.Second)
-	})
-
-	JustBeforeEach(func() {
-		// this will be executed after all BeforeEach have run
+	initialiseExchange := func() {
 		exchange = topology.Exchange{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      exchangeName,
@@ -94,6 +98,18 @@ var _ = Describe("exchange-controller", func() {
 				},
 			},
 		}
+	}
+
+	AfterEach(func() {
+		managerCancel()
+		// Sad workaround to avoid controllers racing for the reconciliation of other's
+		// test cases. Without this wait, the last run test consistently fails because
+		// the previous cancelled manager is just in time to reconcile the Queue of the
+		// new/last test, and use the wrong/unexpected arguments in the queue declare call
+		//
+		// Eventual consistency is nice when you have good means of awaiting. That's not the
+		// case with testenv and kubernetes controllers.
+		<-time.After(time.Second)
 	})
 
 	Context("creation", func() {
@@ -108,6 +124,9 @@ var _ = Describe("exchange-controller", func() {
 					Status:     "418 I'm a teapot",
 					StatusCode: 418,
 				}, errors.New("a failure"))
+				initialiseExchange()
+				exchange.Labels = map[string]string{"test": "test-http-error"}
+				initialiseManager("test", "test-http-error")
 			})
 
 			It("sets the status condition", func() {
@@ -133,6 +152,9 @@ var _ = Describe("exchange-controller", func() {
 			BeforeEach(func() {
 				exchangeName = "test-go-error"
 				fakeRabbitMQClient.DeclareExchangeReturns(nil, errors.New("a go failure"))
+				initialiseExchange()
+				exchange.Labels = map[string]string{"test": "test-go-error"}
+				initialiseManager("test", "test-go-error")
 			})
 
 			It("sets the status condition to indicate a failure to reconcile", func() {
@@ -162,8 +184,12 @@ var _ = Describe("exchange-controller", func() {
 				Status:     "201 Created",
 				StatusCode: http.StatusCreated,
 			}, nil)
+			initialiseExchange()
+			exchange.Labels = map[string]string{"test": "test-last-transition-time"}
+			initialiseManager("test", "test-last-transition-time")
 		})
 
+		// TODO maybe this is a problem because the delete function does not have a fakeClient prepared to return OK for Delete requests
 		AfterEach(func() {
 			Expect(k8sClient.Delete(ctx, &exchange)).To(Succeed())
 		})
@@ -190,7 +216,7 @@ var _ = Describe("exchange-controller", func() {
 				Status:     "204 No Content",
 				StatusCode: http.StatusNoContent,
 			}, nil)
-			exchange.Labels = map[string]string{"k1": "v1"}
+			exchange.Labels["k1"] = "v1"
 			Expect(k8sClient.Update(ctx, &exchange)).To(Succeed())
 			ConsistentlyWithOffset(1, func() []topology.Condition {
 				_ = k8sClient.Get(
@@ -210,7 +236,7 @@ var _ = Describe("exchange-controller", func() {
 				Status:     "500 Internal Server Error",
 				StatusCode: http.StatusInternalServerError,
 			}, errors.New("something went wrong"))
-			exchange.Labels = map[string]string{"k1": "v2"}
+			exchange.Labels["k1"] = "v2"
 			Expect(k8sClient.Update(ctx, &exchange)).To(Succeed())
 			EventuallyWithOffset(1, func() []topology.Condition {
 				_ = k8sClient.Get(
@@ -231,6 +257,8 @@ var _ = Describe("exchange-controller", func() {
 
 	Context("deletion", func() {
 		JustBeforeEach(func() {
+			// Must use a JustBeforeEach to extract this common behaviour
+			// JustBeforeEach runs AFTER all BeforeEach have completed
 			fakeRabbitMQClient.DeclareExchangeReturns(&http.Response{
 				Status:     "201 Created",
 				StatusCode: http.StatusCreated,
@@ -259,6 +287,9 @@ var _ = Describe("exchange-controller", func() {
 					StatusCode: http.StatusBadGateway,
 					Body:       io.NopCloser(bytes.NewBufferString("Hello World")),
 				}, nil)
+				initialiseExchange()
+				exchange.Labels = map[string]string{"test": "delete-exchange-http-error"}
+				initialiseManager("test", "delete-exchange-http-error")
 			})
 
 			It("publishes a 'warning' event", func() {
@@ -275,6 +306,9 @@ var _ = Describe("exchange-controller", func() {
 			BeforeEach(func() {
 				exchangeName = "delete-go-error"
 				fakeRabbitMQClient.DeleteExchangeReturns(nil, errors.New("some error"))
+				initialiseExchange()
+				exchange.Labels = map[string]string{"test": "delete-go-error"}
+				initialiseManager("test", "delete-go-error")
 			})
 
 			It("publishes a 'warning' event", func() {