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

Implement EncryptionKeyRotation spec #4690

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Implement EncryptionKeyRotation spec #4690

merged 1 commit into from
Jul 19, 2024

Conversation

black-dragon74
Copy link
Member

@black-dragon74 black-dragon74 commented Jun 21, 2024 •
edited
Loading

This patch implements the EncryptionKeyRotation spec for ceph-csi

@black-dragon74 black-dragon74 marked this pull request as draft June 21, 2024 10:27
nixpanic
nixpanic reviewed Jun 25, 2024
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Just added a few small comments. Also try to create unit-tests for the helper functions you introduce.

internal/util/file.go Outdated Show resolved Hide resolved
internal/rbd/rbd_util.go Outdated Show resolved Hide resolved
internal/rbd/rbd_util.go Outdated Show resolved Hide resolved
@black-dragon74
Copy link
Member Author

Note: The CI failures are expected until: csi-addons/spec#65 is merged.

@black-dragon74 black-dragon74 changed the title [WIP] Implement EncryptionKeyRotation spec Implement EncryptionKeyRotation spec Jun 26, 2024
@black-dragon74 black-dragon74 marked this pull request as ready for review June 26, 2024 11:33
@black-dragon74
Copy link
Member Author

I’ll take care of the failures since the linked PR is merged :)

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 27, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@iPraveenParihar
Copy link
Contributor

@black-dragon74, please follow these conventions for the commit messages.

@black-dragon74
Copy link
Member Author

@black-dragon74, please follow these conventions for the commit messages.

Thanks Praveen. I'll be squashing all the commits into one before merging.

Rakshith-R
Rakshith-R reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

  • Add shared volumelocks with nodeserver and acquire it before proceeding with rotation ?
  • Block rotation for RWX volumes ?
  • Add testing tool at csi addons and paste manual testing logs or add e2e here to test the procedure.

@black-dragon74
Copy link
Member Author

  • Add shared volumelocks with nodeserver and acquire it before proceeding with rotation ?

We are sending the request to only one node. Is volume lock required in this case?

  • Block rotation for RWX volumes ?

Totally missed it, will do :)

  • Add testing tool at csi addons and paste manual testing logs or add e2e here to test the procedure.

WIP

@Rakshith-R
Copy link
Contributor

  • Add shared volumelocks with nodeserver and acquire it before proceeding with rotation ?

We are sending the request to only one node. Is volume lock required in this case?

To prevent nodeunstage when rotating keys.

  • Block rotation for RWX volumes ?

Totally missed it, will do :)

  • Add testing tool at csi addons and paste manual testing logs or add e2e here to test the procedure.

WIP

Rakshith-R
Rakshith-R reviewed Jul 2, 2024
View reviewed changes
internal/csi-addons/rbd/encryptionkeyrotation.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Outdated Show resolved Hide resolved
@black-dragon74 black-dragon74 force-pushed the pvkr-feat branch 2 times, most recently from 384957c to a2817b9 Compare July 4, 2024 14:49
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 10, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@black-dragon74 black-dragon74 force-pushed the pvkr-feat branch 2 times, most recently from c4ceeaa to 3918c91 Compare July 11, 2024 13:53
@nixpanic
Copy link
Member

How does this relate to #4655 ?

@black-dragon74
Copy link
Member Author

How does this relate to #4655 ?

The linked PR is documentation for this PR.

@black-dragon74 black-dragon74 force-pushed the pvkr-feat branch 2 times, most recently from e3b35b4 to d170da6 Compare July 12, 2024 09:57
Madhu-1
Madhu-1 reviewed Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

nits

internal/rbd/encryption.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Outdated Show resolved Hide resolved
@nixpanic
Copy link
Member

This can be queued for merging once #4655 is merged:

  1. use @mergifyio rebase to rebase the PR on latest devel branch
  2. wait until the Mergify bot has rebased the PR
  3. use @mergifyio queue to trigger CI jobs and get it merged

@nixpanic
Copy link
Member

@Mergifyio rebase

@black-dragon74 @nixpanic
rbd: implement pv key rotation
9f5780e 
This patch implements the EncryptionKeyRotation spec for ceph-csi

Signed-off-by: Niraj Yadav <[email protected]>
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 19, 2024

rebase

✅ Branch has been successfully rebased

@nixpanic
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 19, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jul 19, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 19, 2024
@black-dragon74
Copy link
Member Author

/test ci/centos/mini-e2e-helm/k8s-1.30

@mergify mergify bot merged commit ebc5688 into ceph:devel Jul 19, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rakshith-R Rakshith-R Rakshith-R left review comments

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@nixpanic nixpanic nixpanic approved these changes

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@black-dragon74 @iPraveenParihar @Rakshith-R @nixpanic @ceph-csi-bot @Madhu-1