Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable KeyRotation #659

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 85 additions & 2 deletions internal/controller/csiaddons/persistentvolumeclaim_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ var (
rsCSIAddonsDriverAnnotation = "reclaimspace." + csiaddonsv1alpha1.GroupVersion.Group + "/drivers"

krcJobScheduleTimeAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/schedule"
krcJobDisableAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/enable"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name contains Disable, but the value ends with /enable. This is a little confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'll edit the name and logic accordingly.

krcJobNameAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/cronjob"
krCSIAddonsDriverAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/drivers"

Expand Down Expand Up @@ -308,8 +309,12 @@ 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 an annotation,
// PVCs without that annotation will be enqueued.
// - 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 KeyRotation disable annotation,
// PVCs without the KeyRotation disable annotation will be enqueued.
func (r *PersistentVolumeClaimReconciler) storageClassEventHandler() handler.EventHandler {
black-dragon74 marked this conversation as resolved.
Show resolved Hide resolved
return handler.EnqueueRequestsFromMapFunc(
func(ctx context.Context, obj client.Object) []reconcile.Request {
Expand All @@ -331,6 +336,7 @@ func (r *PersistentVolumeClaimReconciler) storageClassEventHandler() handler.Eve
annotationsToWatch := []string{
rsCronJobScheduleTimeAnnotation,
krcJobScheduleTimeAnnotation,
krcJobDisableAnnotation,
}

var requests []reconcile.Request
Expand Down Expand Up @@ -717,6 +723,62 @@ func (r *PersistentVolumeClaimReconciler) findChildEncryptionKeyRotationCronJob(
return activeJob, nil
}

// checkDisabledAnnotation checks if the annotation is set in the
// PVC, namespace or the storage class. It returns true if the
// annotation is set to `false`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a confusing description if someone does not know the disabled annotation ends with /enabled. Please read it back to yourself, while you forgot the actual annotation key.

func (r *PersistentVolumeClaimReconciler) checkDisabledAnnotation(
ctx context.Context,
logger *logr.Logger,
pvc *corev1.PersistentVolumeClaim,
annotation string,
) (bool, error) {
isFalsy := func(val string) bool {
return strings.ToLower(val) == "false"
}
// Check PVC for the annotation
val, ok := pvc.GetAnnotations()[annotation]
if ok {
return isFalsy(val), nil
}

// Check Namespace
ns := &corev1.Namespace{}
err := r.Client.Get(ctx, types.NamespacedName{Name: pvc.Namespace}, ns)
if err != nil {
logger.Error(err, "Failed to get Namespace", "Namespace", pvc.Namespace)
return false, err
}
val, ok = ns.GetAnnotations()[annotation]
if ok {
return isFalsy(val), nil
}

// Static PVs
if len(*pvc.Spec.StorageClassName) == 0 {
return false, nil
}

// Check StorageClass
sc := &storagev1.StorageClass{}
err = r.Client.Get(ctx, types.NamespacedName{Name: *pvc.Spec.StorageClassName}, sc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StorageClasses are not namespaced, is there an alternative to types.NamespacedName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can use client.ObjectKeyFromObject or client.ObjectKey instead.

if err != nil {
if apierrors.IsNotFound(err) {
logger.Error(err, "StorageClass not found", "StorageClass", *pvc.Spec.StorageClassName)
return false, err
}

logger.Error(err, "Failed to get StorageClass", "StorageClass", *pvc.Spec.StorageClassName)
return false, err
}
val, ok = sc.GetAnnotations()[annotation]
if ok {
return isFalsy(val), nil
}

// Not disabled, not an error
return false, nil
}

// processKeyRotation reconciles EncryptionKeyRotation based on annotations
func (r *PersistentVolumeClaimReconciler) processKeyRotation(
ctx context.Context,
Expand All @@ -733,6 +795,27 @@ func (r *PersistentVolumeClaimReconciler) processKeyRotation(
*logger = logger.WithValues("EncryptionKeyrotationCronJobName", krcJob.Name)
}

// Check if key rotation disable annotation is present
disabled, err := r.checkDisabledAnnotation(ctx, logger, pvc, krcJobDisableAnnotation)
if err != nil {
return err
}

if disabled {
if krcJob != nil {
err = r.Delete(ctx, krcJob)
if client.IgnoreNotFound(err) != nil {
errMsg := "failed to delete child encryptionkeyrotationcronjob"

logger.Error(err, errMsg)
return fmt.Errorf("%s: %w", errMsg, err)
}
}

logger.Info("key rotation is disabled, exiting reconcile")
return nil
}

// Determine schedule
sched, err := r.determineScheduleAndRequeue(ctx, logger, pvc, pv.Spec.CSI.Driver, krcJobScheduleTimeAnnotation)
if errors.Is(err, ErrScheduleNotFound) {
Expand Down
Loading