Fix handling of additional config files

[provokateurin]

Summary

1. Most deployment systems use additional config files to add the configuration, since those can be set to readonly on the filesystem level. This is problematic at the moment, because any values that are read from those configs are written to the main config file. As long as the key exists in the additional config file it will overwrite the value from the main config file. When the key is removed, the old value stays because it is still defined in the main config file. This is very unpleasant for every use-case that is declarative and expects the value to be gone when it is removed from the additional config file.
2. Using the CLI or other methods to set or delete config values doesn't do anything if they are specified in the additional config file. They are removed from the main config file, but are then overwritten again from the additional config file. This needs to be prevented as it is not the expected behavior. Just failing isn't very nice, but the only solution to fix this as we can not write to the additional config files.

With this change all config keys in the main config file will be removed if they are also specified in an additional config file, but that is correct as the value from the additional config file was overwriting the value anyway.

These problems exist at least for https://github.com/nextcloud/helm and https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-apps/nextcloud.nix, and I'm sure other systems experience the same problems.

Another thing to improve would be to throw an error if two config files specify the same key. They are not merged recursively and the order of how the config files are applied might not be stable, so could definitely lead to unexpected results.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[provokateurin]

/backport to stable30

[provokateurin]

/backport to stable29

[provokateurin]

/backport to stable28

[nickvergessen]

This breaks my development setup, where after installing I put in some config files to change e.g. apps_paths, trusted domains, etc. with some further values.

In my set up, I put the files in place, enable maintenance mode and disable it again, to copy the nested arrays all into the main config.php and then delete the temporary config files.

I'm pretty sure that there are cases in bigger installations where extra files are also used, and having a variable there that is also in the main file could be quite common:

1. As a temporary overwrite
2. As a permanent case

Would it instead make more sense to yield a setupcheck instead of breaking any set/delete?

[provokateurin]

I don't understand how this breaks your dev setup. Why can't you just have the options you need in the additional config file without copying something? I'm not sure I understand how your setup works.

As a temporary overwrite

I see, this is indeed a valid use-case that breaks this way. I wonder if it is really the best way to override a value temporarily. It should either be managed outside the config files or we make it possible to have a stable defined order of config files so you can have a 99-overrides.config.php.

As a permanent case

What's the point of this, then the value should just go into the additional config file. It is overwritten by the additional config file anyway, so it's not like the value is ever used in this case. This should not work at all.

Would it instead make more sense to yield a setupcheck instead of breaking any set/delete?

As I said set/delete are already broken, if you have an additional config file that sets the same keys. The exceptions just prevent that someone uses it and gets confused why the new value is never used.
Yes technically we could convert it into a setup check, but then the logic from the first commit gets much more complex because it needs to detect if the value is different and not just if the key is already defined by an additional config file.

[provokateurin]

As a temporary overwrite

I see, this is indeed a valid use-case that breaks this way. I wonder if it is really the best way to override a value temporarily. It should either be managed outside the config files or we make it possible to have a stable defined order of config files so you can have a 99-overrides.config.php.

Actually this doesn't work either, you can only override the value in an additional config file but not in the main config file.
So I don't think this breaks overriding values, since it was already broken before and this just prevents admins to fall for this problem.

[provokateurin]

Another thing to improve would be to throw an error if two config files specify the same key. They are not merged recursively and the order of how the config files are applied might not be stable, so could definitely lead to unexpected results.

And the alternative to this would be to have a proper order, so you can actually override values safely.

[come-nc]

I agree that removing stuff from config.php automagically if present in additional config file is weird and unexpected to users.

Proper fix could be:

• Remove autoadding of additional config values to main file
• Fix the loading order to config.php, then additional files in alphabetic order. Last one to set a value wins.
• Properly fail on delete/set if an additional config file with the value set is readonly

[provokateurin]

Proper fix could be

This is literally what this PR does.

The only unexpected thing might be the removal of duplicate keys from the main config file (if they are already present right now) and we could fix that in some way as I already proposed some solutions for that.

[kesselb]

Thank you very much for picking this up 🙏!

The current behavior has been bothering me for a while, especially because it can lead to so many weird states. For example, with the email settings: if the SMTP parameters are defined in mail.config.php and then changed via the web UI, the changes are written back to the main file but are overridden by the values in mail.config.php. The same issue occurs when variables are set using the incomplete environment integration we currently have.

A nice follow-up could be to maintain a list of immutable variables and signal back to the frontend when something is not configurable.

Honestly, I’ve always been hesitant to touch this part of the code because it feels like a rabbit hole 🙈. That said, I think this is a great improvement and hope to see it land soon!

I’m sorry to bring up another potential issue: the updater currently only reads config.php and ignores additional configuration files. There was an attempt to address this nextcloud/updater#384, but it had to be reverted.

The reason for the revert was that some people followed our example in https://docs.nextcloud.com/server/latest/admin_manual/apps_management.html#using-custom-app-directories and configured additional app directories using OC::$SERVERROOT. That variable isn’t available in the updater, so those additional configuration files don’t work. As a result, the updater currently depends on the state where the additional configuration files are merged into config.php. For more details, see the full bug report here: #44157.

To achieve this, we need to develop a plan or provide documentation on how to update such configurations.

Thanks again for your efforts!