Adopt approach of using ExternalSecrets instead of plain-text Secrets

[smktpd]

There is External Secrets Operator (ESO) that provides support of kind: ExternalSecret CRDs.
This is de-facto the best practice of using secrets for Kubernetes payloads that currently exists in the industry.
The way it works - is pretty simple: basically, you just apply a manifest with a SecretStore (or a ClusterSecretStore) that defines the access to a vault (it supports many kinds of vaults) and an ExternalSecret (or ClusterExternalSecret) manifest that describes which secret (exact path and specific key (or all keys from that path)) to read from that vault and which kind: Secret to save it to in Kubernetes.

It would be nice to make temporal's helm chart have support for it.
Bringing support for ExternalSecrets into this chart would require some changes (introduce new values and change the way the secrets are currently mounted into containers).

[robholland]

Thanks, this makes sense. Do you know of an chart that's already using this we could use as a reference?

[smktpd]

I've asked my colleagues around - turns out we all 'cook' them internally, so no, I don't have an example chart to use as a reference.
I think you don't really even need to provide the manifests with those CRDs as a part of your chart.

Basically, all is needed from the chart is that it'd accept a name of the secret to use passwords from.

I am not an expert with Helm (honestly, I even hate it) and maybe there is already nothing to do on your part and I just misunderstand how to use it properly, as to me it seems that your chart already KINDA works like that.
Let's get into the details:
templates/server-deployment.yaml:103-107

            - name: TEMPORAL_STORE_PASSWORD
              valueFrom:
                secretKeyRef:
                  name: {{ include "temporal.persistence.secretName" (list $ "default") }}
                  key: {{ include "temporal.persistence.secretKey" (list $ "default") }}

this is defined in templates/_helpers.tpl:343-347

{{- define "temporal.persistence.secretName" -}}
{{- $global := index . 0 -}}
{{- $store := index . 1 -}}
{{- include (printf "temporal.persistence.%s.secretName" (include "temporal.persistence.driver" (list $global $store))) (list $global $store) -}}
{{- end -}}

which basically references templates/_helpers.tpl:152-167

{{- define "temporal.persistence.driver" -}}
{{- $global := index . 0 -}}
{{- $store := index . 1 -}}
{{- $storeConfig := index $global.Values.server.config.persistence $store -}}
{{- if $storeConfig.driver -}}
{{- $storeConfig.driver -}}
{{- else if $global.Values.cassandra.enabled -}}
{{- print "cassandra" -}}
{{- else if $global.Values.mysql.enabled -}}
{{- print "sql" -}}
{{- else if $global.Values.postgresql.enabled -}}
{{- print "sql" -}}
{{- else -}}
{{- required (printf "Please specify persistence driver for %s store" $store) $storeConfig.driver -}}
{{- end -}}
{{- end -}}

The way I understand how $global.Values.server.config.persistence works, if my values.yaml had

server:
  config:
    persistence:
      default:
        driver: "sql"
        sql:
          driver: "postgres12"
          secretName: my-secret-name
          secretKey: TEMPORAL_POSTGRESQL_PASS

then the first quoted block from server-deployment.yaml should have been rendered like this:

            - name: TEMPORAL_STORE_PASSWORD
              valueFrom:
                secretKeyRef:
                  name: my-secret-name
                  key: TEMPORAL_POSTGRESQL_PASS

but for some reason it gets rendered into

            - name: TEMPORAL_STORE_PASSWORD
              valueFrom:
                secretKeyRef:
                  name: tpas-temporal-default-store
                  key: password

Maybe it's a bug, I'm not sure.

But if that part would work the way I expected it to - it would be nice.

And all that'd be left to change would be just adding some extra value to control whether to apply templates/server-secret.yaml or to expect the secret to be created 'from outside'.
Maybe it'd be also worth providing templates/server-externalsecret.yaml for 'batteries included' approach.

I'll share my templates/server-externalsecret.yaml, for just in case:

{{- if .Values.flexiblyMappedExternalSecrets -}}
  {{- range $k, $v := .Values.flexiblyMappedExternalSecrets }}
