Add option to disable KeyRotation #659
Open
+85
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mergify
bot
requested review from
iPraveenParihar,
Madhu-1,
nixpanic,
Rakshith-R and
yati1998
TestingSteps❯ oc annotate sc/rook-ceph-block "keyrotation.csiaddons.openshift.io/disable=true"
storageclass.storage.k8s.io/rook-ceph-block annotated
❯ oc get encryptionkeyrotationcronjob
No resources found in rook-ceph namespace.
❯ oc annotate sc/rook-ceph-block "keyrotation.csiaddons.openshift.io/disable-"
storageclass.storage.k8s.io/rook-ceph-block annotated
❯ oc get encryptionkeyrotationcronjob
NAME SCHEDULE SUSPEND ACTIVE LASTSCHEDULE AGE
rbd-pvc-1725353742 @weekly 2sLogs |
Madhu-1
reviewed
Sep 3, 2024
black-dragon74
force-pushed
the
add-disable-keyrotation
branch
from
f0e7c46
to
1f38728
Compare
Madhu-1
reviewed
Sep 16, 2024
Comment on lines
737
to
742
| val, ok := annotations[annotation] | ||
| if ok { | ||
| return strings.ToLower(val) == "true", nil | ||
| } |
There was a problem hiding this comment.
Please add a helper function for it as we have same check in 3 places
| @@ -65,6 +65,7 @@ var ( | |||
| rsCSIAddonsDriverAnnotation = "reclaimspace." + csiaddonsv1alpha1.GroupVersion.Group + "/drivers" | |||
|
|
|||
| krcJobScheduleTimeAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/schedule" | |||
| krcJobDisableAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/disable" | |||
There was a problem hiding this comment.
/enable instead of disable. This is the one we are considering for reclaimspace as well.
Comment on lines
806
to
810
| logger.Error(err, "failed to delete child encryptionkeyrotationcronjob") | ||
|
|
||
| return fmt.Errorf("failed to delete child encryptionkeyrotationcronjob: %w", err) |
There was a problem hiding this comment.
Suggested change
| logger.Error(err, "failed to delete child encryptionkeyrotationcronjob") | |
| return fmt.Errorf("failed to delete child encryptionkeyrotationcronjob: %w", err) | |
| errMsg:=""failed to delete child encryptionkeyrotationcronjob" | |
| logger.Error(err, errMsg) | |
| return fmt.Errorf(%s: %w",errMsg, err) |
black-dragon74
force-pushed
the
add-disable-keyrotation
branch
from
1f38728
to
03c777a
Compare
This patch adds the feature to disable key rotation by annotating either of NS, SC or PVC. The annotation to be used is: `keyrotation.csiaddons.openshift.io/disable=true` Signed-off-by: Niraj Yadav <[email protected]>
black-dragon74
force-pushed
the
add-disable-keyrotation
branch
from
03c777a
to
b457994
Compare
nixpanic
requested changes
Oct 15, 2024
| @@ -65,6 +65,7 @@ var ( | |||
| rsCSIAddonsDriverAnnotation = "reclaimspace." + csiaddonsv1alpha1.GroupVersion.Group + "/drivers" | |||
|
|
|||
| krcJobScheduleTimeAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/schedule" | |||
| krcJobDisableAnnotation = "keyrotation." + csiaddonsv1alpha1.GroupVersion.Group + "/enable" | |||
There was a problem hiding this comment.
The name contains Disable, but the value ends with /enable. This is a little confusing.
There was a problem hiding this comment.
Right. I'll edit the name and logic accordingly.
| @@ -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`. | |||
There was a problem hiding this comment.
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.
|
|
||
| // Check StorageClass | ||
| sc := &storagev1.StorageClass{} | ||
| err = r.Client.Get(ctx, types.NamespacedName{Name: *pvc.Spec.StorageClassName}, sc) |
There was a problem hiding this comment.
StorageClasses are not namespaced, is there an alternative to types.NamespacedName?
There was a problem hiding this comment.
Yes, we can use client.ObjectKeyFromObject or client.ObjectKey instead.
|
Also don't forget to add the new annotation to the documentation! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds the feature to disable key
rotation by annotating any of NS, SC or PVC.
The annotation to be used is:
keyrotation.csiaddons.openshift.io/disable=true