Skip to content

Commit a0c0d28

Browse files
albertzaharovitsalbertzaharovits
committed
Fix security index auto-create and state recovery race (#39582)
Previously, the security index could be wrongfully recreated. This might happen if the index was interpreted as missing, as in the case of a fresh install, but the index existed and the state did not yet recover. This fix will return HTTP SERVICE_UNAVAILABLE (503) for requests that try to write to the security index before the state has not been recovered yet.
1 parent fc76bcb commit a0c0d28

File tree

2 files changed

+85
-2
lines changed

2 files changed

+85
-2
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.apache.logging.log4j.message.ParameterizedMessage;
1111
import org.elasticsearch.ElasticsearchException;
1212
import org.elasticsearch.ElasticsearchParseException;
13+
import org.elasticsearch.ElasticsearchStatusException;
1314
import org.elasticsearch.ExceptionsHelper;
1415
import org.elasticsearch.ResourceAlreadyExistsException;
1516
import org.elasticsearch.Version;
@@ -40,6 +41,7 @@
4041
import org.elasticsearch.gateway.GatewayService;
4142
import org.elasticsearch.index.IndexNotFoundException;
4243
import org.elasticsearch.index.mapper.MapperService;
44+
import org.elasticsearch.rest.RestStatus;
4345
import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames;
4446
import org.elasticsearch.xpack.core.template.TemplateUtils;
4547

@@ -81,7 +83,7 @@ public class SecurityIndexManager implements ClusterStateListener {
8183
private volatile State indexState;
8284

8385
public SecurityIndexManager(Client client, String indexName, ClusterService clusterService) {
84-
this(client, indexName, new State(false, false, false, false, null, null, null));
86+
this(client, indexName, State.UNRECOVERED_STATE);
8587
clusterService.addListener(this);
8688
}
8789

@@ -121,6 +123,10 @@ public boolean isMappingUpToDate() {
121123
return this.indexState.mappingUpToDate;
122124
}
123125

126+
public boolean isStateRecovered() {
127+
return this.indexState != State.UNRECOVERED_STATE;
128+
}
129+
124130
public ElasticsearchException getUnavailableReason() {
125131
final State localState = this.indexState;
126132
if (localState.indexAvailable) {
@@ -297,7 +303,10 @@ public void checkIndexVersionThenExecute(final Consumer<Exception> consumer, fin
297303
public void prepareIndexIfNeededThenExecute(final Consumer<Exception> consumer, final Runnable andThen) {
298304
final State indexState = this.indexState; // use a local copy so all checks execute against the same state!
299305
// TODO we should improve this so we don't fire off a bunch of requests to do the same thing (create or update mappings)
300-
if (indexState.indexExists && indexState.isIndexUpToDate == false) {
306+
if (indexState == State.UNRECOVERED_STATE) {
307+
consumer.accept(new ElasticsearchStatusException("Cluster state has not been recovered yet, cannot write to the security index",
308+
RestStatus.SERVICE_UNAVAILABLE));
309+
} else if (indexState.indexExists && indexState.isIndexUpToDate == false) {
301310
consumer.accept(new IllegalStateException(
302311
"Security index is not on the current version. Security features relying on the index will not be available until " +
303312
"the upgrade API is run on the security index"));
@@ -377,6 +386,7 @@ public static boolean isIndexDeleted(State previousState, State currentState) {
377386
* State of the security index.
378387
*/
379388
public static class State {
389+
public static final State UNRECOVERED_STATE = new State(false, false, false, false, null, null, null);
380390
public final boolean indexExists;
381391
public final boolean isIndexUpToDate;
382392
public final boolean indexAvailable;

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import java.util.concurrent.atomic.AtomicReference;
1616
import java.util.function.BiConsumer;
1717

18+
import org.elasticsearch.ElasticsearchStatusException;
1819
import org.elasticsearch.Version;
1920
import org.elasticsearch.action.Action;
2021
import org.elasticsearch.action.ActionListener;
@@ -27,6 +28,7 @@
2728
import org.elasticsearch.cluster.ClusterChangedEvent;
2829
import org.elasticsearch.cluster.ClusterName;
2930
import org.elasticsearch.cluster.ClusterState;
31+
import org.elasticsearch.cluster.block.ClusterBlocks;
3032
import org.elasticsearch.cluster.health.ClusterHealthStatus;
3133
import org.elasticsearch.cluster.metadata.IndexMetaData;
3234
import org.elasticsearch.cluster.metadata.IndexTemplateMetaData;
@@ -41,10 +43,13 @@
4143
import org.elasticsearch.cluster.service.ClusterService;
4244
import org.elasticsearch.common.UUIDs;
4345
import org.elasticsearch.common.settings.Settings;
46+
import org.elasticsearch.common.util.concurrent.EsExecutors;
4447
import org.elasticsearch.common.util.concurrent.ThreadContext;
4548
import org.elasticsearch.common.xcontent.XContentType;
49+
import org.elasticsearch.gateway.GatewayService;
4650
import org.elasticsearch.index.Index;
4751
import org.elasticsearch.index.shard.ShardId;
52+
import org.elasticsearch.rest.RestStatus;
4853
import org.elasticsearch.test.ESTestCase;
4954
import org.elasticsearch.threadpool.ThreadPool;
5055
import org.elasticsearch.xpack.security.test.SecurityTestUtils;
@@ -56,6 +61,10 @@
5661
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_TEMPLATE_NAME;
5762
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.TEMPLATE_VERSION_PATTERN;
5863
import static org.hamcrest.Matchers.equalTo;
64+
import static org.hamcrest.Matchers.instanceOf;
65+
import static org.hamcrest.Matchers.is;
66+
import static org.hamcrest.Matchers.notNullValue;
67+
import static org.hamcrest.Matchers.nullValue;
5968
import static org.mockito.Mockito.mock;
6069
import static org.mockito.Mockito.when;
6170

@@ -73,6 +82,7 @@ public void setUpManager() {
7382
final Client mockClient = mock(Client.class);
7483
final ThreadPool threadPool = mock(ThreadPool.class);
7584
when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY));
85+
when(threadPool.generic()).thenReturn(EsExecutors.newDirectExecutorService());
7686
when(mockClient.threadPool()).thenReturn(threadPool);
7787
when(mockClient.settings()).thenReturn(Settings.EMPTY);
7888
final ClusterService clusterService = mock(ClusterService.class);
@@ -196,6 +206,67 @@ public void testIndexHealthChangeListeners() throws Exception {
196206
assertEquals(ClusterHealthStatus.GREEN, currentState.get().indexStatus);
197207
}
198208

209+
public void testWriteBeforeStateNotRecovered() throws Exception {
210+
final AtomicBoolean prepareRunnableCalled = new AtomicBoolean(false);
211+
final AtomicReference<Exception> prepareException = new AtomicReference<>(null);
212+
manager.prepareIndexIfNeededThenExecute(ex -> {
213+
prepareException.set(ex);
214+
}, () -> {
215+
prepareRunnableCalled.set(true);
216+
});
217+
assertThat(prepareException.get(), is(notNullValue()));
218+
assertThat(prepareException.get(), instanceOf(ElasticsearchStatusException.class));
219+
assertThat(((ElasticsearchStatusException)prepareException.get()).status(), is(RestStatus.SERVICE_UNAVAILABLE));
220+
assertThat(prepareRunnableCalled.get(), is(false));
221+
prepareException.set(null);
222+
prepareRunnableCalled.set(false);
223+
// state not recovered
224+
final ClusterBlocks.Builder blocks = ClusterBlocks.builder().addGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK);
225+
manager.clusterChanged(event(new ClusterState.Builder(CLUSTER_NAME).blocks(blocks)));
226+
manager.prepareIndexIfNeededThenExecute(ex -> {
227+
prepareException.set(ex);
228+
}, () -> {
229+
prepareRunnableCalled.set(true);
230+
});
231+
assertThat(prepareException.get(), is(notNullValue()));
232+
assertThat(prepareException.get(), instanceOf(ElasticsearchStatusException.class));
233+
assertThat(((ElasticsearchStatusException)prepareException.get()).status(), is(RestStatus.SERVICE_UNAVAILABLE));
234+
assertThat(prepareRunnableCalled.get(), is(false));
235+
prepareException.set(null);
236+
prepareRunnableCalled.set(false);
237+
// state recovered with index
238+
ClusterState.Builder clusterStateBuilder = createClusterState(INDEX_NAME, TEMPLATE_NAME,
239+
SecurityIndexManager.INTERNAL_INDEX_FORMAT);
240+
markShardsAvailable(clusterStateBuilder);
241+
manager.clusterChanged(event(clusterStateBuilder));
242+
manager.prepareIndexIfNeededThenExecute(ex -> {
243+
prepareException.set(ex);
244+
}, () -> {
245+
prepareRunnableCalled.set(true);
246+
});
247+
assertThat(prepareException.get(), is(nullValue()));
248+
assertThat(prepareRunnableCalled.get(), is(true));
249+
}
250+
251+
public void testListeneredNotCalledBeforeStateNotRecovered() throws Exception {
252+
final AtomicBoolean listenerCalled = new AtomicBoolean(false);
253+
manager.addIndexStateListener((prev, current) -> {
254+
listenerCalled.set(true);
255+
});
256+
final ClusterBlocks.Builder blocks = ClusterBlocks.builder().addGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK);
257+
// state not recovered
258+
manager.clusterChanged(event(new ClusterState.Builder(CLUSTER_NAME).blocks(blocks)));
259+
assertThat(manager.isStateRecovered(), is(false));
260+
assertThat(listenerCalled.get(), is(false));
261+
// state recovered with index
262+
ClusterState.Builder clusterStateBuilder = createClusterState(INDEX_NAME, TEMPLATE_NAME,
263+
SecurityIndexManager.INTERNAL_INDEX_FORMAT);
264+
markShardsAvailable(clusterStateBuilder);
265+
manager.clusterChanged(event(clusterStateBuilder));
266+
assertThat(manager.isStateRecovered(), is(true));
267+
assertThat(listenerCalled.get(), is(true));
268+
}
269+
199270
public void testIndexOutOfDateListeners() throws Exception {
200271
final AtomicBoolean listenerCalled = new AtomicBoolean(false);
201272
manager.clusterChanged(event(new ClusterState.Builder(CLUSTER_NAME)));
@@ -240,12 +311,14 @@ private void assertInitialState() {
240311
assertThat(manager.indexExists(), Matchers.equalTo(false));
241312
assertThat(manager.isAvailable(), Matchers.equalTo(false));
242313
assertThat(manager.isMappingUpToDate(), Matchers.equalTo(false));
314+
assertThat(manager.isStateRecovered(), Matchers.equalTo(false));
243315
}
244316

245317
private void assertIndexUpToDateButNotAvailable() {
246318
assertThat(manager.indexExists(), Matchers.equalTo(true));
247319
assertThat(manager.isAvailable(), Matchers.equalTo(false));
248320
assertThat(manager.isMappingUpToDate(), Matchers.equalTo(true));
321+
assertThat(manager.isStateRecovered(), Matchers.equalTo(true));
249322
}
250323

251324
public static ClusterState.Builder createClusterState(String indexName, String templateName) throws IOException {

0 commit comments

Comments
 (0)