---
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: {{ $k }}
spec:
    {{- if ($.Values.externalSecretsSpecs).refreshInterval }}
  refreshInterval: {{ $.Values.externalSecretsSpecs.refreshInterval }}
    {{- end }}
  secretStoreRef:
    kind: SecretStore
    name: {{ $.Release.Namespace }}-secret-store
  target:
    name: {{ $k }}
    creationPolicy: {{ ($.Values.externalSecretsSpecs).creationPolicy | default "Owner" }}
    deletionPolicy: {{ ($.Values.externalSecretsSpecs).type | default "Retain" }}
  data:
    {{- range $v }}
    - remoteRef:
        key: {{ .src_secret_path }}
        property: {{ .src_secret_key }}
      secretKey: {{ .dst_secret_key }}
    {{- end -}}
  {{- end -}}
{{- end }}

and a block I add to my values:

flexiblyMappedExternalSecrets:
  tpas-temporal-default-store: # name of the secret (can't be changed due to above described possible bug)
    - dst_secret_key: password # name of the key in the secret to save value to (can't be changed due to above described possible bug)
      src_secret_path: dev/my_stand/temporal/postgresql_main_db # in my case it's a path in vault_path to the secret
      src_secret_key: DATABASE_PASSWORD # key (in terms of JSON dict) from the secret to read the password from
  tpas-temporal-visibility-store:
    - dst_secret_key: password
      src_secret_path: dev/my_stand/temporal/postgresql_visibility_db
      src_secret_key: DATABASE_PASSWORD

[robholland]

Thanks so much for the details, I'll dig into this soon.

[robholland]

I think the current interface is both confusing and buggy.

How about changing to:

server:
  config:
    persistence:
      default:
        driver: "sql"
        sql:
          driver: "postgres12"
          createSecret: false
          secretName: my-secret-name
          secretKey: TEMPORAL_POSTGRESQL_PASS

So start making use of a secretKey value (it currently isn't read) and also add createSecret a boolean (default true). I think that's a clearer term than existingSecret which is the current interface.

[smktpd]

That would be nice. I'd also argue for leaving more comments explaining values, like

createSecret: true  # use 'true' if you want this chart to generate a manifest of 'kind: Secret' for you, use 'false' if you are going to create it not by the means of this chart (manually, via ExternalSecret or somehow else)

edit: the explanation becomes too long, that's actually a sign of a poorly chosen name for a values key.
generateSecret or useGeneratedSecret would probably require a shorter explanation.

[robholland]

Having looked into this some more, a lot of charts use the existingSecret structure, so I think we should stick to that, although I don't think it's the clearest interface. Anyway, I have made some fixes to check secretKey which wasn't being done properly before. Please can you try the latest release and see if it's working now, or at least is closer to what you need?

[smktpd]

There were too many changes made at once for the test to be easy, a big rewrite.

1. I do not understand how things should work now with ElasticSearch as visibility store, if this block

            - name: ENABLE_ES
              value: "{{ or $.Values.elasticsearch.enabled $.Values.elasticsearch.external }}"
            - name: ES_SEEDS
              value: "{{ $.Values.elasticsearch.host }}"
            - name: ES_PORT
              value: "{{ $.Values.elasticsearch.port }}"
            - name: ES_VERSION
              value: "{{ $.Values.elasticsearch.version }}"
            - name: ES_SCHEME
              value: "{{ $.Values.elasticsearch.scheme }}"
            - name: ES_VIS_INDEX
              value: "{{ $.Values.elasticsearch.visibilityIndex }}"
            - name: ES_USER
              value: "{{ $.Values.elasticsearch.username }}"
            - name: ES_PWD
              value: "{{ $.Values.elasticsearch.password }}"

from server-deployment.yaml was removed.

2. How to use external elasticsearch and take password for it from an existing secret?

3. So you've introduced existingSecret key basically as an overriding alias for secretName that accepts the name of the secret to use, but the to specify which key to read the password from from that secret can be only specified still only via old secretKey, so there's no much use in such an alias other than introducing similarity with other helm charts (I don't have much experience with helm charts, so for me personally there's no 'force of habit' in such a similarity is out - it works as if user specified to use existingSecret, weird behavior.
   It took me some tests to figure that out, I wish you'd recognize the need for commenting keys in values.yaml files :-)

4. And another weirdness left: elasticsearch.enabled: true - what does it do? set up elasticsearch? or a signal to use elasticsearch as a visibility store? why does server.config.peristence.visibility have 2 blocks (while only 1 of them will actually be used)? to demonstrate possible configurations for cassandra and for sql, right? but what about elasticsearch used for visibility? why not add it as 3rd block there?
   I feel so confused...