Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instructions for running a Synapse server are wrong #285688

Closed
RaitoBezarius opened this issue Feb 2, 2024 · 6 comments · Fixed by #302586
Closed

Instructions for running a Synapse server are wrong #285688

RaitoBezarius opened this issue Feb 2, 2024 · 6 comments · Fixed by #302586
Assignees
Labels
0.kind: bug Something is broken

Comments

@RaitoBezarius
Copy link
Member

Describe the bug

The NixOS manual offers a way to run a Synapse server, but it gets wrong the PostgreSQL database setup.

It uses an initial script, but this means that you need to run PGSQL for the first time, which does not happen on an existing system.

Steps To Reproduce

Steps to reproduce the behavior:

  1. Have a system with pre-existing services using PostgreSQL.
  2. Deploy a Synapse server.

Expected behavior

Success.

Additional context

Recommended fix: use a preStart to perform the convergence properly, or work with PostgreSQL maintainers to find a suitable solution.

Notify maintainers

@Ma27 @fadenb @mguentner @Ralith @dali99 @sumnerevans @NickCao (package maintainers)

I had to do #285687 to find you :P.


Add a 👍 reaction to issues you find important.

@RaitoBezarius RaitoBezarius added the 0.kind: bug Something is broken label Feb 2, 2024
@SuperSandro2000
Copy link
Member

The proper way to fix this would probably be to allow running arbitrary SQL code on every postgres startup and in there create the database if it does not exist.

This could be executed from matrix-synapse with the postgres user but that's pretty hacky and would diverge from the standard database creation way a lot.

@RaitoBezarius
Copy link
Member Author

The proper way would be to put this logic in the preStart of matrix-synapse IMHO.

@RaitoBezarius
Copy link
Member Author

This is a special convergence logic special to Synapse.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Feb 2, 2024

The proper way would be to put this logic in the preStart of matrix-synapse IMHO.

The matrix-synapse postgres user does not have enough permissions to do that. Changing the user in preStart to postgres which has very wide permissions is not following steps for privilege separation. Also it would very likely not work for remote postgreses and building more logic around remote/local postgres is also not great.

Putting the logic for creating databases into the applications would also complicate any refractors to the postgres module and spreading the code for it into many other modules would make changes in the future a pain.

The Postgres module should offer an interface for this, as it clearly falls into postgres' domain to provision databases and users for applications.

This is a special convergence logic special to Synapse.

No? Creating a postgres database with specific settings like a none default locale is postgres' step. The application can then connect to the database and create the schema and insert data. If you want to provision minimal permissions then creating the schema should also be done separately but that makes migrations a bit more complicated and the application cannot do it automatically on startup anymore.


We should also probably move the provisioning of databases out of the postStart and move it into a separate service on which applications can depend and which can be easily restarted to provision new things without restarting the entire database.

@Ma27
Copy link
Member

Ma27 commented Feb 2, 2024

As a reminder, the synapse module used to have a way to automatically provision a postgres database: I had to patch it out in #80447 though because upstream mandated some configuration changes to the postgres database that were non-trivial to converge to. Because of that I have reservations about anything that does a setup automatically inside the module in an ad-hoc manner.

Also it would very likely not work for remote postgreses and building more logic around remote/local postgres is also not great.

What I'd suggest (and @RaitoBezarius probably has in mind as well) is adding some kind of $PSQL -c 'create database foo;' to a systemd unit (or prestart hook) in the docs. Perhaps with a note on how $PSQL should look like when connecting to a remote database.

It should also be noted that synapse - as every other module in NixOS as well - has very rudimentary support for provisioning databases and thus it's the administrator's responsibility to ensure that the SQL code remains valid. An alternative that should be noted - and is probably sufficient for ~95% of all setups - is to just execute the SQL manually and deploy synapse after that until there's a better solution.

@Ma27 Ma27 self-assigned this Feb 15, 2024
@Ma27
Copy link
Member

Ma27 commented Feb 15, 2024

Self-assigning: I intend to take care of this, it may take a bit until I get to it.

Ma27 added a commit to Ma27/nixpkgs that referenced this issue Apr 10, 2024
…n setup example

Closes NixOS#285688

This is misleading because `initialScript` will only be executed at the
*very first* run of postgresql. I.e. when deploying synapse to a server
with an existing postgresql, this won't work.

We don't have a good way of automatically provisioning databases
_declaratively_, so for now just explain what needs to be done here and
leave it to the user how to include this into their deployment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants