binding webhook shouldn't compare binding.spec directly

- fixed an issue in bindings.rabbitmq.com webhook. binding.spec.arguments is a map. comparing binding.spec directly with == does not work on map types.
rabbitmq · Mar 23, 2021 · d5c3710 · d5c3710
1 parent bf2e20f
commit d5c3710
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 20 deletions.
diff --git a/api/v1alpha1/binding_webhook.go b/api/v1alpha1/binding_webhook.go
@@ -5,6 +5,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"reflect"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
@@ -31,13 +32,72 @@ func (b *Binding) ValidateUpdate(old runtime.Object) error {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a binding but got a %T", old))
 	}
 
-	if oldBinding.Spec != b.Spec {
-		return apierrors.NewForbidden(
-			b.GroupResource(),
-			b.Name,
-			field.Forbidden(field.NewPath("spec"), "binding.spec is immutable"))
+	var allErrs field.ErrorList
+	detailMsg := "updates on vhost and rabbitmqClusterReference are all forbidden"
+
+	if b.Spec.Vhost != oldBinding.Spec.Vhost {
+		return apierrors.NewForbidden(b.GroupResource(), b.Name,
+			field.Forbidden(field.NewPath("spec", "vhost"), detailMsg))
 	}
-	return nil
+
+	if b.Spec.RabbitmqClusterReference != oldBinding.Spec.RabbitmqClusterReference {
+		return apierrors.NewForbidden(b.GroupResource(), b.Name,
+			field.Forbidden(field.NewPath("spec", "rabbitmqClusterReference"), detailMsg))
+	}
+
+	if b.Spec.Source != oldBinding.Spec.Source {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "source"),
+			b.Spec.Source,
+			"source cannot be updated",
+		))
+	}
+
+	if b.Spec.Destination != oldBinding.Spec.Destination {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "destination"),
+			b.Spec.Destination,
+			"destination cannot be updated",
+		))
+	}
+
+	if b.Spec.DestinationType != oldBinding.Spec.DestinationType {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "destinationType"),
+			b.Spec.DestinationType,
+			"destinationType cannot be updated",
+		))
+	}
+
+	if b.Spec.DestinationType != oldBinding.Spec.DestinationType {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "destinationType"),
+			b.Spec.DestinationType,
+			"destinationType cannot be updated",
+		))
+	}
+
+	if b.Spec.RoutingKey != oldBinding.Spec.RoutingKey {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "routingKey"),
+			b.Spec.RoutingKey,
+			"routingKey cannot be updated",
+		))
+	}
+
+	if !reflect.DeepEqual(b.Spec.Arguments, oldBinding.Spec.Arguments) {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "arguments"),
+			b.Spec.Arguments,
+			"arguments cannot be updated",
+		))
+	}
+
+	if len(allErrs) == 0 {
+		return nil
+	}
+
+	return apierrors.NewInvalid(GroupVersion.WithKind("Binding").GroupKind(), b.Name, allErrs)
 }
 
 // no validation logic on delete

diff --git a/api/v1alpha1/binding_webhook_test.go b/api/v1alpha1/binding_webhook_test.go
@@ -32,42 +32,42 @@ var _ = Describe("Binding webhook", func() {
 		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 
+	It("does not allow updates on RabbitmqClusterReference", func() {
+		newBinding := oldBinding.DeepCopy()
+		newBinding.Spec.RabbitmqClusterReference = RabbitmqClusterReference{
+			Name:      "new-cluster",
+			Namespace: "default",
+		}
+		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+	})
+
 	It("does not allow updates on source", func() {
 		newBinding := oldBinding.DeepCopy()
 		newBinding.Spec.Source = "updated-source"
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+		Expect(apierrors.IsInvalid(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 
 	It("does not allow updates on destination", func() {
 		newBinding := oldBinding.DeepCopy()
 		newBinding.Spec.Destination = "updated-des"
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+		Expect(apierrors.IsInvalid(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 
 	It("does not allow updates on destination type", func() {
 		newBinding := oldBinding.DeepCopy()
 		newBinding.Spec.DestinationType = "exchange"
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+		Expect(apierrors.IsInvalid(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 
 	It("does not allow updates on routing key", func() {
 		newBinding := oldBinding.DeepCopy()
 		newBinding.Spec.RoutingKey = "not-allowed"
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+		Expect(apierrors.IsInvalid(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 
 	It("does not allow updates on binding arguments", func() {
 		newBinding := oldBinding.DeepCopy()
 		newBinding.Spec.Arguments = &runtime.RawExtension{Raw: []byte(`{"new":"new-value"}`)}
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
-	})
-
-	It("does not allow updates on RabbitmqClusterReference", func() {
-		newBinding := oldBinding.DeepCopy()
-		newBinding.Spec.RabbitmqClusterReference = RabbitmqClusterReference{
-			Name:      "new-cluster",
-			Namespace: "default",
-		}
-		Expect(apierrors.IsForbidden(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
+		Expect(apierrors.IsInvalid(newBinding.ValidateUpdate(&oldBinding))).To(BeTrue())
 	})
 })