Merge pull request #494 from fmount/edge_sample_fix

Improve envTest for validation webhooks

Author: openshift-merge-bot[bot]

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
 }

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

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,11 +271,20 @@ 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) {
 		err := th.K8sClient.Get(th.Ctx, name, instance)
 		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)
+}

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() {

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