Skip to content
forked from NixOS/nixpkgs

Commit fc6692c

Browse files
Ma27toastal
authored andcommitted
nixos/matrix-synapse: refactor assertions for missing listener resources
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] NixOS#158605 (comment) [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`.
1 parent 6b9d53d commit fc6692c

File tree

1 file changed

+23
-16
lines changed

1 file changed

+23
-16
lines changed

nixos/modules/services/matrix/synapse.nix

+23-16
Original file line numberDiff line numberDiff line change
@@ -15,26 +15,26 @@ let
1515
usePostgresql && (!(args ? host) || (elem args.host [ "localhost" "127.0.0.1" "::1" ]));
1616
hasWorkers = cfg.workers != { };
1717

18+
listenerSupportsResource = resource: listener:
19+
lib.any ({ names, ... }: builtins.elem resource names) listener.resources;
20+
21+
clientListener = findFirst
22+
(listenerSupportsResource "client")
23+
null
24+
(cfg.settings.listeners
25+
++ concatMap ({ worker_listeners, ... }: worker_listeners) (attrValues cfg.workers));
26+
1827
registerNewMatrixUser =
1928
let
20-
isIpv6 = x: lib.length (lib.splitString ":" x) > 1;
21-
listener =
22-
lib.findFirst (
23-
listener: lib.any (
24-
resource: lib.any (
25-
name: name == "client"
26-
) resource.names
27-
) listener.resources
28-
) (lib.last cfg.settings.listeners) cfg.settings.listeners;
29-
# FIXME: Handle cases with missing client listener properly,
30-
# don't rely on lib.last, this will not work.
29+
isIpv6 = hasInfix ":";
3130

3231
# add a tail, so that without any bind_addresses we still have a useable address
33-
bindAddress = head (listener.bind_addresses ++ [ "127.0.0.1" ]);
34-
listenerProtocol = if listener.tls
32+
bindAddress = head (clientListener.bind_addresses ++ [ "127.0.0.1" ]);
33+
listenerProtocol = if clientListener.tls
3534
then "https"
3635
else "http";
3736
in
37+
assert assertMsg (clientListener != null) "No client listener found in synapse or one of its workers";
3838
pkgs.writeShellScriptBin "matrix-synapse-register_new_matrix_user" ''
3939
exec ${cfg.package}/bin/register_new_matrix_user \
4040
$@ \
@@ -44,7 +44,7 @@ let
4444
"[${bindAddress}]"
4545
else
4646
"${bindAddress}"
47-
}:${builtins.toString listener.port}/"
47+
}:${builtins.toString clientListener.port}/"
4848
'';
4949

5050
defaultExtras = [
@@ -937,6 +937,13 @@ in {
937937

938938
config = mkIf cfg.enable {
939939
assertions = [
940+
{
941+
assertion = clientListener != null;
942+
message = ''
943+
At least one listener which serves the `client` resource via HTTP is required
944+
by synapse in `services.matrix-synapse.settings.listeners` or in one of the workers!
945+
'';
946+
}
940947
{
941948
assertion = hasLocalPostgresDB -> config.services.postgresql.enable;
942949
message = ''
@@ -969,13 +976,13 @@ in {
969976
(
970977
listener:
971978
listener.port == main.port
972-
&& (lib.any (resource: builtins.elem "replication" resource.names) listener.resources)
979+
&& listenerSupportsResource "replication" listener
973980
&& (lib.any (bind: bind == main.host || bind == "0.0.0.0" || bind == "::") listener.bind_addresses)
974981
)
975982
null
976983
cfg.settings.listeners;
977984
in
978-
hasWorkers -> (listener != null);
985+
hasWorkers -> (cfg.settings.instance_map ? main && listener != null);
979986
message = ''
980987
Workers for matrix-synapse require setting `services.matrix-synapse.settings.instance_map.main`
981988
to any listener configured in `services.matrix-synapse.settings.listeners` with a `"replication"`

0 commit comments

Comments
 (0)