Skip to content

refactor(nix,postgres): provision database declaratively, connect via socket#5337

Open
JakobLichterfeld wants to merge 8 commits into
mainfrom
feat-nix-postgresql-config-refactoring
Open

refactor(nix,postgres): provision database declaratively, connect via socket#5337
JakobLichterfeld wants to merge 8 commits into
mainfrom
feat-nix-postgresql-config-refactoring

Conversation

@JakobLichterfeld

@JakobLichterfeld JakobLichterfeld commented May 15, 2026

Copy link
Copy Markdown
Member

Replace the single initialScript with declarative ensureDatabases and ensureUsers, freeing the services.postgresql.initialScript option, which is single-valued and could not be combined with other modules that need it (e.g. to CREATE EXTENSION).

TeslaMate now connects to a local PostgreSQL via the unix socket using peer authentication, so no password is sent over TCP and startup no longer depends on the password being set first. The role password is still applied at runtime from DATABASE_PASS via postStart (kept out of the Nix store) because Grafana and the TCP fallback still require it.

Additionally read RELEASE_COOKIE without sourcing the env file
Sourcing the environment file executed it as a shell script (arbitrary code execution if a secret contained $(...) or backticks) and broke on values containing quotes. Extract RELEASE_COOKIE literally with sed
instead and strip one optional surrounding pair of double quotes, so the value is handled safely whether or not it is quoted in the file.
This closes #4904

@JakobLichterfeld JakobLichterfeld added the enhancement New feature or request label May 15, 2026
@netlify

netlify Bot commented May 15, 2026

Copy link
Copy Markdown

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit bf95352
🔍 Latest deploy log https://app.netlify.com/projects/teslamate/deploys/6a5fa99ce04c320008efcc0b
😎 Deploy Preview https://deploy-preview-5337--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@JakobLichterfeld JakobLichterfeld modified the milestone: v3.1.0 May 15, 2026
@JakobLichterfeld
JakobLichterfeld marked this pull request as draft May 15, 2026 17:09
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

initialDatabases is not in NixOS 25.11, therefore we need to wait with this till it is releases

@brianmay

Copy link
Copy Markdown
Collaborator

I would really like to update this sometime to use the socket connection to the database by default rather then the TCP connection. Not only is there likely to be a tiny performance gain, but means there is one less password to worry about.

Not sure about grafana though, is that supports socket connections or not. And it probably should have a dedicated postgresql user too.

Don't know if any of this is in scope for this change or not. But maybe something to think about.

@JakobLichterfeld

JakobLichterfeld commented May 17, 2026

Copy link
Copy Markdown
Member Author

Exactly. I wonder the same, would it be possible to use socket connection and grafana (yes I do it similar with Prometheus in my homelab) and TeslaMate as well to connect to the DB at the same time? Why not.

As it seems like the options I tried to use won't be available (old links in mynixos.com) I will look more into it.

@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 4a82f1e to e712294 Compare May 18, 2026 12:39
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

As it seems like the options I tried to use won't be available (old links in mynixos.com) I will look more into it.

Replaced with the familiar once and use ensureClauses for superuser and password set. The latter needs testing.

@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from e712294 to 9d9df9f Compare May 18, 2026 12:57
@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 9d9df9f to f8ed8a2 Compare June 15, 2026 13:39
@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from f8ed8a2 to 0260706 Compare June 25, 2026 13:54
@JakobLichterfeld JakobLichterfeld changed the title refactor(nix,postgres): update initial database setup to use structured configuration to improving readability and maintainability refactor(nix,postgres): provision database declaratively, connect via socket Jun 25, 2026
@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 0260706 to cb34f3f Compare June 25, 2026 14:26
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

Replaced with the familiar once and use ensureClauses for superuser and password set. The latter needs testing.

testing showed it didn't work as expected for password, so I solved it correctly.

I would really like to update this sometime to use the socket connection to the database by default rather then the TCP connection

I refactored to use socket connection for TeslaMate in default.

@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 187a076 to 490fce2 Compare June 25, 2026 14:41
@JakobLichterfeld
JakobLichterfeld marked this pull request as ready for review June 25, 2026 17:03
@JakobLichterfeld

JakobLichterfeld commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

