From afae6cf67419c3796d82deb5f1d418c5e689d8ce Mon Sep 17 00:00:00 2001 From: "Chandan Kumar (raukadah)" Date: Wed, 15 Jan 2025 11:56:47 +0530 Subject: [PATCH] Add webhook validation for empty database and rabbitmq databaseInstance and rabbitMqClusterName are required fields. If an user specify databaseInstance and rabbitMqClusterName field as empty string. The webhook should fail it saying as there cannot be empty. This pr adds the validations for the same. Jira: https://issues.redhat.com/browse/OSPRH-11933 Signed-off-by: Chandan Kumar (raukadah) --- api/v1beta1/common_types.go | 4 +- api/v1beta1/watcher_webhook.go | 18 +++++ api/v1beta1/zz_generated.deepcopy.go | 10 +++ controllers/watcher_controller.go | 12 ++-- tests/functional/watcher_controller_test.go | 75 ++++++++++++++++++--- 5 files changed, 102 insertions(+), 17 deletions(-) diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index 26cf138e..494762cb 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -67,7 +67,7 @@ type WatcherTemplate struct { // +kubebuilder:default=rabbitmq // RabbitMQ instance name // Needed to request a transportURL that is created and used in Watcher - RabbitMqClusterName string `json:"rabbitMqClusterName"` + RabbitMqClusterName *string `json:"rabbitMqClusterName"` // +kubebuilder:validation:Optional // +kubebuilder:default=osp-secret @@ -77,7 +77,7 @@ type WatcherTemplate struct { // +kubebuilder:validation:Required // MariaDB instance name // Required to use the mariadb-operator instance to create the DB and user - DatabaseInstance string `json:"databaseInstance"` + DatabaseInstance *string `json:"databaseInstance"` // +kubebuilder:validation:Optional // +kubebuilder:default=watcher diff --git a/api/v1beta1/watcher_webhook.go b/api/v1beta1/watcher_webhook.go index af9a8f00..609bd9e0 100644 --- a/api/v1beta1/watcher_webhook.go +++ b/api/v1beta1/watcher_webhook.go @@ -17,6 +17,8 @@ limitations under the License. package v1beta1 import ( + "errors" + "k8s.io/apimachinery/pkg/runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -64,6 +66,14 @@ var _ webhook.Validator = &Watcher{} func (r *Watcher) ValidateCreate() (admission.Warnings, error) { watcherlog.Info("validate create", "name", r.Name) + if *r.Spec.DatabaseInstance == "" || r.Spec.DatabaseInstance == nil { + return nil, errors.New("databaseInstance field should not be empty") + } + + if *r.Spec.RabbitMqClusterName == "" || r.Spec.RabbitMqClusterName == nil { + return nil, errors.New("rabbitMqClusterName field should not be empty") + } + return nil, nil } @@ -71,6 +81,14 @@ func (r *Watcher) ValidateCreate() (admission.Warnings, error) { func (r *Watcher) ValidateUpdate(runtime.Object) (admission.Warnings, error) { watcherlog.Info("validate update", "name", r.Name) + if *r.Spec.DatabaseInstance == "" || r.Spec.DatabaseInstance == nil { + return nil, errors.New("databaseInstance field should not be empty") + } + + if *r.Spec.RabbitMqClusterName == "" || r.Spec.RabbitMqClusterName == nil { + return nil, errors.New("rabbitMqClusterName field should not be empty") + } + return nil, nil } diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 51e3612b..e67c3232 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -560,6 +560,16 @@ func (in *WatcherSubCrsTemplate) DeepCopy() *WatcherSubCrsTemplate { func (in *WatcherTemplate) DeepCopyInto(out *WatcherTemplate) { *out = *in in.WatcherCommon.DeepCopyInto(&out.WatcherCommon) + if in.RabbitMqClusterName != nil { + in, out := &in.RabbitMqClusterName, &out.RabbitMqClusterName + *out = new(string) + **out = **in + } + if in.DatabaseInstance != nil { + in, out := &in.DatabaseInstance, &out.DatabaseInstance + *out = new(string) + **out = **in + } in.APIServiceTemplate.DeepCopyInto(&out.APIServiceTemplate) } diff --git a/controllers/watcher_controller.go b/controllers/watcher_controller.go index 50f1e448..79331a57 100644 --- a/controllers/watcher_controller.go +++ b/controllers/watcher_controller.go @@ -457,11 +457,11 @@ func (r *WatcherReconciler) ensureDB( // create watcher DB instance // db := mariadbv1.NewDatabaseForAccount( - instance.Spec.DatabaseInstance, // mariadb/galera service to target - watcher.DatabaseName, // name used in CREATE DATABASE in mariadb - watcher.DatabaseCRName, // CR name for MariaDBDatabase - instance.Spec.DatabaseAccount, // CR name for MariaDBAccount - instance.Namespace, // namespace + *instance.Spec.DatabaseInstance, // mariadb/galera service to target + watcher.DatabaseName, // name used in CREATE DATABASE in mariadb + watcher.DatabaseCRName, // CR name for MariaDBDatabase + instance.Spec.DatabaseAccount, // CR name for MariaDBAccount + instance.Namespace, // namespace ) // create or patch the DB @@ -528,7 +528,7 @@ func (r *WatcherReconciler) ensureMQ( } op, err := controllerutil.CreateOrUpdate(ctx, r.Client, transportURL, func() error { - transportURL.Spec.RabbitmqClusterName = instance.Spec.RabbitMqClusterName + transportURL.Spec.RabbitmqClusterName = *instance.Spec.RabbitMqClusterName err := controllerutil.SetControllerReference(instance, transportURL, r.Scheme) return err diff --git a/tests/functional/watcher_controller_test.go b/tests/functional/watcher_controller_test.go index 4af5edae..fe5e0346 100644 --- a/tests/functional/watcher_controller_test.go +++ b/tests/functional/watcher_controller_test.go @@ -15,10 +15,12 @@ import ( mariadbv1 "github.com/openstack-k8s-operators/mariadb-operator/api/v1beta1" watcherv1beta1 "github.com/openstack-k8s-operators/watcher-operator/api/v1beta1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) var ( @@ -42,11 +44,11 @@ var _ = Describe("Watcher controller with minimal spec values", func() { It("should have the Spec fields defaulted", func() { Watcher := GetWatcher(watcherTest.Instance) - Expect(Watcher.Spec.DatabaseInstance).Should(Equal("openstack")) + Expect(*(Watcher.Spec.DatabaseInstance)).Should(Equal("openstack")) Expect(Watcher.Spec.DatabaseAccount).Should(Equal("watcher")) Expect(Watcher.Spec.Secret).Should(Equal("osp-secret")) Expect(Watcher.Spec.PasswordSelectors).Should(Equal(watcherv1beta1.PasswordSelector{Service: "WatcherPassword"})) - Expect(Watcher.Spec.RabbitMqClusterName).Should(Equal("rabbitmq")) + Expect(*(Watcher.Spec.RabbitMqClusterName)).Should(Equal("rabbitmq")) Expect(Watcher.Spec.ServiceUser).Should(Equal("watcher")) Expect(Watcher.Spec.PreserveJobs).Should(BeFalse()) }) @@ -84,11 +86,11 @@ var _ = Describe("Watcher controller", func() { It("should have the Spec fields defaulted", func() { Watcher := GetWatcher(watcherTest.Instance) - Expect(Watcher.Spec.DatabaseInstance).Should(Equal("openstack")) + Expect(*(Watcher.Spec.DatabaseInstance)).Should(Equal("openstack")) Expect(Watcher.Spec.DatabaseAccount).Should(Equal("watcher")) Expect(Watcher.Spec.ServiceUser).Should(Equal("watcher")) Expect(Watcher.Spec.Secret).Should(Equal("test-osp-secret")) - Expect(Watcher.Spec.RabbitMqClusterName).Should(Equal("rabbitmq")) + Expect(*(Watcher.Spec.RabbitMqClusterName)).Should(Equal("rabbitmq")) Expect(Watcher.Spec.PreserveJobs).Should(BeFalse()) }) @@ -197,7 +199,7 @@ var _ = Describe("Watcher controller", func() { mariadb.DeleteDBService, mariadb.CreateDBService( watcherTest.Instance.Namespace, - GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, + *GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, corev1.ServiceSpec{ Ports: []corev1.ServicePort{{Port: 3306}}, }, @@ -459,7 +461,7 @@ var _ = Describe("Watcher controller", func() { mariadb.DeleteDBService, mariadb.CreateDBService( watcherTest.Instance.Namespace, - GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, + *GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, corev1.ServiceSpec{ Ports: []corev1.ServicePort{{Port: 3306}}, }, @@ -531,6 +533,61 @@ var _ = Describe("Watcher controller", func() { Expect(Watcher.Spec.ApplierContainerImageURL).To(Equal("watcher-applier-custom-image-env")) }) }) + + When("Watcher rejects when empty databaseinstance is used", func() { + It("should raise an error for empty databaseInstance", func() { + spec := GetDefaultWatcherAPISpec() + spec["databaseInstance"] = "" + + raw := map[string]interface{}{ + "apiVersion": "watcher.openstack.org/v1beta1", + "kind": "watcher", + "metadata": map[string]interface{}{ + "name": watcherName.Name, + "namespace": watcherName.Namespace, + }, + "spec": spec, + } + + unstructuredObj := &unstructured.Unstructured{Object: raw} + _, err := controllerutil.CreateOrPatch( + th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To( + ContainSubstring( + "admission webhook \"vwatcher.kb.io\" denied the request: " + + "databaseInstance field should not be empty"), + ) + + }) + }) + + When("Watcher is created with empty RabbitMqClusterName", func() { + It("should raise an error for empty RabbitMqClusterName", func() { + spec := GetDefaultWatcherAPISpec() + spec["rabbitMqClusterName"] = "" + + raw := map[string]interface{}{ + "apiVersion": "watcher.openstack.org/v1beta1", + "kind": "watcher", + "metadata": map[string]interface{}{ + "name": watcherName.Name, + "namespace": watcherName.Namespace, + }, + "spec": spec, + } + + unstructuredObj := &unstructured.Unstructured{Object: raw} + _, err := controllerutil.CreateOrPatch( + th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To( + ContainSubstring( + "admission webhook \"vwatcher.kb.io\" denied the request: " + + "rabbitMqClusterName field should not be empty"), + ) + }) + }) When("Watcher with non-default values are created", func() { BeforeEach(func() { DeferCleanup(th.DeleteInstance, CreateWatcher(watcherTest.Instance, GetNonDefaultWatcherSpec())) @@ -546,7 +603,7 @@ var _ = Describe("Watcher controller", func() { mariadb.DeleteDBService, mariadb.CreateDBService( watcherTest.Instance.Namespace, - GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, + *GetWatcher(watcherTest.Instance).Spec.DatabaseInstance, corev1.ServiceSpec{ Ports: []corev1.ServicePort{{Port: 3306}}, }, @@ -557,12 +614,12 @@ var _ = Describe("Watcher controller", func() { It("should have the Spec fields with the expected values", func() { Watcher := GetWatcher(watcherTest.Instance) - Expect(Watcher.Spec.DatabaseInstance).Should(Equal("fakeopenstack")) + Expect(*(Watcher.Spec.DatabaseInstance)).Should(Equal("fakeopenstack")) Expect(Watcher.Spec.DatabaseAccount).Should(Equal("watcher")) Expect(Watcher.Spec.ServiceUser).Should(Equal("fakeuser")) Expect(Watcher.Spec.Secret).Should(Equal("test-osp-secret")) Expect(Watcher.Spec.PreserveJobs).Should(BeTrue()) - Expect(Watcher.Spec.RabbitMqClusterName).Should(Equal("rabbitmq")) + Expect(*(Watcher.Spec.RabbitMqClusterName)).Should(Equal("rabbitmq")) }) It("Should create watcher service with custom values", func() {