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"), + ) + }) +})