search

matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.83k stars 2.12k forks source link

Separate default configuration from configuration documentation #8159

Closed erdnaxeli closed 2 years ago

erdnaxeli commented 4 years ago

This is not really a bug but a really annoying behavior.

The debian package's configuration file change every time. Every time you update your synapse package, you have to go through the process of looking at the diff between the old and the new configuration file to check for relevant modifications. And often there are not.

For example when updating from 1.18.0+buster1 to 1.19.0+buster1 the changes where:

A very common and very frustrating change is the cosmetic one: removing (or adding) a space after a "#", fixing a typo, changing the indentation, …

This bother me every time I update my synapse. I also manage another one with an ansible playbook, and it makes me feel uncomfortable, because I have no way to known if I need to change my config file. Most of the time I do not change it, let it diverge from the one in the package, and every some months I go through the painful process of doing a full diff (btw finding the default conf is not obvious, the easiest way is to just to the update manually, going through the diff process with apt, and copy back the change to the config file in the ansible role).

My suggestion is that config files are not for documentation. That's a good idea when your config file is ten lines long and never changes, but that's not the case here. The nginx configuration documentation is not in the config file, neither the synapse one should. Please find somewhere else to put the doc, and leave our config file alone :p

If there is a relevant change, like an attribute changing name, or a new attribute, please indicate it in a changelog file (apt-listchanges will show it at installation). For everything else, like a documentation change or a typo fix, publish the new documentation somewhere but please (please!) don't do a config file diff.

Thanks you :)

richvdh commented 3 years ago

I would love for this to be fixed. The default config file should not be a 3000-line epic, but rather a minimal configuration that is just enough to get going.

