Skip to content

Commit b32e0cf

Browse files
fmountfmount
committed
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>
1 parent 17fe543 commit b32e0cf

File tree

5 files changed

+241
-77
lines changed

5 files changed

+241
-77
lines changed

‎api/v1beta1/cinder_types.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,16 @@ type CinderSpecCore struct {
128128
// CinderScheduler - Spec definition for the Scheduler service of this Cinder deployment
129129
CinderScheduler CinderSchedulerTemplateCore `json:"cinderScheduler"`
130130

131+
// CinderBackup is DEPRECATED and will be removed in a future release.
132+
// Use the separate CinderBackups resource instead.
131133
// +kubebuilder:validation:Optional
132-
// CinderBackup - Spec definition for the Backup service of this Cinder deployment
133-
CinderBackups *map[string]CinderBackupTemplateCore `json:"cinderBackups"`
134+
// +kubebuilder:deprecated:true
135+
// +kubebuilder:deprecatedversion:warning="Cinder.Spec.CinderBackup is deprecated, use CinderBackups instead"
136+
CinderBackup CinderBackupTemplateCore `json:"cinderBackup"`
134137

135138
// +kubebuilder:validation:Optional
136139
// CinderBackup - Spec definition for the Backup service of this Cinder deployment
137-
CinderBackup CinderBackupTemplateCore `json:"cinderBackup"`
140+
CinderBackups *map[string]CinderBackupTemplateCore `json:"cinderBackups,omitempty"`
138141

139142
// +kubebuilder:validation:Optional
140143
// CinderVolumes - Map of chosen names to spec definitions for the Volume(s) service(s) of this Cinder deployment

‎api/v1beta1/cinder_webhook.go‎

Lines changed: 117 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import (
3737
logf "sigs.k8s.io/controller-runtime/pkg/log"
3838
"sigs.k8s.io/controller-runtime/pkg/webhook"
3939
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
40-
4140
common_webhook "github.com/openstack-k8s-operators/lib-common/modules/common/webhook"
4241
)
4342

@@ -149,6 +148,8 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
149148
cinderlog.Info("validate create", "name", r.Name)
150149

151150
var allErrs field.ErrorList
151+
var allWarns admission.Warnings
152+
152153
basePath := field.NewPath("spec")
153154

154155
// Validate cinderVolume name is valid
@@ -168,8 +169,9 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
168169
GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
169170
allErrs = append(allErrs, err...)
170171

171-
if err := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
172-
allErrs = append(allErrs, err...)
172+
if warn, errs := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
173+
allErrs = append(allErrs, errs...)
174+
allWarns = append(allWarns, warn...)
173175
}
174176

175177
if len(allErrs) != 0 {
@@ -183,28 +185,51 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
183185

184186
// ValidateCreate - Exported function wrapping non-exported validate functions,
185187
// this function can be called externally to validate an cinder spec.
186-
func (spec *CinderSpec) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
188+
func (spec *CinderSpec) ValidateCreate(
189+
basePath *field.Path,
190+
namespace string,
191+
) ([]string, field.ErrorList) {
187192
var allErrs field.ErrorList
193+
var allWarns admission.Warnings
188194

189195
// validate the service override key is valid
190196
allErrs = append(allErrs, service.ValidateRoutedOverrides(
191197
basePath.Child("cinderAPI").Child("override").Child("service"),
192198
spec.CinderAPI.Override.Service)...)
193199

200+
// Cinder Backup validation: it returns errors in case both CinderBackup
201+
// and CinderBackups are used, and it throws a warning by default when the
202+
// deprecated parameter is used (it is a non blocking issue though)
194203
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
195-
return allErrs
204+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
205+
allErrs = append(allErrs, bkpErrs...)
206+
allWarns = append(allWarns, bkpWarns...)
207+
208+
return allWarns, allErrs
196209
}
197210

198-
func (spec *CinderSpecCore) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
211+
func (spec *CinderSpecCore) ValidateCreate(
212+
basePath *field.Path,
213+
namespace string,
214+
) ([]string, field.ErrorList){
199215
var allErrs field.ErrorList
216+
var allWarns admission.Warnings
200217

201218
// validate the service override key is valid
202219
allErrs = append(allErrs, service.ValidateRoutedOverrides(
203220
basePath.Child("cinderAPI").Child("override").Child("service"),
204221
spec.CinderAPI.Override.Service)...)
205222

206223
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
207-
return allErrs
224+
225+
// Cinder Backup validation: it returns errors in case both CinderBackup
226+
// and CinderBackups are used, and it throws a warning by default when the
227+
// deprecated parameter is used (it is a non blocking issue though)
228+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
229+
allErrs = append(allErrs, bkpErrs...)
230+
allWarns = append(allWarns, bkpWarns...)
231+
232+
return allWarns, allErrs
208233
}
209234

210235
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
@@ -217,6 +242,8 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
217242
}
218243

219244
var allErrs field.ErrorList
245+
var allWarns admission.Warnings
246+
220247
basePath := field.NewPath("spec")
221248

222249
// Validate cinderVolume name is valid
@@ -236,8 +263,9 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
236263
GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
237264
allErrs = append(allErrs, err...)
238265

