Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security: reduce garbage during index resolution #30180

Merged
merged 4 commits into from
May 3, 2018
Merged

Security: reduce garbage during index resolution #30180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f9c5a4
Security: reduce garbage during index resoltuion
jaymode Apr 26, 2018
37cb96a
Merge branch 'master' into resolved_indices_array_copies
jaymode Apr 30, 2018
ce5f85d
add changelog entry
jaymode Apr 30, 2018
2f216d9
Merge branch 'master' into resolved_indices_array_copies
jaymode May 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,19 @@ multi-argument versions. ({pull}29623[#29623])
Do not ignore request analysis/similarity settings on index resize operations when the source index already contains such settings ({pull}30216[#30216])


== 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
Open in desktop
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);
}
}
}
}