(as an aside, a workaround for the particular pain felt by users of the Debian package is to put your local changes into separate config files in the conf.d directory. But it's definitely a workaround.)

reivilibre commented 3 years ago

11281 suggests Debian users to use conf.d, which again is just a workaround but thought it was worth linking to this issue.

anoadragon453 commented 2 years ago

Thank you for starting on this with #12368 @H-Shay! You mention in that PR that it's the first step in solving this issue. Could you perhaps list the rest of the steps here?

H-Shay commented 2 years ago

Sure, the next step as I see it would be to delete all the comments from the code that generates the config, so that the manual is the source of truth for configuration, rather than 3000+ lines of comments in the config itself. Contributors adding new config options would then document the new option in the manual rather than in the code that generates the config, as they do now.

This would leave a minimal config, which could take one of two forms: a. one where only the bare essentials for starting a homeserver (i.e. the values you are prompted for when generating the config) are output when the config is generated and admins would add options from the manual as they saw fit, or

b. one where the generated config outputs the bare essentials as above as well as the commented-out flags (but not the comments documenting those flags) and admins could then un-comment options as they saw fit based on guidance from the config manual

I personally find the first option to be preferable but I need to get feedback from the team, which is really the only thing blocking executing the next step.

anoadragon453 commented 2 years ago

@H-Shay I agree that I prefer the first option. Even option names become outdated as their associated features are deprecated, or they are renamed.

Thus I'd prefer the config manual be the single source of truth. The "example configuration" section of each option in the manual should be sufficient for people to copy/paste into their config if desired.

anoadragon453 commented 2 years ago

@H-Shay it would be really handy to do https://github.com/matrix-org/synapse/issues/12751 before / shortly after carrying out this work, as linking people to the documentation for a config option via the sample config is something I often do.

H-Shay commented 2 years ago

I can hop on that, no problem.

deepbluev7 commented 2 years ago

I, as a user, much prefer documentation in the config file. Yes, there are sometimes small merge conflicts when merging the config updates, but having to reach out to a different website, that might not be for the version of your config file or not reachable at the moment, is somewhat annoying. If the documentation is removed from the config file, can we get some offline docs maybe like a manpage or an accompanying file to the config file, that just has the option documentation and doesn't need to be merged, but can just be overwritten?

reivilibre commented 2 years ago

can we get some offline docs maybe like a manpage or an accompanying file to the config file, that just has the option documentation and doesn't need to be merged, but can just be overwritten?

All the docs on the site are markdown files in docs/. In this case docs/usage/configuration/config_documentation.md is the file in question.

deepbluev7 commented 2 years ago

I guess that works, I'll see if I can install that in some good location or point people to it in the config file. Thanks!

clokep commented 2 years ago

I had a couple of random thoughts about this last night (sorry for being a bit late to the game with taking a look at this!):

  1. The guide looks great as is, but it would be helpful to be able to link directly to a single configuration setting (imagine you're trying to figure out what tweak_foo does, it is helpful to link directly to those docs when chatting with your coworkers / or in #synapse:matrix.org; right now you can only link to the section the config is in, I think).
  2. It would be good to document when configuration settings were added / deprecated to this, to somewhat alleviate the concern above.
  3. An example that does some of the above really well is the PowerDNS documentation: https://doc.powerdns.com/recursor/settings.html (their debian packages also include a config file that has briefer comments for each option, I think.

Happy to see us working on this! 🎉

anoadragon453 commented 2 years ago

https://github.com/matrix-org/synapse/issues/11647 may also help with checking settings for a specific Synapse version.

richvdh commented 2 years ago

:tada:

immanuelfodor commented 2 years ago

Is there an option to make Synapse generate the full default configuration when needed? I've been git diff-ing the whole default config across updates to see the changes in defaults or newly introduced options/removals. It would be great to maintain this workflow even without the config docs among options, just to be able to see what is the actual default config a release assumes internally. I could then git diff it, and check the separate docs when needed, so I wouldn't need to rely on the release notes which could possibly introduce an error/attack vector on default/new options. Is there a way to make Synapse generate the default confirm YAML?

eibhear-from-athlone commented 2 years ago

I agree with @immanuelfodor: as an admin, diffing the config file generated by the most recent release gives the quickest and most precise detail as to what config items are being added, deprecated, removed or having their utilisation changes (e.g. changing the default behaviour from True to False).

The advice on the Synapse Admins channel is to consult the upgrade notes and change log, but these often don't provide exact details, or all the details.

The file config_documentation.md also doesn't help much, as it (for example) between 1.61.0 and 1.62.0 there 259 diffs to sift through, when typically the max no. of differences between two releases in the generated documentation would be around 5 or so.

If this change is to remain in place, I would ask that we get some means of quick and easy config comparisons between releases. One example might be a config lint command that we could run that would give us some of this information.

Thanks.

anoadragon453 commented 2 years ago

@immanuelfodor @eibhear-from-athlone Part of this work was removing the majority of the YAML generating code, which was used both for generating docs/sample_config.yaml as well as the default config file that is created when Synapse is installed. Our goal was the reduce the size of the latter, and found that the former - which was primarily used for referencing the available config options - could be replaced by the config manual.

I would still suggest diffing the config manual document, which would be today's equivalent of diffing the sample config file. The reason for the large amount of changes between 1.61 and 1.62 is https://github.com/matrix-org/synapse/pull/13055, which modified the layout of the document. If you exclude the merge commit https://github.com/matrix-org/synapse/commit/cba1c5cbc293b2601d81b0cb9b1a28ec6f1e26a1 from your diff, it should reduce the number of changes you see:

git checkout release-v1.62
git revert cba1c5cbc293b2601d81b0cb9b1a28ec6f1e26a1
git diff release-v1.61 HEAD docs/usage/configuration/config_documentation.md

It's not a perfect solution though, as reverting here actually produces a (simple, but annoying) merge conflict. Here is the diff after resolving the merge conflict:

config diff ```diff --- -Config option: `soft_file_limit` +Config option: `extra_well_known_client_content ` + +This option allows server runners to add arbitrary key-value pairs to the [client-facing `.well-known` response](https://spec.matrix.org/latest/client-server-api/#well-known-uri). +Note that the `public_baseurl` config option must be provided for Synapse to serve a response to `/.well-known/matrix/client` at all. + +If this option is provided, it parses the given yaml to json and +serves it on `/.well-known/matrix/client` endpoint +alongside the standard properties. + +Example configuration: +```yaml +extra_well_known_client_content : + option1: value1 + option2: value2 +``` +--- +### `soft_file_limit` Set the soft limit on the number of file descriptors synapse can use. Zero is used to indicate synapse should set the soft limit to the hard limit. @@ -1137,8 +1153,8 @@ Caching can be configured through the following sub-options: * `sync_response_cache_duration`: Controls how long the results of a /sync request are cached for after a successful response is returned. A higher duration can help clients with intermittent connections, at the cost of higher memory usage. - By default, this is zero, which means that sync responses are not cached - at all. + A value of zero means that sync responses are not cached. + Defaults to 2m. * `cache_autotuning` and its sub-options `max_cache_memory_usage`, `target_cache_memory_usage`, and `min_cache_ttl` work in conjunction with each other to maintain a balance between cache memory usage and cache entry availability. You must be using [jemalloc](https://github.com/matrix-org/synapse#help-synapse-is-slow-and-eats-all-my-ramcpu) @@ -2946,8 +2962,10 @@ Additional sub-options for this setting include: tokens. Defaults to false. * `secret`: This is either the private shared secret or the public key used to decode the contents of the JSON web token. Required if `enabled` is set to true. -* `algorithm`: The algorithm used to sign the JSON web token. Supported algorithms are listed at - https://pyjwt.readthedocs.io/en/latest/algorithms.html Required if `enabled` is set to true. +* `algorithm`: The algorithm used to sign (or HMAC) the JSON web token. + Supported algorithms are listed + [here (section JWS)](https://docs.authlib.org/en/latest/specs/rfc7518.html). + Required if `enabled` is set to true. * `subject_claim`: Name of the claim containing a unique identifier for the user. Optional, defaults to `sub`. * `issuer`: The issuer to validate the "iss" claim against. Optional. If provided the @@ -3578,3 +3596,4 @@ background_updates: min_batch_size: 10 default_batch_size: 50 ```

You'd run into the same problem with the old generated YAML as well though. If we changed our config file formatting, then it would also produce a massive diff between Synapse versions. Large changes like cba1c5cbc293b2601d81b0cb9b1a28ec6f1e26a1 tend to not happen very frequently though - in this case it was left over work from the transition from documentation-in-yaml to markdown, which this issue describes.

clokep commented 2 years ago

diffing the config file generated by the most recent release gives the quickest and most precise detail as to what config items are being added, deprecated, removed or having their utilisation changes (e.g. changing the default behaviour from True to False).

This should also still be possible -- if anything the diff should be reduced since it doesn't include the documentation.

Edit: Never mind, the sample config is smaller now than I thought it was.

immanuelfodor commented 2 years ago

This should also still be possible -- if anything the diff should be reduced since it doesn't include the documentation.

Yes, I completely agree

Edit: Never mind, the sample config is smaller now than I thought it was

Even this edit doesn't change the case since the sample config will probably stay intact for a long time. But the sample config is not what interests us! The interesting part is what is not in the sample config, what only Synapse's internals have as options and option defaults. There still should be a way to get all the applied configuration of an instance, defaults merged with custom config options - if there are no custom config overrides, then it generates the internal defaults. Is it possible?

eibhear-from-athlone commented 2 years ago

Thanks @anoadragon453. The ease with which differences could be identified and immediately applied into the existing homeserver.yaml file will be hard now to replicate. This is a pity. It's important for the management of my homeserver -- and for it's security -- that the config file is properly up-to-date with respect to deprecated and removed config options, or how they are defaulted. And, of course, new options that are being introduced may not be so obvious any more, thus making it harder to decide whether they're worth exploring and trying out.

This change makes all of that harder, but I'll give it a few releases to see how it pans out.

immanuelfodor commented 2 years ago

I truly feel like this PR has removed a lot of transparency, I feel less ownership over my instance since I don't know if the applied configuration would compromise it or not, or just weaken security, or anything, since I can't check it myself as before. I need to trust a separate document now which is unlinked from the actual code running, this is a step back for admins even though it might be easier to maintain for the developers :(

immanuelfodor commented 2 years ago

For a quick example, these changes are what frighten me the most, from a couple of releases earlier:

SmartSelect_20220705-171009_Termux

I didn't specify this option in my config before, this is set by the server only. One small miss in the release notes either by me to look over it or by the developers to not include it, and boom, there is now a less secure server, or less private, or anything alike.

I want to make sure that I know what the server's actual internal config state is, with what configuration it is running to maintain the same transparency and confidence in it as before. I ask the team to please give the transparency/control back to us, admins over the config :)

reivilibre commented 2 years ago

One small miss in the release notes either by me to look over it or by the developers to not include it, and boom, there is now a less secure server, or less private, or anything alike.

This is purely the opposite, the default configuration value changed from 'less private' to 'more private' (as we realised that it's a loss of privacy for not much of a good reason). This isn't about reducing control, or transparency (it's the opposite). Your config file would have given you the impression that you were running a less secure configuration with a different default value to what it actually is. This is why we consider having the documentation for config options in the config file itself harmful: because nobody regenerates their config on every update.

The developers have some duty of care to ensure that the defaults are sensible for admins that don't want to/shouldn't need to care about all the whimsical aspects of Synapse. If you are absolutely sure you want some behaviour, you are always free to set the setting manually of course.

I need to trust a separate document now which is unlinked from the actual code running

The text words in a person's configuration generated back in v1.36.0 are unlinked from the code running of Synapse at version v1.61.0. Consulting the correct manual (or sample config) for your version of Synapse is the only way to know you're getting up-to-date information about what the flags do.

erdnaxeli commented 2 years ago

I truly feel like this PR has removed a lot of transparency, I feel less ownership over my instance since I don't know if the applied configuration would compromise it or not, or just weaken security, or anything, since I can't check it myself as before. I need to trust a separate document now which is unlinked from the actual code running, this is a step back for admins even though it might be easier to maintain for the developers :(

First I agree with @reivilibre: you can trust your config file only if you carefully apply the diff changes on every new release. If not, the comments on your config file are outdated on the best case and wrong on the worst.

Second, that's how every app I know works. When you install Apache or Nginx you don't get a big config file with all the documentation, you only get the bare minimum to make it works with what developers (or the package maintainer) assume are good default. And I think that's ok. With that, newly introduced config variables do not break your update command (by asking you what you want to do with the config file), but they have a default value to make it works, and if you want to know about it you need to check your app version's documentation.

If there is a breaking change or a security issue that implies that you must change your configuration, some distributions provide tools to warn the administrators. For apt there is apt-listchanges for example. But this only works if you use directly the tool of your distribution. If you use a system management tool like Ansible, there is no way to be warn if something changes, no matter if the documentation is in the config file or not, because those tools are automation tools and do not ask if you want to do a diff. If you use those kind of tools, it is excepted that you check on update if the config has changed.

To conclude: I think it is a best practice to

eibhear-from-athlone commented 2 years ago

I think the push back @immanuelfodor and I are getting on this is a bit unfair.

We had upgrade processes that have been broken by this change, processes that gave us confidence as we applied upgrades, and gave us understanding of how our homeservers were going to be affected.

For me, ever since I started using the docker image at v1.32, it has been straight-forward:

  1. Generate a new homeserver.yaml file with the new release
  2. Compare the new file with my existing one to see what's changed
  3. Apply my homeserver's configs to this new file, catering, where relevant, to the changes in this release.
  4. Put this new file into place
  5. Upgrade my homeserver.

This meant that at all times my homeserver.yaml file was using the most up-to-date version.

I'm sure others operate their homeservers differently, but that doesn't at all say that our approaches are invalid.

My upgrade process will now be lengthened, and made more fraught, IMO.

you can trust your config file only if you carefully apply the diff changes on every new release.

I, for one, did exactly this. I would be surprised to learn that I'm unique. As I say elsewhere, it's a usage pattern that wasn't considered fully with this change.

Second, that's how every app I know works

... and it was refreshing that synapse did it differently!

If there is a breaking change or a security issue that implies that you must change your configuration, some distributions provide tools to warn the administrators.

This is a breaking change -- it breaks my upgrade process. Unusually, though, this is the first breaking change I remember since I started managing my homeserver at 0.9x for which synapse gave us no warning.

immanuelfodor commented 2 years ago

I admit that my example was a bit misleading in a way that it became more secure in this case but it could be the other way round, when the team decides that the sane defaults that work for most people would imply a compromise I'm not willing to take. The provided diff example illustrated an internal default change to the exact opposite than it was before. This is what disturbs me, now I can't see the change in place, I need to consult another information source which is maintained separately than the code and error prone even with the best intentions. It was indeed an epic long config file before, and took long to scroll it over, but I surely trusted it more than walking blind.

I agree with @eibhear-from-athlone, I even automated the upgrade change detection with a separate config generator Kubernetes job and some small scripts to help me getting the config diff easily. It worked fine since then, I could do it in my sleep. Now it feels like the rug has been pulled off from under me - or whatever idiom you say for this in English to start over and build up from scratch.

I understand that config changes were a pain for Debian users, so you turned it off, but for Docker, or even for Debian, why not keep the functionality alive to be able to get the actual running config somehow with a CLI option, and just turn off the default config generation to let both party please? I'm sure it could be solved to have a --print-applied-config param that prints out the merged defaults and custom options (if any) like nginx does with the -T flag to print out the whole actual nginx config file with flattened config includes over several folders.

reivilibre commented 2 years ago

First of all, I appreciate that this has bit you in the bum and that it's a break in your workflow, but let's try and put it right, whilst also considering the reasons for this change to be made.

I think I understand your points, but I think they represent quite special cases for unusually diligent administrators (which is great, but we also need something that works well for a lower-maintenance style of administration).

For me, ever since I started using the docker image at v1.32, it has been straight-forward:

Generate a new homeserver.yaml file with the new release
Compare the new file with my existing one to see what's changed
Apply my homeserver's configs to this new file, catering, where relevant, to the changes in this release.
Put this new file into place
Upgrade my homeserver.

I hate speaking without numbers, but I think most homeserver administrators don't go through such a procedure when upgrading. It's respectable that you do, but it's a very arduous process on top of what is already just mundane busywork (upgrading a server). However, the principles of this procedure are still possible: the only thing you need to change is to look at changes to the configuration manual rather than to the sample configuration.

What changes now is that the people that don't do this aren't going to have outdated comments lying around in their config file. Additionally, it's easier to navigate because it isn't a 4000-line file — instead, there are sections. Most of them can be ignored by most admins since you may not care about e.g. SSO, etc.

This meant that at all times my homeserver.yaml file was using the most up-to-date version.

If you generate a config file today and leave it alone across upgrades, your configuration is up-to-date — automatically — because there's very little to get out of date (and options that you specify which actually are out of date will generate warnings in the logs).

This is in contrast to before where homeserver admins had to put that effort in.

I will note from personal experience that we've had to change defaults that would mean that the residual comments in a config file are not only misleading, but actually entirely wrong (the opposite).

I admit that my example was a bit misleading in a way that it became more secure in this case but it could be the other way round, when the team decides that the sane defaults that work for most people would imply a compromise I'm not willing to take.

This can already happen today — although in my experience we are very averse to assuming too much. Your workflow of viewing the diff can still be used to avoid that, it's just a different file to diff.

I need to consult another information source which is maintained separately than the code and error prone even with the best intentions

The sample config is also separate to the code, so I don't think this is any worse.

I even automated the upgrade change detection with a separate config generator Kubernetes job

Sorry for breaking it, though I think this would have been very hard for us to know about.

This may well work for you, but it's very hard for us to guarantee that this will keep working since I've never heard of it or even anyone doing anything similar. — I would never have dreamt someone was doing that. I don't think it's reasonable to expect small homeserver admins to have such a generator job, though.

I'm sure it could be solved to have a --print-applied-config param that prints out the merged defaults and custom options (if any) like nginx does with the -T flag to print out the whole actual nginx config file with flattened config includes over several folders.

I understand the desire for this, but it's likely to be a large amount of work for the actual benefit it will bring. If you are interested enough to the point of PRing it, then I expect it could be considered.

I think your main point comes down to you not trusting the manual as much as the sample config (is that right?). Is there something else that can be done that is more trustworthy, or some way that will make you trust the manual more?

I will note that I don't think there's much/any difference to before. Both sample config and manual are separate to the code. Generating config documentation from code is ... interesting ... but I'm not aware of any such approach in Python :-).

immanuelfodor commented 2 years ago

Good points @reivilibre , I really appreciate the effort you've put into your reply, so thank you! I'm on the brink of letting my previous (precious) update workflow go but let me suggest one more thing.

I assume there must be a point in Synapse where all the user-defined configuration has been read and merged with the internal defaults. At this point, could you dump the whole config structure to YAML and print it out to the logger controlled by a debug flag? Then Matrix would start up just like before. There might be a verbose log flag already (I haven't checked now), this could output the whole actual config at the beginning, then continue running Synapse as it would. We could grab the config there and check whether there is a change, so you wouldn't need to introduce any new CLI flags, just add an if and a YAML dump in the existing code at the right point.

On the other hand, if the GitHub release notes could include the config changes just like the upgrade notes file from version to version, it would be great, and provide the config change visibility we are after.

eibhear-from-athlone commented 2 years ago

I think your main point comes down to you not trusting the manual as much as the sample config (is that right?).

For me, no. It's that well-formed examples of configuration settings, and the documentation that described them, were in the one resource, and that I consulted that resource as a matter of course when upgrading, ensuring that any changes thereto were appropriately applied to my config file. It wasn't ever about trust for me, just a very easy means to make sure my config file kept up with the changes.

Is there something else that can be done that is more trustworthy, or some way that will make you trust the manual more?

For me, yes.

DMRobertson commented 2 years ago

To quote myself on #13189:

FWIW, the best reference for changes are the changelog and the admin-specific upgrade notes. Perhaps we should highlight new config options in the upgrade notes?

reivilibre commented 2 years ago

I assume there must be a point in Synapse where all the user-defined configuration has been read and merged with the internal defaults.

Well you're not wrong, but the way this is done makes it hard to get the defaults in the same structure as the original configuration, I fear. The merged configuration is more or less produced as arbitrary Python objects, where the fields may not have the same names as config options and values might be lightly translated as they have been parsed. (e.g. 5m becomes 300000 or so and some of the worker options don't track the original values in the config, but instead about what the code cares about — i.e. NOT 'who is updating the user directory' but instead 'is this worker updating the user directory?')

Doing anything different (perhaps as an intermediate step) is not a bad idea, but it would be some work.

However, I think we can potentially do something about these points:

immanuelfodor commented 2 years ago

Aaaaand here we are with the new release , and the config has diverged from the docs, what a surprise 😃

Hint: see the per issuer option here: https://github.com/matrix-org/synapse/pull/13125/files
No mention here: https://github.com/matrix-org/synapse/blob/v1.63.0/docs/usage/configuration/config_documentation.md#rc_invites

So the recommended "pls diff the docs" approach fails here.

Do you folks really expect us now to manually review every single PR included in the release notes to find the config changes? How much config has been altered besides this one, can anyone tell it for sure?

The removed transparency over the full set of available config options and timely changes open an easy way to slip in changes without notice. I feel this situation will get worse over time, this is just the first example, I expect there will be more and more undocumented options from now on. My Homeserver's config has just slipped out of my control/hands.

DMRobertson commented 2 years ago

Hint: see the per issuer option here: #13125 (files) No mention here: https://github.com/matrix-org/synapse/blob/v1.63.0/docs/usage/configuration/config_documentation.md#rc_invites

This is an oversight, but it wasn't helped by the fact that this PR landed during the transition to the new config manual. We can fix this up in #13330.

richvdh commented 2 years ago

I'm sorry that #13125 failed to mention the change in the documentation, but I don't accept it's due to this change. It's human error, and it frequently got forgotten before this change.

immanuelfodor commented 2 years ago

This is why a process is needed to eliminate such errors. Code+config+docs should be tied together and be transparent for Homeserver admins.

Yoric commented 2 years ago

My apologies, I should have documented this. I absolutely agree that a process would be quite useful.

DMRobertson commented 2 years ago

Now added to https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#rc_invites.