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.

Sign up for GitHub

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

Jump to bottom

controllers: add option to disable key rotation #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

controllers: add option to disable key rotation #249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions internal/controller/storageclaim_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ import (
)

const (
storageClaimFinalizer = "storageclaim.ocs.openshift.io"
storageClaimAnnotation = "ocs.openshift.io/storageclaim"
keyRotationAnnotation = "keyrotation.csiaddons.openshift.io/schedule"
storageClaimFinalizer = "storageclaim.ocs.openshift.io"
storageClaimAnnotation = "ocs.openshift.io/storageclaim"
keyRotationAnnotation = "keyrotation.csiaddons.openshift.io/schedule"
keyRotationEnableAnnotation = "keyrotation.csiaddons.openshift.io/enable"

pvClusterIDIndexName = "index:persistentVolumeClusterID"
vscClusterIDIndexName = "index:volumeSnapshotContentCSIDriver"
Expand Down Expand Up @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

in case annotations differ

  • yes, I mean these doesn't change often, isn't it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

utils.AddAnnotation(storageClass, keyRotationEnableAnnotation, "false")
}
}
utils.AddAnnotation(storageClass, storageClaimAnnotation, r.storageClaim.Name)
err = r.createOrReplaceStorageClass(storageClass)
Expand Down Expand Up @@ -584,7 +589,8 @@ func (r *StorageClaimReconciler) createOrReplaceStorageClass(storageClass *stora
}

// If present then compare the existing StorageClass with the received StorageClass, and only proceed if they differ.
if reflect.DeepEqual(existing.Parameters, storageClass.Parameters) {
if reflect.DeepEqual(existing.Parameters, storageClass.Parameters) &&
reflect.DeepEqual(existing.Annotations, storageClass.Annotations) {
return nil
}

Expand Down
Loading