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

Apply tpl function for secretKeyRef section of envSecret vars #85

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

begemotik
Copy link
Contributor

There are some cases when templating on the "values" level is useful. I will describe our cases below.

The first one is when the chart is included as subchart with some other charts, e.g. Redis helm chart. In that case, the redis service URL looks as follows "{{ .Release.Name }}-redis". You would like to get rid of the hardcode in values here.

  ...
config:
  engine: "redis"
  redis_sentinel_address: "{{.Release.Name}}-redis:26379"

The second example is when you need to provide additional envSecrets. In our case, we create the secret in a subchart as a result its name depends as well on the release name.

...
envSecret:
  - name: CENTRIFUGO_TOKEN_ECDSA_PUBLIC_KEY
    secretKeyRef:
      name: "{{ .Release.Name }}"
      key: token_ecdsa_public_key

The MR is intended to cover the cases above. Hopefully, it doesn't contradict the current approach.

@begemotik
Copy link
Contributor Author

@FZambia, hope you can find some time to take a look at this one as well :)

@FZambia
Copy link
Member

FZambia commented Jan 26, 2024

Thanks!

Hate to say this - but looks like there is a possible issue because we already have {{}} delimiters in configuration. This one:

"token_jwks_public_endpoint": "https://keycloak:443/{{realm}}/protocol/openid-connect/certs"

helm/helm#10299 has discussion about similar conflicts in other projects. I think ideally would be to have some custom tpldelim [[]] function to override delimiters per template call. So that you could use them for your templated values in Centrifugo. But looks like there is no such possibility in Helm now.

It's also possible to use {{ "{{realm}}" }} literal in config to deal with this and render {{realm}} properly. But I'd like to avoid this requirement by default.

The only idea for now is introducing some options to enable possibility to use templates like: useConfigmapTemplatingForConfig, useDeploymentTemplatingForSecretKeyRef with documentation about possible side effects in regards to Centrifugo configuration values and workaround with escaping over {{ "{{value}}" }}.

This is ugly, I understand, but seems to solve your problem without breaking changes. Probably you have some other ideas?

@begemotik
Copy link
Contributor Author

Completely understand your point.

For our case, as workaround I see that to ensure that the service name for Redis persists regardless of Helm chart release name I can leverage fullnameOverride of bitnami's chart:

redis:
  enabled: true
  fullnameOverride: centrifugo-redis # To make sure that value is preserved despite release-name

Regarding the second point. Though ecdsa and rsa keys contain public part does it make sense for you to have those options as part of the secret as well?

@begemotik begemotik changed the title tpl function to make chart more friendly for subchart customization jwt public key environment variables to secret Jan 29, 2024
@FZambia
Copy link
Member

FZambia commented Jan 29, 2024

Regarding the second point. Though ecdsa and rsa keys contain public part does it make sense for you to have those options as part of the secret as well?

Probably envSecret can help?

@begemotik
Copy link
Contributor Author

begemotik commented Jan 29, 2024

Yes, it's possible to use envSecret, but I need explicitly specify the secret name:

...
existingSecret: "{{ .Release.Name }}" # works perfectly, I generate that secret with subchart, name depends on release name.
envSecret:
  - name: CENTRIFUGO_TOKEN_ECDSA_PUBLIC_KEY
    secretKeyRef:
      name: centrifugo # I need to hardcode that part and keep in mind if release is not "centrifugo" I need to update the value.                             
      key: token_ecdsa_public_key

I create a single secret outside of that chart (in other subchart, ExternalSecret -> Secret) where all "sensitive" data is stored, e.g admin token, admin password, api token. The only part missing - ecdsa public key

@FZambia
Copy link
Member

FZambia commented Jan 29, 2024

Sorry, too many context switches during last days – completely missed that in your original pr you actually added templating to envSecret. My point about tpl was for config mostly. How about adding envSecretTemplated?

            {{- range .Values.envSecretTemplated }}
            - name: {{ .name }}
              valueFrom:
                secretKeyRef: 
                  {{- tpl (toYaml .secretKeyRef) $ | nindent 18 }}
                  {{- toYaml .secretKeyRef | nindent 18 }}
            {{- end }}

And simply iterate over both envSecretTemplated and envSecret in deployment? Seems safe and not too ugly. Just requires a good comment in values.yaml what is the difference

@begemotik
Copy link
Contributor Author

My point about tpl was for config mostly. How about adding envSecretTemplated?

Why would envSecretTemplated be needed then? tpl function used directly in multiple places and placeholder doesn't include "Templated" in it:

@FZambia
Copy link
Member

FZambia commented Jan 30, 2024

I see.. Well, I think adding tpl to secretKeyRef should be rather safe thing - so let's just add then

@begemotik
Copy link
Contributor Author

@FZambia should be ready for review

@FZambia FZambia changed the title jwt public key environment variables to secret Apply tpl function for secretKeyRef section of envSecret vars Jan 30, 2024
@FZambia FZambia merged commit 35be9a4 into centrifugal:master Jan 30, 2024
2 checks passed
@FZambia
Copy link
Member

FZambia commented Jan 30, 2024

Many thanks!

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