Closes #400 - Migrate away from bitnami/postgresql

[mgruner]

Which issue this PR fixes

• fixes Migrate away from bitnami/postgresql #400

Special notes for your reviewer

• Obviously a complex matter and a major update. We combine it with a major PostgreSQL 17 -> 18 update. The manual steps need to be checked carefully.

Checklist

• Chart Version bumped
• Upgrading instructions are documented in the zammad/README.md

[mgruner]

@monotek could you have another look, please? When manually verifying the migration runbook, a few changes were needed. Specifically to make it possible to upgrade the application without starting the Zammad services immediately. Now everything works fine in my test system.

I would probably release this as soon as you confirm. Such a change is never really convenient, but needs to be done anyway. The other breaking MRs (elasticsearch, fs policy default change) could go into another major version, perhaps together with Zammad 7.2.

[monotek]

I currently have no Zammad instance running, and I am also really short on time, so I likely won't be able to test it.
If you tested it, I trust you that it works :)

[fliebe92]

1. Pin the PostgreSQL major version (my main concern). The subchart pins image.tag: "18.4@sha256:3a82e1f…" internally, so this release is deterministic. But the pin lives in the subchart's values, and CloudPirates has shipped a PG major bump inside a chart minor before — their CHANGELOG shows 17.6 → 18 landing around 0.5.x→0.6.x ([postgres]: Fix invalid data dir path on postgres 18). The official postgres image refuses to start on a data dir from an older major, so a routine subchart bump could hard-fail every existing install. Right now .github/updatecli.yaml still has the old bitnami postgresql source commented out and no postgres source, so nothing auto-bumps it today — but that block should be cleaned up either way, and I'd set postgres.image.tag explicitly in zammad/values.yaml so a major upgrade is always a deliberate, documented chart change. The subchart's own escape hatch (a pgautoupgrade init container) is sketched in its values comments around line 395 — worth referencing in the README for the next time.

2. Runbook gaps. Step 1 scales down deploy -l app.kubernetes.io/name=zammad, which covers all four Deployments — but not the reindex CronJob, which can still write during the dump if enabled. It should be suspended too. And there's no verification between step 4 and step 5: kubectl exec streaming a binary dump through the API server can truncate on a dropped connection, and step 5 then happily migrates a partial database. A pg_restore -l zammad_production.dump | head before, and a row-count spot check after, would cost nothing.

3. Zammad now runs as a DB superuser. auth.username in this subchart is documented as "Name for a custom superuser to create at initialisation" — under bitnami, zammad was a plain owner and postgres was separate. Zammad doesn't need it: db/schema.rb:15 only enables pg_catalog.plpgsql. The subchart's customUser block would keep the app unprivileged. Not a blocker, but it should be a conscious choice rather than a side effect, and the README currently states it as a fact without noting it's a change.

4. Small doc inconsistencies in values.yaml. The comments at lines 198/202/205 still say "needs to be the same as the postgresql.auth.*" after the key was renamed to postgres:. Line 201's host: zammad-postgresql default reads stale even though it only applies when the subchart is disabled. And the existingSecret example at line 809 uses adminPasswordKey: postgresql-pass while the README says the default is postgres-password — both correct, but confusing side by side.

5. Minor: capping scheduler/websocket replicas silently (max 0 (min 1 …)) means someone who sets 2 never learns it was ignored. A fail would be louder, though the values comments do document it.

[mgruner]

1. Pin the PostgreSQL major version (my main concern). The subchart pins image.tag: "18.4@sha256:3a82e1f…" internally, so this release is deterministic. But the pin lives in the subchart's values, and CloudPirates has shipped a PG major bump inside a chart minor before — their CHANGELOG shows 17.6 → 18 landing around 0.5.x→0.6.x ([postgres]: Fix invalid data dir path on postgres 18). The official postgres image refuses to start on a data dir from an older major, so a routine subchart bump could hard-fail every existing install. Right now .github/updatecli.yaml still has the old bitnami postgresql source commented out and no postgres source, so nothing auto-bumps it today — but that block should be cleaned up either way, and I'd set postgres.image.tag explicitly in zammad/values.yaml so a major upgrade is always a deliberate, documented chart change. The subchart's own escape hatch (a pgautoupgrade init container) is sketched in its values comments around line 395 — worth referencing in the README for the next time.

Implemented as discussed via updatecli policy, not pinning.

2. Runbook gaps. Step 1 scales down deploy -l app.kubernetes.io/name=zammad, which covers all four Deployments — but not the reindex CronJob, which can still write during the dump if enabled. It should be suspended too.

Added.

And there's no verification between step 4 and step 5: kubectl exec streaming a binary dump through the API server can truncate on a dropped connection, and step 5 then happily migrates a partial database. A pg_restore -l zammad_production.dump | head before, and a row-count spot check after, would cost nothing.

Ignored for now.

3. Zammad now runs as a DB superuser. auth.username in this subchart is documented as "Name for a custom superuser to create at initialisation" — under bitnami, zammad was a plain owner and postgres was separate. Zammad doesn't need it: db/schema.rb:15 only enables pg_catalog.plpgsql. The subchart's customUser block would keep the app unprivileged. Not a blocker, but it should be a conscious choice rather than a side effect, and the README currently states it as a fact without noting it's a change.

