Fix case where empty HTTP response caused panic in reconcile loop

Makefile

@@ -20,6 +20,11 @@ install-tools:
 unit-tests: install-tools generate fmt vet manifests ## Run unit tests
 	ginkgo -r --randomizeAllSpecs api/ internal/
 
+integration-tests: install-tools generate fmt vet manifests ## Run integration tests
+	ginkgo -r --randomizeAllSpecs controllers/
+
+local-tests: unit-tests integration-tests ## Run all local tests (unit & integration)
+
 system-tests: ## run end-to-end tests against Kubernetes cluster defined in ~/.kube/config. Expects cluster operator and messaging topology operator to be installed in the cluster
 	NAMESPACE="rabbitmq-system" ginkgo -randomizeAllSpecs -r system_tests/
 

controllers/common.go

@@ -7,3 +7,14 @@ const (
 	failedGenerateRabbitClient = "Failed to generate http rabbitClient"
 	noSuchRabbitDeletion       = "RabbitmqCluster is already gone: cannot find its connection secret"
 )
+
+// names for each of the controllers
+const (
+	VhostControllerName      = "vhost-controller"
+	QueueControllerName      = "queue-controller"
+	ExchangeControllerName   = "exchange-controller"
+	BindingControllerName    = "binding-controller"
+	UserControllerName       = "user-controller"
+	PolicyControllerName     = "policy-controller"
+	PermissionControllerName = "permission-controller"
+)

controllers/controllers_suite_test.go

@@ -0,0 +1,159 @@
+/*
+RabbitMQ Messaging Topology Kubernetes Operator
+Copyright 2021 VMware, Inc.
+
+This product is licensed to you under the Mozilla Public License 2.0 license (the "License").  You may not use this product except in compliance with the Mozilla 2.0 License.
+
+This product may include a number of subcomponents with separate copyright notices and license terms. Your use of these subcomponents is subject to the terms and conditions of the subcomponent's license, as noted in the LICENSE file.
+*/
+
+package controllers_test
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	topology "github.com/rabbitmq/messaging-topology-operator/api/v1alpha2"
+	"github.com/rabbitmq/messaging-topology-operator/controllers"
+	"github.com/rabbitmq/messaging-topology-operator/internal"
+	"github.com/rabbitmq/messaging-topology-operator/internal/internalfakes"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
+	runtimeClient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+)
+
+func TestControllers(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Controller Suite")
+}
+
+var (
+	testEnv                   *envtest.Environment
+	client                    runtimeClient.Client
+	clientSet                 *kubernetes.Clientset
+	ctx                       = context.Background()
+	fakeRabbitMQClient        internalfakes.FakeRabbitMQClient
+	fakeRabbitMQClientFactory = func(ctx context.Context, c runtimeClient.Client, rmq topology.RabbitmqClusterReference, namespace string) (internal.RabbitMQClient, error) {
+		return &fakeRabbitMQClient, nil
+	}
+	fakeRecorder *record.FakeRecorder
+)
+
+var _ = BeforeSuite(func(done Done) {
+	logf.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(GinkgoWriter)))
+	By("bootstrapping test environment")
+	testEnv = &envtest.Environment{
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "config", "crd", "bases"),
+		},
+	}
+
+	cfg, err := testEnv.Start()
+	Expect(err).ToNot(HaveOccurred())
+	Expect(cfg).ToNot(BeNil())
+
+	Expect(scheme.AddToScheme(scheme.Scheme)).To(Succeed())
+	Expect(topology.AddToScheme(scheme.Scheme)).To(Succeed())
+
+	clientSet, err = kubernetes.NewForConfig(cfg)
+	Expect(err).NotTo(HaveOccurred())
+
+	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
+		Scheme: scheme.Scheme,
+	})
+	Expect(err).ToNot(HaveOccurred())
+
+	fakeRecorder = record.NewFakeRecorder(128)
+
+	err = (&controllers.BindingReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.ExchangeReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.PermissionReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.PolicyReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.QueueReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.UserReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+	err = (&controllers.VhostReconciler{
+		Client:                mgr.GetClient(),
+		Scheme:                mgr.GetScheme(),
+		Recorder:              fakeRecorder,
+		RabbitmqClientFactory: fakeRabbitMQClientFactory,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred())
+
+	go func() {
+		err = mgr.Start(ctrl.SetupSignalHandler())
+		Expect(err).ToNot(HaveOccurred())
+	}()
+
+	client = mgr.GetClient()
+	Expect(client).ToNot(BeNil())
+
+	close(done)
+}, 60)
+
+var _ = BeforeEach(func() {
+	fakeRabbitMQClient = internalfakes.FakeRabbitMQClient{}
+})
+
+var _ = AfterEach(func() {
+	for len(fakeRecorder.Events) > 0 {
+		// Drain any unused events
+		<-fakeRecorder.Events
+	}
+})
+
+var _ = AfterSuite(func() {
+	By("tearing down the test environment")
+	Expect(testEnv.Stop()).To(Succeed())
+})
+
+func observedEvents() []string {
+	var events []string
+	for len(fakeRecorder.Events) > 0 {
+		events = append(events, <-fakeRecorder.Events)
+	}
+	return events
+}

