-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow yaml values for dynamic node settings #85186
Conversation
Environment variables are allowed as substitutions within elasticsearch.yml. Additionally, command line settings are added into the parsed settings. However, both of these are raw strings applied after the node yml file has been parsed. This commit moves environment substitution to occur before parsing elasticsearch.yml, and override processing to happen with a separate yaml parser so that both can allow yaml parsing. Note that environment substitution is not the only type of substitution (there is also setting substitution, which needs parsed setting keys), so the existing replacement mechanism is not touched here except to remove environment handling. closes elastic#65577
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @rjernst, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we have coverage here where we are "directly" setting a property with the value from an environment variable in a Docker context. I think we need a packaging test for something like ES_NODE_ROLES=[]
and verifying we get the correct result.
} | ||
var is = new ByteArrayInputStream(builder.toString().getBytes(StandardCharsets.UTF_8)); | ||
// fake the resource name so it loads yaml | ||
output.loadFromStream("overrides.yml", is, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why this is necessary. Doesn't the later call to initializeSettings
take care of replacing any remaining values? How many "passes" are necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I meant to remove the putProperties in initializeSettings (I've done so now). The idea is that instead of putting the values directly (which may be json), we load them here as if they set in a yaml file.
Given that this has tests, I'm going to go ahead and merge, and I will followup with a higher level integration test. I'd like to avoid package tests because none of this is different across platforms. |
Not across platforms, but environment variable handling is special for Docker packaging. |
rjernst was this fix for 8.4.0 as well ? |
Environment variables are allowed as substitutions within
elasticsearch.yml. Additionally, command line settings are added into
the parsed settings. However, both of these are raw strings applied
after the node yml file has been parsed.
This commit moves environment substitution to occur before parsing
elasticsearch.yml, and override processing to happen with a separate
yaml parser so that both can allow yaml parsing. Note that environment
substitution is not the only type of substitution (there is also setting
substitution, which needs parsed setting keys), so the existing
replacement mechanism is not touched here except to remove environment
handling.
closes #65577