Add webhook validation for permissions userReference

rabbitmq · Jul 1, 2021 · 44d6c54 · 44d6c54
1 parent 3f52a78
commit 44d6c54
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 37 deletions.
diff --git a/api/v1beta1/permission_types.go b/api/v1beta1/permission_types.go
@@ -9,7 +9,7 @@ import (
 // PermissionSpec defines the desired state of Permission
 type PermissionSpec struct {
 	// Name of an existing user; must provide user or userReference, else create/update will fail; cannot be updated
-	User string `json:"user"`
+	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

diff --git a/api/v1beta1/permission_webhook.go b/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
 }
diff --git a/api/v1beta1/permission_webhook_test.go b/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())
+		})
 	})
 })
diff --git a/config/crd/bases/rabbitmq.com_permissions.yaml b/config/crd/bases/rabbitmq.com_permissions.yaml
@@ -80,7 +80,6 @@ spec:
             required:
             - permissions
             - rabbitmqClusterReference
-            - user
             - vhost
             type: object
           status:

diff --git a/system_tests/permissions_system_test.go b/system_tests/permissions_system_test.go
@@ -118,7 +118,7 @@ var _ = Describe("Permission", func() {
 		updateTest := topology.Permission{}
 		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: permission.Name, Namespace: permission.Namespace}, &updateTest)).To(Succeed())
 		updateTest.Spec.Vhost = "/a-new-vhost"
-		Expect(k8sClient.Update(ctx, &updateTest).Error()).To(ContainSubstring("spec.vhost: Forbidden: updates on user, vhost and rabbitmqClusterReference are all forbidden"))
+		Expect(k8sClient.Update(ctx, &updateTest).Error()).To(ContainSubstring("spec.vhost: Forbidden: updates on user, userReference, vhost and rabbitmqClusterReference are all forbidden"))
 
 		By("updating permissions successfully")
 		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: permission.Name, Namespace: permission.Namespace}, permission)).To(Succeed())