Config panel broken because of removed bind

[Gredin67]

what was the purpose of removing all bind in config_panel.toml with commit b435f31
With this, modifications made in homeserver.yaml in CLI are not taken into account in the config panel. And the config that you read in the config panel if the file was modified in CLI does not correspond to the actual synapse config.
So if I'm not wrong, either you make modifications in CLI or in the config panel.
And there are probably other drawbacks, such as crushing the previous config with the default one at upgrade. Keeping custom config at upgrade and at the same time taking into account upstream changes to the config file is one major benefit of the config panel from my point of view.

Could we put back the bind ?

[Gredin67]

While talking about config, I would set turn_allow_guests: false by default. People can still activate it in CLI.
https://github.com/YunoHost-Apps/synapse_ynh/blob/master/conf/homeserver.yaml#L1210C1-L1210C44

# Whether guests should be allowed to use the TURN server.
# This defaults to True, otherwise VoIP will be unreliable for guests.
# However, it does introduce a slight security risk as it allows users to
# connect to arbitrary endpoints without having first signed up for a
# valid account (e.g. by passing a CAPTCHA).
#
turn_allow_guests: false

Otherwise my understanding is that anonymous people could join visio or audio meeting, opening a door for a misusage.

[Josue-T]

Hello,

what was the purpose of removing all bind in config_panel.toml with commit b435f31 With this, modifications made in homeserver.yaml in CLI are not taken into account in the config panel. And the config that you read in the config panel if the file was modified in CLI does not correspond to the actual synapse config. So if I'm not wrong, either you make modifications in CLI or in the config panel..

Firstly, CLI to me is when we use the yunohost CLI yunohost app config ..., but seeing your comment from you it look like more just editing the config file with editor. Note that in yunohost context editing a config file which is managed by an app is not recommended and you could have some unexpected behaviour like having inconsistent values and generally you will lost all change after each upgrade.

So now about b435f31, here are the state before b435f31:

• All change done on the config panel was lost after each upgrade, because the new settings are just saved on the config file and not in the settings.
• All change done directly on the config file are lost after each upgrade.
• The change done directly on the config file was replicated on the config panel. But note that this is not recommended as said before.

After b435f31:

• The change done from the config panel are keep because we save all settings in the app settings.
• The change done directly on the config file are lost but is not a problem as it it shouldn't be done.
• The change done directly on the config file are not replicated on the config panel. But note that this is not recommended as said before.

And there are probably other drawbacks, such as crushing the previous config with the default one at upgrade. Keeping custom config at upgrade and at the same time taking into account upstream changes to the config file is one major benefit of the config panel from my point of view

Well, from what I know in yunohost there was never a system to give the possibility to update the config with the upstream change and in same time to keep the custom change. But well, maybe I pass around something and I would be happy to see some doc or code about this. 😉

Could we put back the bind ?

To me the previous comportment have too many issue so to me no. But you still can argue about this if you still think that there are a good reason to rollback this change.

While talking about config, I would set turn_allow_guests: false by default. People can still activate it in CLI. https://github.com/YunoHost-Apps/synapse_ynh/blob/master/conf/homeserver.yaml#L1210C1-L1210C44

# Whether guests should be allowed to use the TURN server.
# This defaults to True, otherwise VoIP will be unreliable for guests.
# However, it does introduce a slight security risk as it allows users to
# connect to arbitrary endpoints without having first signed up for a
# valid account (e.g. by passing a CAPTCHA).
#
turn_allow_guests: false

Otherwise my understanding is that anonymous people could join visio or audio meeting, opening a door for a misusage.

Please for a different question create a different issue, it's more simple to follow the discussion. 😉

It's already the case:

synapse_ynh/scripts/_common.sh

Line 189 in 1262627

allow_guest_access=false

But note at some time maybe it was different.

And you still can change the value from the config panel.

[zamentur]

• All change done on the config panel was lost after each upgrade, because the new settings are just saved on the config file and not in the settings.

In theory, the options in the config panel are saved in the file and in the settings too when you use the bind notation... That's the case for all other config panel.
I have checked and it seems the option was properly initialized and templated, so the change made from config panel should not disappears from the file even after an upgrade...

If you discover that the change disappears, it's a big bug and it would have been interesting to understand why these changes disappear and are not reapplied.

I see that in recent version, a templating approach is used to fill the homeserver conf after applying a config panel, this approach have the default that it erase all manual change (even if this change are made with a post_app_upgrade hook).

However i understand that we would like in this app manipulates some complex structure in yaml, things that are not managed by default in config panel. So i understand that using jinja seems quite nice to do that.

For me, any of the classic approach and the templating approach are perfect. I guess synapse_ynh could be a good use case to discuss in order to improve globally config panel mechanism.

[Josue-T]

In theory, the options in the config panel are saved in the file and in the settings too when you use the bind notation...

Can you share the part of code which explain this ?

I see that in recent version, a templating approach is used to fill the homeserver conf after applying a config panel, this approach have the default that it erase all manual change (even if this change are made with a post_app_upgrade hook).

Can you explain more how works exactly the post_app_upgrade hook ?

Note the other issue with the binding feature is that in synapse we don't just bind one settings with one simple value in the homeserver.yaml, it's quite more complex. So to me using jinja bring a lot more simplicity to manage this than before.

I could understand that it's great to be able to edit the config file, but if the cost is a lot of code (with a risk of bug) I'm not sure of the interest. I just prefer to add some more settings in the config panel if needed (and adding something like # Warning don't edit this config file... in homeserver.yaml) than adding a lot of complexity.
And finally, currently we have 2 way to manage the config, the yunohost/jinja templating system used by install/upgrade script and the config panel. In synapse case if we want to bind values we need to manage all specific cases in both cases which is not great for code duplication.

[Josue-T]

From what I remember also the reason to rebuild the config for the config panel is also for simplification reason. So we have only one place were we manage the input value and build the new config file.

To we can eventually rollback for the simple value which can be easily mapped, if the new value are correctly saved in the app settings. For the other one I'm not sure it's a good idea as it bring a lot of complexity.

[Josue-T]

For me, any of the classic approach and the templating approach are perfect. I guess synapse_ynh could be a good use case to discuss in order to improve globally config panel mechanism.

Yes I agree that we discus to improve it.