-
Notifications
You must be signed in to change notification settings - Fork 536
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.
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
helm: support encryption config in ceph-csi-cephfs chart #4531
base: devel
Are you sure you want to change the base?
Conversation
Let me know if you would need me to update the |
80ba8b1
to
1dd0f21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor changes needed to pass CI/yamllint
@@ -30,6 +30,19 @@ serviceAccounts: | |||
# netNamespaceFilePath: "{{ .kubeletDir }}/plugins/{{ .driverName }}/net" | |||
csiConfig: [] | |||
|
|||
# Configuration for the encryption KMS | |||
# Ref: https://github.com/ceph/ceph-csi/blob/devel/docs/deploy-cephfs.md#cephfs-volume-encryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line seems to be too long (ci job)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay for me to break the link into two lines? Or perhaps I should add lint silencer for this line? What is the suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a lint silencer for that line is fine. Breaking the URL over multiple lines isn't that clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line directly above the URL should work:
# yamllint disable-line rule:line-length
d8ff294
to
2f39774
Compare
2f39774
to
923b160
Compare
923b160
to
b8bba0d
Compare
Apologies for missing this previously @nixpanic . Is it expected that the pre-commit check doesn't run any of the checkers? Or is my local setup flawed? |
I don't think pre-commit runs all the checks. I usually run |
@@ -3,6 +3,7 @@ kind: ClusterRole | |||
apiVersion: rbac.authorization.k8s.io/v1 | |||
metadata: | |||
name: {{ include "ceph-csi-cephfs.nodeplugin.fullname" . }} | |||
namespace: {{ .Release.Namespace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need namespace for clusterRole
@@ -0,0 +1,21 @@ | |||
{{- if .Values.rbac.create -}} | |||
{{- if and .Values.encryptionKMSConfig .Values.encryptionKMSConfig.secretNamespace .Values.encryptionKMSConfig.secretName (eq .Values.encryptionKMSConfig.secretNamespace .Release.Namespace) -}} | |||
kind: Role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need Role and Role Binding right? we need to have access to secrets in different namespaces as well. i don't see Role https://github.com/ceph/ceph-csi/tree/devel/deploy/cephfs/kubernetes, am i missing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to downgrade the ClusterRole
to just a Role
in case the configured secret in encryptionKMSConfig
was local to the namespace. This was done to help with least privileged and limit the access to secret as much as possible. Happy to remove if you think this is overkill. Arguably, users who want least privileged roles can create manage the RBAC themselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ceph-CSI components which reads the Secrets are usually running in a different namespace than the namespace where Secrets for applications have their encryption key. Ceph-CSI needs to read the Secret from a different namespace than where it is running/mounting the PV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it will create the ClusterRole
automatically. The logic at line 2 is if encryptionKMSConfig.secretNamespace == Release.Namespace
, then create a Role
instead of adding secret:read
to the ClusterRole
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to scratch that optimisation, and create RBAC myself! But just thought it would be a harmless optimisation to put in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have slightly reworked the logic behind it to enforce determinism and added a some documentation in the REAMDE.md
. I'd be nice if you could tell me how that looks, and if you like it, I can spin up a PR with the same feature for RBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic it'd be great if you could let me know your though on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acolombier @nixpanic is on PTO will be back 1st week of Jun, do you want me to block this PR until he is back to confirm on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you! If it is ready to be merged, happy to do so too, and I'll wait and see if @nixpanic likes this approach for RBD and I can make the PR then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic friendly ping in case you missed the notification :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks
58ac9c4
to
7849b15
Compare
Not sure what is going on with Edit my commit to fix mdlint - sorry @Madhu-1 |
@acolombier can you please rebase the PR on top of devel branch? |
7849b15
to
4fa3d47
Compare
Done @Madhu-1 |
@Mergifyio rebase |
this chart currently lack the ability to properly configure encryption, as well as granting sufficent permission to allow controllers to access secret when needed. Signed-off-by: Antoine C <[email protected]>
this allows the encryption KMS config to be granted secret access with a least privilges policy. Signed-off-by: Antoine C <[email protected]>
4fa3d47
to
f78e8ee
Compare
✅ Branch has been successfully rebased |
This PR adds encryption configuration to the
cephfs-csi
chart, similarly to the RBD one,This fixes permission missing when using the
kubernetes
KMSRelated issues
Fixes: ##4470
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>
: retest the<job-name>
after unrelatedfailure (please report the failure too!)