[BUG] Setting validation missing during node startup

[jainankitk]

Describe the bug

While reviewing another PR, I noticed if we use addSettingsUpdateConsumer with validator, that does not apply to the setting value during node startup. Though I could not find many examples, there are definitely few like the one in ClusterManagerTaskThrottler.

Related component

Cluster Manager

To Reproduce

1. Add incorrect value for the setting in yml and start the node
2. Setting is accepted without any errors

Expected behavior

Error should be thrown if the setting value in yml file is incorrect

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[jainankitk]

cc: @ansjcy

[kaushalmahi12]

The validation logic is called if the setting has a validator defined,
the internal settings (from opensearch.yml file) are loaded by class InternalSettingsPreparer#prepareEnvironment.

Now when we init the values for the use case specific setting it first tries to get the value from the internally loaded setting value if not present it returns the default value for the setting

private T get(Settings settings, boolean validate) {
        String value = getRaw(settings);
        try {
            T parsed = parser.apply(value);
            if (validate) {
                final Iterator<Setting<?>> it = validator.settings();
                final Map<Setting<?>, Object> map;
                if (it.hasNext()) {
                    map = new HashMap<>();
                    while (it.hasNext()) {
                        final Setting<?> setting = it.next();
                        if (setting instanceof AffixSetting) {
                            // Collect all possible concrete settings
                            AffixSetting<?> as = ((AffixSetting<?>) setting);
                            for (String ns : as.getNamespaces(settings)) {
                                Setting<?> s = as.getConcreteSettingForNamespace(ns);
                                map.put(s, s.get(settings, false));
                            }
                        } else {
                            map.put(setting, setting.get(settings, false)); // we have to disable validation or we will stack overflow
                        }
                    }
                } else {
                    map = Collections.emptyMap();
                }
                validator.validate(parsed);
                validator.validate(parsed, map);
                validator.validate(parsed, map, exists(settings));
            }

the innerGetRaw gets the default setting if it is not present in the internally loaded Settings instance

String innerGetRaw(final Settings settings) {
        SecureSettings secureSettings = settings.getSecureSettings();
        if (secureSettings != null && secureSettings.getSettingNames().contains(getKey())) {
            throw new IllegalArgumentException(
                "Setting ["
                    + getKey()
                    + "] is a non-secure setting"
                    + " and must be stored inside opensearch.yml, but was found inside the OpenSearch keystore"
            );
        }
        return settings.get(getKey(), defaultValue.apply(settings));
    }

I don't think we need to keep this issue open.

It is true if the setting does not pass a validator at the setting initialisation time it might try to parse and initialise with the incorrect value(Most of the time it will fail at parsing time). But this can only happen to String key types, and that is why we recommend using Enums and provide the parsers for such settings
e,g;

public static final Setting<SearchBackpressureMode> SETTING_MODE = new Setting<>(
        "search_backpressure.mode",
        Defaults.MODE,
        SearchBackpressureMode::fromName,
        Setting.Property.Dynamic,
        Setting.Property.NodeScope
    );

I think these cases hints that it is going to be a implementation bug at the client side at best but not the API itself.

What do you think @msfroh @reta ?

[rajiv-kv]

[Triage - attendees 1 2 3 4] - @jainankitk - Thanks for filing the issue.