I also fixed the RELEASE_COOKIE sourcing issue (from #4904)

@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 89ee6a5 to 8f4b376 Compare June 26, 2026 09:00
@JakobLichterfeld JakobLichterfeld added this to the v4.1.0 milestone Jun 26, 2026
@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch 3 times, most recently from a0f0644 to 682b7df Compare June 27, 2026 11:00
@JakobLichterfeld JakobLichterfeld added the nix releated to nix flake label Jun 28, 2026
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

Self-review notes (maintainer hat on)

Since this touches provisioning and secrets handling, here is a summary of what I verified before asking for review:

Verified:

  • DATABASE_SOCKET_DIR support already exists in config/runtime.exs; the socket branch is only enabled when postgres.enable_server && postgres.user == "teslamate", which is exactly the case where peer auth works (the systemd unit runs as OS user teslamate). Any other role name or an external DB falls back to TCP with password, unchanged.
  • ensureDatabases/ensureUsers are idempotent, so existing installs migrate cleanly: DB and role already exist, ensureDBOwnership merely transfers ownership. Nice side effect: the password is now (re-)applied on every activation, so rotating DATABASE_PASS in the secrets file finally takes effect — initialScript only ever ran on first init.
  • \getenv + :'password' keeps the secret off the command line and quotes it correctly for any value (including single quotes). Requires psql >= 14; our default is postgresql_17 and the requirement is documented in a comment.
  • The sed-based RELEASE_COOKIE extraction handles values containing quotes, $(...) and backticks safely; tail -n1 mirrors systemd's last-assignment-wins.

Known minor follow-ups (happy to fix in this PR if reviewers agree):

  • The : ${RELEASE_COOKIE?...} guard should become : ${RELEASE_COOKIE:?...} — the sed extraction always sets the variable (possibly empty), so the unset-only form can no longer fire.
  • If DATABASE_PASS is missing from the secrets file, the postStart fails with a rather opaque psql syntax error; an explicit test -n guard with a clear message would be friendlier.

Tested on my own NixOS install (fresh provisioning and upgrade from the initialScript state). CI is green. Review welcome, especially on the systemd ordering (after = postgresql-setup.service).

🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source

@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 682b7df to 0b835b3 Compare July 20, 2026 14:12
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

Both follow-ups from my self-review notes above are now fixed:

  • : ${RELEASE_COOKIE?...}: ${RELEASE_COOKIE:?...} in both maintenance scripts, so the guard also catches an empty value (the sed extraction always sets the variable, possibly to the empty string).
  • The postStart of postgresql-setup.service now fails early with a clear message pointing at services.teslamate.secretsFile when DATABASE_PASS is missing, instead of an opaque psql syntax error from the unsubstituted :'password'.

While at it, the secretsFile option docs now mention that the file uses systemd EnvironmentFile syntax, which is why one surrounding pair of double quotes is stripped when extracting values.

🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source

… socket

Replace the single `initialScript` with declarative `ensureDatabases` and `ensureUsers`, freeing the `services.postgresql.initialScript` option, which is single-valued and could not be combined with other modules that need it (e.g. to `CREATE EXTENSION`).

TeslaMate now connects to a local PostgreSQL via the unix socket using peer authentication, so no password is sent over TCP and startup no longer depends on the password being set first. The role password is still applied at runtime from DATABASE_PASS via `postStart` (kept out of the Nix store) because Grafana and the TCP fallback still require it.
Sourcing the environment file executed it as a shell script (arbitrary code execution if a secret contained $(...) or backticks) and broke on values containing quotes. Extract RELEASE_COOKIE literally with sed
instead and strip one optional surrounding pair of double quotes, so the value is handled safely whether or not it is quoted in the file.
The password was interpolated into the SQL string as '$DATABASE_PASS', which breaks (and risks SQL injection) for passwords containing single quotes. Read it from the environment with psql's \getenv and quote it with :'password', producing a correctly escaped SQL string literal. The password is no longer placed on the command line either. Requires PostgreSQL >= 14 for \getenv.
@JakobLichterfeld
JakobLichterfeld force-pushed the feat-nix-postgresql-config-refactoring branch from 38e96c8 to 5a86e81 Compare July 20, 2026 16:51
@brianmay

Copy link
Copy Markdown
Collaborator

This mostly looks good to me.

AI Report:

PR #5337 Review Summary

Refactor of NixOS module's PostgreSQL setup, moving password configuration from initialScript (which sourced env files as shell scripts) to a postgresql-setup.postStart step using \getenv. Overall a security improvement.

Action recommended before merge:

  1. Standby/replica breakage (low severity)nix/module.nix postStart: ExecStartPost runs even when ExecStart short-circuits on ``. On a read-only standby, ALTER USER ... WITH ENCRYPTED PASSWORD will fail with `ON_ERROR_STOP=1`, blocking the `teslamate` service. Guard with `[[ -f ${cfg.dataDir}/standby.signal ]] && exit 0` (or `pg_is_in_recovery`).

Informational:
2. \getenv requires PostgreSQL ≥ 14, but postgres.package is user-overridable. An assertion (lib.versionAtLeast cfg.postgres.package.version "14") would surface misconfig earlier — not a bug with defaults.
3. Maintenance delete scripts (deleteDrive/deleteCharge) won't work with non-default postgres.user due to peer auth — pre-existing, but the PR widens the asymmetry with the new TCP fallback branching. Worth a docs note.

Verified correct — quote-stripping escapes, sed | tail -n1 last-wins semantics, RELEASE_COOKIE fail-fast, ensureDBOwnership conditional, After= no-op when server disabled, \getenv/:'password' SQL literal safety.

Want me to implement the standby guard fix?

Most of these are minor issues I think.

Not quite comprehending the "Standby/replica breakage" stuff right now. Possibly because I have never done this with postgresql :-)

