From 90dbe4ce1aa33b10d95245a2f35a66650e86f6c2 Mon Sep 17 00:00:00 2001 From: Francesco Pantano Date: Wed, 6 Nov 2024 18:28:21 +0100 Subject: [PATCH] Better Error messages Build better error messages to clarify the reason of an invalid GlanceAPI. Update functional tests and clean up some code. Jira: https://issues.redhat.com/browse/OSPRH-11209 Signed-off-by: Francesco Pantano --- api/v1beta1/conditions.go | 28 ++++++++++----------- api/v1beta1/glance_webhook.go | 29 +++++++++++----------- test/functional/validation_webhook_test.go | 13 ++++------ 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/api/v1beta1/conditions.go b/api/v1beta1/conditions.go index f5552c55..6977fcd6 100644 --- a/api/v1beta1/conditions.go +++ b/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" ) diff --git a/api/v1beta1/glance_webhook.go b/api/v1beta1/glance_webhook.go index 48bda50f..b3dca581 100644 --- a/api/v1beta1/glance_webhook.go +++ b/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 diff --git a/test/functional/validation_webhook_test.go b/test/functional/validation_webhook_test.go index f0373cb9..6122457a 100644 --- a/test/functional/validation_webhook_test.go +++ b/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), ) })