Skip to content

Commit e243cc8

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 c826e1f commit e243cc8

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
@@ -36,7 +36,6 @@ import (
3636
logf "sigs.k8s.io/controller-runtime/pkg/log"
3737
"sigs.k8s.io/controller-runtime/pkg/webhook"
3838
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
39-
4039
common_webhook "github.com/openstack-k8s-operators/lib-common/modules/common/webhook"
4140
)
4241

@@ -136,6 +135,8 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
136135
cinderlog.Info("validate create", "name", r.Name)
137136

138137
var allErrs field.ErrorList
138+
var allWarns admission.Warnings
139+
139140
basePath := field.NewPath("spec")
140141

141142
// Validate cinderVolume name is valid
@@ -155,8 +156,9 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
155156
GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
156157
allErrs = append(allErrs, err...)
157158

158-
if err := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
159-
allErrs = append(allErrs, err...)
159+
if warn, errs := r.Spec.ValidateCreate(basePath, r.Namespace); err != nil {
160+
allErrs = append(allErrs, errs...)
161+
allWarns = append(allWarns, warn...)
160162
}
161163

162164
if len(allErrs) != 0 {
@@ -170,28 +172,51 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
170172

171173
// ValidateCreate - Exported function wrapping non-exported validate functions,
172174
// this function can be called externally to validate an cinder spec.
173-
func (spec *CinderSpec) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
175+
func (spec *CinderSpec) ValidateCreate(
176+
basePath *field.Path,
177+
namespace string,
178+
) ([]string, field.ErrorList) {
174179
var allErrs field.ErrorList
180+
var allWarns admission.Warnings
175181

176182
// validate the service override key is valid
177183
allErrs = append(allErrs, service.ValidateRoutedOverrides(
178184
basePath.Child("cinderAPI").Child("override").Child("service"),
179185
spec.CinderAPI.Override.Service)...)
180186

187+
// Cinder Backup validation: it returns errors in case both CinderBackup
188+
// and CinderBackups are used, and it throws a warning by default when the
189+
// deprecated parameter is used (it is a non blocking issue though)
181190
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
182-
return allErrs
191+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
192+
allErrs = append(allErrs, bkpErrs...)
193+
allWarns = append(allWarns, bkpWarns...)
194+
195+
return allWarns, allErrs
183196
}
184197

185-
func (spec *CinderSpecCore) ValidateCreate(basePath *field.Path, namespace string) field.ErrorList {
198+
func (spec *CinderSpecCore) ValidateCreate(
199+
basePath *field.Path,
200+
namespace string,
201+
) ([]string, field.ErrorList){
186202
var allErrs field.ErrorList
203+
var allWarns admission.Warnings
187204

188205
// validate the service override key is valid
189206
allErrs = append(allErrs, service.ValidateRoutedOverrides(
190207
basePath.Child("cinderAPI").Child("override").Child("service"),
191208
spec.CinderAPI.Override.Service)...)
192209

193210
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
194-
return allErrs
211+
212+
// Cinder Backup validation: it returns errors in case both CinderBackup
213+
// and CinderBackups are used, and it throws a warning by default when the
214+
// deprecated parameter is used (it is a non blocking issue though)
215+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
216+
allErrs = append(allErrs, bkpErrs...)
217+
allWarns = append(allWarns, bkpWarns...)
218+
219+
return allWarns, allErrs
195220
}
196221

197222
// 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)
204229
}
205230

206231
var allErrs field.ErrorList
232+
var allWarns admission.Warnings
233+
207234
basePath := field.NewPath("spec")
208235

209236
// Validate cinderVolume name is valid
@@ -223,8 +250,9 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
223250
GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
224251
allErrs = append(allErrs, err...)
225252

226-
if err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
253+
if warns, err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath, r.Namespace); err != nil {
227254
allErrs = append(allErrs, err...)
255+
allWarns = append(allWarns, warns...)
228256
}
229257

230258
if len(allErrs) != 0 {
@@ -233,33 +261,61 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
233261
r.Name, allErrs)
234262
}
235263

236-
return nil, nil
264+
return allWarns, nil
237265
}
238266