…ersion

Address review feedback: ALTER USER cannot run on a read-only standby (the password is replicated from the primary anyway), so postStart now checks pg_is_in_recovery() and exits cleanly. Overriding postgres.package now fails at eval time unless it meets TeslaMate's runtime requirement
(>= 16.7 for 16.x, >= 17.3 for 17.x, or >= 18, mirroring
lib/teslamate/database_check.ex), instead of surfacing as a runtime
error; this also covers the psql \getenv used to set the role password.
@JakobLichterfeld

JakobLichterfeld commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough look, Brian!

Quick explainer on the standby point, since it's a bit obscure: a standby is a streaming-replication replica running in recovery mode — it is read-only, so any write statement fails, including ALTER USER. With ON_ERROR_STOP=1 that would have marked postgresql-setup.service as failed. In practice this is a mostly theoretical concern for this module (TeslaMate itself cannot run against a read-only database — migrations and inserts would fail immediately, and the role password is replicated from the primary anyway), but the guard is cheap, so I added it: postStart now checks SELECT pg_is_in_recovery() and exits cleanly on a standby. I went with pg_is_in_recovery() rather than checking for standby.signal, since the module doesn't reference the data directory.

Also addressed:

  • Added a version assertion, but stricter than suggested: TeslaMate's startup check (lib/teslamate/database_check.ex) already requires >= 16.7 (16.x), >= 17.3 (17.x) or >= 18, so the module now asserts exactly that at eval time — which subsumes the PostgreSQL >= 14 requirement for \getenv. Newer majors are allowed, mirroring the runtime check which only warns for those.
  • The peer-auth asymmetry in the delete scripts (deleteDrive/deleteCharge only work with the default role name) is pre-existing; I'll track it as a follow-up rather than widening this PR. --> fix(nix): maintenance and backup scripts fail with non-default postgres.user due to peer auth #5533

🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source

Without -d, psql connects to the database named after databaseUser, so the DROP SCHEMA/CREATE EXTENSION step ran against the wrong database whenever databaseName differs from databaseUser, while the subsequent restore targeted the correct one and failed on the existing data.
@JakobLichterfeld

JakobLichterfeld commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Pushed one more fix found while testing the restore path: fix(nix): drop schemas in the configured database during restore.

Without -d, psql connects to the database named after the user, so the DROP SCHEMA ... CASCADE / CREATE EXTENSION step ran against the wrong database whenever databaseName differs from databaseUser — worst case dropping schemas in an unrelated database that happens to share the user's name — while the subsequent restore targeted the correct database and then failed on the existing data. The restore invocation below already passed -d ${databaseName}; this makes the drop step consistent.

Two related observations, deliberately left for the follow-up issue (#5533) on the maintenance scripts: the backup/restore scripts share the same peer-auth limitation as the delete scripts (they only work with the default role name), and the restore psql invocations run without ON_ERROR_STOP, so individual failing statements don't abort the script.

🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source

The NixOS module defaults analytics.check_for_plugin_updates to
"declarativePlugins == null", so with our plain nixpkgs-managed plugins
the check ran every 10 minutes against grafana.com. Plugins only ever
change through nixpkgs here, so the check is pure log noise and an
unnecessary external call.
@JakobLichterfeld

Copy link
Copy Markdown
Member Author

One more small hardening found while reviewing the Grafana defaults: the NixOS module disables Grafana's update checks by default, except analytics.check_for_plugin_updates, which defaults to declarativePlugins == null — so with our nixpkgs-managed plugins it phoned grafana.com every 10 minutes for updates that can never be applied. Now explicitly disabled, matching the existing reporting_enabled = false.

🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request nix releated to nix flake

Projects

None yet

Development

Successfully merging this pull request may close these issues.

teslamate-close-charge: syntax error near unexpected token `newline'

2 participants