Merge pull request #177 from rabbitmq/user_permissions

Able to reference a User from permissions.rabbitmq.com crd

Author: ChunyiLyu

api/v1beta1/permission_types.go

@@ -1,15 +1,17 @@
 package v1beta1
 
 import (
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // PermissionSpec defines the desired state of Permission
 type PermissionSpec struct {
-	// Name of an existing user; required property; cannot be updated
-	// +kubebuilder:validation:Required
-	User string `json:"user"`
+	// Name of an existing user; must provide user or userReference, else create/update will fail; cannot be updated
+	User string `json:"user,omitempty"`
+	// Reference to an existing user.rabbitmq.com object; must provide user or userReference, else create/update will fail; cannot be updated
+	UserReference *corev1.LocalObjectReference `json:"userReference,omitempty"`
 	// Name of an existing vhost; required property; cannot be updated
 	// +kubebuilder:validation:Required
 	Vhost string `json:"vhost"`

api/v1beta1/permission_types_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 )
@@ -77,4 +78,33 @@ var _ = Describe("Permission spec", func() {
 		Expect(fetchedPermission.Spec.Permissions.Write).To(Equal(".*"))
 		Expect(fetchedPermission.Spec.Permissions.Read).To(Equal("^?"))
 	})
+
+	It("creates a permission object with user reference provided", func() {
+		permission := Permission{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "user-ref-permission",
+				Namespace: namespace,
+			},
+			Spec: PermissionSpec{
+				UserReference: &corev1.LocalObjectReference{
+					Name: "a-created-user",
+				},
+				Vhost:       "/test",
+				Permissions: VhostPermissions{},
+				RabbitmqClusterReference: RabbitmqClusterReference{
+					Name: "some-cluster",
+				},
+			},
+		}
+		Expect(k8sClient.Create(ctx, &permission)).To(Succeed())
+		fetchedPermission := &Permission{}
+		Expect(k8sClient.Get(ctx, types.NamespacedName{
+			Name:      permission.Name,
+			Namespace: permission.Namespace,
+		}, fetchedPermission)).To(Succeed())
+		Expect(fetchedPermission.Spec.UserReference.Name).To(Equal("a-created-user"))
+		Expect(fetchedPermission.Spec.User).To(Equal(""))
+		Expect(fetchedPermission.Spec.Vhost).To(Equal("/test"))
+		Expect(fetchedPermission.Spec.RabbitmqClusterReference.Name).To(Equal("some-cluster"))
+	})
 })

api/v1beta1/permission_webhook.go

