From e052e633a8c96d988dfd45923450f25bb281e60c Mon Sep 17 00:00:00 2001 From: Francesco Pantano Date: Fri, 29 Mar 2024 11:35:35 +0100 Subject: [PATCH] Improve envTest for validation webhooks This patch fixes the isValidEndpoint function provided by the validation webhook and introduces envTests for Validation webhooks, that are one of the most important part of the operator. Signed-off-by: Francesco Pantano --- api/v1beta1/glance_webhook.go | 31 ++-- .../layout/edge/glance_v1beta1_glance.yaml | 10 +- test/functional/base_test.go | 21 ++- test/functional/glance_controller_test.go | 6 +- test/functional/validation_webhook_test.go | 151 ++++++++++++++++++ 5 files changed, 191 insertions(+), 28 deletions(-) create mode 100644 test/functional/validation_webhook_test.go diff --git a/api/v1beta1/glance_webhook.go b/api/v1beta1/glance_webhook.go index 54b728c9..9ff92740 100644 --- a/api/v1beta1/glance_webhook.go +++ b/api/v1beta1/glance_webhook.go @@ -72,11 +72,9 @@ func (r *Glance) Default() { // Check if the KeystoneEndpoint matches with a deployed glanceAPI func (spec *GlanceSpec) isValidKeystoneEP() bool { for name, api := range spec.GlanceAPIs { - // If it's an Edge API is not a valid endpoint - if api.Type == APIEdge { - return false - } - if spec.KeystoneEndpoint == name { + // A valid keystoneEndpoint can either be applied to + // a single API or split type, but not to an EdgeAPI + if api.Type != APIEdge && spec.KeystoneEndpoint == name { return true } } @@ -184,11 +182,6 @@ var _ webhook.Validator = &Glance{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (r *Glance) ValidateCreate() (admission.Warnings, error) { glancelog.Info("validate create", "name", r.Name) - // At creation time, if the CR has an invalid keystoneEndpoint value (that - // doesn't match with any defined backend), return an error. - if !r.Spec.isValidKeystoneEP() { - return nil, errors.New("KeystoneEndpoint is assigned to an invalid glanceAPI instance") - } // Check if the top-level CR has a "customServiceConfig" with an explicit // "backend:file || empty string" and save the result into topLevel var. // If it's empty it should be ignored and having a file backend depends @@ -205,6 +198,11 @@ func (r *Glance) ValidateCreate() (admission.Warnings, error) { return nil, errors.New("Invalid backend configuration detected") } } + // At creation time, if the CR has an invalid keystoneEndpoint value (that + // doesn't match with any defined backend), return an error. + if !r.Spec.isValidKeystoneEP() { + return nil, errors.New("KeystoneEndpoint is assigned to an invalid glanceAPI instance") + } return nil, nil } @@ -234,12 +232,13 @@ func (r *Glance) ValidateUpdate(old runtime.Object) (admission.Warnings, error) if r.isInvalidBackend(glanceAPI, topLevelFileBackend) { return nil, errors.New("Invalid backend configuration detected") } - // At update time, if the CR has an invalid keystoneEndpoint set - // (e.g. an Edge GlanceAPI instance that can't be registered in keystone) - // return an error message - if !r.Spec.isValidKeystoneEP() { - return nil, errors.New("KeystoneEndpoint is assigned to an invalid glanceAPI instance") - } + } + // At update time, if the CR has an invalid keystoneEndpoint set + // (e.g. an Edge GlanceAPI instance that can't be registered in keystone) + // return an error message + if !r.Spec.isValidKeystoneEP() { + return nil, errors.New( + "KeystoneEndpoint is assigned to an invalid glanceAPI instance") } return nil, nil } diff --git a/config/samples/layout/edge/glance_v1beta1_glance.yaml b/config/samples/layout/edge/glance_v1beta1_glance.yaml index d260acf7..b3ae2c0c 100644 --- a/config/samples/layout/edge/glance_v1beta1_glance.yaml +++ b/config/samples/layout/edge/glance_v1beta1_glance.yaml @@ -11,15 +11,15 @@ spec: databaseAccount: glance keystoneEndpoint: central glanceAPIs: - central: - preserveJobs: false - replicas: 1 - type: single edge1: preserveJobs: false replicas: 1 - type: single + type: edge edge2: + preserveJobs: false + replicas: 1 + type: edge + central: preserveJobs: false replicas: 1 type: single diff --git a/test/functional/base_test.go b/test/functional/base_test.go index 54442dda..f89cb6a1 100644 --- a/test/functional/base_test.go +++ b/test/functional/base_test.go @@ -17,6 +17,7 @@ limitations under the License. package functional import ( + "fmt" "golang.org/x/exp/maps" k8s_errors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -169,6 +170,7 @@ func CreateGlanceSecret(namespace string, name string) *corev1.Secret { ) } +// GetDefaultGlanceSpec - It returns a default API built for testing purposes func GetDefaultGlanceSpec() map[string]interface{} { return map[string]interface{}{ "databaseInstance": "openstack", @@ -178,6 +180,7 @@ func GetDefaultGlanceSpec() map[string]interface{} { } } +// CreateGlanceAPISpec - func CreateGlanceAPISpec(apiType APIType) map[string]interface{} { spec := map[string]interface{}{ "replicas": 1, @@ -193,6 +196,7 @@ func CreateGlanceAPISpec(apiType APIType) map[string]interface{} { return spec } +// GetDefaultGlanceAPISpec - func GetDefaultGlanceAPISpec(apiType APIType) map[string]interface{} { spec := map[string]interface{}{ "replicas": 1, @@ -208,6 +212,7 @@ func GetDefaultGlanceAPISpec(apiType APIType) map[string]interface{} { return spec } +// GetTLSGlanceAPISpec - func GetTLSGlanceAPISpec(apiType APIType) map[string]interface{} { spec := CreateGlanceAPISpec(apiType) maps.Copy(spec, map[string]interface{}{ @@ -229,6 +234,7 @@ func GetTLSGlanceAPISpec(apiType APIType) map[string]interface{} { return spec } +// GlanceAPINotExists - Asserts the GlanceAPI does not exist func GlanceAPINotExists(name types.NamespacedName) { Consistently(func(g Gomega) { instance := &glancev1.GlanceAPI{} @@ -237,6 +243,7 @@ func GlanceAPINotExists(name types.NamespacedName) { }, timeout, interval).Should(Succeed()) } +// GlanceAPIExists - Asserts the GlanceAPI exist func GlanceAPIExists(name types.NamespacedName) { Consistently(func(g Gomega) { instance := &glancev1.GlanceAPI{} @@ -245,7 +252,8 @@ func GlanceAPIExists(name types.NamespacedName) { }, timeout, interval).Should(Succeed()) } -// AssertPVCDoesNotExist ensures the local PVC resource does not exist in a k8s cluster. +// AssertPVCDoesNotExist ensures the local PVC resource does not exist in a k8s +// cluster. func AssertPVCDoesNotExist(name types.NamespacedName) { instance := &corev1.PersistentVolumeClaim{} Eventually(func(g Gomega) { @@ -263,7 +271,8 @@ func AssertPVCExist(name types.NamespacedName) { }, th.Timeout, th.Interval).Should(Succeed()) } -// AssertCronJobDoesNotExist ensures the CronJob resource does not exist in a k8s cluster. +// AssertCronJobDoesNotExist ensures the CronJob resource does not exist in a +// k8s cluster. func AssertCronJobDoesNotExist(name types.NamespacedName) { instance := &batchv1.CronJob{} Eventually(func(g Gomega) { @@ -271,3 +280,11 @@ func AssertCronJobDoesNotExist(name types.NamespacedName) { g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue()) }, th.Timeout, th.Interval).Should(Succeed()) } + +// GetDummyBackend - Utility function that simulates a customServiceConfig +// where a Ceph backend has been set +func GetDummyBackend() string { + section := "[DEFAULT]" + dummyBackend := "enabled_backends=backend1:rbd" + return fmt.Sprintf("%s\n%s", section, dummyBackend) +} diff --git a/test/functional/glance_controller_test.go b/test/functional/glance_controller_test.go index eb849176..f994cc59 100644 --- a/test/functional/glance_controller_test.go +++ b/test/functional/glance_controller_test.go @@ -234,16 +234,12 @@ var _ = Describe("Glance controller", func() { condition.DBReadyCondition, corev1.ConditionTrue, ) - th.ExpectCondition( - glanceTest.Instance, + th.ExpectCondition(glanceTest.Instance, ConditionGetterFunc(GlanceConditionGetter), condition.DBSyncReadyCondition, corev1.ConditionTrue, ) }) - It("GlanceAPI CR is created", func() { - GlanceAPIExists(glanceTest.GlanceSingle) - }) }) When("Glance CR is created without container images defined", func() { BeforeEach(func() { diff --git a/test/functional/validation_webhook_test.go b/test/functional/validation_webhook_test.go new file mode 100644 index 00000000..6a15882e --- /dev/null +++ b/test/functional/validation_webhook_test.go @@ -0,0 +1,151 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package functional + +import ( + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + k8s_errors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +var _ = Describe("Glance validation", func() { + It("webhooks reject the request - invalid keystoneEndpoint", func() { + // GlanceEmptySpec is used to provide a standard Glance CR where no + // field is customized: we can inject our parameters to test webhooks + spec := GetGlanceDefaultSpec() + spec["keystoneEndpoint"] = "foo" + raw := map[string]interface{}{ + "apiVersion": "glance.openstack.org/v1beta1", + "kind": "Glance", + "metadata": map[string]interface{}{ + "name": glanceTest.Instance.Name, + "namespace": glanceTest.Instance.Namespace, + }, + "spec": spec, + } + unstructuredObj := &unstructured.Unstructured{Object: raw} + _, err := controllerutil.CreateOrPatch( + ctx, k8sClient, unstructuredObj, func() error { return nil }) + + Expect(err).Should(HaveOccurred()) + var statusError *k8s_errors.StatusError + Expect(errors.As(err, &statusError)).To(BeTrue()) + Expect(statusError.ErrStatus.Message).To( + ContainSubstring( + "KeystoneEndpoint is assigned to an invalid glanceAPI instance"), + ) + }) + + It("webhooks reject the request - invalid backend", func() { + spec := GetGlanceDefaultSpec() + + gapis := map[string]interface{}{ + "glanceAPIs": map[string]interface{}{ + "default": map[string]interface{}{ + "replicas": 1, + "type": "split", + }, + "edge1": map[string]interface{}{ + "replicas": 1, + "type": "edge", + }, + }, + } + + spec["keystoneEndpoint"] = "edge1" + spec["glanceAPIs"] = gapis + + raw := map[string]interface{}{ + "apiVersion": "glance.openstack.org/v1beta1", + "kind": "Glance", + "metadata": map[string]interface{}{ + "name": glanceTest.Instance.Name, + "namespace": glanceTest.Instance.Namespace, + }, + "spec": spec, + } + unstructuredObj := &unstructured.Unstructured{Object: raw} + _, err := controllerutil.CreateOrPatch( + ctx, k8sClient, unstructuredObj, func() error { return nil }) + + Expect(err).Should(HaveOccurred()) + var statusError *k8s_errors.StatusError + Expect(errors.As(err, &statusError)).To(BeTrue()) + // Webhooks catch that no backend is set before even realize that an + // invalid endpoint has been set + Expect(statusError.ErrStatus.Message).To( + ContainSubstring( + "Invalid backend configuration detected"), + ) + }) + + It("webhooks reject the request - invalid instance", func() { + spec := GetGlanceDefaultSpec() + + gapis := map[string]interface{}{ + "edge2": map[string]interface{}{ + "replicas": 1, + "type": "edge", + // inject a valid Ceph backend + "customServiceConfig": GetDummyBackend(), + }, + "default": map[string]interface{}{ + "replicas": 1, + "type": "split", + // inject a valid Ceph backend + "customServiceConfig": GetDummyBackend(), + }, + "edge1": map[string]interface{}{ + "replicas": 1, + "type": "edge", + // inject a valid Ceph backend + "customServiceConfig": GetDummyBackend(), + }, + } + // Set the KeystoneEndpoint to the wrong instance + spec["keystoneEndpoint"] = "edge1" + // Deploy multiple GlanceAPIs + spec["glanceAPIs"] = gapis + + raw := map[string]interface{}{ + "apiVersion": "glance.openstack.org/v1beta1", + "kind": "Glance", + "metadata": map[string]interface{}{ + "name": glanceTest.Instance.Name, + "namespace": glanceTest.Instance.Namespace, + }, + "spec": spec, + } + unstructuredObj := &unstructured.Unstructured{Object: raw} + _, err := controllerutil.CreateOrPatch( + ctx, k8sClient, unstructuredObj, func() error { return nil }) + + Expect(err).Should(HaveOccurred()) + var statusError *k8s_errors.StatusError + Expect(errors.As(err, &statusError)).To(BeTrue()) + // We shouldn't fail again for the backend, but because the endpoint is + // not valid + Expect(statusError.ErrStatus.Message).To( + ContainSubstring( + "KeystoneEndpoint is assigned to an invalid glanceAPI instance"), + ) + }) +})