-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Allow private settings when system provided #133789
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
base: main
Are you sure you want to change the base?
Allow private settings when system provided #133789
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
Outdated
Show resolved
Hide resolved
The I'm probably missing something here.. |
Looks like we're skipping adding the private settings during template validation via this hack: Lines 210 to 214 in eb005b0
I'd rather have the template validation work on the actual settings than the provider behaving differently during validation. |
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 looked at this and left a couple of comments (nothing of substance). I keep having a sneaking suspicion that there's a more elegant way to do this, but it might be the late-Friday-afternoon brain telling me that. I'm going to take another look at it next week.
for (final String key : settings.keySet()) { | ||
final Setting<?> setting = indexScopedSettings.get(key); | ||
if (setting == null) { | ||
assert indexScopedSettings.isPrivateSetting(key) : "expected [" + key + "] to be private but it was not"; | ||
} else if (setting.isPrivateIndex()) { | ||
validationErrors.add("private index setting [" + key + "] can not be set explicitly"); | ||
} | ||
} else if (setting.isPrivateIndex() | ||
// System-provided settings are always allowed to configure private settings. | ||
// These are typically coming from an IndexSettingProvider. | ||
&& (systemProvided == null || settings.get(key).equals(systemProvided.get(key)) == false)) { | ||
validationErrors.add("private index setting [" + key + "] can not be set explicitly"); | ||
} |
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.
Would it be better to leave the if
statements alone and instead do something like:
for (final String key : settings.filter(s -> systemProvided.hasValue(s.getKey()) == false)).keySet()) {
…
}
to filter the system-provided settings out of the validation list at the beginning?
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.
It would be possible but we can't filter out all settings if they're also in the systemProvided
settings. We also need to check if the value is the same to avoid allowing users to override private settings that are also system provided with a different value.
At this point, I'm not sure if the filtering is more readable. Another option to make it more readable is to extract an isSystemProvided
method so that the condition becomes:
else if (setting.isPrivateIndex() && isSystemProvided(key, settings, systemProvided) == false)
@@ -961,7 +961,7 @@ private void createDownsampleIndex( | |||
projectId, | |||
downsampleIndexName, | |||
downsampleIndexName | |||
).settings(builder.build()).mappings(mapping).waitForActiveShards(ActiveShardCount.ONE); | |||
).settings(builder.build()).settingsSystemProvided(true).mappings(mapping).waitForActiveShards(ActiveShardCount.ONE); |
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.
Can you add a comment about which setting requires this flag?
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.
None, yet. I can remove this from the PR and re-add it when introducing the index.dimensions
setting in #132566.
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Lee Hinman <[email protected]>
…s-if-system-provided
As discussed in #132566, we'd like to provide private index settings with an
IndexSettingsProvider
. This is currently not possible because it fails validation for composable index templates (template v2). That's because we first merge all settings from the index template with the ones from theIndexSettingsProviders
and then check whether there are private settings.I've added a way so pass down the settings provided by the
IndexSettingsProviders
to the validation so that these system-provided settings are allowed to contain private settings.Another area that needs to be enhanced is to allow downsampling to create indices containing private settings that are copied from the source index. For that reason, I've added a
settingsSystemProvided
flag toCreateIndexClusterStateUpdateRequest
.