charts icon indicating copy to clipboard operation
charts copied to clipboard

Permit use of Secrets instead of inline text for iSCSI authentication

Open ndrwstn opened this issue 10 months ago • 4 comments
trafficstars

This patch creates a new key under storageClasses for existingSecret where for each of the secrets can be provided directly (name and namepace) instead of managed by the chart. E.g.:

storageClasses:
	- name: iscsi-delete
		defaultClass: true
		reclaimPolicy: Delete
		volumeBindingMode: WaitForFirstConsumer
    allowVolumeExpansion: true
    parameters:
      fstype: ext4
    existingSecrets:
    	node-stage-secret:
    		name: node-stage-secret-dcsi-iscsi-delete
    		namespace: storage

The secret must be contain validly formatted keys. E.g.:

apiVersion: v1
kind: Secret
metadata:
 name: node-stage-secret-dcsi-iscsi-delete
 # namespace: storage
type: Opaque
stringData:
 node-db.node.session.auth.authmethod: "CHAP"
 node-db.node.session.auth.username: "<ISCSI_USERNAME>"
 node-db.node.session.auth.password: "<ISCSI_PASSWORD>"
 node-db.node.session.auth.username_in: "<ISCSI_USERNAME_IN>"
 node-db.node.session.auth.password_in: "<ISCSI_PASSWORD_IN>"

I chose to implement this as "all or nothing" regarding secrets management, with the belief that if someone is already in the weeds for one of the secrets, they would likely manually define all. So if the existingSecrets value exists (for a specific StorageClass), you must define all of the secrets that are necessary for that class. (Though some StorageClasses might use existingSecrets and others could use secrets--though I don't know why anyone would do that.)

I made this patch as I wasn't able to get any sort of secret injection to properly work on the existing implementation, and found several people referencing that they just encrypt the plain-text credentials in place. That seemed inelegant to me, given there is an entire secrets ecosystem in k8s (and I prefer to use ExternalSecrets to manage as much as possible.)

I'd be happy to write an update to the values file to reflect these changes if you find this PR suitable for inclusion.

ndrwstn avatar Jan 10 '25 17:01 ndrwstn

Hi! I am generally ok with this yeah, let’s get it documented in the values file and then I can pull it in and release with the next version.

travisghansen avatar Apr 04 '25 04:04 travisghansen

I will put together some documentation when I have a bit of free time. It's a little sparse at the moment, so probably next week unless I end up super motivated.

ndrwstn avatar Apr 04 '25 14:04 ndrwstn

Sounds good. I'm a couple week out probably from snapping a new release of the app and chart both anyhow so no big rush.

travisghansen avatar Apr 04 '25 15:04 travisghansen

I added documentation in values.yaml and in the template example for freenas-nfs.yaml. Let me know if you want more depth or if its not clear.

ndrwstn avatar Apr 13 '25 20:04 ndrwstn

@travisghansen Wondering if you need anything else for this PR or if it's ready to be merged? Thanks.

ndrwstn avatar May 25 '25 03:05 ndrwstn

I will review and merge in the next week or so, thanks for the patience!

travisghansen avatar May 25 '25 12:05 travisghansen