Closes#216989
First of all, a bit of context: in PostgreSQL, newly created users don't
have the CREATE privilege on the public schema of a database even with
`ALL PRIVILEGES` granted via `ensurePermissions` which is how most of
the DB users are currently set up "declaratively"[1]. This means e.g. a
freshly deployed Nextcloud service will break early because Nextcloud
itself cannot CREATE any tables in the public schema anymore.
The other issue here is that `ensurePermissions` is a mere hack. It's
effectively a mixture of SQL code (e.g. `DATABASE foo` is relying on how
a value is substituted in a query. You'd have to parse a subset of SQL
to actually know which object are permissions granted to for a user).
After analyzing the existing modules I realized that in every case with
a single exception[2] the UNIX system user is equal to the db user is
equal to the db name and I don't see a compelling reason why people
would change that in 99% of the cases. In fact, some modules would even
break if you'd change that because the declarations of the system user &
the db user are mixed up[3].
So I decided to go with something new which restricts the ways to use
`ensure*` options rather than expanding those[4]. Effectively this means
that
* The DB user _must_ be equal to the DB name.
* Permissions are granted via `ensureDBOwnerhip` for an attribute-set in
`ensureUsers`. That way, the user is actually the owner and can
perform `CREATE`.
* For such a postgres user, a database must be declared in
`ensureDatabases`.
For anything else, a custom state management should be implemented. This
can either be `initialScript`, doing it manual, outside of the module or
by implementing proper state management for postgresql[5], but the
current state of `ensure*` isn't even declarative, but a convergent tool
which is what Nix actually claims to _not_ do.
Regarding existing setups: there are effectively two options:
* Leave everything as-is (assuming that system user == db user == db
name): then the DB user will automatically become the DB owner and
everything else stays the same.
* Drop the `createDatabase = true;` declarations: nothing will change
because a removal of `ensure*` statements is ignored, so it doesn't
matter at all whether this option is kept after the first deploy (and
later on you'd usually restore from backups anyways).
The DB user isn't the owner of the DB then, but for an existing setup
this is irrelevant because CREATE on the public schema isn't revoked
from existing users (only not granted for new users).
[1] not really declarative though because removals of these statements
are simply ignored for instance: https://github.com/NixOS/nixpkgs/issues/206467
[2] `services.invidious`: I removed the `ensure*` part temporarily
because it IMHO falls into the category "manage the state on your
own" (see the commit message). See also
https://github.com/NixOS/nixpkgs/pull/265857
[3] e.g. roundcube had `"DATABASE ${cfg.database.username}" = "ALL PRIVILEGES";`
[4] As opposed to other changes that are considered a potential fix, but
also add more things like collation for DBs or passwords that are
_never_ touched again when changing those.
[5] As suggested in e.g. https://github.com/NixOS/nixpkgs/issues/206467
Right now there's no trivial way to override parts of synapse's log
config such as the log-level because the only thing that's changeable is
the path to the log-file used by synapse and its workers.
Now, there's a new option called `services.matrix-synapse.log`
which contains the default log config as Nix attribute-set (except
`handlers.journal.SYSLOG_IDENTIFIER`). It has default priority, so new
things can be added like
services.matrix-synapse.log = {
my.extra.field = 23;
}
without discarding the rest. If desired, this can still be done via
`lib.mkForce`.
If the log configuration for a single worker or synapse, but not all
workers should be changed, `services.matrix-synapse.settings.log_config`
or `services.matrix-synapse.workers._name_.worker_log_config`
can be used.
Closes#236062
The PR #236062 was submitted because of the following problem: a synapse
instance was running in a NixOS container attached to the host network
and a postgresql instance on the host as database. In this setup,
synapse connected to its DB via 127.0.0.1, but the DB wasn't locally set
up and thus not configured in NixOS (i.e.
`config.services.postgresql.enable` was `false`). This caused the
assertion removed in this patch to fail.
Over three years ago this assertion was introduced when this module
stopped doing autoconfiguration of postgresql entirely[1] because a
breaking change in synapse couldn't be managed via an auto-upgrade on
our side. To make sure people don't deploy their DB away by accident,
this assertion was introduced.
Nowadays this doesn't serve any value anymore because people with
existing instances should've upgraded by now (otherwise it's their job
to carefully read the release notes when missing upgrades for
several years) and people deploying fresh instances are instructed by
the docs to also configure postgresql[2].
Instead, it only causes issues in corner cases like #236062, so after
some discussion in that PR I think it's time to remove the assertion
altogether.
Also, there's no `Requires=` for `postgresql.service` in the systemd
units which means that it's not strictly guaranteed that the DB is up
when synapse starts up. This is fixed now by adding `requires`. To avoid
being bitten by above mentioned cases again, this only happens if
`config.services.postgresql.enable` is `true`.
If somebody uses a non-local postgresql, but has also deployed a local
postgresql instance on the synapse server (rather unlikely IMHO), it's
their job to opt out of this behavior with `mkForce` (this is precisely one
of the use-cases `mkForce` and friends were built for IMHO).
[1] https://github.com/NixOS/nixpkgs/pull/80447
[2] https://nixos.org/manual/nixos/stable/#module-services-matrix-synapse
Currently, when setting a custom media_store_path, which lies outside of
cfg.dataDir, the current ReadWritePaths make it so that Synapse can't
access the media_store_path. So add the media_store_path to
ReadWritePaths to fix that.
While network.target only guarantees that network devices have been
created the `network-online.target` allows delaying service startup
until after a configurable network state has been reached.
This should resolve spurious failures, e.g. when synapse tries to load
the discovery information for its OIDC provider from a remote host.
While reviewing other changes related to synapse I rediscovered the
`lib.findFirst (...) (lib.last resources)` hack to find a listener
supporting the `client` resource. We decided to keep it that way for now
a while ago to avoid scope-creep on the RFC42 refactoring[1]. I wanted
to take care of that and forgot about it.
Anyways, I'm pretty sure that this is bogus: to register a user, you
need the `client` API and not a random listener which happens to be the
last one in the list. Also, you need something which serves the `client`
API to have the entire synapse<->messenger interaction working (whereas
`federation` is for synapse<->synapse).
So I decided to error out if no `client` listener is found. A listener
serving `client` can be defined in either the main synapse process or
one of its workers via `services.matrix-synapse.workers`[2].
However it's generally nicer to use assertions for that because then
it's possible to display multiple configuration errors at once and one
doesn't have to chase one `throw` after another. I decided to also error
out when using the result from `findFirst` though because module
assertions aren't thrown necessarily when you evaluate a single config
attribute, e.g. `config.environment.systemPackages` which depends on an
existing client listener because of `registerNewMatrixUser`[3].
While at it I realized that if `settings.instance_map` is wrongly
configured, e.g. by
settings.instance_map = mkForce {
/* no `main` in here */
}
an `attribute ... missing` error will be thrown while evaluating the
worker assertion.
[1] https://github.com/NixOS/nixpkgs/pull/158605#discussion_r815500487
[2] This also means that `registerNewMatrixUser` will still work if you
offload the entire `client` traffic to a worker.
[3] And getting a useful error message is way better for debugging in such a
case than `value is null while a set was expected`.
This should ensure systemd handles starting all services (main and
workers) in a single transaction, thus preserving unit orderings
defined through After= even when not restarting the target.
Actually, it's supposed to be `listOf (attrsOf str)` because each
list-item can match against multiple properties from `urlsplit`[1]. In
fact, `listOf str` breaks URL previews at runtime:
Sep 14 15:03:47 soost synapse[1100355]: synapse.http.server: [GET-116] Failed handle request via 'PreviewUrlResource': <XForwardedForRequest at 0x7f691bd5f730 method='GET' uri='/_matrix/media/r0/preview_url?url=<redacted>' clientproto='HTTP/1.1' site='8448'>
Traceback (most recent call last):
[...]
File "/nix/store/xk5yksbw09p6qwk0maq2cb2in3z6f4gn-matrix-synapse-1.91.2/lib/python3.10/site-packages/synapse/media/url_previewer.py", line 398, in _is_url_blocked
for attrib, pattern in entry.items():
AttributeError: 'str' object has no attribute 'items'
To make sure that people aren't confused when upgrading their configs, I
decided to work with `types.coercedTo` to "pretend" accepting the old
type signature, but then throwing an error explaining what to do (and
rejecting the broken configuration).
[1] https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlsplit
This implements proper merging of user-provided values and default
values for the settings option.
Without this, the user must provide a complete configuration file, as
changing a single setting replaces the entire JSON value, rendering the
default values completely useless. In fact, the bridge won't even start
using the default settings.
The DynamicUser mechanism does not allow share the bridge
registration.yaml file with matrix-synapse in any easy way:
1. the mautrix-whatsapp group under which the service runs is not in
/etc/group, so you can't add matrix-synapse to this.
This makes the group read bit on the file useless as the group is
effectively always empty.
2. It's not possible to use ACLs or copy the file during preStart or
postStart because the commands are executed with low priviledges.
By using a normal (static) user/group the secret can be shared with
synapse as follows:
services.matrix_synapse.settings.app_service_config_files =
[ "/var/lib/mautrix-whatsapp/whatsapp-registration.yaml" ];
users.users.matrix-synapse.extraGroups = [ "mautrix-whatsapp" ];
As suggested by @nickcao this commit moves the defaults back to the
options. Only `homeserver.domain` stays in the config section since the
documentation module does not support referencing attributes of other
modules.
