Merge pull request #756 from rabbitmq/update-controller-runtime-0.17

Update controller runtime 0.17

Makefile

@@ -50,7 +50,17 @@ $(KUBEBUILDER_ASSETS):
 ### Targets
 
 .PHONY: unit-tests
-unit-tests: install-tools $(KUBEBUILDER_ASSETS) generate fmt vet vuln manifests ## Run unit tests
+unit-tests::install-tools ## Run unit tests
+unit-test::$(KUBEBUILDER_ASSETS)
+unit-test::generate
+unit-test::fmt
+unit-test::vet
+unit-test::vuln
+unit-test::manifests
+unit-test::just-unit-tests
+
+.PHONY: just-unit-tests
+just-unit-tests:
 	ginkgo -r --randomize-all api/ internal/ rabbitmqclient/
 
 .PHONY: integration-tests
@@ -63,7 +73,7 @@ just-integration-tests: $(KUBEBUILDER_ASSETS) vet
 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 --randomize-all -r system_tests/
+	NAMESPACE="rabbitmq-system" ginkgo --randomize-all -r $(GINKGO_EXTRA) system_tests/
 
 # Build manager binary
 manager: generate fmt vet vuln
@@ -207,7 +217,7 @@ generate-manifests:
 # Cert Manager #
 ################
 
-CERT_MANAGER_VERSION ?= v1.7.0
+CERT_MANAGER_VERSION ?= v1.12.3
 CERT_MANAGER_MANIFEST ?= https://github.com/jetstack/cert-manager/releases/download/$(CERT_MANAGER_VERSION)/cert-manager.yaml
 
 CMCTL = $(LOCAL_BIN)/cmctl

api/v1alpha1/superstream_types.go

@@ -70,10 +70,10 @@ type SuperStreamList struct {
 	Items           []SuperStream `json:"items"`
 }
 
