Check for valid service override endpoint type on create and update

Depends-On: openstack-k8s-operators/lib-common#505

api/go.mod

@@ -74,3 +74,5 @@ require (
 // mschuppert: map to latest commit from release-4.13 tag
 // must consistent within modules and service operators
 replace github.com/openshift/api => github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 //allow-merging
+
+replace github.com/openstack-k8s-operators/lib-common/modules/common => github.com/stuggi/lib-common/modules/common v0.0.0-20240507092543-31b40f7dda53

api/go.sum

@@ -73,8 +73,6 @@ github.com/onsi/gomega v1.33.0 h1:snPCflnZrpMsy94p4lXVEkHo12lmPnc3vY5XBbreexE=
 github.com/onsi/gomega v1.33.0/go.mod h1:+925n5YtiFsLzzafLUHzVMBpvvRAzrydIBiSIxjX3wY=
 github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 h1:rncLxJBpFGqBztyxCMwNRnMjhhIDOWHJowi6q8G6koI=
 github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240429052447-09a614506ca6 h1:WLsG3Ko+phW5xZJjncypLWGASoLqKrt05qN9Zxsad6g=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240429052447-09a614506ca6/go.mod h1:lYhFzul37AR/6gAhTAA1KKWbOlzB3F/7014lejn883c=
 github.com/openstack-k8s-operators/lib-common/modules/openstack v0.3.1-0.20240429052447-09a614506ca6 h1:/mhzQQ9FF70z00zZD7dpgOoNXvEu9q68oob3oAiJW08=
 github.com/openstack-k8s-operators/lib-common/modules/openstack v0.3.1-0.20240429052447-09a614506ca6/go.mod h1:mrRNYeg8jb1zgGsufpN1/IB3sdbaST8btTBLwQ+taaA=
 github.com/openstack-k8s-operators/lib-common/modules/test v0.3.1-0.20240429052447-09a614506ca6 h1:Xx8uGcklRNHgBKTftNcK/Ob3qxiKwxUf5fVjtWCvOHI=
@@ -97,6 +95,8 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stuggi/lib-common/modules/common v0.0.0-20240507092543-31b40f7dda53 h1:ulUSOzY7MtQkM/+mCzft8TvsOLgJbaY4JFfkvVK2aKI=
+github.com/stuggi/lib-common/modules/common v0.0.0-20240507092543-31b40f7dda53/go.mod h1:lYhFzul37AR/6gAhTAA1KKWbOlzB3F/7014lejn883c=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=

api/v1beta1/keystoneapi_webhook.go

@@ -23,7 +23,12 @@ limitations under the License.
 package v1beta1
 
 import (
+	"fmt"
+
+	"github.com/openstack-k8s-operators/lib-common/modules/common/service"
+	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"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -88,18 +93,76 @@ var _ webhook.Validator = &KeystoneAPI{}
 func (r *KeystoneAPI) ValidateCreate() (admission.Warnings, error) {
 	keystoneapilog.Info("validate create", "name", r.Name)
 
-	// TODO(user): fill in your validation logic upon object creation.
+	allErrs := field.ErrorList{}
+	basePath := field.NewPath("spec")
+
+	if err := r.Spec.ValidateCreate(basePath); err != nil {
+		allErrs = append(allErrs, err...)
+	}
+
+	if len(allErrs) != 0 {
+		return nil, apierrors.NewInvalid(GroupVersion.WithKind("KeystoneAPI").GroupKind(), r.Name, allErrs)
+	}
+
 	return nil, nil
 }
 
+// ValidateCreate - Exported function wrapping non-exported validate functions,
+// this function can be called externally to validate an KeystoneAPI spec.
+func (spec *KeystoneAPISpec) ValidateCreate(basePath *field.Path) field.ErrorList {
+	return spec.KeystoneAPISpecCore.ValidateCreate(basePath)
+}
+
+func (spec *KeystoneAPISpecCore) ValidateCreate(basePath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+
+	// validate the service override key is valid
+	allErrs = append(allErrs, service.ValidateRoutedOverrides(basePath, spec.Override.Service)...)
+
+	return allErrs
+}
+
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *KeystoneAPI) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
 	keystoneapilog.Info("validate update", "name", r.Name)
 
-	// TODO(user): fill in your validation logic upon object update.
+	oldKeystoneAPI, ok := old.(*KeystoneAPI)
+	if !ok || oldKeystoneAPI == nil {
+		return nil, apierrors.NewInternalError(fmt.Errorf("unable to convert existing object"))
+	}
+
+	allErrs := field.ErrorList{}
+	basePath := field.NewPath("spec")
+
+	// validate the service override key is valid
+	allErrs = append(allErrs, service.ValidateRoutedOverrides(basePath, r.Spec.Override.Service)...)
+
+	if err := r.Spec.ValidateUpdate(oldKeystoneAPI.Spec, basePath); err != nil {
+		allErrs = append(allErrs, err...)
+	}
+
+	if len(allErrs) != 0 {
+		return nil, apierrors.NewInvalid(GroupVersion.WithKind("KeystoneAPI").GroupKind(), r.Name, allErrs)
+	}
+
 	return nil, nil
 }
 
