Skip to content

Conversation

ndrwstn
Copy link

@ndrwstn ndrwstn commented Jan 10, 2025

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.

…de Helm creation of any Secrets

- if existingSecrets found, user must provide for properly named and formatted `Secret` for that storage class.
@travisghansen
Copy link
Member

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.

@ndrwstn
Copy link
Author

ndrwstn commented Apr 4, 2025

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.

@travisghansen
Copy link
Member

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.

@ndrwstn
Copy link
Author

ndrwstn commented Apr 13, 2025

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
Copy link
Author

ndrwstn commented May 25, 2025

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

@travisghansen
Copy link
Member

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

@ndrwstn
Copy link
Author

ndrwstn commented Jul 24, 2025

It's almost August?

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 this pull request may close these issues.

2 participants