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.

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

The Helm chart and the encr-keys volume #477

Closed
dhess opened this issue Sep 19, 2023 · 4 comments · Fixed by #487
Closed

The Helm chart and the encr-keys volume #477

dhess opened this issue Sep 19, 2023 · 4 comments · Fixed by #487

Comments

@dhess
Copy link

dhess commented Sep 19, 2023

Thanks for this project! It appears to be quite comprehensive and well thought-out.

It's possible I'm missing something, but as far as I can tell, this project's Helm chart creates a node DaemonSet that always mounts an encr-keys volume, always uses a hostPath with path /home/keys to back this volume, and provides no way to override this aspect of the configuration.

On Talos systems, at least, /home is read-only, and even if /home/keys were bind-mounted to a writable hostPath, it would likely be ephemeral, as by default, Talos doesn't preserve the contents of the OS volume on upgrades. But even on Kubernetes nodes with writable, persistent hostPaths, personally, I would never store encryption keys this way on a Kubernetes node for any number of reasons.

The Helm chart should make it possible to do any of the following:

  • change the encr-keys volume hostPath from the default /home/keys;
  • back the encr-keys volume with a Kubernetes secret, rather than a hostPath; or
  • elide the encr-keys volume altogether, if the admin has no plans to use encryption
@hrudaya21
Copy link
Contributor

@dhess You can try changing the path in the HelmChart to your custom directory as below.
https://github.com/openebs/zfs-localpv/blob/develop/deploy/helm/charts/templates/zfs-node.yaml#L120

@dhess
Copy link
Author

dhess commented Oct 27, 2023

@hrudaya21 Sorry, I'm not sure I follow. You've pointed to the chart's template, but the template provides no way to override or otherwise change the default /home/keys path, nor the DirectoryOrCreate type. If I try to override it by re-specifying the encr-keys volume in zfsNode.additionalVolumes, then Helm errors out, presumably because it can't be given twice in the volumes list.

Are you suggesting that I edit the template and then apply my own version of the chart?

@hrudaya21
Copy link
Contributor

hrudaya21 commented Nov 1, 2023

@dhess Yes, that is one the option to do if you are doing the local installation. But I will recommend to have the dynamic value in value.yaml which will be read in the template. If possible please contribute to the repo to do this changes.

trunet added a commit to trunet/zfs-localpv that referenced this issue Nov 15, 2023
Helm charts was hardcoding zfs encryption keys directory and on some
distributions /home is read-only. This commit will make it possible to
set it as a helm value.

Fixes openebs#477

Signed-off-by: Wagner Sartori Junior <wsartori@wsartori.com>
trunet added a commit to trunet/zfs-localpv that referenced this issue Nov 15, 2023
Helm charts was hardcoding zfs encryption keys directory and on some
distributions /home is read-only. This commit will make it possible to
set it as a helm value.

Fixes openebs#477

Signed-off-by: Wagner Sartori Junior <wsartori@wsartori.com>
@trunet
Copy link
Contributor

trunet commented Nov 15, 2023

I just submitted #487 PR. The default behavior is still hostPath /home/keys, but this adds the possibility to change it.

Should be low risk to merge and release to make possible to use this CSI in talos.

hrudaya21 pushed a commit that referenced this issue Nov 16, 2023
Helm charts was hardcoding zfs encryption keys directory and on some
distributions /home is read-only. This commit will make it possible to
set it as a helm value.

Fixes #477

Signed-off-by: Wagner Sartori Junior <wsartori@wsartori.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants