From 7654e4e1a41b0e58e3e67cbb9b8772d97a870de8 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Mon, 6 May 2024 17:15:36 +0200 Subject: [PATCH] Check for valid service override endpoint type on create and update Depends-On: https://github.com/openstack-k8s-operators/lib-common/pull/505 --- api/go.mod | 2 + api/go.sum | 4 +- api/v1beta1/keystoneapi_webhook.go | 64 ++++++++- go.mod | 2 + go.sum | 4 +- tests/functional/keystoneapi_webhook_test.go | 138 ++++++++++++++++++- 6 files changed, 205 insertions(+), 9 deletions(-) diff --git a/api/go.mod b/api/go.mod index 763b7e74..f4d2320d 100644 --- a/api/go.mod +++ b/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-20240508083919-01ef8c1d28c5 diff --git a/api/go.sum b/api/go.sum index 0fdfa918..81c445d3 100644 --- a/api/go.sum +++ b/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-20240508083919-01ef8c1d28c5 h1:RwBWawOf7fLsojvTdCeCZ+O5k3VqOqoX5f7PRQfa9lY= +github.com/stuggi/lib-common/modules/common v0.0.0-20240508083919-01ef8c1d28c5/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= diff --git a/api/v1beta1/keystoneapi_webhook.go b/api/v1beta1/keystoneapi_webhook.go index 3cb35e8c..21d8a5e7 100644 --- a/api/v1beta1/keystoneapi_webhook.go +++ b/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,73 @@ 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.Child("override").Child("service"), 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") + + 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.Child("override").Child("service"), 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) diff --git a/go.mod b/go.mod index f987e4eb..969d40e4 100644 --- a/go.mod +++ b/go.mod @@ -86,3 +86,5 @@ replace github.com/openstack-k8s-operators/keystone-operator/api => ./api // 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-20240508083919-01ef8c1d28c5 diff --git a/go.sum b/go.sum index 67cea5f2..6602a2e6 100644 --- a/go.sum +++ b/go.sum @@ -76,8 +76,6 @@ github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7 h1:rncLxJBpFGqBztyxC github.com/openshift/api v0.0.0-20230414143018-3367bc7e6ac7/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4= github.com/openstack-k8s-operators/infra-operator/apis v0.3.1-0.20240429104248-25176c735750 h1:buuvAo48wCKOrn1gT1Br3Z2EMh0726m0Flc+1VVhyLU= github.com/openstack-k8s-operators/infra-operator/apis v0.3.1-0.20240429104248-25176c735750/go.mod h1:QN2DJpfEc+mbvvfhoCuJ/UhQzvw12Mf+8nS0QX1HGIg= -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= @@ -102,6 +100,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-20240508083919-01ef8c1d28c5 h1:RwBWawOf7fLsojvTdCeCZ+O5k3VqOqoX5f7PRQfa9lY= +github.com/stuggi/lib-common/modules/common v0.0.0-20240508083919-01ef8c1d28c5/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= diff --git a/tests/functional/keystoneapi_webhook_test.go b/tests/functional/keystoneapi_webhook_test.go index d327c13e..d648592e 100644 --- a/tests/functional/keystoneapi_webhook_test.go +++ b/tests/functional/keystoneapi_webhook_test.go @@ -17,18 +17,36 @@ 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() { @@ -36,6 +54,31 @@ var _ = Describe("KeystoneAPI Webhook", func() { 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()) @@ -56,9 +99,9 @@ var _ = Describe("KeystoneAPI Webhook", func() { When("A KeystoneAPI instance is created with container images", func() { BeforeEach(func() { - keystoneAPISpec := GetDefaultKeystoneAPISpec() - keystoneAPISpec["containerImage"] = "api-container-image" - DeferCleanup(th.DeleteInstance, CreateKeystoneAPI(keystoneAPIName, keystoneAPISpec)) + spec := GetDefaultKeystoneAPISpec() + spec["containerImage"] = "api-container-image" + DeferCleanup(th.DeleteInstance, CreateKeystoneAPI(keystoneAPIName, spec)) }) It("should use the given values", func() { @@ -68,4 +111,93 @@ var _ = Describe("KeystoneAPI Webhook", func() { )) }) }) + + It("rejects with wrong service override endpoint type", func() { + spec := GetDefaultKeystoneAPISpec() + spec["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": 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( + "invalid: spec.override.service[wrooong]: " + + "Invalid value: \"wrooong\": invalid endpoint type: wrooong"), + ) + }) + + When("A KeystoneAPI instance is updated with wrong service override endpoint", func() { + BeforeEach(func() { + spec := GetDefaultKeystoneAPISpec() + 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("rejects update with wrong service override endpoint type", func() { + 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()).To( + ContainSubstring( + "invalid: spec.override.service[wrooong]: " + + "Invalid value: \"wrooong\": invalid endpoint type: wrooong"), + ) + }) + }) })