nixos/matrix-synapse: migrate to rfc42 settings and formatter #158605
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
mweinelt
force-pushed
the
synapse-rfc42
branch
from
d0722fe to
382feba
Compare
mweinelt
force-pushed
the
synapse-rfc42
branch
2 times, most recently
from
05d9fbd to
2afbef0
Compare
|
there has been a related/previous attempt in #120260 as well |
mweinelt
force-pushed
the
synapse-rfc42
branch
from
2afbef0 to
7aa64df
Compare
I consider that approach superseded. It does not really reflect what RFC42 intended and there hasn't been any work on it in a while. |
mweinelt
force-pushed
the
synapse-rfc42
branch
from
7aa64df to
976b075
Compare
github-actions
bot
added
8.has: changelog
8.has: documentation
mweinelt
commented
Feb 8, 2022
piegamesde
added
8.has: clean-up
8.has: module (new)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/4 |
|
I think I've addressed all remaining review comments. |
|
@ofborg test matrix-synapse matrix-appservice-irc mjolnir pantalaimon |
13 tasks
delroth
added a commit
to delroth/nixpkgs
that referenced
this pull request
Mar 12, 2022
pennae
added a commit
that referenced
this pull request
Mar 20, 2022
matrix-synapse: fix release notes and doc for #158605 changes
|
Has someone a good example how to generate the extraConfigFiles from sops-nix secrets? |
|
The synapse will fail to start with this error: We really need an environment file+splicing or ad hoc |
|
Sounds like an upstream problem with how they merge config files. Not sure we can do much about that. |
|
That was the case before this PR already IIRC. Also, you're free to file a patch %) |
Yes, sure, it's just that the docs now points to this option.
I will probably do so. I'm not sure what's the best way to do it, though. |
|
This was indeed a known issue #158605 (comment). I wish we had a standardized way to do secrethandling templating things in nixpgks since it's so common to need it |
12 tasks
Ma27
added a commit
to Ma27/nixpkgs
that referenced
this pull request
Sep 20, 2023
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`.
toastal
pushed a commit
to toastal/nixpkgs
that referenced
this pull request
Sep 25, 2023
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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
Update the module to use RFC42-style settings and formatters.
Migrations looks messy, but really isn't
For the most part options can be just moved into the
settingsattribute set, so migrations should still be fairly easy. Of course a proper changelog entry will cover the migration bits.It looks a bit messy with all the
mkRemovedOptionModuleimports, but the module just advertises an enormous amount of options. I tried to usemkRenamedOptionModule, but that is not compatible with submodules (cf. #96006).Removed options
I'm also removing a number of options that synapse doesn't even support anymore. And due to their lax config validation they never even complained about that.
Scope of advertised opotions
I'm also removing a number of options that are overly specific and expand the module beyond what the average joe would ever use. This is a freeform type now, so people can still set all the custom options they've ever dreamed of. In fact, if possible, I'd cull even more options still.
Resulting configuration
The rendered configurations of the matrix-synapse test look like this:
sqlite3
app_service_config_files: [] database: args: database: /var/lib/matrix-synapse/homeserver.db name: sqlite3 dynamic_thumbnails: false enable_metrics: false enable_registration: false listeners: - bind_addresses: - 0.0.0.0 port: 8448 resources: - compress: true names: - client - compress: false names: - federation tls: true type: http x_forwarded: true log_config: /var/lib/matrix-synapse/log_config.yaml max_image_pixels: 32M max_upload_size: 50M media_store_path: /var/lib/matrix-synapse/media_store pid_file: /run/matrix-synapse.pid presence: enabled: false report_stats: false server_name: serversqlite signing_key_path: /var/lib/matrix-synapse/homeserver.signing.key tls_certificate_path: /nix/store/hf0blv27r0b04w9qr3qnbi3lib8x0avj-matrix_cert.pem tls_private_key_path: /nix/store/2xzj0asq3iqxx194rm0sgbyq8jiqq00r-matrix_key.pem trusted_key_servers: - server_name: matrix.org verify_keys: ed25519:auto: Noi6WqcDj0QmPxCNQqgezwTlBKrfqehY1u2FyWP9uYw turn_shared_secret: '' turn_uris: [] url_preview_enabled: false url_preview_ip_range_blacklist: - 127.0.0.0/8 - 10.0.0.0/8 - 172.16.0.0/12 - 192.168.0.0/16 - 100.64.0.0/10 - 169.254.0.0/16 - ::1/128 - fe80::/64 - fc00::/7 url_preview_ip_range_whitelist: [] url_preview_url_blacklist: []psycopg2
app_service_config_files: [] database: args: database: matrix-synapse password: synapse user: matrix-synapse name: psycopg2 dynamic_thumbnails: false email: app_name: Matrix notif_from: matrix require_transport_security: true smtp_host: acme.test smtp_port: 25 enable_metrics: false enable_registration: false listeners: - bind_addresses: - 0.0.0.0 port: 8448 resources: - compress: true names: - client - compress: false names: - federation tls: true type: http x_forwarded: true log_config: /var/lib/matrix-synapse/log_config.yaml max_image_pixels: 32M max_upload_size: 50M media_store_path: /var/lib/matrix-synapse/media_store pid_file: /run/matrix-synapse.pid presence: enabled: false public_baseurl: https://example.com registration_shared_secret: unsecure123 report_stats: false server_name: serverpostgres signing_key_path: /var/lib/matrix-synapse/homeserver.signing.key tls_certificate_path: /nix/store/hf0blv27r0b04w9qr3qnbi3lib8x0avj-matrix_cert.pem tls_private_key_path: /nix/store/2xzj0asq3iqxx194rm0sgbyq8jiqq00r-matrix_key.pem trusted_key_servers: - server_name: matrix.org verify_keys: ed25519:auto: Noi6WqcDj0QmPxCNQqgezwTlBKrfqehY1u2FyWP9uYw turn_shared_secret: '' turn_uris: [] url_preview_enabled: false url_preview_ip_range_blacklist: - 127.0.0.0/8 - 10.0.0.0/8 - 172.16.0.0/12 - 192.168.0.0/16 - 100.64.0.0/10 - 169.254.0.0/16 - ::1/128 - fe80::/64 - fc00::/7 url_preview_ip_range_whitelist: [] url_preview_url_blacklist: []Scope of this PR
What I think is out of scope for this PR is:
Supersedes: #120260
Related: #144575
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes