From 7fecca6329b34e45eaba2822b7dfba271e48d1b2 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Mon, 31 Jul 2017 12:34:14 -0600 Subject: [PATCH] Fix handling of exceptions for concurrent attempts to create security index (elastic/x-pack-elasticsearch#2120) This commit fixes the handling of some exceptions when we attempt to create the security index and alias. The issue here is provoked by a test that is currently muted with an AwaitsFix, GroupMappingTests, which will be unmuted in another change. Original commit: elastic/x-pack-elasticsearch@55f6b656cbe30b31072c992bde0b5a57484e3262 --- .../support/IndexLifecycleManager.java | 36 +++-------------- .../IndexLifecycleManagerIntegTests.java | 40 +++++++++++++++++++ 2 files changed, 46 insertions(+), 30 deletions(-) create mode 100644 plugin/src/test/java/org/elasticsearch/xpack/security/support/IndexLifecycleManagerIntegTests.java diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/support/IndexLifecycleManager.java b/plugin/src/main/java/org/elasticsearch/xpack/security/support/IndexLifecycleManager.java index 6a67fa23754..8cb5282a140 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/support/IndexLifecycleManager.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/support/IndexLifecycleManager.java @@ -19,10 +19,11 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.ResourceAlreadyExistsException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.admin.indices.alias.IndicesAliasesResponse; +import org.elasticsearch.action.admin.indices.alias.Alias; import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexResponse; import org.elasticsearch.cluster.ClusterChangedEvent; @@ -265,11 +266,12 @@ public class IndexLifecycleManager extends AbstractComponent { andThen.run(); } else { CreateIndexRequest request = new CreateIndexRequest(INTERNAL_SECURITY_INDEX); + request.alias(new Alias(SECURITY_INDEX_NAME)); client.admin().indices().create(request, new ActionListener() { @Override public void onResponse(CreateIndexResponse createIndexResponse) { if (createIndexResponse.isAcknowledged()) { - setSecurityIndexAlias(listener, andThen); + andThen.run(); } else { listener.onFailure(new ElasticsearchException("Failed to create security index")); } @@ -277,7 +279,8 @@ public class IndexLifecycleManager extends AbstractComponent { @Override public void onFailure(Exception e) { - if (e instanceof ResourceAlreadyExistsException) { + final Throwable cause = ExceptionsHelper.unwrapCause(e); + if (cause instanceof ResourceAlreadyExistsException) { // the index already exists - it was probably just created so this // node hasn't yet received the cluster state update with the index andThen.run(); @@ -288,31 +291,4 @@ public class IndexLifecycleManager extends AbstractComponent { }); } } - - /** - * Sets the security index alias to .security after it has been created. This is required - * because we cannot add the alias as part of the security index template, as the security - * template is also used when the new security index is created during the upgrade API, at - * which point the old .security index already exists and is being reindexed from, so the - * alias cannot be added as part of the template, hence the alias creation has to happen - * manually here after index creation. - */ - private void setSecurityIndexAlias(final ActionListener listener, final Runnable andThen) { - client.admin().indices().prepareAliases().addAlias(INTERNAL_SECURITY_INDEX, SECURITY_INDEX_NAME) - .execute(new ActionListener() { - @Override - public void onResponse(IndicesAliasesResponse response) { - if (response.isAcknowledged()) { - andThen.run(); - } else { - listener.onFailure(new ElasticsearchException("Failed to set security index alias")); - } - } - - @Override - public void onFailure(Exception e) { - listener.onFailure(e); - } - }); - } } diff --git a/plugin/src/test/java/org/elasticsearch/xpack/security/support/IndexLifecycleManagerIntegTests.java b/plugin/src/test/java/org/elasticsearch/xpack/security/support/IndexLifecycleManagerIntegTests.java new file mode 100644 index 00000000000..29ce70b77cb --- /dev/null +++ b/plugin/src/test/java/org/elasticsearch/xpack/security/support/IndexLifecycleManagerIntegTests.java @@ -0,0 +1,40 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.security.support; + +import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.test.SecurityIntegTestCase; +import org.elasticsearch.xpack.security.action.user.PutUserRequest; +import org.elasticsearch.xpack.security.action.user.PutUserResponse; + +import java.util.ArrayList; +import java.util.List; + +public class IndexLifecycleManagerIntegTests extends SecurityIntegTestCase { + + public void testConcurrentOperationsTryingToCreateSecurityIndexAndAlias() throws Exception { + assertSecurityIndexWriteable(); + final int numRequests = scaledRandomIntBetween(4, 16); + List> futures = new ArrayList<>(numRequests); + List requests = new ArrayList<>(numRequests); + for (int i = 0; i < numRequests; i++) { + requests.add(securityClient() + .preparePutUser("user" + i, "password".toCharArray(), randomAlphaOfLengthBetween(1, 16)) + .request()); + } + + for (PutUserRequest request : requests) { + PlainActionFuture responsePlainActionFuture = new PlainActionFuture<>(); + securityClient().putUser(request, responsePlainActionFuture); + futures.add(responsePlainActionFuture); + } + + for (ActionFuture future : futures) { + assertTrue(future.actionGet().created()); + } + } +}