Add webhooks to prevent using both CinderBackup APIs

This patch ensures both cbak interfaces are not used. It adds
deprecation warning at webhooks level as well as it forbids
users to deploy cbak using both the provided APIs.
EnvTest are added to verify that the validation is called and
returns the expected error.

Signed-off-by: Francesco Pantano <fpantano@redhat.com>

Author: fmount

api/bases/cinder.openstack.org_cinderapis.yaml

@@ -60,7 +60,7 @@ spec:
                 description: |-
                   CustomServiceConfig - customize the service config using this parameter to change service defaults,
                   or overwrite rendered information using raw OpenStack config format. The content gets added to
-                  to /etc/<service>/<service>.conf.d directory as a custom config file.
+                  the /etc/<service>/<service>.conf.d directory as a custom config file.
                 type: string
               customServiceConfigSecrets:
                 description: |-

api/bases/cinder.openstack.org_cinderbackups.yaml

@@ -60,7 +60,7 @@ spec:
                 description: |-
                   CustomServiceConfig - customize the service config using this parameter to change service defaults,
                   or overwrite rendered information using raw OpenStack config format. The content gets added to
-                  to /etc/<service>/<service>.conf.d directory as a custom config file.
+                  the /etc/<service>/<service>.conf.d directory as a custom config file.
                 type: string
               customServiceConfigSecrets:
                 description: |-

api/bases/cinder.openstack.org_cinders.yaml

@@ -65,7 +65,7 @@ spec:
                     description: |-
                       CustomServiceConfig - customize the service config using this parameter to change service defaults,
                       or overwrite rendered information using raw OpenStack config format. The content gets added to
-                      to /etc/<service>/<service>.conf.d directory as a custom config file.
+                      the /etc/<service>/<service>.conf.d directory as a custom config file.
                     type: string
                   customServiceConfigSecrets:
                     description: |-
@@ -377,7 +377,7 @@ spec:
                     description: |-
                       CustomServiceConfig - customize the service config using this parameter to change service defaults,
                       or overwrite rendered information using raw OpenStack config format. The content gets added to
-                      to /etc/<service>/<service>.conf.d directory as a custom config file.
+                      the /etc/<service>/<service>.conf.d directory as a custom config file.
                     type: string
                   customServiceConfigSecrets:
                     description: |-
@@ -501,7 +501,7 @@ spec:
                       description: |-
                         CustomServiceConfig - customize the service config using this parameter to change service defaults,
                         or overwrite rendered information using raw OpenStack config format. The content gets added to
-                        to /etc/<service>/<service>.conf.d directory as a custom config file.
+                        the /etc/<service>/<service>.conf.d directory as a custom config file.
                       type: string
                     customServiceConfigSecrets:
                       description: |-
@@ -612,7 +612,7 @@ spec:
                   required:
                   - containerImage
                   type: object
-                description: CinderBackup - Spec definition for the Backup service
+                description: CinderBackups - Spec definition for the Backup service
                   of this Cinder deployment
                 type: object
               cinderScheduler:
@@ -627,7 +627,7 @@ spec:
                     description: |-
                       CustomServiceConfig - customize the service config using this parameter to change service defaults,
                       or overwrite rendered information using raw OpenStack config format. The content gets added to
-                      to /etc/<service>/<service>.conf.d directory as a custom config file.
+                      the /etc/<service>/<service>.conf.d directory as a custom config file.
                     type: string
                   customServiceConfigSecrets:
                     description: |-
@@ -751,7 +751,7 @@ spec:
                       description: |-
                         CustomServiceConfig - customize the service config using this parameter to change service defaults,
                         or overwrite rendered information using raw OpenStack config format. The content gets added to
-                        to /etc/<service>/<service>.conf.d directory as a custom config file.
+                        the /etc/<service>/<service>.conf.d directory as a custom config file.
                       type: string
                     customServiceConfigSecrets:
                       description: |-