+// ValidateUpdate - Exported function wrapping non-exported validate functions,
+// this function can be called externally to validate an ironic spec.
+func (spec *KeystoneAPISpec) ValidateUpdate(old KeystoneAPISpec, basePath *field.Path) field.ErrorList {
+	return spec.KeystoneAPISpecCore.ValidateUpdate(old.KeystoneAPISpecCore, basePath)
+}
+
+func (spec *KeystoneAPISpecCore) ValidateUpdate(old KeystoneAPISpecCore, basePath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+
+	// validate the service override key is valid
+	allErrs = append(allErrs, service.ValidateRoutedOverrides(basePath, spec.Override.Service)...)
+
+	return allErrs
+}
+
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *KeystoneAPI) ValidateDelete() (admission.Warnings, error) {
 	keystoneapilog.Info("validate delete", "name", r.Name)

tests/functional/keystoneapi_webhook_test.go

@@ -17,25 +17,68 @@ limitations under the License.
 package functional_test
 
 import (
+	"fmt"
 	"os"
 
 	. "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports
 	. "github.com/onsi/gomega"    //revive:disable:dot-imports
+
+	//revive:disable-next-line:dot-imports
+	. "github.com/openstack-k8s-operators/lib-common/modules/common/test/helpers"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	memcachedv1 "github.com/openstack-k8s-operators/infra-operator/apis/memcached/v1beta1"
 	keystonev1 "github.com/openstack-k8s-operators/keystone-operator/api/v1beta1"
+	condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition"
+	"github.com/openstack-k8s-operators/lib-common/modules/common/service"
 )
 
 var _ = Describe("KeystoneAPI Webhook", func() {
 
 	var keystoneAPIName types.NamespacedName
+	var keystoneAccountName types.NamespacedName
+	var keystoneDatabaseName types.NamespacedName
+	var dbSyncJobName types.NamespacedName
+	var bootstrapJobName types.NamespacedName
+	var deploymentName types.NamespacedName
+	var memcachedSpec memcachedv1.MemcachedSpec
 
 	BeforeEach(func() {
 
 		keystoneAPIName = types.NamespacedName{
 			Name:      "keystone",
 			Namespace: namespace,
 		}
+		keystoneAccountName = types.NamespacedName{
+			Name:      AccountName,
+			Namespace: namespace,
+		}
+		keystoneDatabaseName = types.NamespacedName{
+			Name:      DatabaseCRName,
+			Namespace: namespace,
+		}
+		dbSyncJobName = types.NamespacedName{
+			Name:      "keystone-db-sync",
+			Namespace: namespace,
+		}
+		bootstrapJobName = types.NamespacedName{
+			Name:      "keystone-bootstrap",
+			Namespace: namespace,
+		}
+		deploymentName = types.NamespacedName{
+			Name:      "keystone",
+			Namespace: namespace,
+		}
+		memcachedSpec = memcachedv1.MemcachedSpec{
+			MemcachedSpecCore: memcachedv1.MemcachedSpecCore{
+				Replicas: ptr.To(int32(3)),
+			},
+		}
 
 		err := os.Setenv("OPERATOR_TEMPLATES", "../../templates")
 		Expect(err).NotTo(HaveOccurred())
@@ -68,4 +111,103 @@ var _ = Describe("KeystoneAPI Webhook", func() {
 			))
 		})
 	})
+
+	When("A KeystoneAPI instance is created with wrong service override endpoint", func() {
+		It("gets blocked by the webhook and fail", func() {
+			keystoneAPISpec := GetDefaultKeystoneAPISpec()
+			keystoneAPISpec["override"] = map[string]interface{}{
+				"service": map[string]interface{}{
+					"internal": map[string]interface{}{},
+					"wrooong":  map[string]interface{}{},
+				},
+			}
+
+			raw := map[string]interface{}{
+				"apiVersion": "keystone.openstack.org/v1beta1",
+				"kind":       "KeystoneAPI",
+				"metadata": map[string]interface{}{
+					"name":      keystoneAPIName.Name,
+					"namespace": keystoneAPIName.Namespace,
+				},
+				"spec": keystoneAPISpec,
+			}
+
+			unstructuredObj := &unstructured.Unstructured{Object: raw}
+			_, err := controllerutil.CreateOrPatch(
+				th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).Should(ContainSubstring(
+				"invalid endpoint type: wrooong",
+			))
+		})
+	})
+
+	When("A KeystoneAPI instance is updated with wrong service override endpoint", func() {
+		BeforeEach(func() {
+			spec := GetDefaultKeystoneAPISpec()
+			serviceOverride := map[string]interface{}{}
+			serviceOverride["public"] = map[string]interface{}{
+				"endpointURL": "http://keystone-openstack.apps-crc.testing",
+			}
+
+			spec["override"] = map[string]interface{}{
+				"service": serviceOverride,
+			}
+
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateKeystoneMessageBusSecret(namespace, "rabbitmq-secret"))
+			keystone := CreateKeystoneAPI(keystoneAPIName, spec)
+			DeferCleanup(th.DeleteInstance, keystone)
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateKeystoneAPISecret(namespace, SecretName))
+			DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec))
+			DeferCleanup(
+				mariadb.DeleteDBService,
+				mariadb.CreateDBService(
+					namespace,
+					GetKeystoneAPI(keystoneAPIName).Spec.DatabaseInstance,
+					corev1.ServiceSpec{
+						Ports: []corev1.ServicePort{{Port: 3306}},
+					},
+				),
+			)
+			mariadb.SimulateMariaDBAccountCompleted(keystoneAccountName)
+			mariadb.SimulateMariaDBDatabaseCompleted(keystoneDatabaseName)
+			infra.SimulateTransportURLReady(types.NamespacedName{
+				Name:      fmt.Sprintf("%s-keystone-transport", keystoneAPIName.Name),
+				Namespace: namespace,
+			})
+			infra.SimulateMemcachedReady(types.NamespacedName{
+				Name:      "memcached",
+				Namespace: namespace,
+			})
+			th.SimulateJobSuccess(dbSyncJobName)
+			th.SimulateJobSuccess(bootstrapJobName)
+			th.SimulateDeploymentReplicaReady(deploymentName)
+
+			th.ExpectCondition(
+				keystoneAPIName,
+				ConditionGetterFunc(KeystoneConditionGetter),
+				condition.ReadyCondition,
+				corev1.ConditionTrue,
+			)
+		})
+
+		It("gets blocked by the webhook and fail", func() {
+			Eventually(func(g Gomega) {
+
+				KeystoneAPI := GetKeystoneAPI(keystoneAPIName)
+				Expect(KeystoneAPI).NotTo(BeNil())
+				if KeystoneAPI.Spec.Override.Service == nil {
+					KeystoneAPI.Spec.Override.Service = map[service.Endpoint]service.RoutedOverrideSpec{}
+				}
+				KeystoneAPI.Spec.Override.Service["wrooong"] = service.RoutedOverrideSpec{}
+				err := k8sClient.Update(ctx, KeystoneAPI)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).Should(ContainSubstring(
+					"invalid endpoint type: wrooong",
+				))
+			}, timeout, interval).Should(Succeed())
+		})
+	})
 })