Skip to content
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

System index auto-creation should not be disabled by user settings #62984

Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -35,8 +35,11 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.indices.SystemIndices;
import org.elasticsearch.test.ESTestCase;

import java.util.HashMap;

import static java.util.Collections.emptyMap;
import static org.hamcrest.Matchers.containsString;

Expand All @@ -60,7 +63,8 @@ public class ReindexSourceTargetValidationTests extends ESTestCase {
.put(index("source2", "source_multi"), true)).build();
private static final IndexNameExpressionResolver INDEX_NAME_EXPRESSION_RESOLVER = new IndexNameExpressionResolver();
private static final AutoCreateIndex AUTO_CREATE_INDEX = new AutoCreateIndex(Settings.EMPTY,
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), INDEX_NAME_EXPRESSION_RESOLVER);
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), INDEX_NAME_EXPRESSION_RESOLVER,
new SystemIndices(new HashMap<>()));

private final BytesReference query = new BytesArray("{ \"foo\" : \"bar\" }");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@
import org.elasticsearch.gateway.TransportNodesListGatewayStartedShards;
import org.elasticsearch.index.seqno.GlobalCheckpointSyncAction;
import org.elasticsearch.index.seqno.RetentionLeaseActions;
import org.elasticsearch.indices.SystemIndices;
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.indices.store.TransportNodesListShardStoreMetadata;
import org.elasticsearch.persistent.CompletionPersistentTaskAction;
Expand Down Expand Up @@ -423,7 +424,7 @@ public class ActionModule extends AbstractModule {
public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpressionResolver,
IndexScopedSettings indexScopedSettings, ClusterSettings clusterSettings, SettingsFilter settingsFilter,
ThreadPool threadPool, List<ActionPlugin> actionPlugins, NodeClient nodeClient,
CircuitBreakerService circuitBreakerService, UsageService usageService) {
CircuitBreakerService circuitBreakerService, UsageService usageService, SystemIndices systemIndices) {
this.settings = settings;
this.indexNameExpressionResolver = indexNameExpressionResolver;
this.indexScopedSettings = indexScopedSettings;
Expand All @@ -433,7 +434,7 @@ public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpr
this.threadPool = threadPool;
actions = setupActions(actionPlugins);
actionFilters = setupActionFilters(actionPlugins);
autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver);
autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver, systemIndices);
destructiveOperations = new DestructiveOperations(settings, clusterSettings);
Set<RestHeaderDefinition> headers = Stream.concat(
actionPlugins.stream().flatMap(p -> p.getRestHeaders().stream()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ protected void doInternalExecute(Task task, BulkRequest bulkRequest, String exec
return;
}

if (needToCheck()) {
final boolean includesSystem = includesSystem(bulkRequest, clusterService.state().metadata().getIndicesLookup(), systemIndices);

if (includesSystem || needToCheck()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already noted in this in the PR description, but this is going to conflict with https://github.com/elastic/elasticsearch/pull/61858/files?file-filters%5B%5D=.java#diff-7c2a294c3d25c427072ee6c64983b560L227 specifically. I think this is the only place they'll meaningfully conflict, though.

// Attempt to create all the indices that we're going to need during the bulk before we start.
// Step 1: collect all the indices in the request
final Map<String, Boolean> indices = bulkRequest.requests.stream()
Expand Down Expand Up @@ -346,15 +348,20 @@ static void prohibitCustomRoutingOnDataStream(DocWriteRequest<?> writeRequest, M
}

boolean isOnlySystem(BulkRequest request, SortedMap<String, IndexAbstraction> indicesLookup, SystemIndices systemIndices) {
final boolean onlySystem = request.getIndices().stream().allMatch(indexName -> {
final IndexAbstraction abstraction = indicesLookup.get(indexName);
if (abstraction != null) {
return abstraction.isSystem();
} else {
return systemIndices.isSystemIndex(indexName);
}
});
return onlySystem;
return request.getIndices().stream().allMatch(indexName -> isSystemIndex(indicesLookup, systemIndices, indexName));
}

boolean includesSystem(BulkRequest request, SortedMap<String, IndexAbstraction> indicesLookup, SystemIndices systemIndices) {
return request.getIndices().stream().anyMatch(indexName -> isSystemIndex(indicesLookup, systemIndices, indexName));
}

private boolean isSystemIndex(SortedMap<String, IndexAbstraction> indicesLookup, SystemIndices systemIndices, String indexName) {
final IndexAbstraction abstraction = indicesLookup.get(indexName);
if (abstraction != null) {
return abstraction.isSystem();
} else {
return systemIndices.isSystemIndex(indexName);
}
}

boolean needToCheck() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.indices.SystemIndices;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -44,10 +45,15 @@ public final class AutoCreateIndex {
new Setting<>("action.auto_create_index", "true", AutoCreate::new, Property.NodeScope, Setting.Property.Dynamic);

private final IndexNameExpressionResolver resolver;
private final SystemIndices systemIndices;
private volatile AutoCreate autoCreate;

public AutoCreateIndex(Settings settings, ClusterSettings clusterSettings, IndexNameExpressionResolver resolver) {
public AutoCreateIndex(Settings settings,
ClusterSettings clusterSettings,
IndexNameExpressionResolver resolver,
SystemIndices systemIndices) {
this.resolver = resolver;
this.systemIndices = systemIndices;
this.autoCreate = AUTO_CREATE_INDEX_SETTING.get(settings);
clusterSettings.addSettingsUpdateConsumer(AUTO_CREATE_INDEX_SETTING, this::setAutoCreate);
}
Expand All @@ -67,9 +73,10 @@ public boolean shouldAutoCreate(String index, ClusterState state) {
if (resolver.hasIndexAbstraction(index, state)) {
return false;
}

// One volatile read, so that all checks are done against the same instance:
final AutoCreate autoCreate = this.autoCreate;
if (autoCreate.autoCreateIndex == false) {
if (autoCreate.autoCreateIndex == false && systemIndices.isSystemIndex(index) == false) {
throw new IndexNotFoundException("[" + AUTO_CREATE_INDEX_SETTING.getKey() + "] is [false]", index);
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/node/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ protected Node(final Environment initialEnvironment,

ActionModule actionModule = new ActionModule(settings, clusterModule.getIndexNameExpressionResolver(),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService);
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService, systemIndices);
modules.add(actionModule);

final RestController restController = actionModule.getRestController();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() {
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), null, emptyList(), null,
null, usageService);
null, usageService, null);
actionModule.initRestHandlers(null);
// At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail
Exception e = expectThrows(IllegalArgumentException.class, () ->
Expand Down Expand Up @@ -148,7 +148,7 @@ public String getName() {
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool,
singletonList(dupsMainAction), null, null, usageService);
singletonList(dupsMainAction), null, null, usageService, null);
Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null));
assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET"));
} finally {
Expand Down Expand Up @@ -182,7 +182,7 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool,
singletonList(registersFakeHandler), null, null, usageService);
singletonList(registersFakeHandler), null, null, usageService, null);
actionModule.initRestHandlers(null);
// At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail
Exception e = expectThrows(IllegalArgumentException.class, () ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,9 @@ class TestTransportBulkAction extends TransportBulkAction {
null, new ActionFilters(Collections.emptySet()), null,
new AutoCreateIndex(
SETTINGS, new ClusterSettings(SETTINGS, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS),
new IndexNameExpressionResolver()
), new IndexingPressure(SETTINGS), new SystemIndices(Map.of())
new IndexNameExpressionResolver(),
new SystemIndices(Map.of())),
new IndexingPressure(SETTINGS), new SystemIndices(Map.of())
);
}
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class TestTransportBulkAction extends TransportBulkAction {
TestTransportBulkAction() {
super(TransportBulkActionTests.this.threadPool, transportService, clusterService, null,
null, new ActionFilters(Collections.emptySet()), new Resolver(),
new AutoCreateIndex(Settings.EMPTY, clusterService.getClusterSettings(), new Resolver()),
new AutoCreateIndex(Settings.EMPTY, clusterService.getClusterSettings(), new Resolver(), new SystemIndices(Map.of())),
new IndexingPressure(Settings.EMPTY), new SystemIndices(Map.of()));
}

Expand Down Expand Up @@ -268,6 +268,27 @@ public void testOnlySystem() {
assertFalse(bulkAction.isOnlySystem(buildBulkRequest(mixed), indicesLookup, systemIndices));
}

public void testIncludesSystem() {
SortedMap<String, IndexAbstraction> indicesLookup = new TreeMap<>();
Settings settings = Settings.builder().put("index.version.created", Version.CURRENT).build();
indicesLookup.put(".foo",
new Index(IndexMetadata.builder(".foo").settings(settings).system(true).numberOfShards(1).numberOfReplicas(0).build()));
indicesLookup.put(".bar",
new Index(IndexMetadata.builder(".bar").settings(settings).system(true).numberOfShards(1).numberOfReplicas(0).build()));
SystemIndices systemIndices = new SystemIndices(Map.of("plugin", List.of(new SystemIndexDescriptor(".test", ""))));
List<String> onlySystem = List.of(".foo", ".bar");
assertTrue(bulkAction.includesSystem(buildBulkRequest(onlySystem), indicesLookup, systemIndices));

onlySystem = List.of(".foo", ".bar", ".test");
assertTrue(bulkAction.includesSystem(buildBulkRequest(onlySystem), indicesLookup, systemIndices));

List<String> nonSystem = List.of("foo", "bar");
assertFalse(bulkAction.includesSystem(buildBulkRequest(nonSystem), indicesLookup, systemIndices));

List<String> mixed = List.of(".foo", ".test", "other");
assertTrue(bulkAction.includesSystem(buildBulkRequest(mixed), indicesLookup, systemIndices));
}

private BulkRequest buildBulkRequest(List<String> indices) {
BulkRequest request = new BulkRequest();
for (String index : indices) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.indices.SystemIndexDescriptor;
import org.elasticsearch.indices.SystemIndices;
import org.elasticsearch.test.ESTestCase;

import java.util.HashMap;
Expand Down Expand Up @@ -89,6 +91,12 @@ public void testAutoCreationDisabled() {
assertEquals("no such index [" + randomIndex + "] and [action.auto_create_index] is [false]", e.getMessage());
}

public void testSystemIndexWithAutoCreationDisabled() {
williamrandolph marked this conversation as resolved.
Show resolved Hide resolved
Settings settings = Settings.builder().put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), false).build();
AutoCreateIndex autoCreateIndex = newAutoCreateIndex(settings);
assertThat(autoCreateIndex.shouldAutoCreate(".foo", buildClusterState()), equalTo(true));
}

public void testAutoCreationEnabled() {
Settings settings = Settings.builder().put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), true).build();
AutoCreateIndex autoCreateIndex = newAutoCreateIndex(settings);
Expand Down Expand Up @@ -177,7 +185,8 @@ public void testUpdate() {

ClusterSettings clusterSettings = new ClusterSettings(settings,
ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
AutoCreateIndex autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, new IndexNameExpressionResolver());
AutoCreateIndex autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, new IndexNameExpressionResolver(),
new SystemIndices(Map.of()));
assertThat(autoCreateIndex.getAutoCreate().isAutoCreateIndex(), equalTo(value));

Settings newSettings = Settings.builder().put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), !value).build();
Expand All @@ -201,8 +210,9 @@ private static ClusterState buildClusterState(String... indices) {
}

private AutoCreateIndex newAutoCreateIndex(Settings settings) {
SystemIndices systemIndices = new SystemIndices(Map.of("plugin", List.of(new SystemIndexDescriptor(".foo", ""))));
williamrandolph marked this conversation as resolved.
Show resolved Hide resolved
return new AutoCreateIndex(settings, new ClusterSettings(settings,
ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), new IndexNameExpressionResolver());
ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), new IndexNameExpressionResolver(), systemIndices);
}

private void expectNotMatch(ClusterState clusterState, AutoCreateIndex autoCreateIndex, String index) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,7 @@ allocationService, new AliasValidator(), shardLimitValidator, environment, index
new AnalysisModule(environment, Collections.emptyList()).getAnalysisRegistry(),
Collections.emptyList(), client),
client, actionFilters, indexNameExpressionResolver,
new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver),
new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver, new SystemIndices(Map.of())),
new IndexingPressure(settings),
new SystemIndices(Map.of())
));
Expand Down