controllers: add option to disable key rotation #249
Conversation
This PR allows an user to annotate the storageclaim with `keyrotation.csiaddons.openshift.io/enable=false` to disable key rotation for the RBD storageclasses. Signed-off-by: Niraj Yadav <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: black-dragon74 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -397,6 +398,10 @@ func (r *StorageClaimReconciler) reconcilePhases() (reconcile.Result, error) { | |||
| storageClass = r.getCephFSStorageClass(data) | |||
| } else if resource.Name == "ceph-rbd" { | |||
| storageClass = r.getCephRBDStorageClass(data) | |||
|
|
|||
| if enable, ok := r.storageClaim.GetAnnotations()[keyRotationEnableAnnotation]; ok && enable != "true" { | |||
There was a problem hiding this comment.
does this ann. value supports anything other than true/false? why to check for a negation, like why not ann[key] == "false" ? set-false
if the value is "true"/"false" then it can be directly used, isn't it?
utils.AddAnnotation(storageClass, keyRotationEnableAnnotation, r.storageClaim.GetAnnotations()[keyRotationEnableAnnotation])
There was a problem hiding this comment.
No it does not support any other value than true/false, values other than true are considered false. I have no good reason on why it cannot be used directly (as it will be parsed in csi-addons anyways)
The reason I did not do it such way is to avoid setting it unconditionally because in utils.AddAnnotation we set the value even if it is empty. Want me to change it?
Also, while we are at it, should I update the code to update the SC by patching it instead of deleting and recreating in case the annotations differ? Would be a little efficient IG?
There was a problem hiding this comment.
Want me to change it?
- No, annotation w/ an empty value is also a valid identifier, let's not change it
Also, while we are at it, should I update the code to update the SC by patching it instead of deleting and recreating in case the annotations differ? Would be a little efficient IG?
- these values converge easily in a couple of reconciles and a patch here makes less difference.
- Nevertheless will revisit when VolumeAttributes (Madhu mentioned it once) feature which helps in storageclass having almost static data.
There was a problem hiding this comment.
- No, annotation w/ an empty value is also a valid identifier, let's not change it
I meant changing the current pr aka my change-set to add annotation unconditionally :)
I am still leaning towards adding the annotation only when its value is not truthy.
There was a problem hiding this comment.
- these values converge easily in a couple of reconciles and a patch here makes less difference.
Would you please explain it to me? Acc to my understanding we would be deleting and recreating the SC in case annotations differ. That is two different actions while patching the annotations on an existing SC will not disrupt any operations and is one shot.
There was a problem hiding this comment.
in case annotations differ
- yes, I mean these doesn't change often, isn't it.
|
/hold |
|
@black-dragon74 In addition, we are phasing out the StorageClaim CR as a concept, so we should not change/add any new nob on that CR |
@nb-ohad, Initially, for the ReclaimSpace disable option we planned to implement on the StorageRequest CR, but IIRC from our last meeting, we agreed to use annotations on the StorageClaim CR for now because we don't have mechanism to pass annotations from the StorageRequest CR to the StorageClass created in the client cluster. cc @Madhu-1 |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This PR allows an user to annotate the storageclaim with
keyrotation.csiaddons.openshift.io/enable=falseto disable key rotation for the RBD storageclasses.