helm: support encryption config in ceph-csi-cephfs chart #4531

acolombier · 2024-04-01T23:11:09Z

This PR adds encryption configuration to the cephfs-csi chart, similarly to the RBD one,

This fixes permission missing when using the kubernetes KMS

Related issues

Fixes: ##4470

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
[?] Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
[-] Unit tests have been added, if necessary.
[-] Integration tests have been added, if necessary.

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 unrelated
failure (please report the failure too!)

acolombier · 2024-04-01T23:11:38Z

Let me know if you would need me to update the Pending release notes for this PR

nixpanic

minor changes needed to pass CI/yamllint

nixpanic · 2024-04-09T11:53:24Z

charts/ceph-csi-cephfs/values.yaml

@@ -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


this line seems to be too long (ci job)

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?

Adding a lint silencer for that line is fine. Breaking the URL over multiple lines isn't that clean.

This line directly above the URL should work:

# yamllint disable-line rule:line-length

charts/ceph-csi-cephfs/values.yaml

nixpanic · 2024-04-16T15:46:44Z

yamllint passed, now MarkDown linting needs to be fixed as well...

acolombier · 2024-04-17T08:49:38Z

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?

nixpanic · 2024-04-17T15:42:57Z

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 make containerized-test to check if everything is OK.

Madhu-1 · 2024-04-18T12:49:38Z

charts/ceph-csi-cephfs/templates/nodeplugin-clusterrole.yaml

@@ -3,6 +3,7 @@ kind: ClusterRole
 apiVersion: rbac.authorization.k8s.io/v1
 metadata:
  name: {{ include "ceph-csi-cephfs.nodeplugin.fullname" . }}
+  namespace: {{ .Release.Namespace }}


we dont need namespace for clusterRole

Madhu-1 · 2024-04-18T12:52:13Z

charts/ceph-csi-cephfs/templates/nodeplugin-role.yaml

@@ -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


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?

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

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.

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.

Happy to scratch that optimisation, and create RBAC myself! But just thought it would be a harmless optimisation to put in

I think the optimization is nice to have, specially if users (like yourself) will benefit from it. Maybe document it, or explain it in a comment, and add it to the rbd chart too?

No problem, I will put some comments in the README. I'll try to see if I can add it the the rbd too, is it okay if I do so in another PR tho? I seem to recall than encryptionKMSConfig doesn't work exactly the same and so it might be best to split the changes if that's okay with you?

Thanks! Yes, a 2nd PR is fine for the RBD enhancement.

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.

Pull request has been modified.

Madhu-1

LGTM, Thanks

Pull request has been modified.

acolombier · 2024-05-15T12:01:42Z

Not sure what is going on with commitlint - is that something that needs to be fixed on my end? Do you want me to merge devel?

Edit my commit to fix mdlint - sorry @Madhu-1

Madhu-1 · 2024-05-15T12:06:01Z

@acolombier can you please rebase the PR on top of devel branch?

acolombier · 2024-05-15T12:06:57Z

Done @Madhu-1

Madhu-1 · 2024-05-15T12:10:01Z

@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 <hi@acolombier.dev>

this allows the encryption KMS config to be granted secret access with a least privilges policy. Signed-off-by: Antoine C <hi@acolombier.dev>

mergify · 2024-05-15T12:10:19Z

rebase

✅ Branch has been successfully rebased

mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Apr 1, 2024

acolombier force-pushed the feat/native-encryption-support branch 2 times, most recently from 80ba8b1 to 1dd0f21 Compare April 1, 2024 23:16

nixpanic reviewed Apr 9, 2024

View reviewed changes

acolombier force-pushed the feat/native-encryption-support branch from d8ff294 to 2f39774 Compare April 14, 2024 12:14

nixpanic requested a review from iPraveenParihar April 15, 2024 11:11

acolombier force-pushed the feat/native-encryption-support branch from 2f39774 to 923b160 Compare April 15, 2024 21:08

nixpanic added the enhancement New feature or request label Apr 16, 2024

nixpanic requested a review from a team April 16, 2024 07:26

acolombier force-pushed the feat/native-encryption-support branch from 923b160 to b8bba0d Compare April 17, 2024 08:48

nixpanic previously approved these changes Apr 18, 2024

View reviewed changes

nixpanic requested a review from a team April 18, 2024 07:05

Madhu-1 requested changes Apr 18, 2024

View reviewed changes

acolombier requested review from Madhu-1 and nixpanic May 15, 2024 11:27

Madhu-1 previously approved these changes May 15, 2024

View reviewed changes

Madhu-1 requested a review from Rakshith-R May 15, 2024 11:55

acolombier force-pushed the feat/native-encryption-support branch from 58ac9c4 to 7849b15 Compare May 15, 2024 12:00

acolombier requested a review from Madhu-1 May 15, 2024 12:01

acolombier force-pushed the feat/native-encryption-support branch from 7849b15 to 4fa3d47 Compare May 15, 2024 12:06

acolombier added 2 commits May 15, 2024 12:10

helm: support encryption config in ceph-csi-cephfs chart

3a19452

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 <hi@acolombier.dev>

helm: add least privileges logic for secrets on ceph-csi-cephfs chart

f78e8ee

this allows the encryption KMS config to be granted secret access with a least privilges policy. Signed-off-by: Antoine C <hi@acolombier.dev>

Madhu-1 force-pushed the feat/native-encryption-support branch from 4fa3d47 to f78e8ee Compare May 15, 2024 12:10

Madhu-1 approved these changes May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: support encryption config in ceph-csi-cephfs chart #4531

helm: support encryption config in ceph-csi-cephfs chart #4531

acolombier commented Apr 1, 2024

acolombier commented Apr 1, 2024

nixpanic left a comment

nixpanic Apr 9, 2024

acolombier Apr 14, 2024

nixpanic Apr 15, 2024

nixpanic Apr 15, 2024

nixpanic commented Apr 16, 2024

acolombier commented Apr 17, 2024 •

edited

nixpanic commented Apr 17, 2024

Madhu-1 Apr 18, 2024

Madhu-1 Apr 18, 2024

acolombier Apr 18, 2024

nixpanic Apr 18, 2024

acolombier Apr 18, 2024 •

edited

acolombier Apr 18, 2024

nixpanic Apr 19, 2024

acolombier Apr 19, 2024

nixpanic Apr 19, 2024

acolombier May 15, 2024

Madhu-1 left a comment

acolombier commented May 15, 2024 •

edited

Madhu-1 commented May 15, 2024

acolombier commented May 15, 2024

Madhu-1 commented May 15, 2024

mergify bot commented May 15, 2024

helm: support encryption config in ceph-csi-cephfs chart #4531

Are you sure you want to change the base?

helm: support encryption config in ceph-csi-cephfs chart #4531

Conversation

acolombier commented Apr 1, 2024

Related issues

acolombier commented Apr 1, 2024

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic commented Apr 16, 2024

acolombier commented Apr 17, 2024 • edited

nixpanic commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

acolombier commented May 15, 2024 • edited

Madhu-1 commented May 15, 2024

acolombier commented May 15, 2024

Madhu-1 commented May 15, 2024

mergify bot commented May 15, 2024

✅ Branch has been successfully rebased

acolombier commented Apr 17, 2024 •

edited

acolombier Apr 18, 2024 •

edited

acolombier commented May 15, 2024 •

edited