Improve db-operator support

[pschichtel]

This PR allows to configure more postgres details (db name, host, port) via secretKeyRefs. This is useful, when using an operator that manages databases and that automatically provides a secret with all the details. One such operator is the db-operator project.

The same approach could also be applied to other secrets, of that is of interest.

[pschichtel]

@Cogitri if you are fine with a breaking change, then I would drop the "existingSecret" field and simplify the helper accordingly.

[pschichtel]

btw I haven't fully tested this yet

[Cogitri]

if you are fine with a breaking change, then I would drop the "existingSecret" field and simplify the helper accordingly.

If we were to drop existingSecret all secrets could be specified like:

password:
  name: my_name
  password: my_pass

Right? That'd also be fine by me.

[pschichtel]

I've added a README section. The last commit removes the existingSecret values for postgresql and keyBase, which would be breaking.

[Cogitri]

Alright, this looks great, good work! The only thing I'm considering is if it's a good idea to name it .secretName instead of .name? That makes it a bit more explicit - but on the other hand it's already somewhat clear from the context 🤔 Do you have an opinion on that?

[pschichtel]

I based my naming on the secretKeyRef field: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.34/#secretkeyselector-v1-core

but I'd be fine either way

[pschichtel]

@Cogitri so, should I go ahead and change name to secretName ?

Also should I migrate the rest of the secrets over to this as well ?

[Cogitri]

Hi, sorry for not getting back to you, I think the naming is fine as-is. I'll test the PR later today and merge it :) I think we can migrate the other secrets too

[pschichtel]

then let's defer the testing and merge until I migrated the rest over this evening, ok ?

[pschichtel]

I pushed a few more commits. The first 2 are unrelated cleanups, which I could pull out of this PR if wanted. the third migrates the rest of the secrets over to the new scheme, which didn't fully work out for the redis password. The 4. commit goes all the way for the redis password, while still keeping the chart preconfigured correctly for the subchart.

It's probably debatable of the last commit is really worth the trouble, but I think it's the only clean way to have the redis password configuration consistent with the other secrets.

If you want I can also write up a summary of the breaking changes and a quick migration guide

[Cogitri]

It's probably debatable of the last commit is really worth the trouble, but I think it's the only clean way to have the redis password configuration consistent with the other secrets.

Hmm, I think it's nice to have it consistent, but it's a bit odd how there's a redis and redis-ha key in the values.yaml now. I think we should at least add a comment explaining users which keys to use when.

If you want I can also write up a summary of the breaking changes and a quick migration guide

That'd be much appreciated, thank you so much for your work! :)

[Cogitri]

Hmmm, maybe I'm doing something wrong, when leaving this at the default value, installing the template fails:

Error: dawarich/templates/secret.yaml:9:107
  executing "dawarich/templates/secret.yaml" at <.Values.keyBase.existingSecret>:
    can't evaluate field existingSecret in type interface {}

[Cogitri]

Hmm, I'm also getting a few warnings here, I'm not quite sure what's the dawarich. prefix doing in dawarich.postgresql.auth.password?

level=INFO msg="warning: destination for dawarich.postgresql.auth.password is a table. Ignoring non-table value (changeme)"
level=INFO msg="warning: destination for dawarich.postgresql.auth.username is a table. Ignoring non-table value (dawarich)"
level=INFO msg="warning: destination for dawarich.postgresql.auth.database is a table. Ignoring non-table value (dawarich)"
level=INFO msg="warning: destination for dawarich.postgresql.auth.username is a table. Ignoring non-table value (dawarich)"
level=INFO msg="warning: destination for dawarich.postgresql.auth.database is a table. Ignoring non-table value (dawarich)"
level=INFO msg="warning: destination for dawarich.postgresql.auth.password is a table. Ignoring non-table value (changeme)"

[pschichtel]

I'll test again later today

[pschichtel]

I fixed the template on defaults (missed removing a reference to a existingSecret).

