Add validation webhook for policies.rabbitmq.com

api/v1alpha1/policy_types.go

@@ -3,6 +3,7 @@ package v1alpha1
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // PolicySpec defines the desired state of Policy
@@ -66,6 +67,13 @@ type PolicyList struct {
 	Items           []Policy `json:"items"`
 }
 
+func (p *Policy) GroupResource() schema.GroupResource {
+	return schema.GroupResource{
+		Group:    p.GroupVersionKind().Group,
+		Resource: p.GroupVersionKind().Kind,
+	}
+}
+
 func init() {
 	SchemeBuilder.Register(&Policy{}, &PolicyList{})
 }

api/v1alpha1/policy_webhook.go

@@ -0,0 +1,55 @@
+package v1alpha1
+
+import (
+	"fmt"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+)
+
+func (p *Policy) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(p).
+		Complete()
+}
+
+// +kubebuilder:webhook:verbs=create;update,path=/validate-rabbitmq-com-v1alpha1-policy,mutating=false,failurePolicy=fail,groups=rabbitmq.com,resources=policies,versions=v1alpha1,name=vpolicy.kb.io,sideEffects=none,admissionReviewVersions=v1
+
+var _ webhook.Validator = &Policy{}
+
+// no validation on create
+func (p *Policy) ValidateCreate() error {
+	return nil
+}
+
+// returns error type 'forbidden' for updates on policy name, vhost and rabbitmqClusterReference
+func (p *Policy) ValidateUpdate(old runtime.Object) error {
+	oldPolicy, ok := old.(*Policy)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a policy but got a %T", old))
+	}
+
+	detailMsg := "updates on name, vhost and rabbitmqClusterReference are all forbidden"
+	if p.Spec.Name != oldPolicy.Spec.Name {
+		return apierrors.NewForbidden(p.GroupResource(), p.Name,
+			field.Forbidden(field.NewPath("spec", "name"), detailMsg))
+	}
+
+	if p.Spec.Vhost != oldPolicy.Spec.Vhost {
+		return apierrors.NewForbidden(p.GroupResource(), p.Name,
+			field.Forbidden(field.NewPath("spec", "vhost"), detailMsg))
+	}
+
+	if p.Spec.RabbitmqClusterReference != oldPolicy.Spec.RabbitmqClusterReference {
+		return apierrors.NewForbidden(p.GroupResource(), p.Name,
+			field.Forbidden(field.NewPath("spec", "rabbitmqClusterReference"), detailMsg))
+	}
+	return nil
+}
+
+// no validation on delete
+func (p *Policy) ValidateDelete() error {
+	return nil
+}

api/v1alpha1/policy_webhook_test.go

@@ -0,0 +1,73 @@
+package v1alpha1
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+var _ = Describe("policy webhook", func() {
+	var policy = Policy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test",
+		},
+		Spec: PolicySpec{
+			Name:     "test",
+			Vhost:    "/test",
+			Pattern:  "a-pattern",
+			ApplyTo:  "all",
+			Priority: 0,
+			RabbitmqClusterReference: RabbitmqClusterReference{
+				Name:      "a-cluster",
+				Namespace: "default",
+			},
+		},
+	}
+
+	It("does not allow updates on policy name", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.Name = "new-name"
+		Expect(apierrors.IsForbidden(newPolicy.ValidateUpdate(&policy))).To(BeTrue())
+	})
+
+	It("does not allow updates on vhost", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.Vhost = "new-vhost"
+		Expect(apierrors.IsForbidden(newPolicy.ValidateUpdate(&policy))).To(BeTrue())
+	})
+
+	It("does not allow updates on RabbitmqClusterReference", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.RabbitmqClusterReference = RabbitmqClusterReference{
+			Name:      "new-cluster",
+			Namespace: "default",
+		}
+		Expect(apierrors.IsForbidden(newPolicy.ValidateUpdate(&policy))).To(BeTrue())
+	})
+
+	It("allows updates on policy.spec.pattern", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.Pattern = "new-pattern"
+		Expect(newPolicy.ValidateUpdate(&policy)).To(Succeed())
+	})
+
+	It("allows updates on policy.spec.applyTo", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.ApplyTo = "queues"
+		Expect(newPolicy.ValidateUpdate(&policy)).To(Succeed())
+	})
+
+	It("allows updates on policy.spec.priority", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.Priority = 1000
+		Expect(newPolicy.ValidateUpdate(&policy)).To(Succeed())
+	})
+
+	It("allows updates on policy.spec.definition", func() {
+		newPolicy := policy.DeepCopy()
+		newPolicy.Spec.Definition = &runtime.RawExtension{Raw: []byte(`{"key":"new-definition-value"}`)}
+		Expect(newPolicy.ValidateUpdate(&policy)).To(Succeed())
+	})
+})