This was addressed with some text in upgrading and an example for manually choosing the more complex configuration. Making customUser default would add complexity.

4. Small doc inconsistencies in values.yaml. The comments at lines 198/202/205 still say "needs to be the same as the postgresql.auth.*" after the key was renamed to postgres:. Line 201's host: zammad-postgresql default reads stale even though it only applies when the subchart is disabled. And the existingSecret example at line 809 uses adminPasswordKey: postgresql-pass while the README says the default is postgres-password — both correct, but confusing side by side.

Addressed by text.

5. Minor: capping scheduler/websocket replicas silently (max 0 (min 1 …)) means someone who sets 2 never learns it was ignored. A fail would be louder, though the values comments do document it.

Not important.

Can you have another look? I need to run another manual test now before final approval/merge.

[fliebe92]

Had another look at d288e7d. Thanks for the point-by-point — I checked the updatecli approach properly rather than just taking it on trust, since it replaced my suggestion.

The updatecli policy works — confirmed

Implemented as discussed via updatecli policy, not pinning.

I wanted to verify that "patch only" actually protects against a major, and it does. CloudPirates bumped PostgreSQL 17.6 → 18.0 in 7592892, and that same commit moved the chart 0.5.4 → 0.6.0 — a minor. PG minor bumps do land in chart patches (0.15.5 ships 18.1, 0.15.6 ships 18.2), but those are in-place compatible, so that is fine. So pattern: "0.19.x" blocks exactly the class of change that would break the data dir. Good call, this is cleaner than pinning the image tag.

I also confirmed $.dependencies[3].version still resolves to postgres after the rename (0=elasticsearch, 1=minio, 2=memcached, 3=postgres, 4=redis).

One residual: the 0.19.x pin will go stale silently. redis and memcached track 0.x.x, so postgres is now the one dependency that needs a manual, remembered bump — and once CloudPirates moves to 0.20.x, updatecli stops proposing postgres updates altogether, including image digest refreshes, with no signal that it has gone quiet. The explanation lives in .github/updatecli.yaml, but whoever bumps chart versions is looking at Chart.yaml. A one-line pointer next to the dependency there would probably be enough.

Cronjob suspend — verified

Checked that this isn't just plausible-looking: app.kubernetes.io/component: zammad-cronjob-reindex really is set in cronjob-reindex.yaml, and zammadConfig.cronJob.reindex.suspend is a real value wired into spec.suspend. Both the kubectl patch in step 1 and the --set in step 3 do what they claim.

Tiny nit: the patch selects only on the component label, so with two releases in one namespace kubectl get -o name returns two lines and the quoted $(...) makes kubectl patch fail. Adding -l app.kubernetes.io/instance=<release_name> would scope it.

New: the customUser example conflicts with the runbook

This was addressed with some text in upgrading and an example for manually choosing the more complex configuration.

The example itself is structurally correct — I checked the subchart's init script and it does CREATE USER / CREATE DATABASE / ALTER DATABASE :udb OWNER TO :uname, so the database ends up owned by zammad.

But it collides with the migration runbook. The note says the dump "has to be done with the superuser credentials", while step 4 restores with pg_restore --no-owner. Restoring as postgres with --no-owner leaves every table and sequence owned by postgres — only the database itself is owned by zammad. Zammad then connects as the unprivileged zammad user and db:migrate fails on the first ALTER TABLE with must be owner of table ....

Either restoring with --role=zammad, or a follow-up REASSIGN OWNED BY postgres TO zammad; inside zammad_production, would fix it. Narrow reach — it only bites someone who both migrates and opts into customUser — but that is precisely the security-conscious user the block was added for, and it's a dead end once hit. Two lines of doc.

Rest

No objection to ignoring the restore verification and the silent replica capping — reasonable calls for a runbook you're executing manually yourself.

Otherwise this looks good to merge after your manual test. All 10 checks green on d288e7d.

[mgruner]

New: the customUser example conflicts with the runbook

This was addressed with some text in upgrading and an example for manually choosing the more complex configuration.

The example itself is structurally correct — I checked the subchart's init script and it does CREATE USER / CREATE DATABASE / ALTER DATABASE :udb OWNER TO :uname, so the database ends up owned by zammad.

But it collides with the migration runbook. The note says the dump "has to be done with the superuser credentials", while step 4 restores with pg_restore --no-owner. Restoring as postgres with --no-owner leaves every table and sequence owned by postgres — only the database itself is owned by zammad. Zammad then connects as the unprivileged zammad user and db:migrate fails on the first ALTER TABLE with must be owner of table ....

Either restoring with --role=zammad, or a follow-up REASSIGN OWNED BY postgres TO zammad; inside zammad_production, would fix it. Narrow reach — it only bites someone who both migrates and opts into customUser — but that is precisely the security-conscious user the block was added for, and it's a dead end once hit. Two lines of doc.

The feedback was correct in substance, but slightly off the mark in its diagnosis of the cause: The conflict did not arise because the runbook uses --no-owner, but because my customUser note required a superuser restore that isn't actually needed. Once this incorrect instruction is removed, step 4 of the runbook will be correct for both configurations—no --role, no second code path variant, and no hard-coded username.

The offending comment was corrected.