controllers/user_controller.go

@@ -323,6 +323,7 @@ func (r *UserReconciler) deleteUser(ctx context.Context, client internal.RabbitM
 		logger.Error(err, msg, "user", user.Name)
 		return err
 	}
+	r.Recorder.Event(user, corev1.EventTypeNormal, "SuccessfulDelete", "successfully deleted user")
 	return r.removeFinalizer(ctx, user)
 }
 

controllers/user_controller_test.go

@@ -0,0 +1,201 @@
+package controllers_test
+
+import (
+	"bytes"
+	"errors"
+	"io/ioutil"
+	"net/http"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"
+	topology "github.com/rabbitmq/messaging-topology-operator/api/v1alpha2"
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+var _ = Describe("UserController", func() {
+	When("creating a user", func() {
+		var user topology.User
+		var userName string
+
+		JustBeforeEach(func() {
+			user = topology.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      userName,
+					Namespace: "default",
+				},
+				Spec: topology.UserSpec{
+					RabbitmqClusterReference: topology.RabbitmqClusterReference{
+						Name: "example-rabbit",
+					},
+				},
+			}
+		})
+
+		Context("Creating a user", func() {
+			When("the RabbitMQ Client returns a HTTP error response", func() {
+				BeforeEach(func() {
+					userName = "test-user-http-error"
+					fakeRabbitMQClient.PutUserReturns(&http.Response{
+						Status:     "418 I'm a teapot",
+						StatusCode: 418,
+					}, errors.New("Some HTTP error"))
+				})
+
+				It("sets the status condition to indicate a failure to reconcile", func() {
+					Expect(client.Create(ctx, &user)).To(Succeed())
+					EventuallyWithOffset(1, func() []topology.Condition {
+						_ = client.Get(
+							ctx,
+							types.NamespacedName{Name: user.Name, Namespace: user.Namespace},
+							&user,
+						)
+
+						return user.Status.Conditions
+					}, 10*time.Second, 1*time.Second).Should(ContainElement(MatchFields(IgnoreExtras, Fields{
+						"Type":    Equal(topology.ConditionType("Ready")),
+						"Reason":  Equal("FailedCreateOrUpdate"),
+						"Status":  Equal(corev1.ConditionFalse),
+						"Message": ContainSubstring("Some HTTP error"),
+					})))
+				})
+			})
+
+			When("the RabbitMQ Client returns a Go error response", func() {
+				BeforeEach(func() {
+					userName = "test-user-go-error"
+					fakeRabbitMQClient.PutUserReturns(nil, errors.New("Hit a exception"))
+				})
+
+				It("sets the status condition to indicate a failure to reconcile", func() {
+					Expect(client.Create(ctx, &user)).To(Succeed())
+					EventuallyWithOffset(1, func() []topology.Condition {
+						_ = client.Get(
+							ctx,
+							types.NamespacedName{Name: user.Name, Namespace: user.Namespace},
+							&user,
+						)
+
+						return user.Status.Conditions
+					}, 10*time.Second, 1*time.Second).Should(ContainElement(MatchFields(IgnoreExtras, Fields{
+						"Type":    Equal(topology.ConditionType("Ready")),
+						"Reason":  Equal("FailedCreateOrUpdate"),
+						"Status":  Equal(corev1.ConditionFalse),
+						"Message": ContainSubstring("Hit a exception"),
+					})))
+				})
+			})
+
+			When("the RabbitMQ Client successfully creates a user", func() {
+				BeforeEach(func() {
+					userName = "test-user-success"
+					fakeRabbitMQClient.PutUserReturns(&http.Response{
+						Status:     "201 Created",
+						StatusCode: http.StatusCreated,
+					}, nil)
+				})
+
+				It("sets the status condition to indicate a success in reconciling", func() {
+					Expect(client.Create(ctx, &user)).To(Succeed())
+					EventuallyWithOffset(1, func() []topology.Condition {
+						_ = client.Get(
+							ctx,
+							types.NamespacedName{Name: user.Name, Namespace: user.Namespace},
+							&user,
+						)
+
+						return user.Status.Conditions
+					}, 10*time.Second, 1*time.Second).Should(ContainElement(MatchFields(IgnoreExtras, Fields{
+						"Type":   Equal(topology.ConditionType("Ready")),
+						"Reason": Equal("SuccessfulCreateOrUpdate"),
+						"Status": Equal(corev1.ConditionTrue),
+					})))
+				})
+			})
+		})
+
+		Context("Deleting a user", func() {
+			JustBeforeEach(func() {
+				fakeRabbitMQClient.PutUserReturns(&http.Response{
+					Status:     "201 Created",
+					StatusCode: http.StatusCreated,
+				}, nil)
+				Expect(client.Create(ctx, &user)).To(Succeed())
+				EventuallyWithOffset(1, func() []topology.Condition {
+					_ = client.Get(
+						ctx,
+						types.NamespacedName{Name: user.Name, Namespace: user.Namespace},
+						&user,
+					)
+
+					return user.Status.Conditions
+				}, 10*time.Second, 1*time.Second).Should(ContainElement(MatchFields(IgnoreExtras, Fields{
+					"Type":   Equal(topology.ConditionType("Ready")),
+					"Reason": Equal("SuccessfulCreateOrUpdate"),
+					"Status": Equal(corev1.ConditionTrue),
+				})))
+			})
+
+			When("the RabbitMQ Client returns a HTTP error response", func() {
+				BeforeEach(func() {
+					userName = "delete-user-http-error"
+					fakeRabbitMQClient.DeleteUserReturns(&http.Response{
+						Status:     "502 Bad Gateway",
+						StatusCode: http.StatusBadGateway,
+						Body:       ioutil.NopCloser(bytes.NewBufferString("Hello World")),
+					}, nil)
+				})
+
+				It("raises an event to indicate a failure to delete", func() {
+					Expect(client.Delete(ctx, &user)).To(Succeed())
+					Consistently(func() bool {
+						err := client.Get(ctx, types.NamespacedName{Name: user.Name, Namespace: user.Namespace}, &topology.User{})
+						return apierrors.IsNotFound(err)
+					}, 5).Should(BeFalse())
+					Expect(observedEvents()).To(ContainElement("Warning FailedDelete failed to delete user"))
+				})
+			})
+
+			When("the RabbitMQ Client returns a Go error response", func() {
+				BeforeEach(func() {
+					userName = "delete-user-go-error"
+					fakeRabbitMQClient.DeleteUserReturns(nil, errors.New("some error"))
+				})
+
+				It("raises an event to indicate a failure to delete", func() {
+					Expect(client.Delete(ctx, &user)).To(Succeed())
+					Consistently(func() bool {
+						err := client.Get(ctx, types.NamespacedName{Name: user.Name, Namespace: user.Namespace}, &topology.User{})
+						return apierrors.IsNotFound(err)
+					}, 5).Should(BeFalse())
+					Expect(observedEvents()).To(ContainElement("Warning FailedDelete failed to delete user"))
+				})
+			})
+
+			When("the RabbitMQ Client successfully deletes a user", func() {
+				BeforeEach(func() {
+					userName = "delete-user-success"
+					fakeRabbitMQClient.DeleteUserReturns(&http.Response{
+						Status:     "204 No Content",
+						StatusCode: http.StatusNoContent,
+					}, nil)
+				})
+
+				It("raises an event to indicate a successful deletion", func() {
+					Expect(client.Delete(ctx, &user)).To(Succeed())
+					Eventually(func() bool {
+						err := client.Get(ctx, types.NamespacedName{Name: user.Name, Namespace: user.Namespace}, &topology.User{})
+						return apierrors.IsNotFound(err)
+					}, 5).Should(BeTrue())
+					observedEvents := observedEvents()
+					Expect(observedEvents).NotTo(ContainElement("Warning FailedDelete failed to delete user"))
+					Expect(observedEvents).To(ContainElement("Normal SuccessfulDelete successfully deleted user"))
+				})
+			})
+		})
+	})
+})