config/crd/kustomization.yaml

@@ -15,12 +15,14 @@ patchesStrategicMerge:
 - patches/webhook_in_queues.yaml
 - patches/webhook_in_exchanges.yaml
 - patches/webhook_in_vhosts.yaml
+- patches/webhook_in_policies.yaml
 # +kubebuilder:scaffold:crdkustomizewebhookpatch
 
 - patches/cainjection_in_bindings.yaml
 - patches/cainjection_in_queues.yaml
-- patches/webhook_in_exchanges.yaml
-- patches/webhook_in_vhosts.yaml
+- patches/cainjection_in_exchanges.yaml
+- patches/cainjection_in_vhosts.yaml
+- patches/cainjection_in_policies.yaml
 # +kubebuilder:scaffold:crdkustomizecainjectionpatch
 
 configurations:

config/crd/patches/cainjection_in_policies.yaml

@@ -1,6 +1,6 @@
 # The following patch adds a directive for certmanager to inject CA into the CRD
 # CRD conversion requires k8s 1.13 or later.
-apiVersion: apiextensions.k8s.io/v1beta1
+apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:

config/crd/patches/webhook_in_policies.yaml

@@ -1,17 +1,17 @@
 # The following patch enables conversion webhook for CRD
 # CRD conversion requires k8s 1.13 or later.
-apiVersion: apiextensions.k8s.io/v1beta1
+apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   name: policies.rabbitmq.com
 spec:
   conversion:
     strategy: Webhook
-    webhookClientConfig:
-      # this is "\n" used as a placeholder, otherwise it will be rejected by the apiserver for being blank,
-      # but we're going to set it later using the cert-manager (or potentially a patch if not using cert-manager)
-      caBundle: Cg==
-      service:
-        namespace: system
-        name: webhook-service
-        path: /convert
+    webhook:
+      conversionReviewVersions: ["v1", "v1beta1"]
+      clientConfig:
+        caBundle: Cg==
+        service:
+          namespace: system
+          name: webhook-service
+          path: /convert

config/webhook/manifests.yaml

@@ -46,6 +46,26 @@ webhooks:
     resources:
     - exchanges
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-rabbitmq-com-v1alpha1-policy
+  failurePolicy: Fail
+  name: vpolicy.kb.io
+  rules:
+  - apiGroups:
+    - rabbitmq.com
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - policies
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

main.go

@@ -134,6 +134,10 @@ func main() {
 		log.Error(err, "unable to create webhook", "webhook", "Vhost")
 		os.Exit(1)
 	}
+	if err = (&topologyv1alpha1.Policy{}).SetupWebhookWithManager(mgr); err != nil {
+		log.Error(err, "unable to create webhook", "webhook", "Policy")
+		os.Exit(1)
+	}
 	// +kubebuilder:scaffold:builder
 
 	log.Info("starting manager")

system_tests/policy_system_tests.go

@@ -78,7 +78,13 @@ var _ = Describe("Policy", func() {
 		By("setting status.observedGeneration")
 		Expect(updatedPolicy.Status.ObservedGeneration).To(Equal(updatedPolicy.GetGeneration()))
 
-		By("updating policy")
+		By("not allowing updates on certain fields")
+		updateTest := topologyv1alpha1.Policy{}
+		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: policy.Name, Namespace: policy.Namespace}, &updateTest)).To(Succeed())
+		updateTest.Spec.Vhost = "/a-new-vhost"
+		Expect(k8sClient.Update(ctx, &updateTest).Error()).To(ContainSubstring("spec.vhost: Forbidden: updates on name, vhost and rabbitmqClusterReference are all forbidden"))
+
+		By("updating policy definitions successfully")
 		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: policy.Name, Namespace: policy.Namespace}, policy)).To(Succeed())
 		policy.Spec.Definition = &runtime.RawExtension{
 			Raw: []byte(`{"ha-mode":"exactly",