Skip to content

Commit

Permalink
Security: reduce garbage during index resolution (#30180)
Browse files Browse the repository at this point in the history 
The IndexAndAliasesResolver resolves the indices and aliases for each
request and also handles local and remote indices. The current
implementation uses the ResolvedIndices class to hold the resolved
indices and aliases. While evaluating the indices and aliases against
the user's permissions, the final value for ResolvedIndices is
constructed. Prior to this change, this was done by creating a
ResolvedIndices for the first set of indices and for each additional
addition, a new ResolvedIndices object is created and merged with
the existing one. With a small number of indices and aliases this does
not pose a large problem; however as the number of indices/aliases
grows more list allocations and array copies are needed resulting in a
large amount of garbage and severely impacted performance.

This change introduces a builder for ResolvedIndices that appends to
mutable lists until the final value has been constructed, which will
ultimately reduce the amount of garbage generated by this code.
  • Loading branch information
@jaymode
jaymode committed May 3, 2018
1 parent f940318 commit b0a1125
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 55 deletions.
13 changes: 13 additions & 0 deletions docs/CHANGELOG.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,19 @@ Machine Learning::

* Account for gaps in data counts after job is reopened ({pull}30294[#30294])

== Elasticsearch version 6.3.1

=== New Features

=== Enhancements

=== Bug Fixes

Reduce the number of object allocations made by {security} when resolving the indices and aliases for a request ({pull}30180[#30180])

=== Regressions

=== Known Issues
//[float]
//=== Regressions

Expand Down
118 changes: 63 additions & 55 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.stream.Collectors;

import static org.elasticsearch.xpack.core.security.authz.IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER;

public class IndicesAndAliasesResolver {

private static final ResolvedIndices NO_INDEX_PLACEHOLDER_RESOLVED =
ResolvedIndices.local(IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER);
//`*,-*` what we replace indices with if we need Elasticsearch to return empty responses without throwing exception
private static final String[] NO_INDICES_ARRAY = new String[] { "*", "-*" };
static final List<String> NO_INDICES_LIST = Arrays.asList(NO_INDICES_ARRAY);
Expand Down Expand Up @@ -87,12 +87,14 @@ public IndicesAndAliasesResolver(Settings settings, ClusterService clusterServic

public ResolvedIndices resolve(TransportRequest request, MetaData metaData, AuthorizedIndices authorizedIndices) {
if (request instanceof IndicesAliasesRequest) {
ResolvedIndices indices = ResolvedIndices.empty();
ResolvedIndices.Builder resolvedIndicesBuilder = new ResolvedIndices.Builder();
IndicesAliasesRequest indicesAliasesRequest = (IndicesAliasesRequest) request;
for (IndicesRequest indicesRequest : indicesAliasesRequest.getAliasActions()) {
indices = ResolvedIndices.add(indices, resolveIndicesAndAliases(indicesRequest, metaData, authorizedIndices));
final ResolvedIndices resolved = resolveIndicesAndAliases(indicesRequest, metaData, authorizedIndices);
resolvedIndicesBuilder.addLocal(resolved.getLocal());
resolvedIndicesBuilder.addRemote(resolved.getRemote());
}
return indices;
return resolvedIndicesBuilder.build();
}

// if for some reason we are missing an action... just for safety we'll reject
Expand All @@ -102,10 +104,10 @@ public ResolvedIndices resolve(TransportRequest request, MetaData metaData, Auth
return resolveIndicesAndAliases((IndicesRequest) request, metaData, authorizedIndices);
}

ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData metaData,
AuthorizedIndices authorizedIndices) {

ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData metaData, AuthorizedIndices authorizedIndices) {
final ResolvedIndices.Builder resolvedIndicesBuilder = new ResolvedIndices.Builder();
boolean indicesReplacedWithNoIndices = false;
final ResolvedIndices indices;
if (indicesRequest instanceof PutMappingRequest && ((PutMappingRequest) indicesRequest).getConcreteIndex() != null) {
/*
* This is a special case since PutMappingRequests from dynamic mapping updates have a concrete index
Expand All @@ -114,7 +116,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
*/
assert indicesRequest.indices() == null || indicesRequest.indices().length == 0
: "indices are: " + Arrays.toString(indicesRequest.indices()); // Arrays.toString() can handle null values - all good
return ResolvedIndices.local(((PutMappingRequest) indicesRequest).getConcreteIndex().getName());
resolvedIndicesBuilder.addLocal(((PutMappingRequest) indicesRequest).getConcreteIndex().getName());
} else if (indicesRequest instanceof IndicesRequest.Replaceable) {
IndicesRequest.Replaceable replaceable = (IndicesRequest.Replaceable) indicesRequest;
final boolean replaceWildcards = indicesRequest.indicesOptions().expandWildcardsOpen()
Expand All @@ -127,13 +129,12 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
indicesOptions.expandWildcardsOpen(), indicesOptions.expandWildcardsClosed());
}

ResolvedIndices result = ResolvedIndices.empty();
// check for all and return list of authorized indices
if (IndexNameExpressionResolver.isAllIndices(indicesList(indicesRequest.indices()))) {
if (replaceWildcards) {
for (String authorizedIndex : authorizedIndices.get()) {
if (isIndexVisible(authorizedIndex, indicesOptions, metaData)) {
result = ResolvedIndices.add(result, ResolvedIndices.local(authorizedIndex));
resolvedIndicesBuilder.addLocal(authorizedIndex);
}
}
}
Expand All @@ -144,7 +145,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
if (allowsRemoteIndices(indicesRequest)) {
split = remoteClusterResolver.splitLocalAndRemoteIndexNames(indicesRequest.indices());
} else {
split = ResolvedIndices.local(indicesRequest.indices());
split = new ResolvedIndices(Arrays.asList(indicesRequest.indices()), Collections.emptyList());
}
List<String> replaced = replaceWildcardsWithAuthorizedIndices(split.getLocal(), indicesOptions, metaData,
authorizedIndices.get(), replaceWildcards);
Expand All @@ -153,22 +154,23 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
//remove all the ones that the current user is not authorized for and ignore them
replaced = replaced.stream().filter(authorizedIndices.get()::contains).collect(Collectors.toList());
}
result = new ResolvedIndices(new ArrayList<>(replaced), split.getRemote());
resolvedIndicesBuilder.addLocal(replaced);
resolvedIndicesBuilder.addRemote(split.getRemote());
}
if (result.isEmpty()) {

if (resolvedIndicesBuilder.isEmpty()) {
if (indicesOptions.allowNoIndices()) {
//this is how we tell es core to return an empty response, we can let the request through being sure
//that the '-*' wildcard expression will be resolved to no indices. We can't let empty indices through
//as that would be resolved to _all by es core.
replaceable.indices(NO_INDICES_ARRAY);
indicesReplacedWithNoIndices = true;
indices = NO_INDEX_PLACEHOLDER_RESOLVED;
resolvedIndicesBuilder.addLocal(NO_INDEX_PLACEHOLDER);
} else {
throw new IndexNotFoundException(Arrays.toString(indicesRequest.indices()));
}
} else {
replaceable.indices(result.toArray());
indices = result;
replaceable.indices(resolvedIndicesBuilder.build().toArray());
}
} else {
if (containsWildcards(indicesRequest)) {
Expand All @@ -182,11 +184,9 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
//That is fine though because they never contain wildcards, as they get replaced as part of the authorization of their
//corresponding parent request on the coordinating node. Hence wildcards don't need to get replaced nor exploded for
// shard level requests.
List<String> resolvedNames = new ArrayList<>();
for (String name : indicesRequest.indices()) {
resolvedNames.add(nameExpressionResolver.resolveDateMathExpression(name));
resolvedIndicesBuilder.addLocal(nameExpressionResolver.resolveDateMathExpression(name));
}
indices = new ResolvedIndices(resolvedNames, new ArrayList<>());
}

if (indicesRequest instanceof AliasesRequest) {
Expand All @@ -207,10 +207,10 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
//if we replaced the indices with '-*' we shouldn't be adding the aliases to the list otherwise the request will
//not get authorized. Leave only '-*' and ignore the rest, result will anyway be empty.
} else {
return ResolvedIndices.add(indices, ResolvedIndices.local(aliasesRequest.aliases()));
resolvedIndicesBuilder.addLocal(aliasesRequest.aliases());
}
}
return indices;
return resolvedIndicesBuilder.build();
}

public static boolean allowsRemoteIndices(IndicesRequest request) {
Expand Down Expand Up @@ -423,24 +423,8 @@ public static class ResolvedIndices {
private final List<String> remote;

ResolvedIndices(List<String> local, List<String> remote) {
this.local = local;
this.remote = remote;
}

/**
* Constructs a new instance of this class where both the {@link #getLocal() local} and {@link #getRemote() remote} index lists
* are empty.
*/
private static ResolvedIndices empty() {
return new ResolvedIndices(Collections.emptyList(), Collections.emptyList());
}

/**
* Constructs a new instance of this class where both the {@link #getLocal() local} index list is populated with <code>names</code>
* and the {@link #getRemote() remote} index list is empty.
*/
private static ResolvedIndices local(String... names) {
return new ResolvedIndices(Arrays.asList(names), Collections.emptyList());
this.local = Collections.unmodifiableList(local);
this.remote = Collections.unmodifiableList(remote);
}

/**
Expand All @@ -449,14 +433,14 @@ private static ResolvedIndices local(String... names) {
* to <code>[ "-a1", "a*" ]</code>. As a consequence, this list <em>may contain duplicates</em>.
*/
public List<String> getLocal() {
return Collections.unmodifiableList(local);
return local;
}

/**
* Returns the collection of index names that have been stored as "remote" indices.
*/
public List<String> getRemote() {
return Collections.unmodifiableList(remote);
return remote;
}

/**
Expand All @@ -471,7 +455,7 @@ public boolean isEmpty() {
* {@link IndicesAndAliasesResolverField#NO_INDEX_PLACEHOLDER no-index-placeholder} and nothing else.
*/
public boolean isNoIndicesPlaceholder() {
return remote.isEmpty() && local.size() == 1 && local.contains(IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER);
return remote.isEmpty() && local.size() == 1 && local.contains(NO_INDEX_PLACEHOLDER);
}

private String[] toArray() {
Expand All @@ -487,19 +471,43 @@ private String[] toArray() {
}

/**
* Returns a new <code>ResolvedIndices</code> contains the {@link #getLocal() local} and {@link #getRemote() remote}
* index lists from <code>b</code> appended to the corresponding lists in <code>a</code>.
* Builder class for ResolvedIndices that allows for the building of a list of indices
* without the need to construct new objects and merging them together
*/
private static ResolvedIndices add(ResolvedIndices a, ResolvedIndices b) {
List<String> local = new ArrayList<>(a.local.size() + b.local.size());
local.addAll(a.local);
local.addAll(b.local);

List<String> remote = new ArrayList<>(a.remote.size() + b.remote.size());
remote.addAll(a.remote);
remote.addAll(b.remote);
return new ResolvedIndices(local, remote);
}
private static class Builder {

private final List<String> local = new ArrayList<>();
private final List<String> remote = new ArrayList<>();

/** add a local index name */
private void addLocal(String index) {
local.add(index);
}

/** adds the array of local index names */
private void addLocal(String[] indices) {
local.addAll(Arrays.asList(indices));
}

/** adds the list of local index names */
private void addLocal(List<String> indices) {
local.addAll(indices);
}

/** adds the list of remote index names */
private void addRemote(List<String> indices) {
remote.addAll(indices);
}

/** @return <code>true</code> if both the local and remote index lists are empty. */
private boolean isEmpty() {
return local.isEmpty() && remote.isEmpty();
}

/** @return a immutable ResolvedIndices instance with the local and remote index lists */
private ResolvedIndices build() {
return new ResolvedIndices(local, remote);
}
}
}
}

0 comments on commit b0a1125

Please sign in to comment.