From 4464b1e26687648322712df848b8f196061e01cc Mon Sep 17 00:00:00 2001 From: Niraj Yadav Date: Wed, 4 Sep 2024 19:14:43 +0530 Subject: [PATCH] Refactor PVC controller This PR refactors the PVC controller to reduce code duplication by extracting common logic into its own functions/packages Signed-off-by: Niraj Yadav --- .../persistentvolumeclaim_controller.go | 393 ++++++++------- .../persistentvolumeclaim_controller_test.go | 457 ++++++++++++++++-- 2 files changed, 602 insertions(+), 248 deletions(-) diff --git a/internal/controller/csiaddons/persistentvolumeclaim_controller.go b/internal/controller/csiaddons/persistentvolumeclaim_controller.go index 84848debe..66e9ded25 100644 --- a/internal/controller/csiaddons/persistentvolumeclaim_controller.go +++ b/internal/controller/csiaddons/persistentvolumeclaim_controller.go @@ -55,6 +55,10 @@ type PersistentVolumeClaimReconciler struct { ConnPool *connection.ConnectionPool } +// Operation defines the sub operation to be performed +// on the PVC. e.g. reclaimspace, keyrotation +type Operation string + var ( rsCronJobScheduleTimeAnnotation = "reclaimspace." + csiaddonsv1alpha1.GroupVersion.Group + "/schedule" rsCronJobNameAnnotation = "reclaimspace." + csiaddonsv1alpha1.GroupVersion.Group + "/cronjob" @@ -70,6 +74,9 @@ var ( const ( defaultSchedule = "@weekly" + + relciamSpaceOp Operation = "reclaimspace" + keyRotationOp Operation = "keyrotation" ) //+kubebuilder:rbac:groups=core,resources=persistentvolumeclaims,verbs=get;list;watch;patch @@ -146,59 +153,61 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, keyRotationErr } -// checkDriverSupportReclaimsSpace checks if the driver supports space -// reclamation or not. If the driver does not support space reclamation, it -// returns false and if the driver supports space reclamation, it returns true. -// If the driver name is not registered in the connection pool, it returns -// false and requeues the request. -func (r *PersistentVolumeClaimReconciler) checkDriverSupportReclaimsSpace(logger *logr.Logger, annotations map[string]string, driver string) (bool, bool) { - reclaimSpaceSupportedByDriver := false - - if drivers, ok := annotations[rsCSIAddonsDriverAnnotation]; ok && slices.Contains(strings.Split(drivers, ","), driver) { - reclaimSpaceSupportedByDriver = true - } - - ok := r.supportsReclaimSpace(driver) - if reclaimSpaceSupportedByDriver && !ok { - logger.Info("Driver supports spacereclamation but driver is not registered in the connection pool, Reqeueing request", "DriverName", driver) - return true, false - } - - if !ok { - logger.Info("Driver does not support spacereclamation, skip Requeue", "DriverName", driver) +// checkDriverSupportCapability checks if the given driver supports the specified capability. +// It is also used to determine the requeue in case the driver supports the capability +// but the capability is not found in connection pool. +// The first return value is true when a requeue is needed. +// The second return value is true when the capability is registered. +func (r *PersistentVolumeClaimReconciler) checkDriverSupportCapability( + logger *logr.Logger, + annotations map[string]string, + driverName string, + cap Operation) (bool, bool) { + driverSupportsCap := false + capFound := false + + var driverAnnotation string + switch cap { + case relciamSpaceOp: + driverAnnotation = rsCSIAddonsDriverAnnotation + case keyRotationOp: + driverAnnotation = krCSIAddonsDriverAnnotation + default: + logger.Info("Unknown capability", "Capability", cap) return false, false } - return false, true -} + if drivers, ok := annotations[driverAnnotation]; ok && slices.Contains(strings.Split(drivers, ","), driverAnnotation) { + driverSupportsCap = true + } -// checkDriverSupportsEncryptionKeyRotate checks if the driver supports key -// rotation or not. If the driver does not support key rotation, it -// returns false and if the driver supports key rotation, it returns true. -// If the driver name is not registered in the connection pool, it returns -// false and requeues the request. -func (r *PersistentVolumeClaimReconciler) checkDriverSupportsEncryptionKeyRotate( - logger *logr.Logger, - annotations map[string]string, - driver string) (bool, bool) { - keyRotationSupportedByDriver := false + conns := r.ConnPool.GetByNodeID(driverName, "") + for _, conn := range conns { + for _, c := range conn.Capabilities { + switch cap { + case relciamSpaceOp: + capFound = c.GetReclaimSpace() != nil + case keyRotationOp: + capFound = c.GetEncryptionKeyRotation() != nil + default: + continue + } - if drivers, ok := annotations[krCSIAddonsDriverAnnotation]; ok && slices.Contains(strings.Split(drivers, ","), driver) { - keyRotationSupportedByDriver = true + if capFound { + return false, true + } + } } - ok := r.supportsEncryptionKeyRotation(driver) - if keyRotationSupportedByDriver && !ok { - logger.Info("Driver supports key rotation but driver is not registered in the connection pool, Reqeueing request", "DriverName", driver) + // If the driver supports the capability but the capability is not found in connection pool, + if driverSupportsCap { + logger.Info(fmt.Sprintf("Driver supports %s but driver is not registered in the connection pool, Requeuing request", cap), "DriverName", driverName) return true, false } - if !ok { - logger.Info("Driver does not support encryptionkeyrotation, skip Requeue", "DriverName", driver) - return false, false - } - - return false, true + // If the driver does not support the capability, skip requeue + logger.Info(fmt.Sprintf("Driver does not support %s, skip Requeue", cap), "DriverName", driverName) + return false, false } // determineScheduleAndRequeue determines the schedule using the following steps @@ -241,16 +250,17 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue( // the driver is registered in the connection pool and if not we are not // requeuing the request. // Depending on requeue value, it will return ErrorConnNotFoundRequeueNeeded. - if annotationKey == krcJobScheduleTimeAnnotation { - requeue, keyRotationSupported := r.checkDriverSupportsEncryptionKeyRotate(logger, ns.Annotations, driverName) + switch annotationKey { + case krcJobScheduleTimeAnnotation: + requeue, keyRotationSupported := r.checkDriverSupportCapability(logger, ns.Annotations, driverName, keyRotationOp) if keyRotationSupported { return schedule, nil } if requeue { return "", ErrConnNotFoundRequeueNeeded } - } else if annotationKey == rsCronJobScheduleTimeAnnotation { - requeue, supportReclaimspace := r.checkDriverSupportReclaimsSpace(logger, ns.Annotations, driverName) + case rsCronJobScheduleTimeAnnotation: + requeue, supportReclaimspace := r.checkDriverSupportCapability(logger, ns.Annotations, driverName, relciamSpaceOp) if supportReclaimspace { // if driver supports space reclamation, // return schedule from ns annotation. @@ -261,6 +271,9 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue( // driver support again. return "", ErrConnNotFoundRequeueNeeded } + default: + logger.Info("Unknown annotation key", "AnnotationKey", annotationKey) + return "", fmt.Errorf("unknown annotation key: %s", annotationKey) } } @@ -295,10 +308,8 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue( // PVCs associated with the changed StorageClass. // // PVCs are enqueued for reconciliation if one of the following is true - -// - If the StorageClass has ReclaimSpace annotation, -// PVCs without ReclaimSpace annotations will be enqueued. -// - If the StorageClass has KeyRotation annotation, -// PVCs without the KeyRotation annotation will be enqueued. +// - If the StorageClass has an annotation, +// PVCs without that annotation will be enqueued. func (r *PersistentVolumeClaimReconciler) storageClassEventHandler() handler.EventHandler { return handler.EnqueueRequestsFromMapFunc( func(ctx context.Context, obj client.Object) []reconcile.Request { @@ -317,25 +328,14 @@ func (r *PersistentVolumeClaimReconciler) storageClassEventHandler() handler.Eve return nil } - _, scHasReclaimSpaceAnnotation := obj.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - _, scHasKeyRotationAnnotation := obj.GetAnnotations()[krcJobScheduleTimeAnnotation] + annotationsToWatch := []string{ + rsCronJobScheduleTimeAnnotation, + krcJobScheduleTimeAnnotation, + } var requests []reconcile.Request for _, pvc := range pvcList.Items { - - _, pvcHasReclaimSpaceAnnotation := pvc.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - _, pvcHasKeyRotationAnnotation := pvc.GetAnnotations()[krcJobScheduleTimeAnnotation] - - needToEnqueue := false - - if scHasReclaimSpaceAnnotation && !pvcHasReclaimSpaceAnnotation { - needToEnqueue = true - } - if scHasKeyRotationAnnotation && !pvcHasKeyRotationAnnotation { - needToEnqueue = true - } - - if needToEnqueue { + if annotationValueMissing(obj.GetAnnotations(), pvc.GetAnnotations(), annotationsToWatch) { requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: pvc.Name, @@ -350,86 +350,63 @@ func (r *PersistentVolumeClaimReconciler) storageClassEventHandler() handler.Eve ) } -// SetupWithManager sets up the controller with the Manager. -func (r *PersistentVolumeClaimReconciler) SetupWithManager(mgr ctrl.Manager, ctrlOptions controller.Options) error { - err := mgr.GetFieldIndexer().IndexField( - context.Background(), - &csiaddonsv1alpha1.ReclaimSpaceCronJob{}, - jobOwnerKey, - extractOwnerNameFromPVCObj) - if err != nil { - return err +// setupIndexers sets up the necessary indexers for the PersistentVolumeClaimReconciler. +// It creates indexers for the following objects: +// - ReclaimSpaceCronJob: indexed by the job owner key +// - EncryptionKeyRotationCronJob: indexed by the job owner key +// - PersistentVolumeClaim: indexed by the storage class names +func (r *PersistentVolumeClaimReconciler) setupIndexers(mgr ctrl.Manager) error { + indices := []struct { + obj client.Object + field string + indexFn client.IndexerFunc + }{ + { + obj: &csiaddonsv1alpha1.ReclaimSpaceCronJob{}, + field: jobOwnerKey, + indexFn: extractOwnerNameFromPVCObj[*csiaddonsv1alpha1.ReclaimSpaceCronJob], + }, + { + obj: &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{}, + field: jobOwnerKey, + indexFn: extractOwnerNameFromPVCObj[*csiaddonsv1alpha1.EncryptionKeyRotationCronJob], + }, + { + obj: &corev1.PersistentVolumeClaim{}, + field: "spec.storageClassName", + indexFn: func(rawObj client.Object) []string { + pvc, ok := rawObj.(*corev1.PersistentVolumeClaim) + if !ok { + return nil + } + return []string{*pvc.Spec.StorageClassName} + }, + }, } - err = mgr.GetFieldIndexer().IndexField( - context.Background(), - &corev1.PersistentVolumeClaim{}, - "spec.storageClassName", - func(rawObj client.Object) []string { - pvc, ok := rawObj.(*corev1.PersistentVolumeClaim) - if !ok { - return nil - } - return []string{*pvc.Spec.StorageClassName} - }) - if err != nil { - return err + // Add the indexers to the manager + for _, index := range indices { + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + index.obj, + index.field, + index.indexFn, + ); err != nil { + return err + } } - err = mgr.GetFieldIndexer().IndexField( - context.Background(), - &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{}, - jobOwnerKey, - func(rawObj client.Object) []string { - job, ok := rawObj.(*csiaddonsv1alpha1.EncryptionKeyRotationCronJob) - if !ok { - return nil - } - owner := metav1.GetControllerOf(job) - if owner == nil { - return nil - } - if owner.APIVersion != "v1" || owner.Kind != "PersistentVolumeClaim" { - return nil - } + return nil +} - return []string{owner.Name} - }) - if err != nil { +// SetupWithManager sets up the controller with the Manager. +func (r *PersistentVolumeClaimReconciler) SetupWithManager(mgr ctrl.Manager, ctrlOptions controller.Options) error { + if err := r.setupIndexers(mgr); err != nil { return err } - pred := predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - if e.ObjectNew == nil || e.ObjectOld == nil { - return false - } - // reconcile only if schedule annotation between old and new objects have changed. - oldSchdeule, oldOk := e.ObjectOld.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - newSchdeule, newOk := e.ObjectNew.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - - krcOldSchdeule, krcOldOk := e.ObjectOld.GetAnnotations()[krcJobScheduleTimeAnnotation] - krcNewSchdeule, krcNewOk := e.ObjectNew.GetAnnotations()[krcJobScheduleTimeAnnotation] - - return (oldOk != newOk || oldSchdeule != newSchdeule) || (krcOldOk != krcNewOk || krcOldSchdeule != krcNewSchdeule) - }, - } - - scPred := predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - if e.ObjectNew == nil || e.ObjectOld == nil { - return false - } - // reconcile only if reclaimspace annotation between old and new objects have changed. - oldSchdeule, oldOk := e.ObjectOld.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - newSchdeule, newOk := e.ObjectNew.GetAnnotations()[rsCronJobScheduleTimeAnnotation] - - krcOldSchdeule, krcOldOk := e.ObjectOld.GetAnnotations()[krcJobScheduleTimeAnnotation] - krcNewSchdeule, krcNewOk := e.ObjectNew.GetAnnotations()[krcJobScheduleTimeAnnotation] - - return (oldOk != newOk || oldSchdeule != newSchdeule) || (krcOldOk != krcNewOk || krcOldSchdeule != krcNewSchdeule) - }, - } + pvcPred := createAnnotationPredicate(rsCronJobScheduleTimeAnnotation, krcJobScheduleTimeAnnotation) + scPred := createAnnotationPredicate(rsCronJobScheduleTimeAnnotation, krcJobScheduleTimeAnnotation) return ctrl.NewControllerManagedBy(mgr). For(&corev1.PersistentVolumeClaim{}). @@ -440,7 +417,7 @@ func (r *PersistentVolumeClaimReconciler) SetupWithManager(mgr ctrl.Manager, ctr ). Owns(&csiaddonsv1alpha1.ReclaimSpaceCronJob{}). Owns(&csiaddonsv1alpha1.EncryptionKeyRotationCronJob{}). - WithEventFilter(pred). + WithEventFilter(pvcPred). WithOptions(ctrlOptions). Complete(r) } @@ -521,10 +498,38 @@ func getScheduleFromAnnotation( return schedule, true } -// constructRSCronJob constructs and returns ReclaimSpaceCronJob. +// constructKRCronJob constructs an EncryptionKeyRotationCronJob object +func constructKRCronJob(name, namespace, schedule, pvcName string) *csiaddonsv1alpha1.EncryptionKeyRotationCronJob { + failedJobHistoryLimit := defaultFailedJobsHistoryLimit + successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit + + return &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: csiaddonsv1alpha1.EncryptionKeyRotationCronJobSpec{ + Schedule: schedule, + JobSpec: csiaddonsv1alpha1.EncryptionKeyRotationJobTemplateSpec{ + Spec: csiaddonsv1alpha1.EncryptionKeyRotationJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: pvcName, + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + } +} + +// constructRSCronJob constructs a ReclaimSpaceCronJob object func constructRSCronJob(name, namespace, schedule, pvcName string) *csiaddonsv1alpha1.ReclaimSpaceCronJob { failedJobsHistoryLimit := defaultFailedJobsHistoryLimit successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit + return &csiaddonsv1alpha1.ReclaimSpaceCronJob{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -546,10 +551,10 @@ func constructRSCronJob(name, namespace, schedule, pvcName string) *csiaddonsv1a } // extractOwnerNameFromPVCObj extracts owner.Name from the object if it is -// of type ReclaimSpaceCronJob and has a PVC as its owner. -func extractOwnerNameFromPVCObj(rawObj client.Object) []string { +// of type `T` and has a PVC as its owner. +func extractOwnerNameFromPVCObj[T client.Object](rawObj client.Object) []string { // extract the owner from job object. - job, ok := rawObj.(*csiaddonsv1alpha1.ReclaimSpaceCronJob) + job, ok := rawObj.(T) if !ok { return nil } @@ -570,34 +575,6 @@ func generateCronJobName(parentName string) string { return fmt.Sprintf("%s-%d", parentName, time.Now().Unix()) } -// supportsReclaimSpace checks if the CSI driver supports ReclaimSpace. -func (r PersistentVolumeClaimReconciler) supportsReclaimSpace(driverName string) bool { - conns := r.ConnPool.GetByNodeID(driverName, "") - for _, v := range conns { - for _, cap := range v.Capabilities { - if cap.GetReclaimSpace() != nil { - return true - } - } - } - - return false -} - -// supportsEncryptionKeyRotation checks if the CSI driver supports EncryptionKeyRotation. -func (r PersistentVolumeClaimReconciler) supportsEncryptionKeyRotation(driverName string) bool { - conns := r.ConnPool.GetByNodeID(driverName, "") - for _, v := range conns { - for _, cap := range v.Capabilities { - if cap.GetEncryptionKeyRotation() != nil { - return true - } - } - } - - return false -} - // processReclaimSpace reconciles ReclaimSpace based on annotations func (r *PersistentVolumeClaimReconciler) processReclaimSpace( ctx context.Context, @@ -740,33 +717,6 @@ func (r *PersistentVolumeClaimReconciler) findChildEncryptionKeyRotationCronJob( return activeJob, nil } -// constructKRCronJob constructs an EncryptionKeyRotationCronJob object -func constructKRCronJob(name, namespace, schedule, pvcName string) *csiaddonsv1alpha1.EncryptionKeyRotationCronJob { - failedJobHistoryLimit := defaultFailedJobsHistoryLimit - successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit - - return &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: csiaddonsv1alpha1.EncryptionKeyRotationCronJobSpec{ - Schedule: schedule, - JobSpec: csiaddonsv1alpha1.EncryptionKeyRotationJobTemplateSpec{ - Spec: csiaddonsv1alpha1.EncryptionKeyRotationJobSpec{ - Target: csiaddonsv1alpha1.TargetSpec{ - PersistentVolumeClaim: pvcName, - }, - BackoffLimit: defaultBackoffLimit, - RetryDeadlineSeconds: defaultRetryDeadlineSeconds, - }, - }, - FailedJobsHistoryLimit: &failedJobHistoryLimit, - SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, - }, - } -} - // processKeyRotation reconciles EncryptionKeyRotation based on annotations func (r *PersistentVolumeClaimReconciler) processKeyRotation( ctx context.Context, @@ -860,3 +810,48 @@ func (r *PersistentVolumeClaimReconciler) processKeyRotation( logger.Info("successfully created new encryptionkeyrotationcronjob") return nil } + +// AnnotationValueMissing checks if any of the specified keys are missing +// from the PVC annotations when they are present in the StorageClass annotations. +func annotationValueMissing(scAnnotations, pvcAnnotations map[string]string, keys []string) bool { + for _, key := range keys { + if _, scHasAnnotation := scAnnotations[key]; scHasAnnotation { + if _, pvcHasAnnotation := pvcAnnotations[key]; !pvcHasAnnotation { + return true + } + } + } + + return false +} + +// AnnotationValueChanged checks if any of the specified keys have different values +// between the old and new annotations maps. +func annotationValueChanged(oldAnnotations, newAnnotations map[string]string, keys []string) bool { + for _, key := range keys { + oldVal, oldExists := oldAnnotations[key] + newVal, newExists := newAnnotations[key] + + if oldExists != newExists || oldVal != newVal { + return true + } + } + return false +} + +// CreateAnnotationPredicate returns a predicate.Funcs that checks if any of the specified +// annotation keys have different values between the old and new annotations maps. +func createAnnotationPredicate(annotations ...string) predicate.Funcs { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectNew == nil || e.ObjectOld == nil { + return false + } + + oldAnnotations := e.ObjectOld.GetAnnotations() + newAnnotations := e.ObjectNew.GetAnnotations() + + return annotationValueChanged(oldAnnotations, newAnnotations, annotations) + }, + } +} diff --git a/internal/controller/csiaddons/persistentvolumeclaim_controller_test.go b/internal/controller/csiaddons/persistentvolumeclaim_controller_test.go index 3290b6351..7dae5ed19 100644 --- a/internal/controller/csiaddons/persistentvolumeclaim_controller_test.go +++ b/internal/controller/csiaddons/persistentvolumeclaim_controller_test.go @@ -30,58 +30,17 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" ) -func TestConstructRSCronJob(t *testing.T) { - type args struct { - name string - namespace string - schedule string - pvcName string - } +type mockObject struct { + client.Object + annotations map[string]string +} - failedJobsHistoryLimit := defaultFailedJobsHistoryLimit - successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit - tests := []struct { - name string - args args - want *csiaddonsv1alpha1.ReclaimSpaceCronJob - }{ - { - name: "check output", - args: args{ - name: "hello", - namespace: "default", - schedule: "@yearly", - pvcName: "pvc-1", - }, - want: &csiaddonsv1alpha1.ReclaimSpaceCronJob{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hello", - Namespace: "default", - }, - Spec: csiaddonsv1alpha1.ReclaimSpaceCronJobSpec{ - Schedule: "@yearly", - JobSpec: csiaddonsv1alpha1.ReclaimSpaceJobTemplateSpec{ - Spec: csiaddonsv1alpha1.ReclaimSpaceJobSpec{ - Target: csiaddonsv1alpha1.TargetSpec{PersistentVolumeClaim: "pvc-1"}, - BackoffLimit: defaultBackoffLimit, - RetryDeadlineSeconds: defaultRetryDeadlineSeconds, - }, - }, - FailedJobsHistoryLimit: &failedJobsHistoryLimit, - SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := constructRSCronJob(tt.args.name, tt.args.namespace, tt.args.schedule, tt.args.pvcName) - assert.Equal(t, tt.want, got) - }) - } +func (m *mockObject) GetAnnotations() map[string]string { + return m.annotations } func TestExtractOwnerNameFromPVCObj(t *testing.T) { @@ -169,7 +128,7 @@ func TestExtractOwnerNameFromPVCObj(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := extractOwnerNameFromPVCObj(tt.args.rawObj) + got := extractOwnerNameFromPVCObj[*csiaddonsv1alpha1.ReclaimSpaceCronJob](tt.args.rawObj) assert.Equal(t, tt.want, got) }) } @@ -365,3 +324,403 @@ func TestDetermineScheduleAndRequeue(t *testing.T) { }) } + +func TestAnnotationValueMissing(t *testing.T) { + tests := []struct { + name string + scAnnotations map[string]string + pvcAnnotations map[string]string + keys []string + expected bool + }{ + { + name: "No annotations", + scAnnotations: map[string]string{}, + pvcAnnotations: map[string]string{}, + keys: []string{"key1", "key2"}, + expected: false, + }, + { + name: "SC has annotation, PVC doesn't", + scAnnotations: map[string]string{"key1": "value1"}, + pvcAnnotations: map[string]string{}, + keys: []string{"key1"}, + expected: true, + }, + { + name: "Both SC and PVC have annotation", + scAnnotations: map[string]string{"key1": "value1"}, + pvcAnnotations: map[string]string{"key1": "value1"}, + keys: []string{"key1"}, + expected: false, + }, + { + name: "SC has multiple annotations, PVC missing one", + scAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + pvcAnnotations: map[string]string{"key1": "value1"}, + keys: []string{"key1", "key2"}, + expected: true, + }, + { + name: "SC has annotation not in keys", + scAnnotations: map[string]string{"key3": "value3"}, + pvcAnnotations: map[string]string{}, + keys: []string{"key1", "key2"}, + expected: false, + }, + { + name: "Empty keys slice", + scAnnotations: map[string]string{"key1": "value1"}, + pvcAnnotations: map[string]string{}, + keys: []string{}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := annotationValueMissing(tt.scAnnotations, tt.pvcAnnotations, tt.keys) + if result != tt.expected { + t.Errorf("AnnotationValueMissing() = %v, want %v", result, tt.expected) + } + }) + } +} + +func TestAnnotationValueChanged(t *testing.T) { + tests := []struct { + name string + oldAnnotations map[string]string + newAnnotations map[string]string + keys []string + expected bool + }{ + { + name: "No changes", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + keys: []string{"key1", "key2"}, + expected: false, + }, + { + name: "Value changed", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "newvalue2"}, + keys: []string{"key1", "key2"}, + expected: true, + }, + { + name: "Key added", + oldAnnotations: map[string]string{"key1": "value1"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + keys: []string{"key1", "key2"}, + expected: true, + }, + { + name: "Key removed", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1"}, + keys: []string{"key1", "key2"}, + expected: true, + }, + { + name: "Change in non-specified key", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2", "key3": "value3"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "value2", "key3": "newvalue3"}, + keys: []string{"key1", "key2"}, + expected: false, + }, + { + name: "Empty keys slice", + oldAnnotations: map[string]string{"key1": "value1"}, + newAnnotations: map[string]string{"key1": "newvalue1"}, + keys: []string{}, + expected: false, + }, + { + name: "Nil maps", + oldAnnotations: nil, + newAnnotations: nil, + keys: []string{"key1"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := annotationValueChanged(tt.oldAnnotations, tt.newAnnotations, tt.keys) + if result != tt.expected { + t.Errorf("AnnotationValueChanged() = %v, want %v", result, tt.expected) + } + }) + } +} + +func TestCreateAnnotationPredicate(t *testing.T) { + tests := []struct { + name string + oldAnnotations map[string]string + newAnnotations map[string]string + annotations []string + expected bool + }{ + { + name: "No change in specified annotations", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + annotations: []string{"key1", "key2"}, + expected: false, + }, + { + name: "Change in specified annotation", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "newvalue1", "key2": "value2"}, + annotations: []string{"key1"}, + expected: true, + }, + { + name: "Change in unspecified annotation", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "newvalue2"}, + annotations: []string{"key1"}, + expected: false, + }, + { + name: "New annotation added", + oldAnnotations: map[string]string{"key1": "value1"}, + newAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + annotations: []string{"key1", "key2"}, + expected: true, + }, + { + name: "Specified annotation removed", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "value1"}, + annotations: []string{"key1", "key2"}, + expected: true, + }, + { + name: "Empty annotations list", + oldAnnotations: map[string]string{"key1": "value1", "key2": "value2"}, + newAnnotations: map[string]string{"key1": "newvalue1", "key2": "newvalue2"}, + annotations: []string{}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + predicate := createAnnotationPredicate(tt.annotations...) + result := predicate.UpdateFunc(event.UpdateEvent{ + ObjectOld: &mockObject{annotations: tt.oldAnnotations}, + ObjectNew: &mockObject{annotations: tt.newAnnotations}, + }) + if result != tt.expected { + t.Errorf("CreateAnnotationPredicate() = %v, want %v", result, tt.expected) + } + }) + } +} + +func TestConstructKRCronJob(t *testing.T) { + failedJobHistoryLimit := defaultFailedJobsHistoryLimit + successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit + tests := []struct { + name string + cronName string + namespace string + schedule string + pvcName string + expected *csiaddonsv1alpha1.EncryptionKeyRotationCronJob + }{ + { + name: "Basic KR CronJob", + cronName: "test-kr-cron", + namespace: "default", + schedule: "0 1 * * *", + pvcName: "test-pvc", + expected: &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-kr-cron", + Namespace: "default", + }, + Spec: csiaddonsv1alpha1.EncryptionKeyRotationCronJobSpec{ + Schedule: "0 1 * * *", + JobSpec: csiaddonsv1alpha1.EncryptionKeyRotationJobTemplateSpec{ + Spec: csiaddonsv1alpha1.EncryptionKeyRotationJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "test-pvc", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + { + name: "KR CronJob with empty schedule", + cronName: "empty-schedule-cron", + namespace: "kube-system", + schedule: "", + pvcName: "data-pvc", + expected: &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-schedule-cron", + Namespace: "kube-system", + }, + Spec: csiaddonsv1alpha1.EncryptionKeyRotationCronJobSpec{ + Schedule: "", + JobSpec: csiaddonsv1alpha1.EncryptionKeyRotationJobTemplateSpec{ + Spec: csiaddonsv1alpha1.EncryptionKeyRotationJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "data-pvc", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + { + name: "KR CronJob with special characters", + cronName: "special-!@#$%^&*()-cron", + namespace: "test-ns", + schedule: "*/5 * * * *", + pvcName: "pvc-with-special-chars-!@#$%^&*()", + expected: &csiaddonsv1alpha1.EncryptionKeyRotationCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "special-!@#$%^&*()-cron", + Namespace: "test-ns", + }, + Spec: csiaddonsv1alpha1.EncryptionKeyRotationCronJobSpec{ + Schedule: "*/5 * * * *", + JobSpec: csiaddonsv1alpha1.EncryptionKeyRotationJobTemplateSpec{ + Spec: csiaddonsv1alpha1.EncryptionKeyRotationJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "pvc-with-special-chars-!@#$%^&*()", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := constructKRCronJob(tt.cronName, tt.namespace, tt.schedule, tt.pvcName) + assert.Equal(t, tt.expected, result) + }) + } +} +func TestConstructRSCronJob(t *testing.T) { + failedJobHistoryLimit := defaultFailedJobsHistoryLimit + successfulJobsHistoryLimit := defaultSuccessfulJobsHistoryLimit + tests := []struct { + name string + cronName string + namespace string + schedule string + pvcName string + expected *csiaddonsv1alpha1.ReclaimSpaceCronJob + }{ + { + name: "Basic RS CronJob", + cronName: "test-rs-cron", + namespace: "default", + schedule: "0 2 * * *", + pvcName: "test-pvc", + expected: &csiaddonsv1alpha1.ReclaimSpaceCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rs-cron", + Namespace: "default", + }, + Spec: csiaddonsv1alpha1.ReclaimSpaceCronJobSpec{ + Schedule: "0 2 * * *", + JobSpec: csiaddonsv1alpha1.ReclaimSpaceJobTemplateSpec{ + Spec: csiaddonsv1alpha1.ReclaimSpaceJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "test-pvc", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + { + name: "RS CronJob with empty schedule", + cronName: "empty-schedule-cron", + namespace: "kube-system", + schedule: "", + pvcName: "data-pvc", + expected: &csiaddonsv1alpha1.ReclaimSpaceCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-schedule-cron", + Namespace: "kube-system", + }, + Spec: csiaddonsv1alpha1.ReclaimSpaceCronJobSpec{ + Schedule: "", + JobSpec: csiaddonsv1alpha1.ReclaimSpaceJobTemplateSpec{ + Spec: csiaddonsv1alpha1.ReclaimSpaceJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "data-pvc", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + { + name: "RS CronJob with special characters", + cronName: "special-!@#$%^&*()-cron", + namespace: "test-ns", + schedule: "*/10 * * * *", + pvcName: "pvc-with-special-chars-!@#$%^&*()", + expected: &csiaddonsv1alpha1.ReclaimSpaceCronJob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "special-!@#$%^&*()-cron", + Namespace: "test-ns", + }, + Spec: csiaddonsv1alpha1.ReclaimSpaceCronJobSpec{ + Schedule: "*/10 * * * *", + JobSpec: csiaddonsv1alpha1.ReclaimSpaceJobTemplateSpec{ + Spec: csiaddonsv1alpha1.ReclaimSpaceJobSpec{ + Target: csiaddonsv1alpha1.TargetSpec{ + PersistentVolumeClaim: "pvc-with-special-chars-!@#$%^&*()", + }, + BackoffLimit: defaultBackoffLimit, + RetryDeadlineSeconds: defaultRetryDeadlineSeconds, + }, + }, + FailedJobsHistoryLimit: &failedJobHistoryLimit, + SuccessfulJobsHistoryLimit: &successfulJobsHistoryLimit, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := constructRSCronJob(tt.cronName, tt.namespace, tt.schedule, tt.pvcName) + assert.Equal(t, tt.expected, result) + }) + } +}