search

openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

document ClusterRole Secret access #273

Closed sepich closed 7 months ago

sepich commented 7 months ago

Describe the problem/challenge you have We are investigating migration to your project from metal-stack/csi-driver-lvm. Unfortunately with more features it seems also comes more complexity, and we cannot find reasons for that:

  1. Most important question is why do you need permission to read all cluster Secrets? https://github.com/openebs/lvm-localpv/blob/db17d08700266368d9de18904a6bef74ef2c4dff/deploy/helm/charts/templates/rbac.yaml#L18 We cannot find any mentions for this in docs, and in go code

  2. Another thing which seems poorly documented is why do you need custom CRDs? Putting aside snapshots, which we do not use. What kind of problem do you solve with lvmnodes.local.openebs.io and lvmvolumes.local.openebs.io? Why do you need separate controller and state, which can just be stored directly on nodes.

Describe the solution you'd like It is nice you have ability to disable crd.volumeSnapshot in your helm chart. Would be nice to also have ability to disable Secrets permissions and the rest of CRD, and clearly understand what would it break.

Thank you.

abhilashshetty04 commented 7 months ago

Hi @sepich , Thanks for raising the issue.

  1. This was the design decision in the zfs local pv as well. By having resources like lvmvolume and lvmnode as CR we make sure: a. We keep node and controller component completely stateless. Rely on k8s etcd to store the resource objects. b. We dont have to have gRPC CRUD interface between controller and node. Since its controller driven, core storage operations are driven by CR state. c. This makes lvm-localpv completely k8s native in a way. users can investigate storage resource with kubectl plugin.

  2. We already have the support to disable snapshot cr install. Part of following PR https://github.com/openebs/lvm-localpv/pull/243 https://github.com/openebs/lvm-localpv/blob/develop/deploy/helm/charts/values.yaml#L171C20-L171C20

abhilashshetty04 commented 7 months ago

For the rbac query, We will raise a PR to remove the rule. Do you want to discuss 2nd and 3rd query? Were you able to disable snapshot crd?

abhilashshetty04 commented 7 months ago

Hi @sepich , We have removed rbac rule for secrets in https://github.com/openebs/lvm-localpv/pull/277. We will proceed with closure of this issue for now. Please let us know incase you have more questions.

sepich commented 7 months ago

You only removed Secrets from the first ClusterRole, but they are still in the second: https://github.com/openebs/lvm-localpv/blob/c6dbe6a87094f77b8884f652f1a80a5be82e57e9/deploy/helm/charts/templates/rbac.yaml#L86 And why does CSI driver needs permission to Services? https://github.com/openebs/lvm-localpv/blob/c6dbe6a87094f77b8884f652f1a80a5be82e57e9/deploy/helm/charts/templates/rbac.yaml#L21

abhilashshetty04 commented 7 months ago

@sepich , Thanks for replying. We will look into these rules. Would you mind raising a PR with all changes that you are suggesting.

Moreover, Have you considered using Mayastor for your storage deployment. Its more actively developed. Its faster , Completely written in rust, Uses faster Nvme Tcp stack.

You could find more information here if you are interested. https://mayastor.gitbook.io/introduction/