From 4761a1fa29b9441cc95217f912b0144c4445cedb Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 6 Jul 2018 08:54:32 +0200 Subject: [PATCH] Do not return all indices if a specific alias is requested via get aliases api. (#29538) If a get alias api call requests a specific alias pattern then indices not having any matching aliases should not be included in the response. This is a second attempt to fix this (first attempt was #28294). The reason that the first attempt was reverted is because when xpack security is enabled then index expression (like * or _all) are resolved prior to when a request is processed in the get aliases transport action, then `MetaData#findAliases` can't know whether requested all where requested since it was already expanded in concrete alias names. This change replaces aliases(...) replaceAliases(...) method on AliasesRequests class and leave the aliases(...) method on subclasses. So there is a distinction between when xpack security replaces aliases and a user setting aliases via the transport or high level http client. Closes #27763 --- .../elasticsearch/action/AliasesRequest.java | 6 +- .../indices/alias/IndicesAliasesRequest.java | 6 +- .../indices/alias/get/GetAliasesRequest.java | 30 +++++++-- .../alias/get/TransportGetAliasesAction.java | 22 ++++++- .../cluster/metadata/MetaData.java | 7 +- .../admin/indices/RestGetAliasesAction.java | 4 ++ .../get/TransportGetAliasesActionTests.java | 64 +++++++++++++++++++ .../elasticsearch/aliases/IndexAliasesIT.java | 31 +++++---- .../authz/IndicesAndAliasesResolver.java | 2 +- 9 files changed, 141 insertions(+), 31 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesActionTests.java diff --git a/server/src/main/java/org/elasticsearch/action/AliasesRequest.java b/server/src/main/java/org/elasticsearch/action/AliasesRequest.java index a4ff57ebd2008..bf7ceb28d5023 100644 --- a/server/src/main/java/org/elasticsearch/action/AliasesRequest.java +++ b/server/src/main/java/org/elasticsearch/action/AliasesRequest.java @@ -33,9 +33,11 @@ public interface AliasesRequest extends IndicesRequest.Replaceable { String[] aliases(); /** - * Sets the array of aliases that the action relates to + * Replaces current aliases with the provided aliases. + * + * Sometimes aliases expressions need to be resolved to concrete aliases prior to executing the transport action. */ - AliasesRequest aliases(String... aliases); + void replaceAliases(String... aliases); /** * Returns true if wildcards expressions among aliases should be resolved, false otherwise diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java index c7e7288e74f55..9249550871c12 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java @@ -302,7 +302,6 @@ public AliasActions index(String index) { /** * Aliases to use with this action. */ - @Override public AliasActions aliases(String... aliases) { if (type == AliasActions.Type.REMOVE_INDEX) { throw new IllegalArgumentException("[aliases] is unsupported for [" + type + "]"); @@ -428,6 +427,11 @@ public String[] aliases() { return aliases; } + @Override + public void replaceAliases(String... aliases) { + this.aliases = aliases; + } + @Override public boolean expandAliasesWildcards() { //remove operations support wildcards among aliases, add operations don't diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java index 04b0843e0ae8f..e8dd93144b10e 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.action.admin.indices.alias.get; +import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.AliasesRequest; import org.elasticsearch.action.support.IndicesOptions; @@ -32,15 +33,12 @@ public class GetAliasesRequest extends MasterNodeReadRequest private String[] indices = Strings.EMPTY_ARRAY; private String[] aliases = Strings.EMPTY_ARRAY; - private IndicesOptions indicesOptions = IndicesOptions.strictExpand(); + private String[] originalAliases = Strings.EMPTY_ARRAY; - public GetAliasesRequest(String[] aliases) { + public GetAliasesRequest(String... aliases) { this.aliases = aliases; - } - - public GetAliasesRequest(String alias) { - this.aliases = new String[]{alias}; + this.originalAliases = aliases; } public GetAliasesRequest() { @@ -51,6 +49,9 @@ public GetAliasesRequest(StreamInput in) throws IOException { indices = in.readStringArray(); aliases = in.readStringArray(); indicesOptions = IndicesOptions.readIndicesOptions(in); + if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + originalAliases = in.readStringArray(); + } } @Override @@ -59,6 +60,9 @@ public void writeTo(StreamOutput out) throws IOException { out.writeStringArray(indices); out.writeStringArray(aliases); indicesOptions.writeIndicesOptions(out); + if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + out.writeStringArray(originalAliases); + } } @Override @@ -67,9 +71,9 @@ public GetAliasesRequest indices(String... indices) { return this; } - @Override public GetAliasesRequest aliases(String... aliases) { this.aliases = aliases; + this.originalAliases = aliases; return this; } @@ -88,6 +92,18 @@ public String[] aliases() { return aliases; } + @Override + public void replaceAliases(String... aliases) { + this.aliases = aliases; + } + + /** + * Returns the aliases as was originally specified by the user + */ + public String[] getOriginalAliases() { + return originalAliases; + } + @Override public boolean expandAliasesWildcards() { return true; diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java index 96ecde4e4c6dd..1bacd652ee734 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java @@ -33,6 +33,7 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import java.util.Collections; import java.util.List; public class TransportGetAliasesAction extends TransportMasterNodeReadAction { @@ -62,7 +63,24 @@ protected GetAliasesResponse newResponse() { @Override protected void masterOperation(GetAliasesRequest request, ClusterState state, ActionListener listener) { String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request); - ImmutableOpenMap> result = state.metaData().findAliases(request.aliases(), concreteIndices); - listener.onResponse(new GetAliasesResponse(result)); + ImmutableOpenMap> aliases = state.metaData().findAliases(request.aliases(), concreteIndices); + listener.onResponse(new GetAliasesResponse(postProcess(request, concreteIndices, aliases))); } + + /** + * Fills alias result with empty entries for requested indices when no specific aliases were requested. + */ + static ImmutableOpenMap> postProcess(GetAliasesRequest request, String[] concreteIndices, + ImmutableOpenMap> aliases) { + boolean noAliasesSpecified = request.getOriginalAliases() == null || request.getOriginalAliases().length == 0; + ImmutableOpenMap.Builder> mapBuilder = ImmutableOpenMap.builder(aliases); + for (String index : concreteIndices) { + if (aliases.get(index) == null && noAliasesSpecified) { + List previous = mapBuilder.put(index, Collections.emptyList()); + assert previous == null; + } + } + return mapBuilder.build(); + } + } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 2bfe0d0a58f70..4ed2adc9a1c9f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -265,8 +265,7 @@ public ImmutableOpenMap> findAliases(final String[] boolean matchAllAliases = matchAllAliases(aliases); ImmutableOpenMap.Builder> mapBuilder = ImmutableOpenMap.builder(); - Iterable intersection = HppcMaps.intersection(ObjectHashSet.from(concreteIndices), indices.keys()); - for (String index : intersection) { + for (String index : concreteIndices) { IndexMetaData indexMetaData = indices.get(index); List filteredValues = new ArrayList<>(); for (ObjectCursor cursor : indexMetaData.getAliases().values()) { @@ -276,11 +275,11 @@ public ImmutableOpenMap> findAliases(final String[] } } - if (!filteredValues.isEmpty()) { + if (filteredValues.isEmpty() == false) { // Make the list order deterministic CollectionUtil.timSort(filteredValues, Comparator.comparing(AliasMetaData::alias)); + mapBuilder.put(index, Collections.unmodifiableList(filteredValues)); } - mapBuilder.put(index, Collections.unmodifiableList(filteredValues)); } return mapBuilder.build(); } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java index 8a1e4e74e819e..0d6d46e95b602 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java @@ -77,6 +77,10 @@ public String getName() { @Override public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { + // The TransportGetAliasesAction was improved do the same post processing as is happening here. + // We can't remove this logic yet to support mixed clusters. We should be able to remove this logic here + // in when 8.0 becomes the new version in the master branch. + final boolean namesProvided = request.hasParam("name"); final String[] aliases = request.paramAsStringArrayOrEmptyIfAll("name"); final GetAliasesRequest getAliasesRequest = new GetAliasesRequest(aliases); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesActionTests.java new file mode 100644 index 0000000000000..d445a63aea170 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesActionTests.java @@ -0,0 +1,64 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.action.admin.indices.alias.get; + +import org.elasticsearch.cluster.metadata.AliasMetaData; +import org.elasticsearch.common.collect.ImmutableOpenMap; +import org.elasticsearch.test.ESTestCase; + +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; + +public class TransportGetAliasesActionTests extends ESTestCase { + + public void testPostProcess() { + GetAliasesRequest request = new GetAliasesRequest(); + ImmutableOpenMap> aliases = ImmutableOpenMap.>builder() + .fPut("b", Collections.singletonList(new AliasMetaData.Builder("y").build())) + .build(); + ImmutableOpenMap> result = + TransportGetAliasesAction.postProcess(request, new String[]{"a", "b", "c"}, aliases); + assertThat(result.size(), equalTo(3)); + assertThat(result.get("a").size(), equalTo(0)); + assertThat(result.get("b").size(), equalTo(1)); + assertThat(result.get("c").size(), equalTo(0)); + + request = new GetAliasesRequest(); + request.replaceAliases("y", "z"); + aliases = ImmutableOpenMap.>builder() + .fPut("b", Collections.singletonList(new AliasMetaData.Builder("y").build())) + .build(); + result = TransportGetAliasesAction.postProcess(request, new String[]{"a", "b", "c"}, aliases); + assertThat(result.size(), equalTo(3)); + assertThat(result.get("a").size(), equalTo(0)); + assertThat(result.get("b").size(), equalTo(1)); + assertThat(result.get("c").size(), equalTo(0)); + + request = new GetAliasesRequest("y", "z"); + aliases = ImmutableOpenMap.>builder() + .fPut("b", Collections.singletonList(new AliasMetaData.Builder("y").build())) + .build(); + result = TransportGetAliasesAction.postProcess(request, new String[]{"a", "b", "c"}, aliases); + assertThat(result.size(), equalTo(1)); + assertThat(result.get("b").size(), equalTo(1)); + } + +} diff --git a/server/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java b/server/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java index 8bf074be551b1..d72b4c5f1ec16 100644 --- a/server/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java +++ b/server/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java @@ -570,24 +570,20 @@ public void testIndicesGetAliases() throws Exception { logger.info("--> getting alias1"); GetAliasesResponse getResponse = admin().indices().prepareGetAliases("alias1").get(); assertThat(getResponse, notNullValue()); - assertThat(getResponse.getAliases().size(), equalTo(5)); + assertThat(getResponse.getAliases().size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").get(0), notNullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).alias(), equalTo("alias1")); assertThat(getResponse.getAliases().get("foobar").get(0).getFilter(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getIndexRouting(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getSearchRouting(), nullValue()); - assertTrue(getResponse.getAliases().get("test").isEmpty()); - assertTrue(getResponse.getAliases().get("test123").isEmpty()); - assertTrue(getResponse.getAliases().get("foobarbaz").isEmpty()); - assertTrue(getResponse.getAliases().get("bazbar").isEmpty()); AliasesExistResponse existsResponse = admin().indices().prepareAliasesExist("alias1").get(); assertThat(existsResponse.exists(), equalTo(true)); logger.info("--> getting all aliases that start with alias*"); getResponse = admin().indices().prepareGetAliases("alias*").get(); assertThat(getResponse, notNullValue()); - assertThat(getResponse.getAliases().size(), equalTo(5)); + assertThat(getResponse.getAliases().size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").size(), equalTo(2)); assertThat(getResponse.getAliases().get("foobar").get(0), notNullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).alias(), equalTo("alias1")); @@ -599,10 +595,6 @@ public void testIndicesGetAliases() throws Exception { assertThat(getResponse.getAliases().get("foobar").get(1).getFilter(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(1).getIndexRouting(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(1).getSearchRouting(), nullValue()); - assertTrue(getResponse.getAliases().get("test").isEmpty()); - assertTrue(getResponse.getAliases().get("test123").isEmpty()); - assertTrue(getResponse.getAliases().get("foobarbaz").isEmpty()); - assertTrue(getResponse.getAliases().get("bazbar").isEmpty()); existsResponse = admin().indices().prepareAliasesExist("alias*").get(); assertThat(existsResponse.exists(), equalTo(true)); @@ -687,13 +679,12 @@ public void testIndicesGetAliases() throws Exception { logger.info("--> getting f* for index *bar"); getResponse = admin().indices().prepareGetAliases("f*").addIndices("*bar").get(); assertThat(getResponse, notNullValue()); - assertThat(getResponse.getAliases().size(), equalTo(2)); + assertThat(getResponse.getAliases().size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").get(0), notNullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).alias(), equalTo("foo")); assertThat(getResponse.getAliases().get("foobar").get(0).getFilter(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getIndexRouting(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getSearchRouting(), nullValue()); - assertTrue(getResponse.getAliases().get("bazbar").isEmpty()); existsResponse = admin().indices().prepareAliasesExist("f*") .addIndices("*bar").get(); assertThat(existsResponse.exists(), equalTo(true)); @@ -702,14 +693,13 @@ public void testIndicesGetAliases() throws Exception { logger.info("--> getting f* for index *bac"); getResponse = admin().indices().prepareGetAliases("foo").addIndices("*bac").get(); assertThat(getResponse, notNullValue()); - assertThat(getResponse.getAliases().size(), equalTo(2)); + assertThat(getResponse.getAliases().size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").size(), equalTo(1)); assertThat(getResponse.getAliases().get("foobar").get(0), notNullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).alias(), equalTo("foo")); assertThat(getResponse.getAliases().get("foobar").get(0).getFilter(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getIndexRouting(), nullValue()); assertThat(getResponse.getAliases().get("foobar").get(0).getSearchRouting(), nullValue()); - assertTrue(getResponse.getAliases().get("bazbar").isEmpty()); existsResponse = admin().indices().prepareAliasesExist("foo") .addIndices("*bac").get(); assertThat(existsResponse.exists(), equalTo(true)); @@ -727,6 +717,19 @@ public void testIndicesGetAliases() throws Exception { .addIndices("foobar").get(); assertThat(existsResponse.exists(), equalTo(true)); + for (String aliasName : new String[]{null, "_all", "*"}) { + logger.info("--> getting {} alias for index foobar", aliasName); + getResponse = aliasName != null ? admin().indices().prepareGetAliases(aliasName).addIndices("foobar").get() : + admin().indices().prepareGetAliases().addIndices("foobar").get(); + assertThat(getResponse, notNullValue()); + assertThat(getResponse.getAliases().size(), equalTo(1)); + assertThat(getResponse.getAliases().get("foobar").size(), equalTo(4)); + assertThat(getResponse.getAliases().get("foobar").get(0).alias(), equalTo("alias1")); + assertThat(getResponse.getAliases().get("foobar").get(1).alias(), equalTo("alias2")); + assertThat(getResponse.getAliases().get("foobar").get(2).alias(), equalTo("bac")); + assertThat(getResponse.getAliases().get("foobar").get(3).alias(), equalTo("foo")); + } + // alias at work again logger.info("--> getting * for index *bac"); getResponse = admin().indices().prepareGetAliases("*").addIndices("*bac").get(); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java index 941bf13daf584..77170f7a1cbfb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java @@ -200,7 +200,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData if (aliasesRequest.expandAliasesWildcards()) { List aliases = replaceWildcardsWithAuthorizedAliases(aliasesRequest.aliases(), loadAuthorizedAliases(authorizedIndices.get(), metaData)); - aliasesRequest.aliases(aliases.toArray(new String[aliases.size()])); + aliasesRequest.replaceAliases(aliases.toArray(new String[aliases.size()])); } if (indicesReplacedWithNoIndices) { if (indicesRequest instanceof GetAliasesRequest == false) {