From 79d38eee10e0f5dbb2ae4594e0772b88d23bc6a2 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Tue, 16 Apr 2019 18:29:13 +1000 Subject: [PATCH] Use alias name from rollover request to query indices stats (#40774) In `TransportRolloverAction` before doing rollover we resolve source index name (write index) from the alias in the rollover request. Before evaluating the conditions and executing rollover action, we retrieve stats, but to do so we used the source index name resolved from the alias instead of alias from the index. This fails when the user is assigned a role with index privilege on the alias instead of the concrete index. This commit fixes this by using the alias from the request. After this change, verified that when we retrieve all the stats (including write + read indexes) we are considering only source index. Closes #40771 --- .../rollover/TransportRolloverAction.java | 4 +- .../TransportRolloverActionTests.java | 146 +++++++++++++++++- .../authz/31_rollover_using_alias.yml | 139 +++++++++++++++++ 3 files changed, 281 insertions(+), 8 deletions(-) create mode 100644 x-pack/plugin/src/test/resources/rest-api-spec/test/security/authz/31_rollover_using_alias.yml diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index ce1f1dc240426..edd59f8b18ca1 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -120,7 +120,7 @@ protected void masterOperation(final RolloverRequest rolloverRequest, final Clus final String rolloverIndexName = indexNameExpressionResolver.resolveDateMathExpression(unresolvedName); MetaDataCreateIndexService.validateIndexName(rolloverIndexName, state); // will fail if the index already exists checkNoDuplicatedAliasInIndexTemplate(metaData, rolloverIndexName, rolloverRequest.getAlias()); - client.admin().indices().prepareStats(sourceIndexName).clear().setDocs(true).execute( + client.admin().indices().prepareStats(rolloverRequest.getAlias()).clear().setDocs(true).execute( new ActionListener() { @Override public void onResponse(IndicesStatsResponse statsResponse) { @@ -249,7 +249,7 @@ static Map evaluateConditions(final Collection> co static Map evaluateConditions(final Collection> conditions, final IndexMetaData metaData, final IndicesStatsResponse statsResponse) { - return evaluateConditions(conditions, statsResponse.getPrimaries().getDocs(), metaData); + return evaluateConditions(conditions, statsResponse.getIndex(metaData.getIndex().getName()).getPrimaries().getDocs(), metaData); } static void validate(MetaData metaData, RolloverRequest request) { diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java index ec3c82ba70b2f..058dcc7243029 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java @@ -20,17 +20,30 @@ package org.elasticsearch.action.admin.indices.rollover; import org.elasticsearch.Version; +import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.indices.alias.IndicesAliasesClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.stats.CommonStats; +import org.elasticsearch.action.admin.indices.stats.IndexStats; +import org.elasticsearch.action.admin.indices.stats.IndicesStatsRequestBuilder; import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse; +import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.ActiveShardCount; +import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.client.AdminClient; +import org.elasticsearch.client.Client; +import org.elasticsearch.client.IndicesAdminClient; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.AliasAction; import org.elasticsearch.cluster.metadata.AliasMetaData; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.IndexTemplateMetaData; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.metadata.MetaDataCreateIndexService; +import org.elasticsearch.cluster.metadata.MetaDataIndexAliasesService; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; @@ -39,9 +52,12 @@ import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.index.shard.DocsStats; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; import org.mockito.ArgumentCaptor; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -51,7 +67,9 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -64,7 +82,9 @@ public void testDocStatsSelectionFromPrimariesOnly() { long docsInShards = 200; final Condition condition = createTestCondition(); - evaluateConditions(Sets.newHashSet(condition), createMetaData(), createIndicesStatResponse(docsInShards, docsInPrimaryShards)); + String indexName = randomAlphaOfLengthBetween(5, 7); + evaluateConditions(Sets.newHashSet(condition), createMetaData(indexName), + createIndicesStatResponse(indexName, docsInShards, docsInPrimaryShards)); final ArgumentCaptor argument = ArgumentCaptor.forClass(Condition.Stats.class); verify(condition).evaluate(argument.capture()); @@ -286,7 +306,7 @@ public void testRejectDuplicateAlias() { .patterns(Arrays.asList("foo-*", "bar-*")) .putAlias(AliasMetaData.builder("foo-write")).putAlias(AliasMetaData.builder("bar-write").writeIndex(randomBoolean())) .build(); - final MetaData metaData = MetaData.builder().put(createMetaData(), false).put(template).build(); + final MetaData metaData = MetaData.builder().put(createMetaData(randomAlphaOfLengthBetween(5, 7)), false).put(template).build(); String indexName = randomFrom("foo-123", "bar-xyz"); String aliasName = randomFrom("foo-write", "bar-write"); final IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, @@ -294,9 +314,92 @@ public void testRejectDuplicateAlias() { assertThat(ex.getMessage(), containsString("index template [test-template]")); } - private IndicesStatsResponse createIndicesStatResponse(long totalDocs, long primaryDocs) { + public void testConditionEvaluationWhenAliasToWriteAndReadIndicesConsidersOnlyPrimariesFromWriteIndex() { + final TransportService mockTransportService = mock(TransportService.class); + final ClusterService mockClusterService = mock(ClusterService.class); + final ThreadPool mockThreadPool = mock(ThreadPool.class); + final MetaDataCreateIndexService mockCreateIndexService = mock(MetaDataCreateIndexService.class); + final IndexNameExpressionResolver mockIndexNameExpressionResolver = mock(IndexNameExpressionResolver.class); + when(mockIndexNameExpressionResolver.resolveDateMathExpression(any())).thenReturn("logs-index-000003"); + final ActionFilters mockActionFilters = mock(ActionFilters.class); + final MetaDataIndexAliasesService mdIndexAliasesService = mock(MetaDataIndexAliasesService.class); + + final Client mockClient = mock(Client.class); + final AdminClient mockAdminClient = mock(AdminClient.class); + final IndicesAdminClient mockIndicesAdminClient = mock(IndicesAdminClient.class); + when(mockClient.admin()).thenReturn(mockAdminClient); + when(mockAdminClient.indices()).thenReturn(mockIndicesAdminClient); + + final IndicesStatsRequestBuilder mockIndicesStatsBuilder = mock(IndicesStatsRequestBuilder.class); + when(mockIndicesAdminClient.prepareStats(any())).thenReturn(mockIndicesStatsBuilder); + final Map indexStats = new HashMap<>(); + int total = randomIntBetween(500, 1000); + indexStats.put("logs-index-000001", createIndexStats(200L, total)); + indexStats.put("logs-index-000002", createIndexStats(300L, total)); + final IndicesStatsResponse statsResponse = createAliasToMultipleIndicesStatsResponse(indexStats); + when(mockIndicesStatsBuilder.clear()).thenReturn(mockIndicesStatsBuilder); + when(mockIndicesStatsBuilder.setDocs(true)).thenReturn(mockIndicesStatsBuilder); + + assert statsResponse.getPrimaries().getDocs().getCount() == 500L; + assert statsResponse.getTotal().getDocs().getCount() == (total + total); + + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 1; + ActionListener listener = (ActionListener) args[0]; + listener.onResponse(statsResponse); + return null; + }).when(mockIndicesStatsBuilder).execute(any(ActionListener.class)); + + final IndexMetaData.Builder indexMetaData = IndexMetaData.builder("logs-index-000001") + .putAlias(AliasMetaData.builder("logs-alias").writeIndex(false).build()).settings(settings(Version.CURRENT)) + .numberOfShards(1).numberOfReplicas(1); + final IndexMetaData.Builder indexMetaData2 = IndexMetaData.builder("logs-index-000002") + .putAlias(AliasMetaData.builder("logs-alias").writeIndex(true).build()).settings(settings(Version.CURRENT)) + .numberOfShards(1).numberOfReplicas(1); + final ClusterState stateBefore = ClusterState.builder(ClusterName.DEFAULT) + .metaData(MetaData.builder().put(indexMetaData).put(indexMetaData2)).build(); + + final TransportRolloverAction transportRolloverAction = new TransportRolloverAction(mockTransportService, mockClusterService, + mockThreadPool, mockCreateIndexService, mockActionFilters, mockIndexNameExpressionResolver, mdIndexAliasesService, + mockClient); + + // For given alias, verify that condition evaluation fails when the condition doc count is greater than the primaries doc count + // (primaries from only write index is considered) + PlainActionFuture future = new PlainActionFuture<>(); + RolloverRequest rolloverRequest = new RolloverRequest("logs-alias", "logs-index-000003"); + rolloverRequest.addMaxIndexDocsCondition(500L); + rolloverRequest.dryRun(true); + transportRolloverAction.masterOperation(rolloverRequest, stateBefore, future); + + RolloverResponse response = future.actionGet(); + assertThat(response.getOldIndex(), equalTo("logs-index-000002")); + assertThat(response.getNewIndex(), equalTo("logs-index-000003")); + assertThat(response.isDryRun(), equalTo(true)); + assertThat(response.isRolledOver(), equalTo(false)); + assertThat(response.getConditionStatus().size(), equalTo(1)); + assertThat(response.getConditionStatus().get("[max_docs: 500]"), is(false)); + + // For given alias, verify that the condition evaluation is successful when condition doc count is less than the primaries doc count + // (primaries from only write index is considered) + future = new PlainActionFuture<>(); + rolloverRequest = new RolloverRequest("logs-alias", "logs-index-000003"); + rolloverRequest.addMaxIndexDocsCondition(300L); + rolloverRequest.dryRun(true); + transportRolloverAction.masterOperation(rolloverRequest, stateBefore, future); + + response = future.actionGet(); + assertThat(response.getOldIndex(), equalTo("logs-index-000002")); + assertThat(response.getNewIndex(), equalTo("logs-index-000003")); + assertThat(response.isDryRun(), equalTo(true)); + assertThat(response.isRolledOver(), equalTo(false)); + assertThat(response.getConditionStatus().size(), equalTo(1)); + assertThat(response.getConditionStatus().get("[max_docs: 300]"), is(true)); + } + + private IndicesStatsResponse createIndicesStatResponse(String indexName, long totalDocs, long primariesDocs) { final CommonStats primaryStats = mock(CommonStats.class); - when(primaryStats.getDocs()).thenReturn(new DocsStats(primaryDocs, 0, between(1, 10000))); + when(primaryStats.getDocs()).thenReturn(new DocsStats(primariesDocs, 0, between(1, 10000))); final CommonStats totalStats = mock(CommonStats.class); when(totalStats.getDocs()).thenReturn(new DocsStats(totalDocs, 0, between(1, 10000))); @@ -304,18 +407,49 @@ private IndicesStatsResponse createIndicesStatResponse(long totalDocs, long prim final IndicesStatsResponse response = mock(IndicesStatsResponse.class); when(response.getPrimaries()).thenReturn(primaryStats); when(response.getTotal()).thenReturn(totalStats); + final IndexStats indexStats = mock(IndexStats.class); + when(response.getIndex(indexName)).thenReturn(indexStats); + when(indexStats.getPrimaries()).thenReturn(primaryStats); + when(indexStats.getTotal()).thenReturn(totalStats); + return response; + } + + private IndicesStatsResponse createAliasToMultipleIndicesStatsResponse(Map indexStats) { + final IndicesStatsResponse response = mock(IndicesStatsResponse.class); + final CommonStats primariesStats = new CommonStats(); + final CommonStats totalStats = new CommonStats(); + for (String indexName : indexStats.keySet()) { + when(response.getIndex(indexName)).thenReturn(indexStats.get(indexName)); + primariesStats.add(indexStats.get(indexName).getPrimaries()); + totalStats.add(indexStats.get(indexName).getTotal()); + } + when(response.getPrimaries()).thenReturn(primariesStats); + when(response.getTotal()).thenReturn(totalStats); return response; } - private static IndexMetaData createMetaData() { + private IndexStats createIndexStats(long primaries, long total) { + final CommonStats primariesCommonStats = mock(CommonStats.class); + when(primariesCommonStats.getDocs()).thenReturn(new DocsStats(primaries, 0, between(1, 10000))); + + final CommonStats totalCommonStats = mock(CommonStats.class); + when(totalCommonStats.getDocs()).thenReturn(new DocsStats(total, 0, between(1, 10000))); + + IndexStats indexStats = mock(IndexStats.class); + when(indexStats.getPrimaries()).thenReturn(primariesCommonStats); + when(indexStats.getTotal()).thenReturn(totalCommonStats); + return indexStats; + } + + private static IndexMetaData createMetaData(String indexName) { final Settings settings = Settings.builder() .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) .put(IndexMetaData.SETTING_INDEX_UUID, UUIDs.randomBase64UUID()) .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) .build(); - return IndexMetaData.builder(randomAlphaOfLength(10)) + return IndexMetaData.builder(indexName) .creationDate(System.currentTimeMillis() - TimeValue.timeValueHours(3).getMillis()) .settings(settings) .build(); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/security/authz/31_rollover_using_alias.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/security/authz/31_rollover_using_alias.yml new file mode 100644 index 0000000000000..52b6259f7ccf0 --- /dev/null +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/security/authz/31_rollover_using_alias.yml @@ -0,0 +1,139 @@ +--- + +setup: + - skip: + features: headers + + - do: + cluster.health: + wait_for_status: yellow + + - do: + security.put_role: + name: "alias_write_manage_role" + body: > + { + "indices": [ + { "names": ["write_manage_alias"], "privileges": ["write", "manage"] } + ] + } + + - do: + security.put_user: + username: "test_user" + body: > + { + "password" : "x-pack-test-password", + "roles" : [ "alias_write_manage_role" ], + "full_name" : "user with privileges to write, manage via alias" + } + + - do: + indices.create: + index: logs-000001 + body: + settings: + index: + number_of_shards: 1 + number_of_replicas: 0 + + - do: + indices.put_alias: + index: logs-000001 + name: write_manage_alias + +--- +teardown: + - do: + security.delete_user: + username: "test_user" + ignore: 404 + + - do: + security.delete_role: + name: "alias_write_role" + ignore: 404 + + - do: + indices.delete_alias: + index: "logs-000001" + name: [ "write_manage_alias" ] + ignore: 404 + + - do: + indices.delete: + index: [ "logs-000001" ] + ignore: 404 + +--- +"Test rollover, index via write alias of index": + + # index using alias + - do: + headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user + create: + id: 1 + index: write_manage_alias + body: > + { + "name" : "doc1" + } + + - do: + headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user + create: + id: 2 + index: write_manage_alias + body: > + { + "name" : "doc2" + } + + - do: + indices.refresh: {} + + # rollover using alias + - do: + headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user + indices.rollover: + alias: "write_manage_alias" + wait_for_active_shards: 1 + body: + conditions: + max_docs: 1 + + - match: { old_index: logs-000001 } + - match: { new_index: logs-000002 } + - match: { rolled_over: true } + - match: { dry_run: false } + - match: { conditions: { "[max_docs: 1]": true } } + + # ensure new index is created + - do: + indices.exists: + index: logs-000002 + + - is_true: '' + + # index using alias + - do: + headers: { Authorization: "Basic dGVzdF91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" } # test_user + create: + id: 3 + index: write_manage_alias + body: > + { + "name" : "doc3" + } + + - do: + indices.refresh: {} + + # check alias points to the new index and the doc was indexed + - do: + search: + rest_total_hits_as_int: true + index: write_manage_alias + + - match: { hits.total: 1 } + - match: { hits.hits.0._index: "logs-000002"}