Skip to content

Commit

Permalink
Do not return all indices if a specific alias is requested via get al…
Browse files Browse the repository at this point in the history
…iases 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
  • Loading branch information
martijnvg committed Jul 6, 2018
1 parent f6fb424 commit cade257
Show file tree
Hide file tree
Showing 9 changed files with 142 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 + "]");
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.elasticsearch.action.admin.indices.alias.get;

import org.elasticsearch.Version;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.AliasesRequest;
import org.elasticsearch.action.support.IndicesOptions;
Expand All @@ -32,15 +34,12 @@ public class GetAliasesRequest extends MasterNodeReadRequest<GetAliasesRequest>

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() {
Expand All @@ -52,9 +51,9 @@ public GetAliasesRequest indices(String... indices) {
return this;
}

@Override
public GetAliasesRequest aliases(String... aliases) {
this.aliases = aliases;
this.originalAliases = aliases;
return this;
}

Expand All @@ -73,6 +72,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;
Expand All @@ -94,6 +105,9 @@ public void readFrom(StreamInput in) throws IOException {
indices = in.readStringArray();
aliases = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
originalAliases = in.readStringArray();
}
}

@Override
Expand All @@ -102,5 +116,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeStringArray(indices);
out.writeStringArray(aliases);
indicesOptions.writeIndicesOptions(out);
if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
out.writeStringArray(originalAliases);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<GetAliasesRequest, GetAliasesResponse> {
Expand Down Expand Up @@ -62,7 +63,24 @@ protected GetAliasesResponse newResponse() {
@Override
protected void masterOperation(GetAliasesRequest request, ClusterState state, ActionListener<GetAliasesResponse> listener) {
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request);
ImmutableOpenMap<String, List<AliasMetaData>> result = state.metaData().findAliases(request.aliases(), concreteIndices);
listener.onResponse(new GetAliasesResponse(result));
ImmutableOpenMap<String, List<AliasMetaData>> 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<String, List<AliasMetaData>> postProcess(GetAliasesRequest request, String[] concreteIndices,
ImmutableOpenMap<String, List<AliasMetaData>> aliases) {
boolean noAliasesSpecified = request.getOriginalAliases() == null || request.getOriginalAliases().length == 0;
ImmutableOpenMap.Builder<String, List<AliasMetaData>> mapBuilder = ImmutableOpenMap.builder(aliases);
for (String index : concreteIndices) {
if (aliases.get(index) == null && noAliasesSpecified) {
List<AliasMetaData> previous = mapBuilder.put(index, Collections.emptyList());
assert previous == null;
}
}
return mapBuilder.build();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,7 @@ public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final String[]

boolean matchAllAliases = matchAllAliases(aliases);
ImmutableOpenMap.Builder<String, List<AliasMetaData>> mapBuilder = ImmutableOpenMap.builder();
Iterable<String> intersection = HppcMaps.intersection(ObjectHashSet.from(concreteIndices), indices.keys());
for (String index : intersection) {
for (String index : concreteIndices) {
IndexMetaData indexMetaData = indices.get(index);
List<AliasMetaData> filteredValues = new ArrayList<>();
for (ObjectCursor<AliasMetaData> cursor : indexMetaData.getAliases().values()) {
Expand All @@ -276,11 +275,11 @@ public ImmutableOpenMap<String, List<AliasMetaData>> 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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String, List<AliasMetaData>> aliases = ImmutableOpenMap.<String, List<AliasMetaData>>builder()
.fPut("b", Collections.singletonList(new AliasMetaData.Builder("y").build()))
.build();
ImmutableOpenMap<String, List<AliasMetaData>> 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.<String, List<AliasMetaData>>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.<String, List<AliasMetaData>>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));
}

}
31 changes: 17 additions & 14 deletions server/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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));

Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
if (aliasesRequest.expandAliasesWildcards()) {
List<String> 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) {
Expand Down

0 comments on commit cade257

Please sign in to comment.