From de8c0244f7466d26fe9fd7f2d9f9ab46035551a2 Mon Sep 17 00:00:00 2001 From: Aitor Perez <1515757+Zerpet@users.noreply.github.com> Date: Tue, 20 May 2025 12:26:42 +0100 Subject: [PATCH 1/4] Fix deletion policy tests The deletion policy was not being set at all for tests. Why? Because BeforeEach nodes run BEFORE the JustBeforeEach. Therefore, the BeforeEach was setting the deletion policy in the spec, and right after the JustAfterEach in the top container was assigning a "zero" value for the topology type. This effectively "overrode" the value set for deletion policy. This was probably passing because Create/Delete were called one after the other, and sometimes the deletion behaviour simply skipped because the object was not found, since it was "too soon" and not returned by the informer cache. These tests should have been rightfuly red. The were flaking to green. --- controllers/federation_controller_test.go | 13 +++++++++---- controllers/queue_controller_test.go | 12 ++++++++---- controllers/shovel_controller_test.go | 12 +++++++++--- controllers/vhost_controller_test.go | 13 +++++++++---- 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/controllers/federation_controller_test.go b/controllers/federation_controller_test.go index 6bb6a399..1621855c 100644 --- a/controllers/federation_controller_test.go +++ b/controllers/federation_controller_test.go @@ -233,26 +233,31 @@ var _ = Describe("federation-controller", func() { When("the Federation has DeletionPolicy set to retain", func() { BeforeEach(func() { federationName = "federation-with-retain-policy" - federation.Spec.DeletionPolicy = "retain" fakeRabbitMQClient.DeleteFederationUpstreamReturns(&http.Response{ Status: "200 OK", StatusCode: http.StatusOK, }, nil) + fakeRabbitMQClient.PutFederationUpstreamReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) }) It("deletes the k8s resource but preserves the federation in RabbitMQ server", func() { + federation.Spec.DeletionPolicy = "retain" Expect(k8sClient.Create(ctx, &federation)).To(Succeed()) - Expect(k8sClient.Delete(ctx, &federation)).To(Succeed()) + Eventually(fakeRabbitMQClient.PutFederationUpstreamCallCount). + WithPolling(time.Second). + Within(time.Second*3). + Should(BeNumerically(">=", 1), "Expected to call RMQ API to create federation") + Expect(k8sClient.Delete(ctx, &federation)).To(Succeed()) Eventually(func() bool { err := k8sClient.Get(ctx, types.NamespacedName{Name: federation.Name, Namespace: federation.Namespace}, &federation) return apierrors.IsNotFound(err) }). Within(statusEventsUpdateTimeout). WithPolling(time.Second). - Should(BeTrue()) + Should(BeTrue(), "Federation should not be found") - Expect(fakeRabbitMQClient.DeleteFederationUpstreamCallCount()).To(Equal(0)) + Expect(fakeRabbitMQClient.DeleteFederationUpstreamCallCount()).To(Equal(0), "Expected Federation to be deleted and no calls to RMQ API") }) }) }) diff --git a/controllers/queue_controller_test.go b/controllers/queue_controller_test.go index adb26c4a..2499dbcf 100644 --- a/controllers/queue_controller_test.go +++ b/controllers/queue_controller_test.go @@ -229,7 +229,6 @@ var _ = Describe("queue-controller", func() { When("the Queue has DeletionPolicy set to retain", func() { BeforeEach(func() { queueName = "queue-with-retain-policy" - queue.Spec.DeletionPolicy = "retain" fakeRabbitMQClient.DeleteQueueReturns(&http.Response{ Status: "200 OK", StatusCode: http.StatusOK, @@ -237,18 +236,23 @@ var _ = Describe("queue-controller", func() { }) It("deletes the k8s resource but preserves the queue in RabbitMQ server", func() { + queue.Spec.DeletionPolicy = "retain" Expect(k8sClient.Create(ctx, &queue)).To(Succeed()) - Expect(k8sClient.Delete(ctx, &queue)).To(Succeed()) + Eventually(fakeRabbitMQClient.DeclareQueueCallCount). + WithPolling(time.Second). + Within(time.Second*3). + Should(BeNumerically(">=", 1), "Expected to call RMQ API to declare queue") + Expect(k8sClient.Delete(ctx, &queue)).To(Succeed()) Eventually(func() bool { err := k8sClient.Get(ctx, types.NamespacedName{Name: queue.Name, Namespace: queue.Namespace}, &queue) return apierrors.IsNotFound(err) }). Within(statusEventsUpdateTimeout). WithPolling(time.Second). - Should(BeTrue()) + Should(BeTrue(), "Queue should not be found") - Expect(fakeRabbitMQClient.DeleteQueueCallCount()).To(Equal(0)) + Expect(fakeRabbitMQClient.DeleteQueueCallCount()).To(Equal(0), "Expected to delete queue and no calls to RMQ API") }) }) }) diff --git a/controllers/shovel_controller_test.go b/controllers/shovel_controller_test.go index 8eeb7d74..b282f441 100644 --- a/controllers/shovel_controller_test.go +++ b/controllers/shovel_controller_test.go @@ -290,18 +290,24 @@ var _ = Describe("shovel-controller", func() { }) }) }) + When("the Shovel has DeletionPolicy set to retain", func() { BeforeEach(func() { shovelName = "shovel-with-retain-policy" - shovel.Spec.DeletionPolicy = "retain" fakeRabbitMQClient.DeleteShovelReturns(&http.Response{ Status: "200 OK", StatusCode: http.StatusOK, }, nil) + fakeRabbitMQClient.DeclareShovelReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) }) It("deletes the k8s resource but preserves the shovel in RabbitMQ server", func() { + shovel.Spec.DeletionPolicy = "retain" Expect(k8sClient.Create(ctx, &shovel)).To(Succeed()) + Eventually(fakeRabbitMQClient.DeclareShovelCallCount). + WithPolling(time.Second). + Within(time.Second*3). + Should(BeNumerically(">=", 1), "Expected to call RMQ API to declare shovel") Expect(k8sClient.Delete(ctx, &shovel)).To(Succeed()) Eventually(func() bool { @@ -310,9 +316,9 @@ var _ = Describe("shovel-controller", func() { }). Within(statusEventsUpdateTimeout). WithPolling(time.Second). - Should(BeTrue()) + Should(BeTrue(), "Expected shovel to not be found") - Expect(fakeRabbitMQClient.DeleteShovelCallCount()).To(Equal(0)) + Expect(fakeRabbitMQClient.DeleteShovelCallCount()).To(Equal(0), "Shovel object should have been deleted and no calls to RMQ API") }) }) }) diff --git a/controllers/vhost_controller_test.go b/controllers/vhost_controller_test.go index 14478af0..c2900fd9 100644 --- a/controllers/vhost_controller_test.go +++ b/controllers/vhost_controller_test.go @@ -369,26 +369,31 @@ var _ = Describe("vhost-controller", func() { When("the Vhost has DeletionPolicy set to retain", func() { BeforeEach(func() { vhostName = "vhost-with-retain-policy" - vhost.Spec.DeletionPolicy = "retain" fakeRabbitMQClient.DeleteVhostReturns(&http.Response{ Status: "200 OK", StatusCode: http.StatusOK, }, nil) + fakeRabbitMQClient.PutVhostReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) }) It("deletes the k8s resource but preserves the vhost in RabbitMQ server", func() { + vhost.Spec.DeletionPolicy = "retain" Expect(k8sClient.Create(ctx, &vhost)).To(Succeed()) - Expect(k8sClient.Delete(ctx, &vhost)).To(Succeed()) + Eventually(fakeRabbitMQClient.PutVhostCallCount). + WithPolling(time.Second). + Within(time.Second*3). + Should(BeNumerically(">=", 1), "Expected to call RMQ API to create vhost") + Expect(k8sClient.Delete(ctx, &vhost)).To(Succeed()) Eventually(func() bool { err := k8sClient.Get(ctx, types.NamespacedName{Name: vhost.Name, Namespace: vhost.Namespace}, &vhost) return apierrors.IsNotFound(err) }). Within(statusEventsUpdateTimeout). WithPolling(time.Second). - Should(BeTrue()) + Should(BeTrue(), "vhost should not be found") - Expect(fakeRabbitMQClient.DeleteVhostCallCount()).To(Equal(0)) + Expect(fakeRabbitMQClient.DeleteVhostCallCount()).To(Equal(0), "Expected vhost to be deleted and no calls to RMQ API") }) }) }) From 4d0d3ccf4f0934b98f19c4389a9d69603179a09e Mon Sep 17 00:00:00 2001 From: Aitor Perez <1515757+Zerpet@users.noreply.github.com> Date: Tue, 20 May 2025 12:30:11 +0100 Subject: [PATCH 2/4] Use go run for ginkgo CLI [Why] To avoid mismatch version between Ginkgo version in Go mod vs the Ginkgo CLI installed locally in GOBIN. The change to the port number in testenv is to avoid using ports outside of valid range (1-65536) when Ginkgo parallel processes was over 10. Previously, the process number 10 would render port 81810, which is outside of the valid range. --- Makefile | 9 ++++++--- controllers/suite_test.go | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 6dfdf12d..614d0107 100644 --- a/Makefile +++ b/Makefile @@ -54,6 +54,7 @@ export KUBEBUILDER_ASSETS = $(LOCAL_TESTBIN)/k8s/$(ENVTEST_K8S_VERSION)-$(platfo .PHONY: kubebuilder-assets kubebuilder-assets: $(KUBEBUILDER_ASSETS) + @echo "export KUBEBUILDER_ASSETS = $(LOCAL_TESTBIN)/k8s/$(ENVTEST_K8S_VERSION)-$(platform)-$(ARCHITECTURE)" $(KUBEBUILDER_ASSETS): setup-envtest --os $(platform) --arch $(ARCHITECTURE) --bin-dir $(LOCAL_TESTBIN) use $(ENVTEST_K8S_VERSION) @@ -73,6 +74,8 @@ $(YTT): | $(LOCAL_BIN) ############## #### Tests ### ############## +GINKGO := go run github.com/onsi/ginkgo/v2/ginkgo + .PHONY: unit-tests unit-tests::install-tools ## Run unit tests unit-tests::$(KUBEBUILDER_ASSETS) @@ -84,7 +87,7 @@ unit-tests::just-unit-tests .PHONY: just-unit-tests just-unit-tests: - ginkgo -r --randomize-all api/ internal/ rabbitmqclient/ + $(GINKGO) -r --randomize-all api/ internal/ rabbitmqclient/ .PHONY: integration-tests integration-tests::install-tools ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run' @@ -96,14 +99,14 @@ integration-tests::manifests integration-tests::just-integration-tests just-integration-tests: $(KUBEBUILDER_ASSETS) - ginkgo --randomize-all -r -p $(GINKGO_EXTRA) controllers/ + $(GINKGO) --randomize-all -r -p $(GINKGO_EXTRA) controllers/ .PHONY: local-tests local-tests: unit-tests integration-tests ## Run all local tests (unit & integration) .PHONY: system-tests system-tests: ## Run E2E tests using current context in ~/.kube/config. Expects cluster operator and topology operator to be installed in the cluster - NAMESPACE="rabbitmq-system" ginkgo --randomize-all -r $(GINKGO_EXTRA) system_tests/ + NAMESPACE="rabbitmq-system" $(GINKGO) --randomize-all -r $(GINKGO_EXTRA) system_tests/ ################### diff --git a/controllers/suite_test.go b/controllers/suite_test.go index fffdcd1f..61c86ece 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -112,7 +112,7 @@ var _ = BeforeSuite(func() { CRDDirectoryPaths: operatorCrds, ErrorIfCRDPathMissing: true, Config: &rest.Config{ - Host: fmt.Sprintf("localhost:818%d", GinkgoParallelProcess()), + Host: fmt.Sprintf("localhost:218%d", GinkgoParallelProcess()), }, } From cffee68e4acbac1b7565e84f506c01c5cc2ff1c0 Mon Sep 17 00:00:00 2001 From: Aitor Perez <1515757+Zerpet@users.noreply.github.com> Date: Thu, 22 May 2025 11:25:55 +0100 Subject: [PATCH 3/4] 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. --- controllers/binding_controller_test.go | 63 +++++++++--- controllers/exchange_controller_test.go | 72 ++++++++++---- controllers/federation_controller_test.go | 67 ++++++++++--- controllers/operatorpolicy_controller_test.go | 65 +++++++++---- controllers/permission_controller_test.go | 91 +++++++++++++---- controllers/policy_controller_test.go | 63 +++++++++--- controllers/queue_controller_test.go | 66 ++++++++++--- controllers/shovel_controller_test.go | 73 +++++++++++--- .../topicpermission_controller_test.go | 97 ++++++++++++++----- controllers/user_controller_test.go | 72 +++++++++++--- controllers/vhost_controller_test.go | 83 ++++++++++++---- 11 files changed, 621 insertions(+), 191 deletions(-) diff --git a/controllers/binding_controller_test.go b/controllers/binding_controller_test.go index d2c993b6..9b8af892 100644 --- a/controllers/binding_controller_test.go +++ b/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() { diff --git a/controllers/exchange_controller_test.go b/controllers/exchange_controller_test.go index 1d9ea72a..d2126299 100644 --- a/controllers/exchange_controller_test.go +++ b/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() { diff --git a/controllers/federation_controller_test.go b/controllers/federation_controller_test.go index 1621855c..58e3bc60 100644 --- a/controllers/federation_controller_test.go +++ b/controllers/federation_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,29 @@ var _ = Describe("federation-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 federationMgr, 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{federationNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{federationNamespace: { + LabelSelector: sel, + }}, + ByObject: map[runtimeClient.Object]cache.ByObject{ + // Not sure why, but restricting the NS to the test-ns fails the tests :shrug: + &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 +84,9 @@ var _ = Describe("federation-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.FederationReconciler{Client: federationMgr.GetClient()}, }).SetupWithManager(federationMgr)).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() { + initialiseFederation := func() { federation = topology.Federation{ ObjectMeta: metav1.ObjectMeta{ Name: federationName, @@ -95,6 +101,18 @@ var _ = Describe("federation-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) }) When("creation", func() { @@ -109,6 +127,9 @@ var _ = Describe("federation-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("some HTTP error")) + initialiseFederation() + federation.Labels = map[string]string{"test": "test-federation-http-error"} + initialiseManager("test", "test-federation-http-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -137,6 +158,9 @@ var _ = Describe("federation-controller", func() { BeforeEach(func() { federationName = "test-federation-go-error" fakeRabbitMQClient.PutFederationUpstreamReturns(nil, errors.New("some go failure here")) + initialiseFederation() + federation.Labels = map[string]string{"test": "test-federation-go-error"} + initialiseManager("test", "test-federation-go-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -164,6 +188,8 @@ var _ = Describe("federation-controller", func() { When("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.PutFederationUpstreamReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -195,6 +221,9 @@ var _ = Describe("federation-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialiseFederation() + federation.Labels = map[string]string{"test": "delete-federation-http-error"} + initialiseManager("test", "delete-federation-http-error") }) It("raises an event to indicate a failure to delete", func() { @@ -214,6 +243,9 @@ var _ = Describe("federation-controller", func() { BeforeEach(func() { federationName = "delete-federation-go-error" fakeRabbitMQClient.DeleteFederationUpstreamReturns(nil, errors.New("some error")) + initialiseFederation() + federation.Labels = map[string]string{"test": "delete-federation-go-error"} + initialiseManager("test", "delete-federation-go-error") }) It("publishes a 'warning' event", func() { @@ -238,6 +270,9 @@ var _ = Describe("federation-controller", func() { StatusCode: http.StatusOK, }, nil) fakeRabbitMQClient.PutFederationUpstreamReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) + initialiseFederation() + federation.Labels = map[string]string{"test": "federation-with-retain-policy"} + initialiseManager("test", "federation-with-retain-policy") }) It("deletes the k8s resource but preserves the federation in RabbitMQ server", func() { diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index 1a27f191..b05817eb 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/controllers/operatorpolicy_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" @@ -37,14 +40,28 @@ var _ = Describe("operatorpolicy-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 policyMgr, 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{policyNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{policyNamespace: { + 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{ @@ -69,21 +86,9 @@ var _ = Describe("operatorpolicy-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.OperatorPolicyReconciler{}, }).SetupWithManager(policyMgr)).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() { + initialisePolicy := func() { policy = topology.OperatorPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: policyName, @@ -98,6 +103,18 @@ var _ = Describe("operatorpolicy-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() { @@ -112,6 +129,9 @@ var _ = Describe("operatorpolicy-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialisePolicy() + policy.Labels = map[string]string{"test": "test-http-error"} + initialiseManager("test", "test-http-error") }) It("sets the status condition", func() { @@ -140,6 +160,9 @@ var _ = Describe("operatorpolicy-controller", func() { BeforeEach(func() { policyName = "test-go-error" fakeRabbitMQClient.PutOperatorPolicyReturns(nil, errors.New("a go failure")) + initialisePolicy() + policy.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() { @@ -167,11 +190,13 @@ var _ = Describe("operatorpolicy-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.PutOperatorPolicyReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, }, nil) - Expect(k8sClient.Create(ctx, &policy)).To(Succeed()) + Expect(k8sClient.Create(context.TODO(), &policy)).To(Succeed()) Eventually(func() []topology.Condition { _ = k8sClient.Get( ctx, @@ -198,6 +223,9 @@ var _ = Describe("operatorpolicy-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialisePolicy() + policy.Labels = map[string]string{"test": "delete-policy-http-error"} + initialiseManager("test", "delete-policy-http-error") }) It("publishes a 'warning' event", func() { @@ -216,6 +244,9 @@ var _ = Describe("operatorpolicy-controller", func() { BeforeEach(func() { policyName = "delete-go-error" fakeRabbitMQClient.DeleteOperatorPolicyReturns(nil, errors.New("some error")) + initialisePolicy() + policy.Labels = map[string]string{"test": policyName} + initialiseManager("test", policyName) }) It("publishes a 'warning' event", func() { diff --git a/controllers/permission_controller_test.go b/controllers/permission_controller_test.go index 24f43cf1..bfd5bec1 100644 --- a/controllers/permission_controller_test.go +++ b/controllers/permission_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" @@ -37,14 +40,28 @@ var _ = Describe("permission-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 permissionMgr, 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{permissionNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{permissionNamespace: { + 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{ @@ -69,7 +86,23 @@ var _ = Describe("permission-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.PermissionReconciler{Client: permissionMgr.GetClient(), Scheme: permissionMgr.GetScheme()}, }).SetupWithManager(permissionMgr)).To(Succeed()) - }) + } + + initialisePermission := func() { + permission = topology.Permission{ + ObjectMeta: metav1.ObjectMeta{ + Name: permissionName, + Namespace: permissionNamespace, + }, + Spec: topology.PermissionSpec{ + RabbitmqClusterReference: topology.RabbitmqClusterReference{ + Name: "example-rabbit", + }, + User: "example", + Vhost: "example", + }, + } + } AfterEach(func() { managerCancel() @@ -84,22 +117,6 @@ var _ = Describe("permission-controller", func() { }) When("validating RabbitMQ Client failures with username", func() { - JustBeforeEach(func() { - permission = topology.Permission{ - ObjectMeta: metav1.ObjectMeta{ - Name: permissionName, - Namespace: permissionNamespace, - }, - Spec: topology.PermissionSpec{ - RabbitmqClusterReference: topology.RabbitmqClusterReference{ - Name: "example-rabbit", - }, - User: "example", - Vhost: "example", - }, - } - }) - Context("creation", func() { AfterEach(func() { Expect(k8sClient.Delete(ctx, &permission)).To(Succeed()) @@ -112,6 +129,9 @@ var _ = Describe("permission-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialisePermission() + permission.Labels = map[string]string{"test": "test-with-username-http-error"} + initialiseManager("test", "test-with-username-http-error") }) It("sets the status condition", func() { @@ -137,6 +157,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-username-go-error" fakeRabbitMQClient.UpdatePermissionsInReturns(nil, errors.New("a go failure")) + initialisePermission() + permission.Labels = map[string]string{"test": "test-with-username-go-error"} + initialiseManager("test", "test-with-username-go-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -161,6 +184,8 @@ var _ = Describe("permission-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.UpdatePermissionsInReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -189,6 +214,9 @@ var _ = Describe("permission-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialisePermission() + permission.Labels = map[string]string{"test": "delete-with-username-permission-http-error"} + initialiseManager("test", "delete-with-username-permission-http-error") }) It("publishes a 'warning' event", func() { @@ -205,6 +233,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "delete-with-username-go-error" fakeRabbitMQClient.ClearPermissionsInReturns(nil, errors.New("some error")) + initialisePermission() + permission.Labels = map[string]string{"test": "delete-with-username-go-error"} + initialiseManager("test", "delete-with-username-go-error") }) It("publishes a 'warning' event", func() { @@ -221,10 +252,13 @@ var _ = Describe("permission-controller", func() { When("validating RabbitMQ Client failures with userRef", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed user = topology.User{ ObjectMeta: metav1.ObjectMeta{ Name: userName, Namespace: permissionNamespace, + Labels: map[string]string{"test": permissionName}, }, Spec: topology.UserSpec{ RabbitmqClusterReference: topology.RabbitmqClusterReference{ @@ -237,6 +271,7 @@ var _ = Describe("permission-controller", func() { ObjectMeta: metav1.ObjectMeta{ Name: permissionName, Namespace: permissionNamespace, + Labels: map[string]string{"test": permissionName}, }, Spec: topology.PermissionSpec{ RabbitmqClusterReference: topology.RabbitmqClusterReference{ @@ -284,6 +319,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-userref-create-not-exist" userName = "example-create-not-exist" + initialisePermission() + permission.Labels = map[string]string{"test": "test-with-userref-create-not-exist"} + initialiseManager("test", "test-with-userref-create-not-exist") }) It("sets the status condition 'Ready' to 'true' ", func() { @@ -309,6 +347,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-userref-create-success" userName = "example-create-success" + initialisePermission() + permission.Labels = map[string]string{"test": "test-with-userref-create-success"} + initialiseManager("test", "test-with-userref-create-success") }) It("sets the status condition 'Ready' to 'true' ", func() { @@ -358,6 +399,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-userref-delete-secret" userName = "example-delete-secret-first" + initialisePermission() + permission.Labels = map[string]string{"test": permissionName} + initialiseManager("test", permissionName) }) It("publishes a 'warning' event", func() { @@ -385,6 +429,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-userref-delete-user" userName = "example-delete-user-first" + initialisePermission() + permission.Labels = map[string]string{"test": permissionName} + initialiseManager("test", permissionName) }) It("publishes a 'warning' event", func() { @@ -408,6 +455,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "test-with-userref-delete-success" userName = "example-delete-success" + initialisePermission() + permission.Labels = map[string]string{"test": "test-with-userref-delete-success"} + initialiseManager("test", "test-with-userref-delete-success") }) It("publishes a 'warning' event", func() { @@ -427,6 +477,9 @@ var _ = Describe("permission-controller", func() { BeforeEach(func() { permissionName = "ownerref-with-userref-test" userName = "example-ownerref" + initialisePermission() + permission.Labels = map[string]string{"test": permissionName} + initialiseManager("test", permissionName) }) AfterEach(func() { diff --git a/controllers/policy_controller_test.go b/controllers/policy_controller_test.go index 2cc75262..12658e19 100644 --- a/controllers/policy_controller_test.go +++ b/controllers/policy_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" @@ -36,14 +39,28 @@ var _ = Describe("policy-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 policyMgr, 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{policyNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{policyNamespace: { + 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{ @@ -68,21 +85,9 @@ var _ = Describe("policy-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.PolicyReconciler{}, }).SetupWithManager(policyMgr)).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() { + initialisePolicy := func() { policy = topology.Policy{ ObjectMeta: metav1.ObjectMeta{ Name: policyName, @@ -97,6 +102,18 @@ var _ = Describe("policy-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() { @@ -111,6 +128,9 @@ var _ = Describe("policy-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialisePolicy() + policy.Labels = map[string]string{"test": "test-http-error"} + initialiseManager("test", policyName) }) It("sets the status condition", func() { @@ -139,6 +159,9 @@ var _ = Describe("policy-controller", func() { BeforeEach(func() { policyName = "test-go-error" fakeRabbitMQClient.PutPolicyReturns(nil, errors.New("a go failure")) + initialisePolicy() + policy.Labels = map[string]string{"test": "test-go-error"} + initialiseManager("test", policyName) }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -166,6 +189,8 @@ var _ = Describe("policy-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.PutPolicyReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -197,6 +222,9 @@ var _ = Describe("policy-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialisePolicy() + policy.Labels = map[string]string{"test": "delete-policy-http-error"} + initialiseManager("test", policyName) }) It("publishes a 'warning' event", func() { @@ -215,6 +243,9 @@ var _ = Describe("policy-controller", func() { BeforeEach(func() { policyName = "delete-go-error" fakeRabbitMQClient.DeletePolicyReturns(nil, errors.New("some error")) + initialisePolicy() + policy.Labels = map[string]string{"test": "delete-go-error"} + initialiseManager("test", policyName) }) It("publishes a 'warning' event", func() { diff --git a/controllers/queue_controller_test.go b/controllers/queue_controller_test.go index 2499dbcf..f4822dfd 100644 --- a/controllers/queue_controller_test.go +++ b/controllers/queue_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("queue-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 queueMgr, 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{queueNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{queueNamespace: { + 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,21 +84,9 @@ var _ = Describe("queue-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.QueueReconciler{}, }).SetupWithManager(queueMgr)).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() { + initialiseQueue := func() { queue = topology.Queue{ ObjectMeta: metav1.ObjectMeta{ Name: queueName, @@ -93,6 +98,18 @@ var _ = Describe("queue-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() { @@ -107,6 +124,9 @@ var _ = Describe("queue-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialiseQueue() + queue.Labels = map[string]string{"test": "test-http-error"} + initialiseManager("test", "test-http-error") }) It("sets the status condition", func() { @@ -135,6 +155,9 @@ var _ = Describe("queue-controller", func() { BeforeEach(func() { queueName = "test-go-error" fakeRabbitMQClient.DeclareQueueReturns(nil, errors.New("a go failure")) + initialiseQueue() + queue.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,6 +185,8 @@ var _ = Describe("queue-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.DeclareQueueReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -193,6 +218,9 @@ var _ = Describe("queue-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialiseQueue() + queue.Labels = map[string]string{"test": "delete-queue-http-error"} + initialiseManager("test", "delete-queue-http-error") }) It("publishes a 'warning' event", func() { @@ -211,6 +239,9 @@ var _ = Describe("queue-controller", func() { BeforeEach(func() { queueName = "delete-go-error" fakeRabbitMQClient.DeleteQueueReturns(nil, errors.New("some error")) + initialiseQueue() + queue.Labels = map[string]string{"test": "delete-go-error"} + initialiseManager("test", "delete-go-error") }) It("publishes a 'warning' event", func() { @@ -233,6 +264,9 @@ var _ = Describe("queue-controller", func() { Status: "200 OK", StatusCode: http.StatusOK, }, nil) + initialiseQueue() + queue.Labels = map[string]string{"test": "queue-with-retain-policy"} + initialiseManager("test", "queue-with-retain-policy") }) It("deletes the k8s resource but preserves the queue in RabbitMQ server", func() { diff --git a/controllers/shovel_controller_test.go b/controllers/shovel_controller_test.go index b282f441..672ada34 100644 --- a/controllers/shovel_controller_test.go +++ b/controllers/shovel_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" @@ -36,14 +39,28 @@ var _ = Describe("shovel-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 shovelMgr, 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{shovelNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{shovelNamespace: { + 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{ @@ -68,21 +85,9 @@ var _ = Describe("shovel-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.ShovelReconciler{Client: shovelMgr.GetClient()}, }).SetupWithManager(shovelMgr)).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() { + initialiseShovel := func() { shovel = topology.Shovel{ ObjectMeta: metav1.ObjectMeta{ Name: shovelName, @@ -97,6 +102,18 @@ var _ = Describe("shovel-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) }) When("creation", func() { @@ -111,6 +128,9 @@ var _ = Describe("shovel-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("some HTTP error")) + initialiseShovel() + shovel.Labels = map[string]string{"test": "test-shovel-http-error"} + initialiseManager("test", "test-shovel-http-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -139,6 +159,9 @@ var _ = Describe("shovel-controller", func() { BeforeEach(func() { shovelName = "test-shovel-go-error" fakeRabbitMQClient.DeclareShovelReturns(nil, errors.New("a go failure")) + initialiseShovel() + shovel.Labels = map[string]string{"test": "test-shovel-go-error"} + initialiseManager("test", "test-shovel-go-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -170,6 +193,9 @@ var _ = Describe("shovel-controller", func() { Status: "201 Created", StatusCode: http.StatusCreated, }, nil) + initialiseShovel() + shovel.Labels = map[string]string{"test": "test-shovel-success"} + initialiseManager("test", "test-shovel-success") }) It("works", func() { @@ -178,6 +204,7 @@ var _ = Describe("shovel-controller", func() { Eventually(komega.Object(&shovel)).WithTimeout(2 * time.Second).Should(HaveField("ObjectMeta.Finalizers", ConsistOf("deletion.finalizers.shovels.rabbitmq.com"))) By("sets the status condition 'Ready' to 'true'") + // TODO rewrite this using komega.Object Eventually(func() []topology.Condition { _ = k8sClient.Get( ctx, @@ -200,6 +227,8 @@ var _ = Describe("shovel-controller", func() { When("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.DeclareShovelReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -231,6 +260,9 @@ var _ = Describe("shovel-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialiseShovel() + shovel.Labels = map[string]string{"test": "delete-shovel-http-error"} + initialiseManager("test", "delete-shovel-http-error") }) It("raises an event to indicate a failure to delete", func() { @@ -250,6 +282,9 @@ var _ = Describe("shovel-controller", func() { BeforeEach(func() { shovelName = "delete-shovel-go-error" fakeRabbitMQClient.DeleteShovelReturns(nil, errors.New("some error")) + initialiseShovel() + shovel.Labels = map[string]string{"test": "delete-shovel-go-error"} + initialiseManager("test", "delete-shovel-go-error") }) It("publishes a 'warning' event", func() { @@ -272,6 +307,9 @@ var _ = Describe("shovel-controller", func() { Status: "204 No Content", StatusCode: http.StatusNoContent, }, nil) + initialiseShovel() + shovel.Labels = map[string]string{"test": "delete-shovel-success"} + initialiseManager("test", "delete-shovel-success") }) It("publishes a normal event", func() { @@ -299,6 +337,9 @@ var _ = Describe("shovel-controller", func() { StatusCode: http.StatusOK, }, nil) fakeRabbitMQClient.DeclareShovelReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) + initialiseShovel() + shovel.Labels = map[string]string{"test": "shovel-with-retain-policy"} + initialiseManager("test", "shovel-with-retain-policy") }) It("deletes the k8s resource but preserves the shovel in RabbitMQ server", func() { diff --git a/controllers/topicpermission_controller_test.go b/controllers/topicpermission_controller_test.go index fb229a2c..23933764 100644 --- a/controllers/topicpermission_controller_test.go +++ b/controllers/topicpermission_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" @@ -36,14 +39,28 @@ var _ = Describe("topicpermission-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 topicPermissionMgr, 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{topicPermissionNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{topicPermissionNamespace: { + 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{ @@ -68,7 +85,26 @@ var _ = Describe("topicpermission-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.TopicPermissionReconciler{Client: topicPermissionMgr.GetClient(), Scheme: topicPermissionMgr.GetScheme()}, }).SetupWithManager(topicPermissionMgr)).To(Succeed()) - }) + } + + initialiseTopicPermission := func() { + topicperm = topology.TopicPermission{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: topicPermissionNamespace, + }, + Spec: topology.TopicPermissionSpec{ + RabbitmqClusterReference: topology.RabbitmqClusterReference{ + Name: "example-rabbit", + }, + User: "example", + Vhost: "example", + Permissions: topology.TopicPermissionConfig{ + Exchange: "some", + }, + }, + } + } AfterEach(func() { managerCancel() @@ -83,25 +119,6 @@ var _ = Describe("topicpermission-controller", func() { }) When("validating RabbitMQ Client failures with username", func() { - JustBeforeEach(func() { - topicperm = topology.TopicPermission{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: topicPermissionNamespace, - }, - Spec: topology.TopicPermissionSpec{ - RabbitmqClusterReference: topology.RabbitmqClusterReference{ - Name: "example-rabbit", - }, - User: "example", - Vhost: "example", - Permissions: topology.TopicPermissionConfig{ - Exchange: "some", - }, - }, - } - }) - Context("creation", func() { AfterEach(func() { Expect(k8sClient.Delete(ctx, &topicperm)).To(Succeed()) @@ -114,6 +131,9 @@ var _ = Describe("topicpermission-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("sets the status condition", func() { @@ -142,6 +162,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-username-go-error" fakeRabbitMQClient.UpdateTopicPermissionsInReturns(nil, errors.New("a go failure")) + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -169,6 +192,8 @@ var _ = Describe("topicpermission-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.UpdateTopicPermissionsInReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -200,6 +225,9 @@ var _ = Describe("topicpermission-controller", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("publishes a 'warning' event", func() { @@ -219,6 +247,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "delete-with-username-go-error" fakeRabbitMQClient.DeleteTopicPermissionsInReturns(nil, errors.New("some error")) + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("publishes a 'warning' event", func() { @@ -242,6 +273,7 @@ var _ = Describe("topicpermission-controller", func() { ObjectMeta: metav1.ObjectMeta{ Name: userName, Namespace: topicPermissionNamespace, + Labels: map[string]string{"test": name}, }, Spec: topology.UserSpec{ RabbitmqClusterReference: topology.RabbitmqClusterReference{ @@ -254,6 +286,7 @@ var _ = Describe("topicpermission-controller", func() { ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: topicPermissionNamespace, + Labels: map[string]string{"test": name}, }, Spec: topology.TopicPermissionSpec{ RabbitmqClusterReference: topology.RabbitmqClusterReference{ @@ -296,6 +329,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-userref-create-not-exist" userName = "topic-perm-example-create-not-exist" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("sets the status condition 'Ready' to 'true' ", func() { @@ -324,6 +360,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-userref-create-success" userName = "topic-perm-example-create-success" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("sets the status condition 'Ready' to 'true' ", func() { @@ -353,6 +392,8 @@ var _ = Describe("topicpermission-controller", func() { Context("deletion", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed Expect(k8sClient.Create(ctx, &user)).To(Succeed()) user.Status.Username = userName Expect(k8sClient.Status().Update(ctx, &user)).To(Succeed()) @@ -379,6 +420,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-userref-delete-secret" userName = "topic-perm-example-delete-secret-first" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("publishes a 'warning' event", func() { @@ -409,6 +453,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-userref-delete-user" userName = "topic-perm-example-delete-user-first" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("publishes a 'warning' event", func() { @@ -440,6 +487,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "test-with-userref-delete-success" userName = "topic-perm-example-delete-success" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) It("publishes a 'warning' event", func() { @@ -463,6 +513,9 @@ var _ = Describe("topicpermission-controller", func() { BeforeEach(func() { name = "ownerref-with-userref-test" userName = "topic-perm-topic-perm-user" + initialiseTopicPermission() + topicperm.Labels = map[string]string{"test": name} + initialiseManager("test", name) }) AfterEach(func() { diff --git a/controllers/user_controller_test.go b/controllers/user_controller_test.go index e97519b9..68385ade 100644 --- a/controllers/user_controller_test.go +++ b/controllers/user_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" @@ -39,14 +42,28 @@ var _ = Describe("UserController", func() { channels int32 ) - 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 userMgr, 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{userNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{userNamespace: { + 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{ @@ -71,21 +88,9 @@ var _ = Describe("UserController", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.UserReconciler{Client: userMgr.GetClient(), Scheme: userMgr.GetScheme()}, }).SetupWithManager(userMgr)).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() { + initialiseUser := func() { user = topology.User{ ObjectMeta: metav1.ObjectMeta{ Name: userName, @@ -98,6 +103,18 @@ var _ = Describe("UserController", func() { UserLimits: &userLimits, }, } + } + + 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 user", func() { @@ -112,6 +129,9 @@ var _ = Describe("UserController", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("some HTTP error")) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -140,6 +160,9 @@ var _ = Describe("UserController", func() { BeforeEach(func() { userName = "test-user-go-error" fakeRabbitMQClient.PutUserReturns(nil, errors.New("hit a exception")) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -187,6 +210,9 @@ var _ = Describe("UserController", func() { Message: "Object Not Found", Reason: "Not Found", }) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("should create the user limits", func() { @@ -241,6 +267,9 @@ var _ = Describe("UserController", func() { Status: "204 No Content", StatusCode: http.StatusNoContent, }, nil) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("should update the existing user limit and delete the unused old limit", func() { @@ -277,6 +306,8 @@ var _ = Describe("UserController", func() { When("deleting a user", func() { JustBeforeEach(func() { + // Must use a JustBeforeEach to extract this common behaviour + // JustBeforeEach runs AFTER all BeforeEach have completed fakeRabbitMQClient.PutUserReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -316,6 +347,9 @@ var _ = Describe("UserController", func() { StatusCode: http.StatusBadGateway, Body: io.NopCloser(bytes.NewBufferString("Hello World")), }, nil) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("raises an event to indicate a failure to delete", func() { @@ -335,6 +369,9 @@ var _ = Describe("UserController", func() { BeforeEach(func() { userName = "delete-user-go-error" fakeRabbitMQClient.DeleteUserReturns(nil, errors.New("some error")) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("raises an event to indicate a failure to delete", func() { @@ -357,6 +394,9 @@ var _ = Describe("UserController", func() { Status: "204 No Content", StatusCode: http.StatusNoContent, }, nil) + initialiseUser() + user.Labels = map[string]string{"test": userName} + initialiseManager("test", userName) }) It("raises an event to indicate a successful deletion", func() { diff --git a/controllers/vhost_controller_test.go b/controllers/vhost_controller_test.go index c2900fd9..8e597345 100644 --- a/controllers/vhost_controller_test.go +++ b/controllers/vhost_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" @@ -36,14 +39,28 @@ var _ = Describe("vhost-controller", func() { vhostLimits *topology.VhostLimits ) - 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 vhostMgr, 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{vhostNamespace: {}}, + DefaultNamespaces: map[string]cache.Config{vhostNamespace: { + 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{ @@ -68,21 +85,9 @@ var _ = Describe("vhost-controller", func() { RabbitmqClientFactory: fakeRabbitMQClientFactory, ReconcileFunc: &controllers.VhostReconciler{Client: vhostMgr.GetClient()}, }).SetupWithManager(vhostMgr)).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() { + initialiseVhost := func() { vhost = topology.Vhost{ ObjectMeta: metav1.ObjectMeta{ Name: vhostName, @@ -96,6 +101,18 @@ var _ = Describe("vhost-controller", func() { VhostLimits: vhostLimits, }, } + } + + 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() { @@ -110,6 +127,9 @@ var _ = Describe("vhost-controller", func() { Status: "418 I'm a teapot", StatusCode: 418, }, errors.New("a failure")) + initialiseVhost() + vhost.Labels = map[string]string{"test": "creation-http-error"} + initialiseManager("test", "creation-http-error") }) It("sets the status condition", func() { @@ -138,6 +158,9 @@ var _ = Describe("vhost-controller", func() { BeforeEach(func() { vhostName = "test-go-error" fakeRabbitMQClient.PutVhostReturns(nil, errors.New("a go failure")) + initialiseVhost() + vhost.Labels = map[string]string{"test": "creation-go-error"} + initialiseManager("test", "creation-go-error") }) It("sets the status condition to indicate a failure to reconcile", func() { @@ -187,6 +210,9 @@ var _ = Describe("vhost-controller", func() { Message: "Object Not Found", Reason: "Not Found", }) + initialiseVhost() + vhost.Labels = map[string]string{"test": "vhost-with-limits"} + initialiseManager("test", "vhost-with-limits") }) It("puts the vhost limits", func() { @@ -224,6 +250,9 @@ var _ = Describe("vhost-controller", func() { Message: "Object Not Found", Reason: "Not Found", }) + initialiseVhost() + vhost.Labels = map[string]string{"test": "vhost-without-limits"} + initialiseManager("test", "vhost-without-limits") }) It("does not set vhost limits", func() { @@ -260,6 +289,9 @@ var _ = Describe("vhost-controller", func() { Status: "204 No Content", StatusCode: http.StatusNoContent, }, nil) + initialiseVhost() + vhost.Labels = map[string]string{"test": "vhost-updated-limits"} + initialiseManager("test", "vhost-updated-limits") }) It("updates the provided limits and removes unspecified limits", func() { @@ -299,7 +331,7 @@ var _ = Describe("vhost-controller", func() { }) Context("deletion", func() { - JustBeforeEach(func() { + createVhost := func() { fakeRabbitMQClient.PutVhostReturns(&http.Response{ Status: "201 Created", StatusCode: http.StatusCreated, @@ -321,11 +353,17 @@ var _ = Describe("vhost-controller", func() { "Reason": Equal("SuccessfulCreateOrUpdate"), "Status": Equal(corev1.ConditionTrue), }))) - }) + } When("the RabbitMQ Client returns a HTTP error response", func() { BeforeEach(func() { vhostName = "delete-vhost-http-error" + initialiseVhost() + vhost.Labels = map[string]string{"test": vhostName} + initialiseManager("test", vhostName) + + createVhost() + fakeRabbitMQClient.DeleteVhostReturns(&http.Response{ Status: "502 Bad Gateway", StatusCode: http.StatusBadGateway, @@ -349,6 +387,12 @@ var _ = Describe("vhost-controller", func() { When("the RabbitMQ Client returns a Go error response", func() { BeforeEach(func() { vhostName = "delete-go-error" + initialiseVhost() + vhost.Labels = map[string]string{"test": vhostName} + initialiseManager("test", vhostName) + + createVhost() + fakeRabbitMQClient.DeleteVhostReturns(nil, errors.New("some error")) }) @@ -374,6 +418,9 @@ var _ = Describe("vhost-controller", func() { StatusCode: http.StatusOK, }, nil) fakeRabbitMQClient.PutVhostReturns(&http.Response{StatusCode: http.StatusCreated, Status: "201 Created"}, nil) + initialiseVhost() + vhost.Labels = map[string]string{"test": "vhost-with-retain-policy"} + initialiseManager("test", "vhost-with-retain-policy") }) It("deletes the k8s resource but preserves the vhost in RabbitMQ server", func() { From 1bcc485e366d14242663e7be01168daf5a5a0d2f Mon Sep 17 00:00:00 2001 From: Aitor Perez <1515757+Zerpet@users.noreply.github.com> Date: Thu, 22 May 2025 12:48:14 +0100 Subject: [PATCH 4/4] Fix vhost controller test flakes There was test pollution from tests that set vhost limits. The vhost limits in RMQ HTTP API are implemented as separate API endpoints. When a vhost has a limit, the controller declares the vhost first, then declares the limits. The flake happened when a "limits" tests ran before the "deletion" tests, and left behind a value for `vhostLimits` variable. This variable is taken unconditionally to initialise a vhost variable (as it should). The flake is simply fixed by setting the `vhostLimits` variable back to `nil` after each "limits" test. Additionally, the "creation" tests should not delete the objects after each test suite, because the fake client is not prepared to return appropriate responses to the delete requests, and that would leave the managers in an infinite reconcile loop, which can delay manager shutdown after each test. --- controllers/vhost_controller_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/controllers/vhost_controller_test.go b/controllers/vhost_controller_test.go index 8e597345..7171cd6a 100644 --- a/controllers/vhost_controller_test.go +++ b/controllers/vhost_controller_test.go @@ -116,10 +116,6 @@ var _ = Describe("vhost-controller", func() { }) Context("creation", func() { - AfterEach(func() { - Expect(k8sClient.Delete(ctx, &vhost)).To(Succeed()) - }) - When("the RabbitMQ Client returns a HTTP error response", func() { BeforeEach(func() { vhostName = "test-http-error" @@ -188,6 +184,11 @@ var _ = Describe("vhost-controller", func() { Context("vhost limits", func() { var connections, queues int32 + AfterEach(func() { + // Must reset the vhost limits to avoid test pollution + vhostLimits = nil + }) + When("vhost limits are provided", func() { BeforeEach(func() { connections = 708