Not sure how you are producing those warnings, are those application logs from dawarich ?

[Cogitri]

Ah sorry, should've mentioned that in my initial post. I got those warnings when doing helm install of this chart with your changes applied with the default values.yaml with a few small changes. I'll test again with the updated PR later today and if that still produces the warnings post values.yaml.

[Cogitri]

This is the values.yaml I use for my testing environment:

dawarich:
  replicaCount: 1
  hosts:
    - "..."
sidekiq:
  replicaCount: 1
postgresql:
  host: dawarich-postgres-rw
  auth:
    database:
      name: dawarich-postgresql-secret
      key: database
    username:
      name: dawarich-postgresql-secret
      key: username
    password:
      name: dawarich-postgresql-secret
      key: password
redis:
  auth: false
  password: ""

Installed it via helm upgrade --install dawarich . --values=staging.yaml --namespace=dawarich-test --create-namespace

[Cogitri]

Oh and ignoring those log messages, the resulting pod doesn't start due to Error: couldn't find key photonApiKey in Secret dawarich-test/dawarich - which shouldn't be mandatory.

[pschichtel]

time was a little tight last week, I'll test this through sometime this week.

[pschichtel]

Ok so the warnings are being produced by helm, because the additional values override a non-object value (e.g. dawarich.postgresql.auth.database which defaults to dawarich) with an object. The warnings says, that it's using the object value, so everything works as expected, but the warning is still ugly. Surprisingly this warnings is not produced when using helm template, which I did most of my testing with and argocd also uses helm template internally. We could avoid these by moving the value to a subkey like value and giving the secretKeyRef precedence over the value. even though we are not currently doing that, it could be problematic when defaulting things to secrets, which can then not be overridden.

since we are breaking things anyway, I can propose a different approach, which might simplify things.

[pschichtel]

The commit Rename ha-redis to bundled-redis renames ha-redis to bundled-redis, however I'm not sure that rename is worth the migration.

The commit Convert all secrets to secretKeyRefs converts all secrets in the values.yaml to secretKeyRefs with a default
key and introduces a "secret" section to configure a helm-managed secret, which all the secretKeyRefs refer to by default. This avoids helm's warnings at the cost of some more breakage

[Cogitri]

Thanks, I'll test this later tomorrow. Could you also add a short section in the README how existing users can port their values.yaml? I suppose simply replacing e.g. database.existingSecret with database.existingSecret.name (and changing database.existingSecret.key if it doesn't happen to match) should be sufficient?

[Cogitri]

Hmm, I don't have a good suggestion for how to change this, but it'd be nice if .secret already reflected that that's only to be used if you really want the helm chart to create the secrets for you, which isn't necessarily what you'd do in prod. Maybe a comment would already be sufficient ("Change the values below if you don't use existing secrets and want the chart to create secrets for you").

It'd also be nice to know which values are mandatory in the default configuration (all non-null values are required + redisPassword, or does the combination of .redis.auth=true and secret.stringData.redisPassword=null work?)

[Cogitri]

Upgrading to the new version fails with the values.yaml I linked here:

#224 (comment)

level=INFO msg="warning: cannot overwrite table with non table for dawarich.redis.password (map[key:redisPassword])"
level=INFO msg="warning: cannot overwrite table with non table for dawarich.redis.password (map[key:redisPassword])"
Error: UPGRADE FAILED: dawarich/templates/deployment.yaml:35:11
  executing "dawarich/templates/deployment.yaml" at <include "dawarich.initContainers" .>:
    error calling include:
dawarich/templates/_helpers.tpl:195:8
  executing "dawarich.initContainers" at <include "dawarich.secretValueEnvRef" (dict "EnvName" "DATABASE_HOST" "Key" "postgresHost" "Value" .Values.postgresql.host "Root" .)>:
    error calling include:
dawarich/templates/_helpers.tpl:243:21
  executing "dawarich.secretValueEnvRef" at <.Value.name>:
    can't evaluate field name in type interface {}