Skip to content

Commit

Permalink
Handle setting merge conflicts for overruling settings providers (ela…
Browse files Browse the repository at this point in the history 
…stic#115217)

* 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)
  • Loading branch information
@kkrik-es
kkrik-es committed Oct 22, 2024
1 parent 2bfbb42 commit 752a607
Show file tree
Hide file tree
Showing 8 changed files with 362 additions and 41 deletions.
17 changes: 16 additions & 1 deletion ...lasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

import java.time.Instant;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -270,6 +271,7 @@ public static Template resolveTemplate(
// First apply settings sourced from index settings providers
final var now = Instant.now();
Settings.Builder additionalSettings = Settings.builder();
Set<String> overrulingSettings = new HashSet<>();
for (var provider : indexSettingProviders) {
Settings result = provider.getAdditionalIndexSettings(
indexName,
Expand All @@ -283,8 +285,21 @@ public static Template resolveTemplate(
MetadataCreateIndexService.validateAdditionalSettings(provider, result, additionalSettings);
dummySettings.put(result);
additionalSettings.put(result);
if (provider.overrulesTemplateAndRequestSettings()) {
overrulingSettings.addAll(result.keySet());
}
}
// Then apply settings resolved from templates:

if (overrulingSettings.isEmpty() == false) {
// Filter any conflicting settings from overruling providers, to avoid overwriting their values from templates.
final Settings.Builder filtered = Settings.builder().put(templateSettings);
for (String setting : overrulingSettings) {
filtered.remove(setting);
}
templateSettings = filtered.build();
}

// Apply settings resolved from templates.
dummySettings.put(templateSettings);

final IndexMetadata indexMetadata = IndexMetadata.builder(indexName)
Expand Down
60 changes: 35 additions & 25 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,7 @@ static Settings aggregateIndexSettings(
// additionalIndexSettings map
final Settings.Builder additionalIndexSettings = Settings.builder();
final var resolvedAt = Instant.ofEpochMilli(request.getNameResolvedAt());
Set<String> overrulingSettings = new HashSet<>();
for (IndexSettingProvider provider : indexSettingProviders) {
var newAdditionalSettings = provider.getAdditionalIndexSettings(
request.index(),
Expand All @@ -1004,36 +1005,45 @@ static Settings aggregateIndexSettings(
);
validateAdditionalSettings(provider, newAdditionalSettings, additionalIndexSettings);
additionalIndexSettings.put(newAdditionalSettings);
if (provider.overrulesTemplateAndRequestSettings()) {
overrulingSettings.addAll(newAdditionalSettings.keySet());
}
}

// For all the explicit settings, we go through the template and request level settings
// and see if either a template or the request has "cancelled out" an explicit default
// setting. For example, if a plugin had as an explicit setting:
// "index.mysetting": "blah
// And either a template or create index request had:
// "index.mysetting": null
// We want to remove the explicit setting not only from the explicitly set settings, but
// also from the template and request settings, so that from the newly create index's
// perspective it is as though the setting has not been set at all (using the default
// value).
for (String explicitSetting : additionalIndexSettings.keys()) {
if (templateSettings.keys().contains(explicitSetting) && templateSettings.get(explicitSetting) == null) {
logger.debug(
"removing default [{}] setting as it in set to null in a template for [{}] creation",
explicitSetting,
request.index()
);
additionalIndexSettings.remove(explicitSetting);
if (overrulingSettings.contains(explicitSetting)) {
// Remove any conflicting template and request settings to use the provided values.
templateSettings.remove(explicitSetting);
}
if (requestSettings.keys().contains(explicitSetting) && requestSettings.get(explicitSetting) == null) {
logger.debug(
"removing default [{}] setting as it in set to null in the request for [{}] creation",
explicitSetting,
request.index()
);
additionalIndexSettings.remove(explicitSetting);
requestSettings.remove(explicitSetting);
} else {
// For all the explicit settings, we go through the template and request level settings
// and see if either a template or the request has "cancelled out" an explicit default
// setting. For example, if a plugin had as an explicit setting:
// "index.mysetting": "blah
// And either a template or create index request had:
// "index.mysetting": null
// We want to remove the explicit setting not only from the explicitly set settings, but
// also from the template and request settings, so that from the newly create index's
// perspective it is as though the setting has not been set at all (using the default
// value).
if (templateSettings.keys().contains(explicitSetting) && templateSettings.get(explicitSetting) == null) {
logger.debug(
"removing default [{}] setting as it is set to null in a template for [{}] creation",
explicitSetting,
request.index()
);
additionalIndexSettings.remove(explicitSetting);
templateSettings.remove(explicitSetting);
}
if (requestSettings.keys().contains(explicitSetting) && requestSettings.get(explicitSetting) == null) {
logger.debug(
"removing default [{}] setting as it is set to null in the request for [{}] creation",
explicitSetting,
request.index()
);
additionalIndexSettings.remove(explicitSetting);
requestSettings.remove(explicitSetting);
}
}
}

Expand Down
11 changes: 11 additions & 0 deletions server/src/main/java/org/elasticsearch/index/IndexSettingProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,15 @@ Settings getAdditionalIndexSettings(
record Parameters(CheckedFunction<IndexMetadata, MapperService, IOException> mapperServiceFactory) {

}

/**
* Indicates whether the additional settings that this provider returns can overrule the settings defined in matching template
* or in create index request.
*
* Note that this is not used during index template validation, to avoid overruling template settings that may apply to
* different contexts (e.g. the provider is not used, or it returns different setting values).
*/
default boolean overrulesTemplateAndRequestSettings() {
return false;
}
}
23 changes: 22 additions & 1 deletion ...csearch/action/admin/indices/template/post/TransportSimulateIndexTemplateActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ public void testSettingsProviderIsOverridden() throws Exception {
matchingTemplate,
ComposableIndexTemplate.builder()
.indexPatterns(List.of("test_index*"))
.template(new Template(Settings.builder().put("test-setting", 1).build(), null, null))
.template(
new Template(Settings.builder().put("test-setting", 1).put("test-setting-2", 2).build(), null, null)
)
.build()
)
)
Expand Down Expand Up @@ -78,6 +80,24 @@ public Settings getAdditionalIndexSettings(
) {
return Settings.builder().put("test-setting", 0).build();
}
}, new IndexSettingProvider() {
@Override
public Settings getAdditionalIndexSettings(
String indexName,
String dataStreamName,
IndexMode templateIndexMode,
Metadata metadata,
Instant resolvedAt,
Settings indexTemplateAndCreateRequestSettings,
List<CompressedXContent> combinedTemplateMappings
) {
return Settings.builder().put("test-setting-2", 10).build();
}

@Override
public boolean overrulesTemplateAndRequestSettings() {
return true;
}
});

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

assertThat(resolvedTemplate.settings().getAsInt("test-setting", -1), is(1));
assertThat(resolvedTemplate.settings().getAsInt("test-setting-2", -1), is(10));
}
}
175 changes: 175 additions & 0 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.index.IndexSettingProvider;
import org.elasticsearch.index.IndexSettingProviders;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.IndexVersion;
Expand Down Expand Up @@ -71,6 +73,7 @@
import org.junit.Before;

import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -686,6 +689,178 @@ public void testAggregateSettingsAppliesSettingsFromTemplatesAndRequest() {
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
}

public void testAggregateSettingsProviderOverrulesSettingsFromRequest() {
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
builder.settings(Settings.builder().put("template_setting", "value1"));
});
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
request.settings(Settings.builder().put("request_setting", "value2").build());