@@ -2,6 +2,7 @@ package v1beta1
 
 import (
 	"fmt"
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -19,25 +20,57 @@ func (p *Permission) SetupWebhookWithManager(mgr ctrl.Manager) error {
 
 var _ webhook.Validator = &Permission{}
 
-// no validation on create
+// ValidateCreate checks if only one of spec.user and spec.userReference is specified
 func (p *Permission) ValidateCreate() error {
+	var errorList field.ErrorList
+	if p.Spec.User == "" && p.Spec.UserReference == nil {
+		errorList = append(errorList, field.Required(field.NewPath("spec", "user and userReference"),
+			"must specify either spec.user or spec.userReference"))
+		return apierrors.NewInvalid(GroupVersion.WithKind("Permission").GroupKind(), p.Name, errorList)
+	}
+
+	if p.Spec.User != "" && p.Spec.UserReference != nil {
+		errorList = append(errorList, field.Required(field.NewPath("spec", "user and userReference"),
+			"cannot specify spec.user and spec.userReference at the same time"))
+		return apierrors.NewInvalid(GroupVersion.WithKind("Permission").GroupKind(), p.Name, errorList)
+	}
+
 	return nil
 }
 
-// do not allow updates on spec.vhost, spec.user, and spec.rabbitmqClusterReference
+// ValidateUpdate do not allow updates on spec.vhost, spec.user, spec.userReference, and spec.rabbitmqClusterReference
 // updates on spec.permissions are allowed
+// only one of spec.user and spec.userReference can be specified
 func (p *Permission) ValidateUpdate(old runtime.Object) error {
 	oldPermission, ok := old.(*Permission)
 	if !ok {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a permission but got a %T", old))
 	}
 
-	detailMsg := "updates on user, vhost and rabbitmqClusterReference are all forbidden"
+	var errorList field.ErrorList
+	if p.Spec.User == "" && p.Spec.UserReference == nil {
+		errorList = append(errorList, field.Required(field.NewPath("spec", "user and userReference"),
+			"must specify either spec.user or spec.userReference"))
+		return apierrors.NewInvalid(GroupVersion.WithKind("Permission").GroupKind(), p.Name, errorList)
+	}
+
+	if p.Spec.User != "" && p.Spec.UserReference != nil {
+		errorList = append(errorList, field.Required(field.NewPath("spec", "user and userReference"),
+			"cannot specify spec.user and spec.userReference at the same time"))
+		return apierrors.NewInvalid(GroupVersion.WithKind("Permission").GroupKind(), p.Name, errorList)
+	}
+
+	detailMsg := "updates on user, userReference, vhost and rabbitmqClusterReference are all forbidden"
 	if p.Spec.User != oldPermission.Spec.User {
 		return apierrors.NewForbidden(p.GroupResource(), p.Name,
 			field.Forbidden(field.NewPath("spec", "user"), detailMsg))
 	}
 
+	if userReferenceUpdated(p.Spec.UserReference, oldPermission.Spec.UserReference) {
+		return apierrors.NewForbidden(p.GroupResource(), p.Name,
+			field.Forbidden(field.NewPath("spec", "userReference"), detailMsg))
+	}
+
 	if p.Spec.Vhost != oldPermission.Spec.Vhost {
 		return apierrors.NewForbidden(p.GroupResource(), p.Name,
 			field.Forbidden(field.NewPath("spec", "vhost"), detailMsg))
@@ -50,7 +83,22 @@ func (p *Permission) ValidateUpdate(old runtime.Object) error {
 	return nil
 }
 
-// no validation on delete
+// returns true if userReference, which is a pointer to corev1.LocalObjectReference, has changed
+func userReferenceUpdated(new, old *corev1.LocalObjectReference) bool {
+	if new == nil && old == nil {
+		return false
+	}
+	if (new == nil && old != nil) ||
+		(new != nil && old == nil) {
+		return true
+	}
+	if new.Name != old.Name {
+		return true
+	}
+	return false
+}
+
+// ValidateDelete no validation on delete
 func (p *Permission) ValidateDelete() error {
 	return nil
 }

api/v1beta1/permission_webhook_test.go

@@ -3,6 +3,7 @@ package v1beta1
 import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -26,41 +27,80 @@ var _ = Describe("permission webhook", func() {
 		},
 	}
 
-	It("does not allow updates on user", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.User = "new-user"
-		Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
-	})
+	Context("ValidateUpdate", func() {
+		It("does not allow updates on user", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.User = "new-user"
+			Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
+		})
 
-	It("does not allow updates on vhost", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.Vhost = "new-vhost"
-		Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
-	})
+		It("does not allow updates on userReference", func() {
+			permissionWithUserRef := permission.DeepCopy()
+			permissionWithUserRef.Spec.User = ""
+			permissionWithUserRef.Spec.UserReference = &corev1.LocalObjectReference{Name: "a-user"}
+			newPermission := permissionWithUserRef.DeepCopy()
+			newPermission.Spec.UserReference = &corev1.LocalObjectReference{Name: "a-new-user"}
+			Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(permissionWithUserRef))).To(BeTrue())
+		})
 
-	It("does not allow updates on RabbitmqClusterReference", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.RabbitmqClusterReference = RabbitmqClusterReference{
-			Name: "new-cluster",
-		}
-		Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
-	})
+		It("does not allow updates on vhost", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.Vhost = "new-vhost"
+			Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
+		})
 
-	It("allows updates on permission.spec.permissions.configure", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.Permissions.Configure = "?"
-		Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
-	})
+		It("does not allow updates on RabbitmqClusterReference", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.RabbitmqClusterReference = RabbitmqClusterReference{
+				Name: "new-cluster",
+			}
+			Expect(apierrors.IsForbidden(newPermission.ValidateUpdate(&permission))).To(BeTrue())
+		})
+
+		It("allows updates on permission.spec.permissions.configure", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.Permissions.Configure = "?"
+			Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
+		})
+
+		It("allows updates on permission.spec.permissions.read", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.Permissions.Read = "?"
+			Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
+		})
+
+		It("allows updates on permission.spec.permissions.write", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.Permissions.Write = "?"
+			Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
+		})
+
+		It("does not allow user and userReference to be specified at the same time", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.UserReference = &corev1.LocalObjectReference{Name: "invalid"}
+			Expect(apierrors.IsInvalid(newPermission.ValidateUpdate(&permission))).To(BeTrue())
+		})
 
-	It("allows updates on permission.spec.permissions.read", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.Permissions.Read = "?"
-		Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
+		It("does not allow both user and userReference to be unset", func() {
+			newPermission := permission.DeepCopy()
+			newPermission.Spec.User = ""
+			newPermission.Spec.UserReference = nil
+			Expect(apierrors.IsInvalid(newPermission.ValidateUpdate(&permission))).To(BeTrue())
+		})
 	})
 