-func (q *SuperStream) GroupResource() schema.GroupResource {
+func (s *SuperStream) GroupResource() schema.GroupResource {
 	return schema.GroupResource{
-		Group:    q.GroupVersionKind().Group,
-		Resource: q.GroupVersionKind().Kind,
+		Group:    s.GroupVersionKind().Group,
+		Resource: s.GroupVersionKind().Kind,
 	}
 }
 

api/v1alpha1/superstream_webhook.go

@@ -10,6 +10,7 @@ This product may include a number of subcomponents with separate copyright notic
 package v1alpha1
 
 import (
+	"context"
 	"fmt"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -27,51 +28,60 @@ func (s *SuperStream) SetupWebhookWithManager(mgr ctrl.Manager) error {
 
 // +kubebuilder:webhook:verbs=create;update,path=/validate-rabbitmq-com-v1alpha1-superstream,mutating=false,failurePolicy=fail,groups=rabbitmq.com,resources=superstreams,versions=v1alpha1,name=vsuperstream.kb.io,sideEffects=none,admissionReviewVersions=v1
 
-var _ webhook.Validator = &SuperStream{}
+var _ webhook.CustomValidator = &SuperStream{}
 
-// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
-// either rabbitmqClusterReference.name or rabbitmqClusterReference.connectionSecret must be provided but not both
-func (s *SuperStream) ValidateCreate() (admission.Warnings, error) {
-	return s.Spec.RabbitmqClusterReference.ValidateOnCreate(s.GroupResource(), s.Name)
+// ValidateCreate - either rabbitmqClusterReference.name or
+// rabbitmqClusterReference.connectionSecret must be provided but not both
+func (s *SuperStream) ValidateCreate(_ context.Context, obj runtime.Object) (warnings admission.Warnings, err error) {
+	ss, ok := obj.(*SuperStream)
+	if !ok {
+		return nil, fmt.Errorf("expected a RabbitMQ super stream but got a %T", obj)
+	}
+	return ss.Spec.RabbitmqClusterReference.ValidateOnCreate(ss.GroupResource(), ss.Name)
 }
 
 // ValidateUpdate returns error type 'forbidden' for updates on superstream name, vhost and rabbitmqClusterReference
-func (s *SuperStream) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	oldSuperStream, ok := old.(*SuperStream)
+func (s *SuperStream) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (warnings admission.Warnings, err error) {
+	oldSuperStream, ok := oldObj.(*SuperStream)
+	if !ok {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a superstream but got a %T", oldObj))
+	}
+
+	newSuperStream, ok := newObj.(*SuperStream)
 	if !ok {
-		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a superstream but got a %T", old))
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a superstream but got a %T", newObj))
 	}
 
-	detailMsg := "updates on name, vhost and rabbitmqClusterReference are all forbidden"
-	if s.Spec.Name != oldSuperStream.Spec.Name {
-		return nil, apierrors.NewForbidden(s.GroupResource(), s.Name,
+	const detailMsg = "updates on name, vhost and rabbitmqClusterReference are all forbidden"
+	if newSuperStream.Spec.Name != oldSuperStream.Spec.Name {
+		return nil, apierrors.NewForbidden(newSuperStream.GroupResource(), newSuperStream.Name,
 			field.Forbidden(field.NewPath("spec", "name"), detailMsg))
 	}
-	if s.Spec.Vhost != oldSuperStream.Spec.Vhost {
-		return nil, apierrors.NewForbidden(s.GroupResource(), s.Name,
+	if newSuperStream.Spec.Vhost != oldSuperStream.Spec.Vhost {
+		return nil, apierrors.NewForbidden(newSuperStream.GroupResource(), newSuperStream.Name,
 			field.Forbidden(field.NewPath("spec", "vhost"), detailMsg))
 	}
 
-	if !oldSuperStream.Spec.RabbitmqClusterReference.Matches(&s.Spec.RabbitmqClusterReference) {
-		return nil, apierrors.NewForbidden(s.GroupResource(), s.Name,
+	if !oldSuperStream.Spec.RabbitmqClusterReference.Matches(&newSuperStream.Spec.RabbitmqClusterReference) {
+		return nil, apierrors.NewForbidden(newSuperStream.GroupResource(), newSuperStream.Name,
 			field.Forbidden(field.NewPath("spec", "rabbitmqClusterReference"), detailMsg))
 	}
 
-	if !routingKeyUpdatePermitted(oldSuperStream.Spec.RoutingKeys, s.Spec.RoutingKeys) {
-		return nil, apierrors.NewForbidden(s.GroupResource(), s.Name,
+	if !routingKeyUpdatePermitted(oldSuperStream.Spec.RoutingKeys, newSuperStream.Spec.RoutingKeys) {
+		return nil, apierrors.NewForbidden(newSuperStream.GroupResource(), newSuperStream.Name,
 			field.Forbidden(field.NewPath("spec", "routingKeys"), "updates may only add to the existing list of routing keys"))
 	}
 
-	if s.Spec.Partitions < oldSuperStream.Spec.Partitions {
-		return nil, apierrors.NewForbidden(s.GroupResource(), s.Name,
+	if newSuperStream.Spec.Partitions < oldSuperStream.Spec.Partitions {
+		return nil, apierrors.NewForbidden(newSuperStream.GroupResource(), newSuperStream.Name,
 			field.Forbidden(field.NewPath("spec", "partitions"), "updates may only increase the partition count, and may not decrease it"))
 	}
 
 	return nil, nil
 }
 
 // ValidateDelete no validation on delete
-func (s *SuperStream) ValidateDelete() (admission.Warnings, error) {
+func (s *SuperStream) ValidateDelete(_ context.Context, _ runtime.Object) (warnings admission.Warnings, err error) {
 	return nil, nil
 }
 

api/v1alpha1/superstream_webhook_test.go

@@ -1,6 +1,7 @@
 package v1alpha1
 
 import (
+	"context"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	topologyv1beta1 "github.com/rabbitmq/messaging-topology-operator/api/v1beta1"
@@ -10,7 +11,11 @@ import (
 )
 
 var _ = Describe("superstream webhook", func() {
-	var superstream = SuperStream{}
+	var (
+		superstream = SuperStream{}
+		rootCtx     = context.Background()
+	)
+
 	BeforeEach(func() {
 		superstream = SuperStream{
 			ObjectMeta: metav1.ObjectMeta{
@@ -31,84 +36,91 @@ var _ = Describe("superstream webhook", func() {
 		It("does not allow both spec.rabbitmqClusterReference.name and spec.rabbitmqClusterReference.connectionSecret be configured", func() {
 			notAllowed := superstream.DeepCopy()
 			notAllowed.Spec.RabbitmqClusterReference.ConnectionSecret = &corev1.LocalObjectReference{Name: "some-secret"}
-			_, err := notAllowed.ValidateCreate()
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := notAllowed.ValidateCreate(rootCtx, notAllowed)
+			Expect(err).To(MatchError(ContainSubstring("invalid RabbitmqClusterReference: do not provide both name and connectionSecret")))
 		})
 
 		It("spec.rabbitmqClusterReference.name and spec.rabbitmqClusterReference.connectionSecret cannot both be empty", func() {
 			notAllowed := superstream.DeepCopy()
 			notAllowed.Spec.RabbitmqClusterReference.Name = ""
 			notAllowed.Spec.RabbitmqClusterReference.ConnectionSecret = nil
-			_, err := notAllowed.ValidateCreate()
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := notAllowed.ValidateCreate(rootCtx, notAllowed)
+			Expect(err).To(MatchError(ContainSubstring("invalid RabbitmqClusterReference: must provide either name or connectionSecret")))
 		})
 	})
 
 	Context("ValidateUpdate", func() {
 		It("does not allow updates on superstream name", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.Name = "new-name"
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates on name, vhost and rabbitmqClusterReference are all forbidden")))
 		})
 
 		It("does not allow updates on superstream vhost", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.Vhost = "new-vhost"
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates on name, vhost and rabbitmqClusterReference are all forbidden")))
 		})
 
 		It("does not allow updates on RabbitmqClusterReference", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.RabbitmqClusterReference = topologyv1beta1.RabbitmqClusterReference{
 				Name: "new-cluster",
 			}
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates on name, vhost and rabbitmqClusterReference are all forbidden")))
 		})
 
 		It("does not allow updates on rabbitmqClusterReference.connectionSecret", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.RabbitmqClusterReference = topologyv1beta1.RabbitmqClusterReference{ConnectionSecret: &corev1.LocalObjectReference{Name: "a-secret"}}
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates on name, vhost and rabbitmqClusterReference are all forbidden")))
 		})
 
 		It("does not allow updates on superstream.spec.routingKeys", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.RoutingKeys = []string{"a1", "d6"}
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates may only add to the existing list of routing keys")))
 		})
 
-		It("if the superstream previously had routing keys and the update only appends, the update succeeds", func() {
+		Specify("if the superstream previously had routing keys and the update only appends, the update succeeds", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.RoutingKeys = []string{"a1", "b2", "f17", "z66"}
-			_, err := newSuperStream.ValidateUpdate(&superstream)
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
 			Expect(err).NotTo(HaveOccurred())
 		})
 
-		It("if the superstream previously had no routing keys but now does, the update fails", func() {
+		Specify("if the superstream previously had no routing keys but now does, the update fails", func() {
 			superstream.Spec.RoutingKeys = nil
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.RoutingKeys = []string{"a1", "b2", "f17"}
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates may only add to the existing list of routing keys")))
 		})
 
 		It("allows superstream.spec.partitions to be increased", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.Partitions = 1000
-			_, err := newSuperStream.ValidateUpdate(&superstream)
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
 			Expect(err).NotTo(HaveOccurred())
 		})
 
 		It("does not allow superstream.spec.partitions to be decreased", func() {
 			newSuperStream := superstream.DeepCopy()
 			newSuperStream.Spec.Partitions = 1
-			_, err := newSuperStream.ValidateUpdate(&superstream)
-			Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			_, err := newSuperStream.ValidateUpdate(rootCtx, &superstream, newSuperStream)
+			Expect(apierrors.IsForbidden(err)).To(BeTrue(), "expected error type to be 'forbidden'")
+			Expect(err).To(MatchError(ContainSubstring("updates may only increase the partition count, and may not decrease it")))
 		})
 	})
 })