cleanup and standardize settings placeholders

[jaymode]

In #10918, a new placeholder was added for prompting settings. We already allow placeholders to read values from the environment/system properties. During the review, @uboness noted that it would be good to standardize the naming and cleanup the code that prompts the place holders.

The new changes mentioned in the comments of the PR are:

• Consolidate code that handles replacing the placeholders to one place. Right now we have it in the Settings and the InternalSettingsPreparer. This logic is really internal and should not be exposed in the Settings.
• The system placeholders should follow the same convention and be in the form: ${env::foobar}
• We can also introduce a new setting: config.ignore_placeholders:
  This will ignore all placeholder settings:

config.ignore_placeholders: true

will only ignore env placeholders:

config.ignore_placeholders: env

will only ignore prompt placeholders:

config.ignore_placeholders: prompt

will ignore both the env and prompt settings:

config.ignore_placeholders: env, prompt

[colings86]

Would all not be a more descriptive name for ignoring all placeholder settings. If I saw a setting which has a value of true, I would not think that it has any other options apart from true and false whereas seeing all would indicate to me that other values are available and there might be more granular values I can use.

[clintongormley]

@jaymode Why do we need the config.ignore_placeholders setting? If somebody has permission to start Elasticsearch or edit the config file, then they also have permission to change this setting?

[jaymode]

The config.ignore_placeholders was a suggestion from @uboness, so he may be able to provide additional details about the suggestion of adding it. My opinion is that this may not really be needed, since it controls internal behavior.

For clarification and background, I think the overall idea here is standardization of the placeholder naming. Looking at the replacePropertyPlaceholders method, we actually have two different ways of ignoring placeholders, the shouldIgnoreMissing method and then a check against completely ignored values passed in to the method, which is a bit of a mess (that I introduced).

There is a reason for this mess. The prompt placeholders use the format ${prompt::text}, which matches the format of other property placeholders. Where it differs is the use of :: as a separator. The preexisting property placeholders use : to indicate there is a default value and . as a type of a prefix, such as ${env.foo}. In order to support the ${prompt::text} format, we cannot use the shouldIgnoreMissing method because the value would never be missing, it would always have a default value.

If we standardize the naming, then we can clean this up and at the same time remove the replacePropertyPlaceholders method from Settings since it is only useful when preparing the settings and doesn't need to be in that class.

[clintongormley]

Wondering if it wouldn't be easier (and more bwc) to use this then:

${env.foo:default}
${prompt.text:default}

[uboness]

@clintongormley perhaps, it'll be more aligned with what we have today

[clintongormley]

@jaymode @uboness anything more to do here?

[jaymode]

@clintongormley I think we still need to cleanup the code a little, (copied the first bullet from above):

• Consolidate code that handles replacing the placeholders to one place. Right now we have it in the Settings and the InternalSettingsPreparer. This logic is really internal and should not be exposed in the Settings

We also didn't decide on the config.ignore_placeholders setting. Maybe @uboness can add his thoughts around that setting?

[rjernst]

Prompting settings were removed in #27216, thus this issue is no longer relevant.