rbd: Allow user to disable key rotation #2817
Conversation
| @@ -281,6 +283,7 @@ func newCephBlockPoolStorageClassConfiguration(initData *ocsv1.StorageCluster) S | |||
| persistentVolumeReclaimDelete := corev1.PersistentVolumeReclaimDelete | |||
| allowVolumeExpansion := true | |||
| managementSpec := initData.Spec.ManagedResources.CephBlockPools | |||
| disableKeyRotation := !util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation) | |||
There was a problem hiding this comment.
I believe we should use the variable name 'isKeyRotationEnabled'. What do you think?
There was a problem hiding this comment.
I decided to go with disableKeyRotation as it conveys the intent more clearly. If we go with enable we will have to reverse the checks.
The goal is to add the annotation on SC only when KR is disabled.
WDYT?
There was a problem hiding this comment.
What I mean here is something like this:
isKeyRotationEnabled := util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation)
Later you do the logic based on this var,
The main reason to use the value as is in the variable and perform the logic accordingly. Both ways are the same, but it's your choice which one you prefer.
| @@ -367,6 +374,10 @@ func newNonResilientCephBlockPoolStorageClassConfiguration(initData *ocsv1.Stora | |||
| }, | |||
| isClusterExternal: initData.Spec.ExternalStorage.Enable, | |||
| } | |||
| if disableKeyRotation { | |||
There was a problem hiding this comment.
The same piece of code is in 2 places. I think we can refactor it into a single method and reuse it. it.
openshift-merge-robot
added
the
needs-rebase
black-dragon74
force-pushed
the
kr-disable-rbd
branch
from
e1b9a4c
to
59c091c
Compare
openshift-merge-robot
removed
the
needs-rebase
black-dragon74
force-pushed
the
kr-disable-rbd
branch
from
59c091c
to
288a026
Compare
| @@ -27,6 +27,8 @@ const ( | |||
|
|
|||
| //storage class driver name prefix | |||
| storageclassDriverNamePrefix = "openshift-storage" | |||
|
|
|||
There was a problem hiding this comment.
No need for a separating line, both const are private.
| util.If(!util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation), func() { | ||
| util.AddAnnotation(scc.storageClass, keyRotationEnableAnnotation, "false") | ||
| }) |
There was a problem hiding this comment.
Why use a util that invokes a function for a simple if?
| util.If(!util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation), func() { | |
| util.AddAnnotation(scc.storageClass, keyRotationEnableAnnotation, "false") | |
| }) | |
| if !util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation) { | |
| util.AddAnnotation(scc.storageClass, keyRotationEnableAnnotation, "false") | |
| } |
There was a problem hiding this comment.
This is exactly how it was initially. It was suggested to have a util to prevent duplication in an earlier review so I added it :D
| @@ -314,6 +316,9 @@ func newCephBlockPoolStorageClassConfiguration(initData *ocsv1.StorageCluster) S | |||
| if initData.Spec.ManagedResources.CephBlockPools.DefaultStorageClass { | |||
| scc.storageClass.Annotations[defaultStorageClassAnnotation] = "true" | |||
| } | |||
| util.If(!util.IsAnnotationTruthy(initData, keyRotationEnableAnnotation), func() { | |||
| util.AddAnnotation(scc.storageClass, keyRotationEnableAnnotation, "false") | |||
| }) | |||
| return scc | |||
There was a problem hiding this comment.
Don't we need to issue an update comment for the storagecluster to apply the new annotation?
There was a problem hiding this comment.
newCephBlockPoolStorageClassConfiguration generates config for a new SC that is going to be created. Upon creation, it will have the annotation present.
controllers/util/util.go
Outdated
| annotations := obj.GetAnnotations() | ||
|
|
||
| if val, found := annotations[key]; found { | ||
| return strings.ToLower(val) == "true" | ||
| } | ||
| return false |
There was a problem hiding this comment.
The entire function just equal to
return obj.GetAnnotations()(key) == "true"
And it that case we don't need a util at all, just use the above line in the call sites
controllers/util/util.go
Outdated
| // Execute the provided function if the condition is true. | ||
| func If(cond bool, fn func()) { | ||
| if cond { | ||
| fn() | ||
| } | ||
| } |
There was a problem hiding this comment.
This util does not make sense. What would be a good reason to call a function and pass another function into it instead of just putting a normal if in place?
Usually one creates an IfElse utils for cases where you want to treat the if as an expression (not as a statement). In that case the implementation will be something like
func IfElse[T any](cond bool, trueVal, falseVal T) T {
if cond {
return trueVal
} else {
return falseVal
}
}
To enable you doing stuff like:
instance := someStruct {
...
b: util.IfElse(someCond, Val1, val2),
...
}
But this is not the case here and the implementation above does not provide any value.
Please remove it
There was a problem hiding this comment.
This util does not make sense.
Couldn't agree more :)
This patch allows user to disable automatic key rotation by annotating StorageCluster with `keyrotation.csiaddons.openshift.io/enable=false` Signed-off-by: Niraj Yadav <niryadav@redhat.com>
black-dragon74
force-pushed
the
kr-disable-rbd
branch
from
288a026
to
cde359c
Compare
|
[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 |
This patch allows user to disable automatic
key rotation by annotating StorageCluster
with
keyrotation.csiaddons.openshift.io/enable=false