Open Gabrielopesantos opened 8 months ago
Thanks for handle this @Gabrielopesantos, just left some comments, nice work 💪🏽
Thanks for handle this @Gabrielopesantos, just left some comments, nice work 💪🏽
Thank you for the feedback, I will have a look as soon as I can. :+1:
@Gabrielopesantos could you check CI?
@Gabrielopesantos the CI detect an format issue. Probably an issue related to the {if} {end}
Helm is always a pain when running into this issue :(
My bad and thank you for the patience. I could have easily run helm lint
locally and fixed these issues. Should be good to be reviewed now.
I'm not a fan of my implementation but couldn't think of another way to implement this logic of having precedence of values for the secret (even worse with go templates).
awesome thank you @Gabrielopesantos !
@celian-garcia and/or @nicolastakashi if you have time, could you please take a look ? Thank you !
Hello, thanks for this PR. I am trying to review but it sounds complex to me.
If I understand properly:
- if user give an
encryptionKey
orencryptionKeyFile
it uses it in the config. That part is simple.- Now if the user give an
encryptionKeyFile
it means that we need mount the volume. Two solution : the user has a secret or the user want us to create a secret.- And if a user doesn't provide a file but provide a secret we want the file to be set.
- Also I don't see a case where the encryptionKeyFile would exist without a k8s secret but maybe I'm wrong
Did I forget something? :D
If I try to translate it to a yaml self explanatory, I can draft a proposal that in my feeling would be less confusing for the user and easier to develop on your side. WDYT?
security: encryptionKey: "" encryptionKeyFile: activated: false # this is to ease the configuration if a user want to setup the secret but activate it or not easily secretName: '' # default to perses-full-name encryptionsecret secretKey: 'key' # (setting the default in the values file directly would prevent from forgetting a "| default" in the code)
Note: For the mutually exclusive fields, you could do a check with helm on top of the templates using them. It would save you from some checks.
{{- if and .Values.security.encryptionKey .Values.security.encryptionKeyFile.activated }} {{- fail ( printf "encryptionKey and encryptionKeyFile are mutually exclusive" ) }} {{- end }}
Hey @celian-garcia ,
Thanks for the feedback. Your suggestion simplifies the UX and also the wiring on the templates. Still, unless I misunderstood your suggestion I don't think encryptionKey
and encryptionKeyFile.activated
are mutually exclusive as you commented.
To ensure we are on the same page, the following are the ways I believe were discussed to make the encryption key available to perses.
encryption_key
on the config file;encryption_key_file
set in the config file; encryption_key_file
set in the config file;The following is how each method of setting the encryption key would be configured with your suggested YAML config.
1.
security:
encryptionKey: "asdasdas"
encryptionKeyFile:
activated: false
secretName: ''
secretKey: 'key'
2.
security:
encryptionKey: "asdasdas"
encryptionKeyFile:
activated: true
secretName: ''
secretKey: 'key'
3.
encryptionKey: ""
encryptionKeyFile:
activated: true
secretName: 'secret-asd'
secretKey: 'key'
It works nicely for 1 and 3 and they don't have "mutually" exclusive fields used, however, for 2 encryptionKey
and encryptionKeyFile.activated
are needed at the same time for a new secret to be created.
Also, there's no way to set the path where the encryption key file would be mounted but that's not an issue as a default like /etc/perses/security/encryptionkey
could be used.
Hey @celian-garcia , Thanks for the feedback. Your suggestion simplifies the UX and also the wiring on the templates. Still, unless I misunderstood your suggestion I don't think
encryptionKey
andencryptionKeyFile.activated
are mutually exclusive as you commented.To ensure we are on the same page, the following are the ways I believe were discussed to make the encryption key available to perses.
- Set in
encryption_key
on the config file;- New secret, holding encryption key, mount as a file somewhere in the deployment resource and
encryption_key_file
set in the config file;- Existing secret, holding encryption key, mount as a file somewhere in the deployment resource and
encryption_key_file
set in the config file;The following is how each method of setting the encryption key would be configured with your suggested YAML config.
security: encryptionKey: "asdasdas" encryptionKeyFile: activated: false secretName: '' secretKey: 'key'
security: encryptionKey: "asdasdas" encryptionKeyFile: activated: true secretName: '' secretKey: 'key'
encryptionKey: "" encryptionKeyFile: activated: true secretName: 'secret-asd' secretKey: 'key'
It works nicely for 1 and 3 and they don't have "mutually" exclusive fields used, however, for 2
encryptionKey
andencryptionKeyFile.activated
are needed at the same time for a new secret to be created.Also, there's no way to set the path where the encryption key file would be mounted but that's not an issue as a default like
/etc/perses/security/encryptionkey
could be used.
I'm reading myself again and I don't even know what I meant :smile: It's been a while. Now reading myself I'm challenging the existence of encryptionKey field. It should always use a secret no?
I would even do now
encryptionKey:
secret: '' // Default value is {{perses full name}}
useExistingSecret: false // by default it creates a secret
key: 'key' // Default value that you've no reason to update if you don't have your own secret
value: 'dfkskdfjhskdfjh' // this is the default value that you can use but not advised.
Then if you want to reuse a secret
encryptionKey:
secret: 'myAlreadyExistingSecret'
useExistingSecret: true
key: 'myEncryptionKey'
If you want to create a secret with default props
encryptionKey:
value: 'mysecretvalue'
If you want to create a secret with custom props
encryptionKey:
secret: 'myNotExistingCustomSecretName' // will be created
key: 'mySecretKey' // will be created
value: 'mysecretvalue'
Description
~Still ironing some things on this solution proposal, will add a description and mark the ticket as ready to be reviewed once I'm done. If you see the PR in the meanwhile and think there's a better way to achieve this, feel free to comment.~
Adds support for
encryption_key
andencryption_key_file
values to be set, both "" by default. Ifencryption_key
is set, the key is made available as an environment variable. If both values are set, the key is set on a secret, mount as a volume and made available to the perses process as a file. In casesencryption_key
isn't set, the value ofencryption_key_file
, if set, is ignored.At the time of development I thought of this approach or one where we would expect secrets with a given name to be present so they could be mount or loaded as env vars. I'm still not sure what would be better but feel free to leave feedback.
The PR doesn't add support auth providers as discussed but I intend to open another PR for it.
Checklist
[<catalog_entry>] <commit message>
naming convention using one of the followingcatalog_entry
values:FEATURE
,ENHANCEMENT
,BUGFIX
,BREAKINGCHANGE
,IGNORE
.