Skip to content

Commit de08bc1

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 d7ff6eb commit de08bc1

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;
@@ -26,6 +27,7 @@
2627
import org.elasticsearch.cluster.ClusterChangedEvent;
2728
import org.elasticsearch.cluster.ClusterName;
2829
import org.elasticsearch.cluster.ClusterState;
30+
import org.elasticsearch.cluster.block.ClusterBlocks;
2931
import org.elasticsearch.cluster.health.ClusterHealthStatus;
3032
import org.elasticsearch.cluster.metadata.IndexMetaData;
3133
import org.elasticsearch.cluster.metadata.IndexTemplateMetaData;
@@ -40,10 +42,13 @@
4042
import org.elasticsearch.cluster.service.ClusterService;
4143
import org.elasticsearch.common.UUIDs;
4244
import org.elasticsearch.common.settings.Settings;
45+
import org.elasticsearch.common.util.concurrent.EsExecutors;
4346
import org.elasticsearch.common.util.concurrent.ThreadContext;
4447
import org.elasticsearch.common.xcontent.XContentType;
48+
import org.elasticsearch.gateway.GatewayService;
4549
import org.elasticsearch.index.Index;
4650
import org.elasticsearch.index.shard.ShardId;
51+
import org.elasticsearch.rest.RestStatus;
4752
import org.elasticsearch.test.ESTestCase;
4853
import org.elasticsearch.threadpool.ThreadPool;
4954
import org.elasticsearch.xpack.security.test.SecurityTestUtils;
@@ -55,6 +60,10 @@
5560
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_TEMPLATE_NAME;
5661
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.TEMPLATE_VERSION_PATTERN;
5762
import static org.hamcrest.Matchers.equalTo;
63+
import static org.hamcrest.Matchers.instanceOf;
64+
import static org.hamcrest.Matchers.is;
65+
import static org.hamcrest.Matchers.notNullValue;
66+
import static org.hamcrest.Matchers.nullValue;
5867
import static org.mockito.Mockito.mock;
5968
import static org.mockito.Mockito.when;
6069

@@ -72,6 +81,7 @@ public void setUpManager() {
7281
final Client mockClient = mock(Client.class);
7382
final ThreadPool threadPool = mock(ThreadPool.class);
7483
when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY));
84+
when(threadPool.generic()).thenReturn(EsExecutors.newDirectExecutorService());
7585
when(mockClient.threadPool()).thenReturn(threadPool);
7686
when(mockClient.settings()).thenReturn(Settings.EMPTY);
7787
final ClusterService clusterService = mock(ClusterService.class);
@@ -192,6 +202,67 @@ public void testIndexHealthChangeListeners() throws Exception {
192202
assertEquals(ClusterHealthStatus.GREEN, currentState.get().indexStatus);
193203
}
194204

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

241313
private void assertIndexUpToDateButNotAvailable() {
242314
assertThat(manager.indexExists(), Matchers.equalTo(true));
243315
assertThat(manager.isAvailable(), Matchers.equalTo(false));
244316
assertThat(manager.isMappingUpToDate(), Matchers.equalTo(true));
317+
assertThat(manager.isStateRecovered(), Matchers.equalTo(true));
245318
}
246319

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

0 commit comments

Comments
 (0)