239-
if err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
266+
if warns, err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
240267
allErrs = append(allErrs, err...)
268+
allWarns = append(allWarns, warns...)
241269
}
242270

243271
if len(allErrs) != 0 {
@@ -246,33 +274,61 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
246274
r.Name, allErrs)
247275
}
248276

249-
return nil, nil
277+
return allWarns, nil
250278
}
251279

252280
// ValidateUpdate - Exported function wrapping non-exported validate functions,
253281
// this function can be called externally to validate an cinder spec.
254-
func (spec *CinderSpec) ValidateUpdate(old CinderSpec, basePath *field.Path, namespace string) field.ErrorList {
282+
func (spec *CinderSpec) ValidateUpdate(
283+
old CinderSpec,
284+
basePath *field.Path,
285+
namespace string,
286+
) ([]string, field.ErrorList) {
287+
255288
var allErrs field.ErrorList
289+
var allWarns []string
256290

257291
// validate the service override key is valid
258292
allErrs = append(allErrs, service.ValidateRoutedOverrides(
259293
basePath.Child("cinderAPI").Child("override").Child("service"),
260294
spec.CinderAPI.Override.Service)...)
261295

262296
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
263-
return allErrs
297+
298+
// Cinder Backup validation: it returns errors in case both CinderBackup
299+
// and CinderBackups are used, and it throws a warning by default when the
300+
// deprecated parameter is used (it is a non blocking issue though)
301+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
302+
allErrs = append(allErrs, bkpErrs...)
303+
allWarns = append(allWarns, bkpWarns...)
304+
305+
return allWarns, allErrs
264306
}
265307

266-
func (spec *CinderSpecCore) ValidateUpdate(old CinderSpecCore, basePath *field.Path, namespace string) field.ErrorList {
308+
func (spec *CinderSpecCore) ValidateUpdate(
309+
old CinderSpecCore,
310+
basePath *field.Path,
311+
namespace string,
312+
) ([]string, field.ErrorList) {
313+
267314
var allErrs field.ErrorList
315+
var allWarns []string
268316

269317
// validate the service override key is valid
270318
allErrs = append(allErrs, service.ValidateRoutedOverrides(
271319
basePath.Child("cinderAPI").Child("override").Child("service"),
272320
spec.CinderAPI.Override.Service)...)
273321

274322
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
275-
return allErrs
323+
324+
// Cinder Backup validation: it returns errors in case both CinderBackup
325+
// and CinderBackups are used, and it throws a warning by default when the
326+
// deprecated parameter is used (it is a non blocking issue though)
327+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
328+
allErrs = append(allErrs, bkpErrs...)
329+
allWarns = append(allWarns, bkpWarns...)
330+
331+
return allWarns, allErrs
276332
}
277333

278334
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
@@ -349,7 +405,7 @@ func (spec *CinderSpecCore) ValidateCinderTopology(basePath *field.Path, namespa
349405
bkPath := basePath.Child("cinderBackup")
350406
allErrs = append(allErrs,
351407
spec.CinderBackup.ValidateTopology(bkPath, namespace)...)
352-
408+
353409
// When a TopologyRef CR is referenced with an override to an instance of
354410
// CinderBackups, fail if a different Namespace is referenced because not
355411
// supported
@@ -418,3 +474,50 @@ func (spec *CinderSpec) ValidateCinderTopology(basePath *field.Path, namespace s
418474
}
419475
return allErrs
420476
}
477+
478+
// TODO: Remove this function when refactoring CinderSpec to include CinderSpecCore
479+
func (spec *CinderSpec) ValidateCinderBackup(basePath *field.Path) ([]string, field.ErrorList) {
480+
var allErrs field.ErrorList
481+
var allWarns []string
482+
483+
// Check if the deprecated field is configured
484+
isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
485+
// Check if CinderBackup list is configured
486+
isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
487+
488+
// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
489+
// are used together
490+
if isDeprecatedUsed && isCinderBackupListUsed {
491+
errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when 'cinderBackups' is defined."
492+
allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
493+
}
494+
// Append a warning depending on the cinderBackup field usage
495+
if isDeprecatedUsed {
496+
warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
497+
allWarns = append(allWarns, warningMsg)
498+
}
499+
return allWarns, allErrs
500+
}
501+
502+
func (spec *CinderSpecCore) ValidateCinderBackup(basePath *field.Path)([]string, field.ErrorList) {
503+
var allErrs field.ErrorList
504+
var allWarns []string
505+
506+
// Check if the deprecated field is configured
507+
isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
508+
// Check if CinderBackup list is configured
509+
isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
510+
511+
// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
512+
// are used together
513+
if isDeprecatedUsed && isCinderBackupListUsed {
514+
errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when the new 'cinderBackups' API is used."
515+
allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
516+
}
517+
// Append a warning depending on the cinderBackup field usage
518+
if isDeprecatedUsed {
519+
warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
520+
allWarns = append(allWarns, warningMsg)
521+
}
522+
return allWarns, allErrs
523+
}

‎api/v1beta1/zz_generated.deepcopy.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)