Merge pull request #650 from fmount/webh-msgs

Better Error messages

api/v1beta1/conditions.go

@@ -19,22 +19,8 @@ import (
 	condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 )
 
-// Glance Condition Types used by API objects.
-const (
-	// GlanceAPIReadyCondition Status=True condition which indicates if the GlanceAPI is configured and operational
-	GlanceAPIReadyCondition condition.Type = "GlanceAPIReady"
-	// CinderCondition
-	CinderCondition= "CinderReady"
-)
-
-// Glance Reasons used by API objects.
-const ()
-
 // Common Messages used by API objects.
 const (
-	//
-	// GlanceAPIReady condition messages
-	//
 	// GlanceAPIReadyInitMessage
 	GlanceAPIReadyInitMessage = "GlanceAPI not started"
 	// GlanceAPIReadyErrorMessage
@@ -43,4 +29,18 @@ const (
 	CinderInitMessage = "Waiting for Cinder resources"
 	// CinderReadyMessage
 	CinderReadyMessage = "Cinder resources exist"
+	// GlanceAPIReadyCondition Status=True condition which indicates if the GlanceAPI is configured and operational
+	GlanceAPIReadyCondition condition.Type = "GlanceAPIReady"
+	// CinderCondition
+	CinderCondition= "CinderReady"
+	// GlanceLayoutUpdateErrorMessage
+	GlanceLayoutUpdateErrorMessage = "The GlanceAPI layout (type) cannot be modified. To proceed, please add a new API with the desired layout and then decommission the previous API"
+	// KeystoneEndpointErrorMessage
+	KeystoneEndpointErrorMessage = "KeystoneEndpoint is assigned to an invalid GlanceAPI instance"
+	// InvalidBackendErrorMessageGeneric
+	InvalidBackendErrorMessageGeneric = "Invalid backend configuration detected"
+	// InvalidBackendErrorMessageSplit
+	InvalidBackendErrorMessageSplit = "The GlanceAPI layout type: split cannot be used in combination with File and NFS backend"
+	// InvalidBackendErrorMessageSingle
+	InvalidBackendErrorMessageSingle = "The GlanceAPI layout type: single can only be used in combination with File and NFS backend"
 )

api/v1beta1/glance_webhook.go

@@ -187,27 +187,27 @@ func isFileBackend(customServiceConfig string, topLevel bool) bool {
 }
 
 // Check if the File is used in combination with a wrong layout
-func (r *GlanceSpecCore) isInvalidBackend(glanceAPI GlanceAPITemplate, topLevel bool) bool {
+func (r *GlanceSpecCore) isInvalidBackend(glanceAPI GlanceAPITemplate, topLevel bool) (bool, string) {
 	var rep int32 = 0
 
 	// Do not take assumptions about the backend when replicas: 0, because it
 	// means the human operator has not made any choice or has no backend
 	// available yet.
 	if *glanceAPI.Replicas == rep {
-		return false
+		return false, ""
 	}
 	// For the current glanceAPI instance, detect an invalid configuration
 	// made by "type: split && backend: file": raise an issue if this config
 	// is found.
 	if glanceAPI.Type == "split" && isFileBackend(glanceAPI.CustomServiceConfig, topLevel) {
-		return true
+		return true, InvalidBackendErrorMessageSplit
 	}
 	// Do not allow to deploy a glanceAPI with "type: single" and a backend
 	// different than File (Cinder, Swift, Ceph): we must split in that case
 	if glanceAPI.Type == APISingle && !isFileBackend(glanceAPI.CustomServiceConfig, topLevel) {
-		return true
+		return true, InvalidBackendErrorMessageSingle
 	}
-	return false
+	return false, ""
 }
 
 var _ webhook.Validator = &Glance{}
@@ -275,9 +275,8 @@ func (r *GlanceSpecCore) ValidateCreate(basePath *field.Path) field.ErrorList {
 		path := basePath.Child("glanceAPIs").Key(key)
 
 		// fail if an invalid configuration/layout is detected
-		if r.isInvalidBackend(glanceAPI, topLevelFileBackend) {
-			allErrs = append(allErrs, field.Invalid(
-				path, key, "Invalid backend configuration detected"))
+		if ok, err := r.isInvalidBackend(glanceAPI, topLevelFileBackend); ok {
+			allErrs = append(allErrs, field.Invalid(path, key, err))
 		}
 
 		// validate the service override key is valid
@@ -291,7 +290,7 @@ func (r *GlanceSpecCore) ValidateCreate(basePath *field.Path) field.ErrorList {
 	if !r.isValidKeystoneEP() {
 		path := basePath.Child("keystoneEndpoint")
 		allErrs = append(allErrs, field.Invalid(
-			path, r.KeystoneEndpoint, "KeystoneEndpoint is assigned to an invalid glanceAPI instance"))
+			path, r.KeystoneEndpoint, KeystoneEndpointErrorMessage))
 	}
 
 	return allErrs
@@ -364,19 +363,19 @@ func (r *GlanceSpecCore) ValidateUpdate(old GlanceSpecCore, basePath *field.Path
 		if _, found := old.GlanceAPIs[key]; !found {
 			// Fail if an invalid configuration/layout is detected for the current
 			// // glanceAPI instance
-			if r.isInvalidBackend(glanceAPI, topLevelFileBackend) {
-				allErrs = append(allErrs, field.Invalid(path, key, "New API - Invalid backend configuration detected"))
+			if ok, err := r.isInvalidBackend(glanceAPI, topLevelFileBackend); ok {
+				allErrs = append(allErrs, field.Invalid(path, key, err))
 			}
 			continue
 		}
 		// The current glanceAPI exists and the layout is different
 		if glanceAPI.Type != old.GlanceAPIs[key].Type {
-			allErrs = append(allErrs, field.Invalid(path, key, "GlanceAPI deployment layout can't be updated"))
+			allErrs = append(allErrs, field.Invalid(path, key, GlanceLayoutUpdateErrorMessage))
 		}
 		// Fail if an invalid configuration/layout is detected for the current
 		// glanceAPI instance
-		if r.isInvalidBackend(glanceAPI, topLevelFileBackend) {
-			allErrs = append(allErrs, field.Invalid(path, key, "Invalid backend configuration detected"))
+		if ok, err := r.isInvalidBackend(glanceAPI, topLevelFileBackend); ok {
+			allErrs = append(allErrs, field.Invalid(path, key, err))
 		}
 		// validate the service override key is valid
 		allErrs = append(allErrs, service.ValidateRoutedOverrides(
@@ -390,7 +389,7 @@ func (r *GlanceSpecCore) ValidateUpdate(old GlanceSpecCore, basePath *field.Path
 	if !r.isValidKeystoneEP() {
 		path := basePath.Child("keystoneEndpoint")
 		allErrs = append(allErrs, field.Invalid(
-			path, r.KeystoneEndpoint, "KeystoneEndpoint is assigned to an invalid glanceAPI instance"))
+			path, r.KeystoneEndpoint, KeystoneEndpointErrorMessage))
 	}
 
 	return allErrs

test/functional/validation_webhook_test.go

@@ -20,10 +20,10 @@ import (
 
 	. "github.com/onsi/ginkgo/v2" //revive:disable:dot-imports
 	. "github.com/onsi/gomega"    //revive:disable:dot-imports
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-
+	glancev1 "github.com/openstack-k8s-operators/glance-operator/api/v1beta1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 var _ = Describe("Glance validation", func() {
@@ -49,8 +49,7 @@ var _ = Describe("Glance validation", func() {
 		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"),
+			ContainSubstring(glancev1.KeystoneEndpointErrorMessage),
 		)
 	})
 
@@ -99,8 +98,7 @@ var _ = Describe("Glance validation", func() {
 		// 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"),
+			ContainSubstring(glancev1.InvalidBackendErrorMessageSplit),
 		)
 	})
 
@@ -151,8 +149,7 @@ var _ = Describe("Glance validation", func() {
 		// 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"),
+			ContainSubstring(glancev1.KeystoneEndpointErrorMessage),
 		)
 	})