api/bases/cinder.openstack.org_cinderschedulers.yaml

@@ -60,7 +60,7 @@ spec:
                 description: |-
                   CustomServiceConfig - customize the service config using this parameter to change service defaults,
                   or overwrite rendered information using raw OpenStack config format. The content gets added to
-                  to /etc/<service>/<service>.conf.d directory as a custom config file.
+                  the /etc/<service>/<service>.conf.d directory as a custom config file.
                 type: string
               customServiceConfigSecrets:
                 description: |-

api/bases/cinder.openstack.org_cindervolumes.yaml

@@ -60,7 +60,7 @@ spec:
                 description: |-
                   CustomServiceConfig - customize the service config using this parameter to change service defaults,
                   or overwrite rendered information using raw OpenStack config format. The content gets added to
-                  to /etc/<service>/<service>.conf.d directory as a custom config file.
+                  the /etc/<service>/<service>.conf.d directory as a custom config file.
                 type: string
               customServiceConfigSecrets:
                 description: |-

api/v1beta1/cinder_types.go

@@ -128,13 +128,16 @@ type CinderSpecCore struct {
 	// CinderScheduler - Spec definition for the Scheduler service of this Cinder deployment
 	CinderScheduler CinderSchedulerTemplateCore `json:"cinderScheduler"`
 
+	// CinderBackup is DEPRECATED and will be removed in a future release.
+	// Use the separate CinderBackups resource instead.
 	// +kubebuilder:validation:Optional
-	// CinderBackup - Spec definition for the Backup service of this Cinder deployment
-	CinderBackups *map[string]CinderBackupTemplateCore `json:"cinderBackups"`
+	// +kubebuilder:deprecated:true
+	// +kubebuilder:deprecatedversion:warning="Cinder.Spec.CinderBackup is deprecated, use CinderBackups instead"
+	CinderBackup CinderBackupTemplateCore `json:"cinderBackup"`
 
 	// +kubebuilder:validation:Optional
-	// CinderBackup - Spec definition for the Backup service of this Cinder deployment
-	CinderBackup CinderBackupTemplateCore `json:"cinderBackup"`
+	// CinderBackups - Spec definition for the Backup service of this Cinder deployment
+	CinderBackups *map[string]CinderBackupTemplateCore `json:"cinderBackups,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// CinderVolumes - Map of chosen names to spec definitions for the Volume(s) service(s) of this Cinder deployment
@@ -158,7 +161,7 @@ type CinderSpec struct {
 	CinderBackup CinderBackupTemplate `json:"cinderBackup"`
 
 	// +kubebuilder:validation:Optional
-	// CinderBackup - Spec definition for the Backup service of this Cinder deployment
+	// CinderBackups - Spec definition for the Backup service of this Cinder deployment
 	CinderBackups *map[string]CinderBackupTemplate `json:"cinderBackups,omitempty"`
 
 	// +kubebuilder:validation:Optional

api/v1beta1/cinder_webhook.go

@@ -36,7 +36,6 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-
 	common_webhook "github.com/openstack-k8s-operators/lib-common/modules/common/webhook"
 )
 
@@ -136,6 +135,8 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
 	cinderlog.Info("validate create", "name", r.Name)
 
 	var allErrs field.ErrorList
+	var allWarns admission.Warnings
+
 	basePath := field.NewPath("spec")
 
 	// Validate cinderVolume name is valid
@@ -155,8 +156,9 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
 		GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
 	allErrs = append(allErrs, err...)
 
-	if err := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
-		allErrs = append(allErrs, err...)
+	if warn, errs := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
+		allErrs = append(allErrs, errs...)
+		allWarns = append(allWarns, warn...)
 	}
 
 	if len(allErrs) != 0 {
@@ -170,28 +172,51 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
 
 // ValidateCreate - Exported function wrapping non-exported validate functions,
 // this function can be called externally to validate an cinder spec.
-func (spec *CinderSpec) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
+func (spec *CinderSpec) ValidateCreate(
+	basePath *field.Path,
+	namespace string,
+) ([]string, field.ErrorList) {
 	var allErrs field.ErrorList
+	var allWarns admission.Warnings
 
 	// validate the service override key is valid
 	allErrs = append(allErrs, service.ValidateRoutedOverrides(
 		basePath.Child("cinderAPI").Child("override").Child("service"),
 		spec.CinderAPI.Override.Service)...)
 
+	// Cinder Backup validation: it returns errors in case both CinderBackup
+	// and CinderBackups are used, and it throws a warning by default when the
+	// deprecated parameter is used (it is a non blocking issue though)
 	allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
-	return allErrs
+	bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
+	allErrs = append(allErrs, bkpErrs...)
+	allWarns = append(allWarns, bkpWarns...)
+
+	return allWarns, allErrs
 }
 
-func (spec *CinderSpecCore) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
+func (spec *CinderSpecCore) ValidateCreate(
+	basePath *field.Path,
+	namespace string,
+) ([]string, field.ErrorList){
 	var allErrs field.ErrorList
+	var allWarns admission.Warnings
 
 	// validate the service override key is valid
 	allErrs = append(allErrs, service.ValidateRoutedOverrides(
 		basePath.Child("cinderAPI").Child("override").Child("service"),
 		spec.CinderAPI.Override.Service)...)
 
 	allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
-	return allErrs
+
+	// Cinder Backup validation: it returns errors in case both CinderBackup
+	// and CinderBackups are used, and it throws a warning by default when the
+	// deprecated parameter is used (it is a non blocking issue though)
+	bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
+	allErrs = append(allErrs, bkpErrs...)
+	allWarns = append(allWarns, bkpWarns...)
+
+	return allWarns, allErrs
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
@@ -204,6 +229,8 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
 	}
 
 	var allErrs field.ErrorList
+	var allWarns admission.Warnings
+
 	basePath := field.NewPath("spec")
 
 	// Validate cinderVolume name is valid
@@ -223,8 +250,9 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
 		GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
 	allErrs = append(allErrs, err...)
 
-	if err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
+	if warns, err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
 		allErrs = append(allErrs, err...)
+		allWarns = append(allWarns, warns...)
 	}
 
 	if len(allErrs) != 0 {
@@ -233,33 +261,61 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
 			r.Name, allErrs)
 	}
 
-	return nil, nil
+	return allWarns, nil
 }
 
 // ValidateUpdate - Exported function wrapping non-exported validate functions,
 // this function can be called externally to validate an cinder spec.
-func (spec *CinderSpec) ValidateUpdate(old CinderSpec, basePath *field.Path, namespace string) field.ErrorList {
+func (spec *CinderSpec) ValidateUpdate(
+	old CinderSpec,
+	basePath *field.Path,
+	namespace string,
+) ([]string, field.ErrorList) {
+
 	var allErrs field.ErrorList
+	var allWarns []string
 
 	// validate the service override key is valid
 	allErrs = append(allErrs, service.ValidateRoutedOverrides(
 		basePath.Child("cinderAPI").Child("override").Child("service"),
 		spec.CinderAPI.Override.Service)...)
 
 	allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
-	return allErrs
+
+	// Cinder Backup validation: it returns errors in case both CinderBackup
+	// and CinderBackups are used, and it throws a warning by default when the
+	// deprecated parameter is used (it is a non blocking issue though)
+	bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
+	allErrs = append(allErrs, bkpErrs...)
+	allWarns = append(allWarns, bkpWarns...)
+
+	return allWarns, allErrs
 }
 
-func (spec *CinderSpecCore) ValidateUpdate(old CinderSpecCore, basePath *field.Path, namespace string) field.ErrorList {
+func (spec *CinderSpecCore) ValidateUpdate(
+	old CinderSpecCore,
+	basePath *field.Path,
+	namespace string,
+) ([]string, field.ErrorList) {
+
 	var allErrs field.ErrorList
+	var allWarns []string
 
 	// validate the service override key is valid
 	allErrs = append(allErrs, service.ValidateRoutedOverrides(
 		basePath.Child("cinderAPI").Child("override").Child("service"),
 		spec.CinderAPI.Override.Service)...)
 
 	allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
-	return allErrs
+
+	// Cinder Backup validation: it returns errors in case both CinderBackup
+	// and CinderBackups are used, and it throws a warning by default when the
+	// deprecated parameter is used (it is a non blocking issue though)
+	bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
+	allErrs = append(allErrs, bkpErrs...)
+	allWarns = append(allWarns, bkpWarns...)
+
+	return allWarns, allErrs
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
@@ -336,7 +392,7 @@ func (spec *CinderSpecCore) ValidateCinderTopology(basePath *field.Path, namespa
 	bkPath := basePath.Child("cinderBackup")
 	allErrs = append(allErrs,
 		spec.CinderBackup.ValidateTopology(bkPath, namespace)...)
-	
+
 	// When a TopologyRef CR is referenced with an override to an instance of
 	// CinderBackups, fail if a different Namespace is referenced because not
 	// supported
@@ -405,3 +461,50 @@ func (spec *CinderSpec) ValidateCinderTopology(basePath *field.Path, namespace s
 	}
 	return allErrs
 }
+
+// TODO: Remove this function when refactoring CinderSpec to include CinderSpecCore
+func (spec *CinderSpec) ValidateCinderBackup(basePath *field.Path) ([]string, field.ErrorList) {
+	var allErrs field.ErrorList
+	var allWarns []string
+
+	// Check if the deprecated field is configured
+	isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
+	// Check if CinderBackup list is configured
+	isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
+
+	// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
+	// are used together
+	if isDeprecatedUsed && isCinderBackupListUsed {
+		errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when 'cinderBackups' is defined."
+		allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
+	}
+	// Append a warning depending on the cinderBackup field usage
+	if isDeprecatedUsed {
+		warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
+		allWarns = append(allWarns, warningMsg)
+	}
+	return allWarns, allErrs
+}
+
+func (spec *CinderSpecCore) ValidateCinderBackup(basePath *field.Path)([]string, field.ErrorList) {
+	var allErrs field.ErrorList
+	var allWarns []string
+
+	// Check if the deprecated field is configured
+	isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
+	// Check if CinderBackup list is configured
+	isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
+
+	// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
+	// are used together
+	if isDeprecatedUsed && isCinderBackupListUsed {
+		errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when the new 'cinderBackups' API is used."
+		allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
+	}
+	// Append a warning depending on the cinderBackup field usage
+	if isDeprecatedUsed {
+		warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
+		allWarns = append(allWarns, warningMsg)
+	}
+	return allWarns, allErrs
+}

api/v1beta1/common_types.go

@@ -56,7 +56,7 @@ type CinderServiceTemplate struct {
 	// +kubebuilder:validation:Optional
 	// CustomServiceConfig - customize the service config using this parameter to change service defaults,
 	// or overwrite rendered information using raw OpenStack config format. The content gets added to
-	// to /etc/<service>/<service>.conf.d directory as a custom config file.
+	// the /etc/<service>/<service>.conf.d directory as a custom config file.
 	CustomServiceConfig string `json:"customServiceConfig,omitempty"`
 
 	// +kubebuilder:validation:Optional

api/v1beta1/zz_generated.deepcopy.go

@@ -60,7 +60,7 @@ spec:
                 description: |-
                   CustomServiceConfig - customize the service config using this parameter to change service defaults,
                   or overwrite rendered information using raw OpenStack config format. The content gets added to
-                  to /etc/<service>/<service>.conf.d directory as a custom config file.
+                  the /etc/<service>/<service>.conf.d directory as a custom config file.
                 type: string
               customServiceConfigSecrets:
                 description: |-