Settings aggregatedIndexSettings = aggregateIndexSettings(
clusterState,
request,
templateMetadata.settings(),
null,
null,
Settings.EMPTY,
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
randomShardLimitService(),
Set.of(new IndexSettingProvider() {
@Override
public Settings getAdditionalIndexSettings(
String indexName,
String dataStreamName,
IndexMode templateIndexMode,
Metadata metadata,
Instant resolvedAt,
Settings indexTemplateAndCreateRequestSettings,
List<CompressedXContent> combinedTemplateMappings
) {
return Settings.builder().put("request_setting", "overrule_value").put("other_setting", "other_value").build();
}

@Override
public boolean overrulesTemplateAndRequestSettings() {
return true;
}
})
);

assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("value1"));
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("overrule_value"));
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
}

public void testAggregateSettingsProviderOverrulesNullFromRequest() {
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
builder.settings(Settings.builder().put("template_setting", "value1"));
});
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
request.settings(Settings.builder().putNull("request_setting").build());

Settings aggregatedIndexSettings = aggregateIndexSettings(
clusterState,
request,
templateMetadata.settings(),
null,
null,
Settings.EMPTY,
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
randomShardLimitService(),
Set.of(new IndexSettingProvider() {
@Override
public Settings getAdditionalIndexSettings(
String indexName,
String dataStreamName,
IndexMode templateIndexMode,
Metadata metadata,
Instant resolvedAt,
Settings indexTemplateAndCreateRequestSettings,
List<CompressedXContent> combinedTemplateMappings
) {
return Settings.builder().put("request_setting", "overrule_value").put("other_setting", "other_value").build();
}

@Override
public boolean overrulesTemplateAndRequestSettings() {
return true;
}
})
);

assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("value1"));
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("overrule_value"));
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
}

public void testAggregateSettingsProviderOverrulesSettingsFromTemplates() {
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
builder.settings(Settings.builder().put("template_setting", "value1"));
});
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
request.settings(Settings.builder().put("request_setting", "value2").build());

Settings aggregatedIndexSettings = aggregateIndexSettings(
clusterState,
request,
templateMetadata.settings(),
null,
null,
Settings.EMPTY,
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
randomShardLimitService(),
Set.of(new IndexSettingProvider() {
@Override
public Settings getAdditionalIndexSettings(
String indexName,
String dataStreamName,
IndexMode templateIndexMode,
Metadata metadata,
Instant resolvedAt,
Settings indexTemplateAndCreateRequestSettings,
List<CompressedXContent> combinedTemplateMappings
) {
return Settings.builder().put("template_setting", "overrule_value").put("other_setting", "other_value").build();
}

@Override
public boolean overrulesTemplateAndRequestSettings() {
return true;
}
})
);

assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("overrule_value"));
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
}

public void testAggregateSettingsProviderOverrulesNullFromTemplates() {
IndexTemplateMetadata templateMetadata = addMatchingTemplate(builder -> {
builder.settings(Settings.builder().putNull("template_setting"));
});
Metadata metadata = new Metadata.Builder().templates(Map.of("template_1", templateMetadata)).build();
ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT).metadata(metadata).build();
request.settings(Settings.builder().put("request_setting", "value2").build());

Settings aggregatedIndexSettings = aggregateIndexSettings(
clusterState,
request,
templateMetadata.settings(),
null,
null,
Settings.EMPTY,
IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
randomShardLimitService(),
Set.of(new IndexSettingProvider() {
@Override
public Settings getAdditionalIndexSettings(
String indexName,
String dataStreamName,
IndexMode templateIndexMode,
Metadata metadata,
Instant resolvedAt,
Settings indexTemplateAndCreateRequestSettings,
List<CompressedXContent> combinedTemplateMappings
) {
return Settings.builder().put("template_setting", "overrule_value").put("other_setting", "other_value").build();
}

@Override
public boolean overrulesTemplateAndRequestSettings() {
return true;
}
})
);

assertThat(aggregatedIndexSettings.get("template_setting"), equalTo("overrule_value"));
assertThat(aggregatedIndexSettings.get("request_setting"), equalTo("value2"));
assertThat(aggregatedIndexSettings.get("other_setting"), equalTo("other_value"));
}

public void testInvalidAliasName() {
final String[] invalidAliasNames = new String[] { "-alias1", "+alias2", "_alias3", "a#lias", "al:ias", ".", ".." };
String aliasName = randomFrom(invalidAliasNames);
Expand Down
Loading

0 comments on commit 752a607

Please sign in to comment.