-	It("allows updates on permission.spec.permissions.write", func() {
-		newPermission := permission.DeepCopy()
-		newPermission.Spec.Permissions.Write = "?"
-		Expect(newPermission.ValidateUpdate(&permission)).To(Succeed())
+	Context("ValidateCreate", func() {
+		It("does not allow user and userReference to be specified at the same time", func() {
+			invalidPermission := permission.DeepCopy()
+			invalidPermission.Spec.UserReference = &corev1.LocalObjectReference{Name: "invalid"}
+			invalidPermission.Spec.User = "test-user"
+			Expect(apierrors.IsInvalid(invalidPermission.ValidateCreate())).To(BeTrue())
+		})
+		It("does not allow both user and userReference to be unset", func() {
+			invalidPermission := permission.DeepCopy()
+			invalidPermission.Spec.UserReference = nil
+			invalidPermission.Spec.User = ""
+			Expect(apierrors.IsInvalid(invalidPermission.ValidateCreate())).To(BeTrue())
+		})
 	})
 })

api/v1beta1/zz_generated.deepcopy.go

@@ -60,17 +60,26 @@ spec:
                 - name
                 type: object
               user:
-                description: Name of an existing user; required property; cannot be
-                  updated
+                description: Name of an existing user; must provide user or userReference,
+                  else create/update will fail; cannot be updated
                 type: string
+              userReference:
+                description: Reference to an existing user.rabbitmq.com object; must
+                  provide user or userReference, else create/update will fail; cannot
+                  be updated
+                properties:
+                  name:
+                    description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+                      TODO: Add other useful fields. apiVersion, kind, uid?'
+                    type: string
+                type: object
               vhost:
                 description: Name of an existing vhost; required property; cannot
                   be updated
                 type: string
             required:
             - permissions
             - rabbitmqClusterReference
-            - user
             - vhost
             type: object
           status:

config/crd/bases/rabbitmq.com_permissions.yaml

@@ -190,9 +190,10 @@ var _ = Describe("bindingController", func() {
 					err := client.Get(ctx, types.NamespacedName{Name: binding.Name, Namespace: binding.Namespace}, &topology.Binding{})
 					return apierrors.IsNotFound(err)
 				}, 5).Should(BeTrue())
-				observedEvents := observedEvents()
-				Expect(observedEvents).NotTo(ContainElement("Warning FailedDelete failed to delete binding"))
-				Expect(observedEvents).To(ContainElement("Normal SuccessfulDelete successfully deleted binding"))
+				Expect(observedEvents()).To(SatisfyAll(
+					Not(ContainElement("Warning FailedDelete failed to delete binding")),
+					ContainElement("Normal SuccessfulDelete successfully deleted binding"),
+				))
 			})
 		})
 	})

controllers/binding_controller_test.go

@@ -190,9 +190,10 @@ var _ = Describe("exchange-controller", func() {
 					err := client.Get(ctx, types.NamespacedName{Name: exchange.Name, Namespace: exchange.Namespace}, &topology.Exchange{})
 					return apierrors.IsNotFound(err)
 				}, 5).Should(BeTrue())
-				observedEvents := observedEvents()
-				Expect(observedEvents).NotTo(ContainElement("Warning FailedDelete failed to delete exchange"))
-				Expect(observedEvents).To(ContainElement("Normal SuccessfulDelete successfully deleted exchange"))
+				Expect(observedEvents()).To(SatisfyAll(
+					Not(ContainElement("Warning FailedDelete failed to delete exchange")),
+					ContainElement("Normal SuccessfulDelete successfully deleted exchange"),
+				))
 			})
 		})
 	})

controllers/exchange_controller_test.go

@@ -193,9 +193,10 @@ var _ = Describe("federation-controller", func() {
 					err := client.Get(ctx, types.NamespacedName{Name: federation.Name, Namespace: federation.Namespace}, &topology.Federation{})
 					return apierrors.IsNotFound(err)
 				}, 5).Should(BeTrue())
-				observedEvents := observedEvents()
-				Expect(observedEvents).NotTo(ContainElement("Warning FailedDelete failed to delete federation upstream parameter"))
-				Expect(observedEvents).To(ContainElement("Normal SuccessfulDelete successfully deleted federation upstream parameter"))
+				Expect(observedEvents()).To(SatisfyAll(
+					Not(ContainElement("Warning FailedDelete failed to delete federation upstream parameter")),
+					ContainElement("Normal SuccessfulDelete successfully deleted federation upstream parameter"),
+				))
 			})
 		})
 	})