How to handle deprecated/removed config options

[lunny]

Both a system failure and a critical issue can impact the admin user. To ensure awareness, we should implement a mandatory warning page that appears on every login, requiring the admin to acknowledge it before proceeding.

Originally posted by @lunny in #33707 (comment)

[TheFox0x7]

After some work with huma they have a pretty nice system for body validation which I think might be reusable in this context. It doesn't quick fail when on bad arguments, but collects them instead into []error which gets returned to user. So my proposal for this is to do the following:

1. deprecatedSetting should return an error if the config option was removed more than n releases ago, otherwise it should just print a warning as it does now. If it's in error mode we can remove the workaround permanently.
2. Errors from each loader should be gathered and joined, if the resulting err!=nil, print it on fatal level and prevent running from startup
3. Set a clear cutoff period from where those warnings and errors are removed or move them to their own file (similar to migrations).

See: TheFox0x7@5286afe for quick initial proposal (currently it lacks version comparison).
Only downside I think there is, is that in k8s and other autorestarting environments it will just endlessly reboot until someone will look at it - which is expected but a waste.

If we were to split removed settings into it's own file/func for clarity, it should be loaded before all the actual settings IMO providing early return.

---

unless this should come after some other refactor of settings? @wxiaoguang mentioned a rewrite of parsing package might be needed if I remember correctly?

[wxiaoguang]

I do not think it should stop starting when a "deprecated error" occurs. The deprecation could occur in many "load config" functions.

See case 2: #33707 (comment)

[TheFox0x7]

The way I understood it is that Fatal implies a critical error (here a config option is no longer used and was detected in the file) which in turn terminates server/aborts startup.
And to be exact I do not mean for setting which was deprecated a release ago to cause an error - 2nd release or later seems fair though.
This would avoid the loop you mentioned as the settings would load as normal but collect still configured deprecated removed settings and list them with Fatal once all settings are loaded.

[wxiaoguang]

Sorry I misread, I thought "returning err" implies "fatal". Because there is another case, the setting package also has many technical debt:

• Some "load" functions depends on others
• Some "load" functions do changes (so if there is something wrong ahead, it shouldn't do the change anymore)

So ideally, when a non-removed-option error occurs, it should "fatal" as soon as possible to avoid other side-effects.

So to be honest, instead of "errors.Join", I think it's worth to use a separate error list to manage the "removed-option errors", and the "removed-option" check should be done before other "load" functions IMO.

[TheFox0x7]

So something more like this then?
TheFox0x7@3750a2b#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749

Though maybe it would be worth solving technical debt first so it doesn't pile up.

[wxiaoguang]

So something more like this then?
TheFox0x7@3750a2b#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749

Yup, it overall looks good.