Support dependent validation for index settings

[henningandersen]

A setting validator can declare settings that the validation depends on,
but when updating index settings, we eagerly validate the settings
before submitting the cluster state update and here we do not know the
existing settings. With this commit, we ensure that the pre-validation
works with such validators, letting the validator validate syntax
(validate(value)) only.

Relates #70141.

[elasticmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[rjernst]

After reading through this, I was surprised to learn about this validateWithoutDependencies method. I did not realize it even existed, and it appears it is only used in exactly one place (for updates) and does not have any tests. A method with a single use is something we are likely to re-evaluate its usefulness, and here I think it is no different. I'm not sure if a PR came up today adding this method that we would accept it.

With that said, I'm trying to understand more about why not validating dependent settings is necessary in this case.

we eagerly validate the settings before submitting the cluster state update and here we do not know the existing settings

Why would we not know all the settings? If we need to wait for a cluster state update, should the master be doing this validation and the update code should defer to that and trust the cluster state update will take care of it?

Ultimately I'm concerned that adding more uses of an untested method will make it harder to cleanup the settings infrastructure, which is in dire need of some work.

[henningandersen]

@rjernst thanks for looking into this.

I believe you are right for a long term solution, we could certainly improve this. However, this needs to go into 7.12.0 and I think shaking the code too much is not the right choice there.

The line causing this to fail for index settings with validators that depend on other settings is this:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java

Line 82 in a92a647

indexScopedSettings.validate(

where we validate most of the incoming settings. Then we validate some more inside the cluster state update. It is not entirely clear to me why we prevalidate the settings. If we remove that, the need for the validateDependencies boolean also goes away. We can also let the prevalidation calculate the combined settings first based on "current state".

There is a little bit of trickyness in removing it:

• The check inside the cluster state update deliberately does not check private settings (but only those listed in IndexScopedSettings.isPrivateSetting. The prevalidation also checks those.
• The check inside the cluster state update only checks if a change happens. Hopefully existing settings are valid, so not a big deal.

I will try to work on an alternative today.

[henningandersen]

AFAICS, we prevalidate the settings primarily to ensure the keys are valid before trying to apply it to all the indices. I went in the direction of disabling value validation for this prevalidation, alleviating your concern about the extra use of the validateWithoutDependencies method.

This gives an error report of all illegal keys up front and then for the first failing index an error report of the illegal values.

[rjernst]

Thanks Henning! Changing this parameter to validateValues makes a lot more sense to me, and I'm glad you were able to avoid using validateWithoutDependencies. LGTM.

[henningandersen]

Thanks for the speedy review, Ryan.