Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -490,20 +490,7 @@ public void testProvidersAffectMode() {
ClusterSettings.createBuiltInClusterSettings(),
dataStreamGlobalRetentionSettings,
emptyDataStreamFailureStoreSettings,
new IndexSettingProviders(
Set.of(
(
indexName,
dataStreamName,
templateIndexMode,
metadata,
resolvedAt,
indexTemplateAndCreateRequestSettings,
combinedTemplateMappings,
additionalSettings,
additionalCustomMetadata) -> additionalSettings.put("index.mode", IndexMode.LOOKUP)
)
),
IndexSettingProviders.of((additionalSettings) -> additionalSettings.put("index.mode", IndexMode.LOOKUP)),
null
);
assertThat(response.getDataStreams().getFirst().getIndexModeName(), equalTo("lookup"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ public class CreateIndexClusterStateUpdateRequest {

private ComposableIndexTemplate matchingTemplate;

private boolean settingsSystemProvided = false;

/**
* @deprecated project id ought always be specified
*/
Expand Down Expand Up @@ -223,6 +225,19 @@ public CreateIndexClusterStateUpdateRequest setMatchingTemplate(ComposableIndexT
return this;
}

/**
* Indicates whether the {@link #settings} of this request are system provided.
* If this is true, private settings will be allowed to be set in the request.
*/
public CreateIndexClusterStateUpdateRequest settingsSystemProvided(boolean settingsSystemProvided) {
this.settingsSystemProvided = settingsSystemProvided;
return this;
}

public boolean settingsSystemProvided() {
return settingsSystemProvided;
}

@Override
public String toString() {
return "CreateIndexClusterStateUpdateRequest{"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1607,12 +1607,12 @@ private static void validateActiveShardCount(ActiveShardCount waitForActiveShard

private void validate(CreateIndexClusterStateUpdateRequest request, ProjectMetadata projectMetadata, RoutingTable routingTable) {
validateIndexName(request.index(), projectMetadata, routingTable);
validateIndexSettings(request.index(), request.settings(), forbidPrivateIndexSettings);
validateIndexSettings(request.index(), request.settings(), forbidPrivateIndexSettings && request.settingsSystemProvided() == false);
}

public void validateIndexSettings(String indexName, final Settings settings, final boolean forbidPrivateIndexSettings)
throws IndexCreationException {
List<String> validationErrors = getIndexSettingsValidationErrors(settings, forbidPrivateIndexSettings);
List<String> validationErrors = getIndexSettingsValidationErrors(settings, null, forbidPrivateIndexSettings);

if (validationErrors.isEmpty() == false) {
ValidationException validationException = new ValidationException();
Expand All @@ -1621,23 +1621,34 @@ public void validateIndexSettings(String indexName, final Settings settings, fin
}
}

List<String> getIndexSettingsValidationErrors(final Settings settings, final boolean forbidPrivateIndexSettings) {
List<String> getIndexSettingsValidationErrors(
final Settings settings,
@Nullable Settings systemProvided,
final boolean forbidPrivateIndexSettings
) {
List<String> validationErrors = validateIndexCustomPath(settings, env.sharedDataDir());
if (forbidPrivateIndexSettings) {
validationErrors.addAll(validatePrivateSettingsNotExplicitlySet(settings, indexScopedSettings));
validationErrors.addAll(validatePrivateSettingsNotExplicitlySet(settings, systemProvided, indexScopedSettings));
}
return validationErrors;
}

private static List<String> validatePrivateSettingsNotExplicitlySet(Settings settings, IndexScopedSettings indexScopedSettings) {
private static List<String> validatePrivateSettingsNotExplicitlySet(
Settings settings,
@Nullable Settings systemProvided,
IndexScopedSettings indexScopedSettings
) {
List<String> validationErrors = new ArrayList<>();
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");
}
Comment on lines 1642 to +1651
Copy link
Member

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?

Copy link
Member Author

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)

}
return validationErrors;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -788,8 +788,7 @@ void validateIndexTemplateV2(ProjectMetadata projectMetadata, String name, Compo

final var combinedMappings = collectMappings(indexTemplate, projectMetadata.componentTemplates(), "tmp_idx");
final var combinedSettings = resolveSettings(indexTemplate, projectMetadata.componentTemplates());
// First apply settings sourced from index setting providers:
var finalSettings = Settings.builder();
var additionalSettingsBuilder = Settings.builder();
ImmutableOpenMap.Builder<String, Map<String, String>> customMetadataBuilder = ImmutableOpenMap.builder();
for (var provider : indexSettingProviders) {
Settings.Builder builder = Settings.builder();
Expand All @@ -805,9 +804,13 @@ void validateIndexTemplateV2(ProjectMetadata projectMetadata, String name, Compo
customMetadataBuilder::put
);
var newAdditionalSettings = builder.build();
MetadataCreateIndexService.validateAdditionalSettings(provider, newAdditionalSettings, finalSettings);
finalSettings.put(newAdditionalSettings);
MetadataCreateIndexService.validateAdditionalSettings(provider, newAdditionalSettings, additionalSettingsBuilder);
additionalSettingsBuilder.put(newAdditionalSettings);
}
Settings additionalSettings = additionalSettingsBuilder.build();
var finalSettings = Settings.builder();
// First apply settings sourced from index setting providers:
finalSettings.put(additionalSettings);
// Then apply setting from component templates:
finalSettings.put(combinedSettings);
// Then finally apply settings resolved from index template:
Expand All @@ -817,7 +820,7 @@ void validateIndexTemplateV2(ProjectMetadata projectMetadata, String name, Compo

var templateToValidate = indexTemplate.toBuilder().template(Template.builder(finalTemplate).settings(finalSettings)).build();

validate(name, templateToValidate);
validate(name, templateToValidate, additionalSettings);
validateDataStreamsStillReferenced(projectMetadata, name, templateToValidate);
validateLifecycle(projectMetadata, name, templateToValidate, globalRetentionSettings.get(false));
validateDataStreamOptions(projectMetadata, name, templateToValidate, globalRetentionSettings.get(true));
Expand Down Expand Up @@ -2057,18 +2060,19 @@ public static void validateTemplate(Settings validateSettings, CompressedXConten
}

public void validate(String name, ComponentTemplate template) {
validate(name, template.template(), Collections.emptyList());
validate(name, template.template(), Collections.emptyList(), null);
}

private void validate(String name, ComposableIndexTemplate template) {
validate(name, template.template(), template.indexPatterns());
private void validate(String name, ComposableIndexTemplate template, Settings systemProvided) {
validate(name, template.template(), template.indexPatterns(), systemProvided);
}

private void validate(String name, Template template, List<String> indexPatterns) {
private void validate(String name, Template template, List<String> indexPatterns, @Nullable Settings systemProvided) {
Optional<Template> maybeTemplate = Optional.ofNullable(template);
validate(
name,
maybeTemplate.map(Template::settings).orElse(Settings.EMPTY),
systemProvided,
indexPatterns,
maybeTemplate.map(Template::aliases).orElse(emptyMap()).values().stream().map(MetadataIndexTemplateService::toAlias).toList()
);
Expand All @@ -2087,10 +2091,16 @@ private static Alias toAlias(AliasMetadata aliasMeta) {
}

private void validate(PutRequest putRequest) {
validate(putRequest.name, putRequest.settings, putRequest.indexPatterns, putRequest.aliases);
validate(putRequest.name, putRequest.settings, null, putRequest.indexPatterns, putRequest.aliases);
}

private void validate(String name, @Nullable Settings settings, List<String> indexPatterns, List<Alias> aliases) {
private void validate(
String name,
@Nullable Settings settings,
@Nullable Settings systemProvided,
List<String> indexPatterns,
List<Alias> aliases
) {
List<String> validationErrors = new ArrayList<>();
if (name.contains(" ")) {
validationErrors.add("name must not contain a space");
Expand Down Expand Up @@ -2143,7 +2153,11 @@ private void validate(String name, @Nullable Settings settings, List<String> ind
validationErrors.add(t.getMessage());
}
}
List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(settings, true);
List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(
settings,
systemProvided,
true
);
validationErrors.addAll(indexSettingsValidation);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

package org.elasticsearch.index;

import org.elasticsearch.common.settings.Settings;

import java.util.Collections;
import java.util.Set;
import java.util.function.Consumer;

/**
* Keeps track of the {@link IndexSettingProvider} instances defined by plugins and
Expand All @@ -22,6 +25,31 @@ public final class IndexSettingProviders {

private final Set<IndexSettingProvider> indexSettingProviders;

/**
* Utility method which creates an {@link IndexSettingProviders} instance that uses the provided consumer to add settings
* to the index being created.
* The primary use case is for tests that want to add specific settings without having to create a full implementation.
*
* @param settingsBuilderConsumer A consumer that adds index settings
* @return An {@link IndexSettingProviders} instance that uses the provided consumer to add settings
*/
public static IndexSettingProviders of(Consumer<Settings.Builder> settingsBuilderConsumer) {
return new IndexSettingProviders(
Set.of(
(
indexName,
dataStreamName,
templateIndexMode,
projectMetadata,
resolvedAt,
indexTemplateAndCreateRequestSettings,
combinedTemplateMappings,
additionalSettings,
additionalCustomMetadata) -> settingsBuilderConsumer.accept(additionalSettings)
)
);
}

public IndexSettingProviders(Set<IndexSettingProvider> indexSettingProviders) {
this.indexSettingProviders = Collections.unmodifiableSet(indexSettingProviders);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import org.elasticsearch.index.shard.IndexLongFieldRange;
import org.elasticsearch.indices.EmptySystemIndices;
import org.elasticsearch.indices.IndexCreationException;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.indices.InvalidAliasNameException;
import org.elasticsearch.indices.InvalidIndexNameException;
import org.elasticsearch.indices.ShardLimitValidator;
Expand All @@ -82,6 +83,7 @@
import org.elasticsearch.xcontent.XContentFactory;
import org.hamcrest.Matchers;
import org.junit.Before;
import org.mockito.ArgumentCaptor;

import java.io.IOException;
import java.time.Instant;
Expand Down Expand Up @@ -128,6 +130,9 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.startsWith;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

public class MetadataCreateIndexServiceTests extends ESTestCase {

Expand Down Expand Up @@ -1739,6 +1744,81 @@ public void testCreateClusterBlocksTransformerForIndexCreation() {
);
}

public void testSetPrivateSettings() throws Exception {
request.settings(Settings.builder().put(IndexMetadata.INDEX_DOWNSAMPLE_SOURCE_UUID.getKey(), "private_setting").build());
boolean settingsSystemProvided = randomBoolean();
request.settingsSystemProvided(settingsSystemProvided);

IndicesService indicesService = mock(IndicesService.class);
withTemporaryClusterService(((clusterService, threadPool) -> {
MetadataCreateIndexService service = new MetadataCreateIndexService(
Settings.EMPTY,
clusterService,
indicesService,
null,
createTestShardLimitService(randomIntBetween(1, 1000), clusterService),
newEnvironment(),
new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS),
threadPool,
null,
EmptySystemIndices.INSTANCE,
true,
IndexSettingProviders.EMPTY
);

try {
service.applyCreateIndexRequest(clusterService.state(), request, false, ActionListener.wrap(r -> {}, e -> {}));
if (settingsSystemProvided == false) {
fail("expected private setting to be rejected");
}
} catch (Exception e) {
if (settingsSystemProvided) {
fail(e, "did not expect private setting to be rejected when system provided");
}
}
}));

if (settingsSystemProvided) {
ArgumentCaptor<IndexMetadata> captor = ArgumentCaptor.forClass(IndexMetadata.class);
verify(indicesService).withTempIndexService(captor.capture(), any());
IndexMetadata indexMetadata = captor.getValue();
assertThat(indexMetadata.getSettings().get(IndexMetadata.INDEX_DOWNSAMPLE_SOURCE_UUID.getKey()), equalTo("private_setting"));
}
}

public void testIndexSettingProviderPrivateSetting() throws Exception {
IndicesService indicesService = mock(IndicesService.class);
withTemporaryClusterService(((clusterService, threadPool) -> {
MetadataCreateIndexService service = new MetadataCreateIndexService(
Settings.EMPTY,
clusterService,
indicesService,
null,
createTestShardLimitService(randomIntBetween(1, 1000), clusterService),
newEnvironment(),
new IndexScopedSettings(Settings.EMPTY, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS),
threadPool,
null,
EmptySystemIndices.INSTANCE,
true,
IndexSettingProviders.of(
(additionalSettings) -> additionalSettings.put(IndexMetadata.INDEX_DOWNSAMPLE_SOURCE_NAME.getKey(), "private_setting")
)
);

try {
service.applyCreateIndexRequest(clusterService.state(), request, false, ActionListener.wrap(r -> {}, e -> {}));
} catch (Exception e) {
fail(e, "did not expect private setting to be rejected when added via IndexSettingProvider");
}
}));

ArgumentCaptor<IndexMetadata> captor = ArgumentCaptor.forClass(IndexMetadata.class);
verify(indicesService).withTempIndexService(captor.capture(), any());
IndexMetadata indexMetadata = captor.getValue();
assertThat(indexMetadata.getSettings().get(IndexMetadata.INDEX_DOWNSAMPLE_SOURCE_NAME.getKey()), equalTo("private_setting"));
}

private IndexTemplateMetadata addMatchingTemplate(Consumer<IndexTemplateMetadata.Builder> configurator) {
IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*");
configurator.accept(builder);
Expand Down
Loading