From aa0d7c73f8e2a6a13577e491d1f6ee02caab8e5a Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Thu, 3 May 2018 12:48:23 -0600 Subject: [PATCH] Security: reduce garbage during index resolution (#30180) 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. --- docs/CHANGELOG.asciidoc | 13 ++ .../authz/IndicesAndAliasesResolver.java | 116 ++++++++++-------- 2 files changed, 75 insertions(+), 54 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 1fb8534a19c..452fee34d6c 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -113,6 +113,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 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 5c0000d4304..4c4d0afc10d 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 @@ -40,10 +40,10 @@ import java.util.SortedMap; 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 NO_INDICES_LIST = Arrays.asList(NO_INDICES_ARRAY); @@ -87,12 +87,14 @@ public class IndicesAndAliasesResolver { 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 @@ -102,10 +104,10 @@ public class IndicesAndAliasesResolver { 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 @@ -114,7 +116,7 @@ public class IndicesAndAliasesResolver { */ 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() @@ -127,13 +129,12 @@ public class IndicesAndAliasesResolver { 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); } } } @@ -144,7 +145,7 @@ public class IndicesAndAliasesResolver { if (allowsRemoteIndices(indicesRequest)) { split = remoteClusterResolver.splitLocalAndRemoteIndexNames(indicesRequest.indices()); } else { - split = ResolvedIndices.local(indicesRequest.indices()); + split = new ResolvedIndices(Arrays.asList(indicesRequest.indices()), Collections.emptyList()); } List replaced = replaceWildcardsWithAuthorizedIndices(split.getLocal(), indicesOptions, metaData, authorizedIndices.get(), replaceWildcards); @@ -153,22 +154,23 @@ public class IndicesAndAliasesResolver { //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)) { @@ -182,11 +184,9 @@ public class IndicesAndAliasesResolver { //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 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) { @@ -207,10 +207,10 @@ public class IndicesAndAliasesResolver { //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) { @@ -423,24 +423,8 @@ public class IndicesAndAliasesResolver { private final List remote; ResolvedIndices(List local, List 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 names - * 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); } /** @@ -449,14 +433,14 @@ public class IndicesAndAliasesResolver { * to [ "-a1", "a*" ]. As a consequence, this list may contain duplicates. */ public List getLocal() { - return Collections.unmodifiableList(local); + return local; } /** * Returns the collection of index names that have been stored as "remote" indices. */ public List getRemote() { - return Collections.unmodifiableList(remote); + return remote; } /** @@ -471,7 +455,7 @@ public class IndicesAndAliasesResolver { * {@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() { @@ -487,19 +471,43 @@ public class IndicesAndAliasesResolver { } /** - * Returns a new ResolvedIndices contains the {@link #getLocal() local} and {@link #getRemote() remote} - * index lists from b appended to the corresponding lists in a. + * 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 local = new ArrayList<>(a.local.size() + b.local.size()); - local.addAll(a.local); - local.addAll(b.local); + private static class Builder { - List remote = new ArrayList<>(a.remote.size() + b.remote.size()); - remote.addAll(a.remote); - remote.addAll(b.remote); - return new ResolvedIndices(local, remote); + private final List local = new ArrayList<>(); + private final List 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 indices) { + local.addAll(indices); + } + + /** adds the list of remote index names */ + private void addRemote(List indices) { + remote.addAll(indices); + } + + /** @return true 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); + } } - } }