Skip to content

Commit f9a1561

Browse files
kkrik-eskkrik-es
authored
Handle setting merge conflicts for overruling settings providers (#115217) (#115325)
* Handle setting merge conflicts for overruling settings providers * spotless * update TransportSimulateIndexTemplateAction * update comment and add test * fix flakiness * fix flakiness (cherry picked from commit e3c198a)
1 parent 86a9b7d commit f9a1561

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
@@ -41,8 +41,10 @@
4141
import org.elasticsearch.common.settings.Settings;
4242
import org.elasticsearch.core.TimeValue;
4343
import org.elasticsearch.index.Index;
44+
import org.elasticsearch.index.IndexMode;
4445
import org.elasticsearch.index.IndexModule;
4546
import org.elasticsearch.index.IndexNotFoundException;
47+
import org.elasticsearch.index.IndexSettingProvider;
4648
import org.elasticsearch.index.IndexSettingProviders;
4749
import org.elasticsearch.index.IndexSettings;
4850
import org.elasticsearch.index.IndexVersion;
@@ -71,6 +73,7 @@
7173
import org.junit.Before;
7274

7375
import java.io.IOException;
76+
import java.time.Instant;
7477
import java.util.ArrayList;
7578
import java.util.Arrays;
7679
import java.util.Collection;
@@ -686,6 +689,178 @@ public void testAggregateSettingsAppliesSettingsFromTemplatesAndRequest() {
686689
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
687690
}
688691

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

0 commit comments

Comments
 (0)