239267
// ValidateUpdate - Exported function wrapping non-exported validate functions,
240268
// this function can be called externally to validate an cinder spec.
241-
func (spec *CinderSpec) ValidateUpdate(old CinderSpec, basePath *field.Path, namespace string) field.ErrorList {
269+
func (spec *CinderSpec) ValidateUpdate(
270+
old CinderSpec,
271+
basePath *field.Path,
272+
namespace string,
273+
) ([]string, field.ErrorList) {
274+
242275
var allErrs field.ErrorList
276+
var allWarns []string
243277

244278
// validate the service override key is valid
245279
allErrs = append(allErrs, service.ValidateRoutedOverrides(
246280
basePath.Child("cinderAPI").Child("override").Child("service"),
247281
spec.CinderAPI.Override.Service)...)
248282

249283
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
250-
return allErrs
284+
285+
// Cinder Backup validation: it returns errors in case both CinderBackup
286+
// and CinderBackups are used, and it throws a warning by default when the
287+
// deprecated parameter is used (it is a non blocking issue though)
288+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
289+
allErrs = append(allErrs, bkpErrs...)
290+
allWarns = append(allWarns, bkpWarns...)
291+
292+
return allWarns, allErrs
251293
}
252294

253-
func (spec *CinderSpecCore) ValidateUpdate(old CinderSpecCore, basePath *field.Path, namespace string) field.ErrorList {
295+
func (spec *CinderSpecCore) ValidateUpdate(
296+
old CinderSpecCore,
297+
basePath *field.Path,
298+
namespace string,
299+
) ([]string, field.ErrorList) {
300+
254301
var allErrs field.ErrorList
302+
var allWarns []string
255303

256304
// validate the service override key is valid
257305
allErrs = append(allErrs, service.ValidateRoutedOverrides(
258306
basePath.Child("cinderAPI").Child("override").Child("service"),
259307
spec.CinderAPI.Override.Service)...)
260308

261309
allErrs = append(allErrs, spec.ValidateCinderTopology(basePath, namespace)...)
262-
return allErrs
310+
311+
// Cinder Backup validation: it returns errors in case both CinderBackup
312+
// and CinderBackups are used, and it throws a warning by default when the
313+
// deprecated parameter is used (it is a non blocking issue though)
314+
bkpWarns, bkpErrs := spec.ValidateCinderBackup(basePath)
315+
allErrs = append(allErrs, bkpErrs...)
316+
allWarns = append(allWarns, bkpWarns...)
317+
318+
return allWarns, allErrs
263319
}
264320

265321
// 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
336392
bkPath := basePath.Child("cinderBackup")
337393
allErrs = append(allErrs,
338394
spec.CinderBackup.ValidateTopology(bkPath, namespace)...)
339-
395+
340396
// When a TopologyRef CR is referenced with an override to an instance of
341397
// CinderBackups, fail if a different Namespace is referenced because not
342398
// supported
@@ -405,3 +461,50 @@ func (spec *CinderSpec) ValidateCinderTopology(basePath *field.Path, namespace s
405461
}
406462
return allErrs
407463
}
464+
465+
// TODO: Remove this function when refactoring CinderSpec to include CinderSpecCore
466+
func (spec *CinderSpec) ValidateCinderBackup(basePath *field.Path) ([]string, field.ErrorList) {
467+
var allErrs field.ErrorList
468+
var allWarns []string
469+
470+
// Check if the deprecated field is configured
471+
isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
472+
// Check if CinderBackup list is configured
473+
isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
474+
475+
// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
476+
// are used together
477+
if isDeprecatedUsed && isCinderBackupListUsed {
478+
errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when 'cinderBackups' is defined."
479+
allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
480+
}
481+
// Append a warning depending on the cinderBackup field usage
482+
if isDeprecatedUsed {
483+
warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
484+
allWarns = append(allWarns, warningMsg)
485+
}
486+
return allWarns, allErrs
487+
}
488+
489+
func (spec *CinderSpecCore) ValidateCinderBackup(basePath *field.Path)([]string, field.ErrorList) {
490+
var allErrs field.ErrorList
491+
var allWarns []string
492+
493+
// Check if the deprecated field is configured
494+
isDeprecatedUsed := spec.CinderBackup.Replicas != nil && *spec.CinderBackup.Replicas > 0
495+
// Check if CinderBackup list is configured
496+
isCinderBackupListUsed := spec.CinderBackups != nil && len(*spec.CinderBackups) > 0
497+
498+
// Fail if both cinderBackup (the deprecated field) and cinderBackups (the new list)
499+
// are used together
500+
if isDeprecatedUsed && isCinderBackupListUsed {
501+
errMsg := "Usage of the deprecated 'cinderBackup' is forbidden when the new 'cinderBackups' API is used."
502+
allErrs = append(allErrs, field.Invalid(basePath, "cinderBackup", errMsg))
503+
}
504+
// Append a warning depending on the cinderBackup field usage
505+
if isDeprecatedUsed {
506+
warningMsg := "The 'cinderBackup' field is deprecated and will be removed in a future release. Please migrate to 'cinderBackups'."
507+
allWarns = append(allWarns, warningMsg)
508+
}
509+
return allWarns, allErrs
510+
}

‎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)