Skip to content

Commit e3c198a

Browse files
kkrik-eskkrik-es
authored
Handle setting merge conflicts for overruling settings providers (#115217)
* Handle setting merge conflicts for overruling settings providers * spotless * update TransportSimulateIndexTemplateAction * update comment and add test * fix flakiness * fix flakiness
1 parent 003fbc7 commit e3c198a

File tree

8 files changed

+362
-41
lines changed

8 files changed

+362
-41
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848

4949
import java.time.Instant;
5050
import java.util.HashMap;
51+
import java.util.HashSet;
5152
import java.util.List;
5253
import java.util.Locale;
5354
import java.util.Map;
@@ -270,6 +271,7 @@ public static Template resolveTemplate(
270271
// First apply settings sourced from index settings providers
271272
final var now = Instant.now();
272273
Settings.Builder additionalSettings = Settings.builder();
274+
Set<String> overrulingSettings = new HashSet<>();
273275
for (var provider : indexSettingProviders) {
274276
Settings result = provider.getAdditionalIndexSettings(
275277
indexName,
@@ -283,8 +285,21 @@ public static Template resolveTemplate(
283285
MetadataCreateIndexService.validateAdditionalSettings(provider, result, additionalSettings);
284286
dummySettings.put(result);
285287
additionalSettings.put(result);
288+
if (provider.overrulesTemplateAndRequestSettings()) {
289+
overrulingSettings.addAll(result.keySet());
290+
}
286291
}
287-
// Then apply settings resolved from templates:
292+
293+
if (overrulingSettings.isEmpty() == false) {
294+
// Filter any conflicting settings from overruling providers, to avoid overwriting their values from templates.
295+
final Settings.Builder filtered = Settings.builder().put(templateSettings);
296+
for (String setting : overrulingSettings) {
297+
filtered.remove(setting);
298+
}
299+
templateSettings = filtered.build();
300+
}
301+
302+
// Apply settings resolved from templates.
288303
dummySettings.put(templateSettings);
289304

290305
final IndexMetadata indexMetadata = IndexMetadata.builder(indexName)

‎server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java‎

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -992,6 +992,7 @@ static Settings aggregateIndexSettings(
992992
// additionalIndexSettings map
993993
final Settings.Builder additionalIndexSettings = Settings.builder();
994994
final var resolvedAt = Instant.ofEpochMilli(request.getNameResolvedAt());
995+
Set<String> overrulingSettings = new HashSet<>();
995996
for (IndexSettingProvider provider : indexSettingProviders) {
996997
var newAdditionalSettings = provider.getAdditionalIndexSettings(
997998
request.index(),
@@ -1004,36 +1005,45 @@ static Settings aggregateIndexSettings(
10041005
);
10051006
validateAdditionalSettings(provider, newAdditionalSettings, additionalIndexSettings);
10061007
additionalIndexSettings.put(newAdditionalSettings);
1008+
if (provider.overrulesTemplateAndRequestSettings()) {
1009+
overrulingSettings.addAll(newAdditionalSettings.keySet());
1010+
}
10071011
}
10081012

1009-
// For all the explicit settings, we go through the template and request level settings
1010-
// and see if either a template or the request has "cancelled out" an explicit default
1011-
// setting. For example, if a plugin had as an explicit setting:
1012-
// "index.mysetting": "blah
1013-
// And either a template or create index request had:
1014-
// "index.mysetting": null
1015-
// We want to remove the explicit setting not only from the explicitly set settings, but
1016-
// also from the template and request settings, so that from the newly create index's
1017-
// perspective it is as though the setting has not been set at all (using the default
1018-
// value).
10191013
for (String explicitSetting : additionalIndexSettings.keys()) {
1020-
if (templateSettings.keys().contains(explicitSetting) && templateSettings.get(explicitSetting) == null) {
1021-
logger.debug(
1022-
"removing default [{}] setting as it in set to null in a template for [{}] creation",
1023-
explicitSetting,
1024-
request.index()
1025-
);
1026-
additionalIndexSettings.remove(explicitSetting);
1014+
if (overrulingSettings.contains(explicitSetting)) {
1015+
// Remove any conflicting template and request settings to use the provided values.
10271016
templateSettings.remove(explicitSetting);
1028-
}
1029-
if (requestSettings.keys().contains(explicitSetting) && requestSettings.get(explicitSetting) == null) {
1030-
logger.debug(
1031-
"removing default [{}] setting as it in set to null in the request for [{}] creation",
1032-
explicitSetting,
1033-
request.index()
1034-
);
1035-
additionalIndexSettings.remove(explicitSetting);
10361017
requestSettings.remove(explicitSetting);
1018+
} else {
1019+
// For all the explicit settings, we go through the template and request level settings
1020+
// and see if either a template or the request has "cancelled out" an explicit default
1021+
// setting. For example, if a plugin had as an explicit setting:
1022+
// "index.mysetting": "blah
1023+
// And either a template or create index request had:
1024+
// "index.mysetting": null
1025+
// We want to remove the explicit setting not only from the explicitly set settings, but
1026+
// also from the template and request settings, so that from the newly create index's
1027+
// perspective it is as though the setting has not been set at all (using the default
1028+
// value).
1029+
if (templateSettings.keys().contains(explicitSetting) && templateSettings.get(explicitSetting) == null) {
1030+
logger.debug(
1031+
"removing default [{}] setting as it is set to null in a template for [{}] creation",
1032+
explicitSetting,
1033+
request.index()
1034+
);
1035+
additionalIndexSettings.remove(explicitSetting);
1036+
templateSettings.remove(explicitSetting);
1037+
}
1038+
if (requestSettings.keys().contains(explicitSetting) && requestSettings.get(explicitSetting) == null) {
1039+
logger.debug(
1040+
"removing default [{}] setting as it is set to null in the request for [{}] creation",
1041+
explicitSetting,
1042+
request.index()
1043+
);
1044+
additionalIndexSettings.remove(explicitSetting);
1045+
requestSettings.remove(explicitSetting);
1046+
}
10371047
}
10381048
}
10391049

‎server/src/main/java/org/elasticsearch/index/IndexSettingProvider.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,15 @@ Settings getAdditionalIndexSettings(
5757
record Parameters(CheckedFunction<IndexMetadata, MapperService, IOException> mapperServiceFactory) {
5858

5959
}
60+
61+
/**
62+
* Indicates whether the additional settings that this provider returns can overrule the settings defined in matching template
63+
* or in create index request.
64+
*
65+
* Note that this is not used during index template validation, to avoid overruling template settings that may apply to
66+
* different contexts (e.g. the provider is not used, or it returns different setting values).
67+
*/
68+
default boolean overrulesTemplateAndRequestSettings() {
69+
return false;
70+
}
6071
}

‎server/src/test/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateActionTests.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ public void testSettingsProviderIsOverridden() throws Exception {
4949
matchingTemplate,
5050
ComposableIndexTemplate.builder()
5151
.indexPatterns(List.of("test_index*"))
52-
.template(new Template(Settings.builder().put("test-setting", 1).build(), null, null))
52+
.template(
53+
new Template(Settings.builder().put("test-setting", 1).put("test-setting-2", 2).build(), null, null)
54+
)
5355
.build()
5456
)
5557
)
@@ -78,6 +80,24 @@ public Settings getAdditionalIndexSettings(
7880
) {
7981
return Settings.builder().put("test-setting", 0).build();
8082
}
83+
}, new IndexSettingProvider() {
84+
@Override
85+
public Settings getAdditionalIndexSettings(
86+
String indexName,
87+
String dataStreamName,
88+
IndexMode templateIndexMode,
89+
Metadata metadata,
90+
Instant resolvedAt,
91+
Settings indexTemplateAndCreateRequestSettings,
92+
List<CompressedXContent> combinedTemplateMappings
93+
) {
94+
return Settings.builder().put("test-setting-2", 10).build();
95+
}
96+
97+
@Override
98+
public boolean overrulesTemplateAndRequestSettings() {
99+
return true;
100+
}
81101
});
82102

83103
Template resolvedTemplate = TransportSimulateIndexTemplateAction.resolveTemplate(
@@ -92,5 +112,6 @@ public Settings getAdditionalIndexSettings(
92112
);
93113

94114
assertThat(resolvedTemplate.settings().getAsInt("test-setting", -1), is(1));
115+
assertThat(resolvedTemplate.settings().getAsInt("test-setting-2", -1), is(10));
95116
}
96117
}

‎server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@
4444
import org.elasticsearch.core.TimeValue;
4545
import org.elasticsearch.core.UpdateForV9;
4646
import org.elasticsearch.index.Index;
47+
import org.elasticsearch.index.IndexMode;
4748
import org.elasticsearch.index.IndexModule;
4849
import org.elasticsearch.index.IndexNotFoundException;
50+
import org.elasticsearch.index.IndexSettingProvider;
4951
import org.elasticsearch.index.IndexSettingProviders;
5052
import org.elasticsearch.index.IndexSettings;
5153
import org.elasticsearch.index.IndexVersion;
@@ -74,6 +76,7 @@
7476
import org.junit.Before;
7577

7678
import java.io.IOException;
79+
import java.time.Instant;
7780
import java.util.ArrayList;
7881
import java.util.Arrays;
7982
import java.util.Collection;
@@ -691,6 +694,178 @@ public void testAggregateSettingsAppliesSettingsFromTemplatesAndRequest() {
691694
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
692695
}
693696

697+
public void testAggregateSettingsProviderOverrulesSettingsFromRequest() {
698+
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
699+
builder.settings(Settings.builder().put("template_setting", "value1"));
700+
});
701+
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
702+
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
703+
request.settings(Settings.builder().put("request_setting", "value2").build());
704+
705+
Settings aggregatedIndexSettings = aggregateIndexSettings(
706+
clusterState,
707+
request,
708+
templateMetadata.settings(),
709+
null,
710+
null,
711+
Settings.EMPTY,
712+
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
713+
randomShardLimitService(),
714+
Set.of(new IndexSettingProvider() {
715+
@Override
716+
public Settings getAdditionalIndexSettings(
717+
String indexName,
718+
String dataStreamName,
719+
IndexMode templateIndexMode,
720+
Metadata metadata,
721+
Instant resolvedAt,
722+
Settings indexTemplateAndCreateRequestSettings,
723+
List<CompressedXContent> combinedTemplateMappings
724+
) {
725+
return Settings.builder().put("request_setting", "overrule_value").put("other_setting", "other_value").build();
726+
}
727+
728+
@Override
729+
public boolean overrulesTemplateAndRequestSettings() {
730+
return true;
731+
}
732+
})
733+
);
734+
735+
assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("value1"));
736+
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("overrule_value"));
737+
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
738+
}
739+
740+
public void testAggregateSettingsProviderOverrulesNullFromRequest() {
741+
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
742+
builder.settings(Settings.builder().put("template_setting", "value1"));
743+
});
744+
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
745+
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
746+
request.settings(Settings.builder().putNull("request_setting").build());
747+
748+
Settings aggregatedIndexSettings = aggregateIndexSettings(
749+
clusterState,
750+
request,
751+
templateMetadata.settings(),
752+
null,
753+
null,
754+
Settings.EMPTY,
755+
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
756+
randomShardLimitService(),
757+
Set.of(new IndexSettingProvider() {
758+
@Override
759+
public Settings getAdditionalIndexSettings(
760+
String indexName,
761+
String dataStreamName,
762+
IndexMode templateIndexMode,
763+
Metadata metadata,
764+
Instant resolvedAt,
765+
Settings indexTemplateAndCreateRequestSettings,
766+
List<CompressedXContent> combinedTemplateMappings
767+
) {
768+
return Settings.builder().put("request_setting", "overrule_value").put("other_setting", "other_value").build();
769+
}
770+
771+
@Override
772+
public boolean overrulesTemplateAndRequestSettings() {
773+
return true;
774+
}
775+
})
776+
);
777+
778+
assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("value1"));
779+
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("overrule_value"));
780+
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
781+
}
782+
783+
public void testAggregateSettingsProviderOverrulesSettingsFromTemplates() {
784+
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
785+
builder.settings(Settings.builder().put("template_setting", "value1"));
786+
});
787+
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
788+
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
789+
request.settings(Settings.builder().put("request_setting", "value2").build());
790+
791+
Settings aggregatedIndexSettings = aggregateIndexSettings(
792+
clusterState,
793+
request,
794+
templateMetadata.settings(),
795+
null,
796+
null,
797+
Settings.EMPTY,
798+
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
799+
randomShardLimitService(),
800+
Set.of(new IndexSettingProvider() {
801+
@Override
802+
public Settings getAdditionalIndexSettings(
803+
String indexName,
804+
String dataStreamName,
805+
IndexMode templateIndexMode,
806+
Metadata metadata,
807+
Instant resolvedAt,
808+
Settings indexTemplateAndCreateRequestSettings,
809+
List<CompressedXContent> combinedTemplateMappings
810+
) {
811+
return Settings.builder().put("template_setting", "overrule_value").put("other_setting", "other_value").build();
812+
}
813+
814+
@Override
815+
public boolean overrulesTemplateAndRequestSettings() {
816+
return true;
817+
}
818+
})
819+
);
820+
821+
assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("overrule_value"));
822+
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
823+
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
824+
}
825+
826+
public void testAggregateSettingsProviderOverrulesNullFromTemplates() {
827+
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
828+
builder.settings(Settings.builder().putNull("template_setting"));
829+
});
830+
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
831+
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
832+
request.settings(Settings.builder().put("request_setting", "value2").build());
833+
834+
Settings aggregatedIndexSettings = aggregateIndexSettings(
835+
clusterState,
836+
request,
837+
templateMetadata.settings(),
838+
null,
839+
null,
840+
Settings.EMPTY,
841+
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
842+
randomShardLimitService(),
843+
Set.of(new IndexSettingProvider() {
844+
@Override
845+
public Settings getAdditionalIndexSettings(
846+
String indexName,
847+
String dataStreamName,
848+
IndexMode templateIndexMode,
849+
Metadata metadata,
850+
Instant resolvedAt,
851+
Settings indexTemplateAndCreateRequestSettings,
852+
List<CompressedXContent> combinedTemplateMappings
853+
) {
854+
return Settings.builder().put("template_setting", "overrule_value").put("other_setting", "other_value").build();
855+
}
856+
857+
@Override
858+
public boolean overrulesTemplateAndRequestSettings() {
859+
return true;
860+
}
861+
})
862+
);
863+
864+
assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("overrule_value"));
865+
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
866+
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
867+
}
868+
694869
public void testInvalidAliasName() {
695870
final String[] invalidAliasNames = new String[] { "-alias1", "+alias2", "_alias3", "a#lias", "al:ias", ".", ".." };
696871
String aliasName = randomFrom(invalidAliasNames);

0 commit comments

Comments
 (0)