Skip to content

Commit b53ca1f

Browse files
authored
fix(postgres): refuse system databases and never drop them (#5155)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `databases` accepts `postgres`, `template0` and `template1`, and the init job treats them like application databases: it comments them `database managed by helm`, creates `<db>_admin` and `<db>_readonly`, makes `<db>_admin` the owner of the database and its schemas, and installs the `trigger_auto_grant` event trigger. When the entry later leaves values, the delete pass runs `DROP DATABASE` on it, which can only fail: - `template1`, `template0`: `cannot drop a template database`; - `postgres`: `cannot drop the currently open database`, since the job is connected to it. The init job then fails on every upgrade, so the HelmRelease sits in `UpgradeFailed` with `post-upgrade hooks failed`, and putting the entry back does not help. `template1` also hands the takeover (owner of `public`, default privileges, trigger) to every database created afterwards. Once its roles are dropped by hand, the trigger left behind names a role that no longer exists, so `CREATE SCHEMA` fails in every new database, even as superuser. This PR: - **Refuses** the three names at render with a message that says what to do. `cozyvalues-gen` has no way to constrain the keys of a `map[string]Database`, so the refusal sits next to the existing database-name guard, like the `users.postgres` refusal. The field description says it too. - **Never selects them for `DROP DATABASE`.** The delete pass excludes them whatever values say. - **Hands a taken-over system database back.** In a new `release system databases` pass before the delete pass, for each of the three still commented `database managed by helm`: drop the trigger and `auto_grant_schema_privileges()`, `REASSIGN OWNED` / `DROP OWNED` for its two roles in that database, give `public` back to `pg_database_owner` (PostgreSQL 15+), and clear the comment. The existing orphaned-roles pass then drops the roles; it never visits `template1`, hence the per-database step. `template0` accepts no connection and the job that commented it never got further, so only its comment is cleared. **Upgrade note.** A release that declares one of the three names fails to render until the entry is removed. Removing it is now safe: the next run releases the database instead of dropping it. **Testing** - `tests/system_databases_test.yaml`: 5 of its 6 cases fail on `main` without the change. `helm unittest packages/apps/postgres`: 215 passed. - I replayed the scenario against a `postgres:17` container, running the rendered `init.sh` with `PGUSER=postgres`: 1. The `main` script with `databases: {appdb, template1, postgres}` reproduces the takeover. 2. The `main` script with only `appdb` then fails with `cannot drop the currently open database`. 3. This branch's script on that same state exits 0. It releases both databases (owner `postgres`, no comment, no event trigger, `public` owned by `pg_database_owner`) and drops `postgres_admin`, `postgres_readonly`, `template1_admin` and `template1_readonly`. A database created afterwards gets no trigger, and `CREATE SCHEMA` succeeds in it. 4. A second run is a no-op. 5. A `template0` commented and given roles by hand is released the same way. Open PR #5091 touches the same script (stable database order); the hunks do not overlap. Backport: the automatic cherry-pick onto `release-1.6` conflicts in `init-script.yaml` and the generated cozyrd, so the backport to `release-1.6` is done by hand in #5157 (same SQL, replayed against that branch's script). `release-1.7` keeps `kind/backport`. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change (walked the map: a template guard, the init script, a test suite and one field description regenerated into README, schema, Go types and cozyrd; no field, default, enum or name change) - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(postgres): databases now refuses the PostgreSQL system databases postgres, template0 and template1, and the init job never runs DROP DATABASE on them. A system database a previous version took over is handed back to the superuser on the next upgrade (event trigger removed, ownership and public schema restored, its <db>_admin and <db>_readonly roles dropped). A release that declares one of them must remove the entry before upgrading. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Behavior Changes** * PostgreSQL system databases (`postgres`, `template0`, and `template1`) are refused as managed application databases. * Previously managed system databases are released during cleanup and excluded from database deletion. Cleanup restores eligible ownership and security settings and clears system database comments. * **Documentation** * Updated PostgreSQL configuration guidance to clarify that these system database names are refused. Names that merely contain these strings remain accepted. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 10e0ef9 + f003851 commit b53ca1f

10 files changed

Lines changed: 635 additions & 12 deletions

File tree

‎api/apps/v1alpha1/postgresql/types.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎hack/e2e-chainsaw/postgres/chainsaw-test.yaml‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,3 +347,148 @@ spec:
347347
set -eu
348348
cd ../../..
349349
NAMESPACE="$NAMESPACE" examples/backups/postgres/cleanup.sh
350+
---
351+
# A system database that a version before the refusal took over is handed back
352+
# by the next init-job run, and only what this chart marked changes hands.
353+
#
354+
# The helm-unittest and bats suites check the rendered SQL against a fake psql;
355+
# only a real PostgreSQL can say that the predicates select what they claim. The
356+
# state an older version left is rebuilt by hand as the superuser, since the
357+
# chart now refuses to produce it:
358+
# * postgres is commented as managed and owned by a postgres_admin role that
359+
# carries the cozystack:managed mark, as the chart left it: it must come back
360+
# to the superuser, and the role must be dropped;
361+
# * template1 is commented as managed, but its template1_admin carries only the
362+
# role comment, which a CREATEROLE holder can forge, and not the mark: the
363+
# comment must be cleared and everything that role owns or was granted must
364+
# stay as it is, in template1 and in a database cloned from it.
365+
apiVersion: chainsaw.kyverno.io/v1alpha1
366+
kind: Test
367+
metadata:
368+
name: postgres-3-system-databases
369+
spec:
370+
catch:
371+
- events: {}
372+
- describe:
373+
apiVersion: helm.toolkit.fluxcd.io/v2
374+
kind: HelmRelease
375+
name: postgres-sysdb
376+
- podLogs:
377+
selector: job-name=postgres-sysdb-init-job
378+
tail: 200
379+
steps:
380+
- name: create-postgres
381+
try:
382+
- apply:
383+
file: postgres-sysdb.yaml
384+
# 11m: above the 600s install timeout cozystack-api gives this release;
385+
# see "Sizing an HR-Ready budget" in docs/agents/e2e-testing.md.
386+
- assert:
387+
timeout: 11m
388+
resource:
389+
apiVersion: helm.toolkit.fluxcd.io/v2
390+
kind: HelmRelease
391+
metadata:
392+
name: postgres-sysdb
393+
status:
394+
(conditions[?type == 'Ready']):
395+
- status: "True"
396+
- assert:
397+
timeout: 2m
398+
resource:
399+
apiVersion: batch/v1
400+
kind: Job
401+
metadata:
402+
name: postgres-sysdb-init-job
403+
status:
404+
(conditions[?type == 'Complete']):
405+
- status: "True"
406+
407+
- name: rebuild-taken-over-system-databases
408+
try:
409+
- script:
410+
timeout: 3m
411+
content: |
412+
set -eu
413+
pod=$(kubectl -n "$NAMESPACE" get pod \
414+
-l cnpg.io/cluster=postgres-sysdb,cnpg.io/instanceRole=primary -o name | head -n1)
415+
[ -n "$pod" ] || { echo "no primary pod for postgres-sysdb" >&2; exit 1; }
416+
sql() { kubectl -n "$NAMESPACE" exec -i "$pod" -c postgres -- psql -v ON_ERROR_STOP=1 -U postgres -d "$1"; }
417+
sql postgres <<'SQL'
418+
CREATE ROLE postgres_admin NOLOGIN;
419+
COMMENT ON ROLE postgres_admin IS 'role managed by helm';
420+
GRANT "cozystack:managed" TO postgres_admin WITH INHERIT FALSE, SET FALSE;
421+
COMMENT ON DATABASE postgres IS 'database managed by helm';
422+
ALTER DATABASE postgres OWNER TO postgres_admin;
423+
CREATE ROLE template1_admin NOLOGIN;
424+
COMMENT ON ROLE template1_admin IS 'role managed by helm';
425+
CREATE ROLE template1_readonly NOLOGIN;
426+
COMMENT ON DATABASE template1 IS 'database managed by helm';
427+
ALTER DATABASE template1 OWNER TO template1_admin;
428+
SQL
429+
sql template1 <<'SQL'
430+
ALTER SCHEMA public OWNER TO template1_admin;
431+
CREATE TABLE public.kept (id int);
432+
ALTER TABLE public.kept OWNER TO template1_admin;
433+
GRANT SELECT ON public.kept TO template1_readonly;
434+
SQL
435+
sql postgres <<'SQL'
436+
CREATE DATABASE sysdb_clone TEMPLATE template1;
437+
SQL
438+
# The next run must be a new Job: remove the completed one so the
439+
# assert below cannot match it.
440+
kubectl -n "$NAMESPACE" delete job postgres-sysdb-init-job --wait=true
441+
# Declaring a user is enough for an upgrade, and runs the init-job.
442+
kubectl -n "$NAMESPACE" patch postgres.apps.cozystack.io sysdb --type merge \
443+
-p '{"spec":{"users":{"sysdbuser":{},"sysdbprobe":{}}}}'
444+
check:
445+
($error == null): true
446+
- assert:
447+
timeout: 11m
448+
resource:
449+
apiVersion: batch/v1
450+
kind: Job
451+
metadata:
452+
name: postgres-sysdb-init-job
453+
status:
454+
(conditions[?type == 'Complete']):
455+
- status: "True"
456+
457+
- name: verify-release
458+
try:
459+
- script:
460+
timeout: 2m
461+
content: |
462+
set -eu
463+
pod=$(kubectl -n "$NAMESPACE" get pod \
464+
-l cnpg.io/cluster=postgres-sysdb,cnpg.io/instanceRole=primary -o name | head -n1)
465+
q() { kubectl -n "$NAMESPACE" exec "$pod" -c postgres -- psql -v ON_ERROR_STOP=1 -U postgres -tA -d "$1" -c "$2"; }
466+
fail=0
467+
expect() {
468+
got=$(q "$1" "$3")
469+
if [ "$got" != "$4" ]; then
470+
echo "FAIL: $2: got '$got', want '$4'" >&2
471+
fail=1
472+
fi
473+
}
474+
comment="SELECT coalesce(shobj_description(oid, 'pg_database'), '<none>') FROM pg_database WHERE datname = "
475+
owner="SELECT pg_get_userbyid(datdba) FROM pg_database WHERE datname = "
476+
expect postgres "postgres comment cleared" "$comment'postgres'" '<none>'
477+
expect postgres "postgres back to the superuser" "$owner'postgres'" postgres
478+
expect postgres "marked postgres_admin dropped" "SELECT count(*) FROM pg_roles WHERE rolname = 'postgres_admin'" 0
479+
expect postgres "template1 comment cleared" "$comment'template1'" '<none>'
480+
expect postgres "unmarked template1_admin still exists" "SELECT count(*) FROM pg_roles WHERE rolname = 'template1_admin'" 1
481+
expect postgres "template1 still owned by the unmarked role" "$owner'template1'" template1_admin
482+
for db in template1 sysdb_clone; do
483+
expect "$db" "$db public still owned by the unmarked role" \
484+
"SELECT pg_get_userbyid(nspowner) FROM pg_namespace WHERE nspname = 'public'" template1_admin
485+
expect "$db" "$db kept table still owned by the unmarked role" \
486+
"SELECT tableowner FROM pg_tables WHERE schemaname = 'public' AND tablename = 'kept'" template1_admin
487+
expect "$db" "$db grant to the unmarked readonly role kept" \
488+
"SELECT has_table_privilege('template1_readonly', 'public.kept', 'SELECT')" t
489+
done
490+
exit "$fail"
491+
check:
492+
($error == null): true
493+
# The Postgres CR was applied by chainsaw; deleting it removes the whole
494+
# cluster, the databases and roles built above included.
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
apiVersion: apps.cozystack.io/v1alpha1
2+
kind: Postgres
3+
metadata:
4+
name: sysdb
5+
spec:
6+
external: false
7+
size: 1Gi
8+
replicas: 1
9+
storageClass: ""
10+
quorum:
11+
minSyncReplicas: 0
12+
maxSyncReplicas: 0
13+
users:
14+
sysdbuser: {}
15+
databases:
16+
appdb: {}
17+
resources: {}
18+
resourcesPreset: "nano"

0 commit comments

Comments
 (0)