Skip to content
Closed
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -124,12 +125,13 @@ ResolvedIndices resolveIndicesAndAliases(String action, IndicesRequest indicesRe
: "indices are: " + Arrays.toString(indicesRequest.indices()); // Arrays.toString() can handle null values - all good
resolvedIndicesBuilder.addLocal(getPutMappingIndexOrAlias((PutMappingRequest) indicesRequest, authorizedIndices, metadata));
} else if (indicesRequest instanceof IndicesRequest.Replaceable) {
final String[] indices = indicesRequest.indices();
final IndicesRequest.Replaceable replaceable = (IndicesRequest.Replaceable) indicesRequest;
final IndicesOptions indicesOptions = indicesRequest.indicesOptions();
final boolean replaceWildcards = indicesOptions.expandWildcardsOpen() || indicesOptions.expandWildcardsClosed();

// check for all and return list of authorized indices
if (IndexNameExpressionResolver.isAllIndices(indicesList(indicesRequest.indices()))) {
if (IndexNameExpressionResolver.isAllIndices(indicesList(indices))) {
if (replaceWildcards) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you realized that the lone * index expression with replaceWildcards == false now behaves slightly different.
Before, Security would leave the * in-place and would leave core do its thing (fail in some way).
Now, we either fail in Security with index not found exception, or pass it to core as *, -*, which would also probably fail (again because request options say to not replace wildcards).
I'm not terribly concerned, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't realise that. Thanks for raising it. I was relying on the tests to catch any inconsistent behaviour and it seems we have some test gap.

I did some more digging and found there are behaviour differences:

  1. GET */_search?expand_wildcards=none&allow_no_indices=false -> 404 Error
    • Existing -> 404 Error
    • Proposed -> 404 Error
  2. GET */_search?expand_wildcards=none&allow_no_indices=true ->
    • Existing -> Empty search result
    • Proposed -> 404 Error

The difference is about how allow_no_indices is intepreted. I'd argue that the existing behaviour is not accurate because allow_no_indices implies "expansion" of either wildcards or aliases. But the request explicilty asks for no expansion. Hence the name * should be treated as a literal index name. In that case, the relevant option should be ignore_unavailable instead of allow_no_indices. That is the following request should return empty result

  1. GET */_search?expand_wildcards=none&ignore_unavailable=true
    • Existing -> Empty result
    • Proposed -> Empty result

Based on the following comment, it seems that core is not fully convinced with its own behaviour either.

// If only one index is specified then whether we fail a request if an index is missing depends on the allow_no_indices
// option. At some point we should change this, because there shouldn't be a reason why whether a single index
// or multiple indices are specified yield different behaviour.
final boolean failNoIndices = indexExpressions.length == 1
? options.allowNoIndices() == false
: options.ignoreUnavailable() == false;

That said, behaviour change is what I tried to avoid and it is the reason why I bothered to add special handling for error message. I'll make some adjustments. Thanks again for noticing it.

for (String authorizedIndex : authorizedIndices) {
if (IndexAbstractionResolver.isIndexVisible("*", authorizedIndex, indicesOptions, metadata, nameExpressionResolver,
Expand All @@ -143,16 +145,22 @@ ResolvedIndices resolveIndicesAndAliases(String action, IndicesRequest indicesRe
} else {
final ResolvedIndices split;
if (indicesRequest.allowsRemoteIndices()) {
split = remoteClusterResolver.splitLocalAndRemoteIndexNames(indicesRequest.indices());
split = remoteClusterResolver.splitLocalAndRemoteIndexNames(indices);
} else {
split = new ResolvedIndices(Arrays.asList(indicesRequest.indices()), Collections.emptyList());
split = new ResolvedIndices(Arrays.asList(indices), Collections.emptyList());
}
List<String> replaced = indexAbstractionResolver.resolveIndexAbstractions(split.getLocal(), indicesOptions, metadata,
authorizedIndices, replaceWildcards, indicesRequest.includeDataStreams());

assert indices != null && indices.length != 0 : "null or empty indices should be handled by isAllIndices";
// Filtering is needed only if the request needs filter out unavailable indices
if (indicesOptions.ignoreUnavailable()) {
//out of all the explicit names (expanded from wildcards and original ones that were left untouched)
//remove all the ones that the current user is not authorized for and ignore them
replaced = replaced.stream().filter(authorizedIndices::contains).collect(Collectors.toList());
final List<String> filtered = filterOutUnavailable(authorizedIndices, replaced);
if (filtered != null) {
replaced = filtered;
}
}
resolvedIndicesBuilder.addLocal(replaced);
resolvedIndicesBuilder.addRemote(split.getRemote());
Expand All @@ -167,7 +175,7 @@ ResolvedIndices resolveIndicesAndAliases(String action, IndicesRequest indicesRe
indicesReplacedWithNoIndices = true;
resolvedIndicesBuilder.addLocal(NO_INDEX_PLACEHOLDER);
} else {
throw new IndexNotFoundException(Arrays.toString(indicesRequest.indices()));
throw new IndexNotFoundException(Arrays.toString(indices));
}
} else {
replaceable.indices(resolvedIndicesBuilder.build().toArray());
Expand Down Expand Up @@ -244,6 +252,28 @@ ResolvedIndices resolveIndicesAndAliases(String action, IndicesRequest indicesRe
return resolvedIndicesBuilder.build();
}

private List<String> filterOutUnavailable(Set<String> authorizedIndices, List<String> replaced) {
// Avoid creating filtered list if all replaced indices are available.
// Also avoid resizing the filtered list by creating the list with sufficient size in the beginning.
List<String> filtered = null;
for (ListIterator<String> itr = replaced.listIterator(); itr.hasNext();) {
final String index = itr.next();
if (authorizedIndices.contains(index) == false) {
// Only creating the filtered list when there are unavailable indices
if (filtered == null) {
filtered = new ArrayList<>(replaced.size() - 1);
filtered.addAll(replaced.subList(0, itr.previousIndex()));
}
} else {
if (filtered != null) {
filtered.add(index);
}
// If filtered is null that means all indices so far is available, so no need to filter
}
}
return filtered;
}

/**
* Special handling of the value to authorize for a put mapping request. Dynamic put mapping
* requests use a concrete index, but we allow permissions to be defined on aliases so if the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ public void testResolveNoExpandStrict() {
}

public void testResolveNoExpandIgnoreUnavailable() {
SearchRequest request = new SearchRequest("missing*");
SearchRequest request = new SearchRequest(randomFrom("missing*", "*"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh .. I realised there is another edge case where we cannot safely skip filtering unavailable indices for a single *. If the cluster truly has no indices at all, * will be resolved into itself, i.e. a literal * and it should be removed from the resolved indices if ingoreUnavailable is requested (this test is updated to ensure that).

As a result, I decided to drop my original proposal for the special handling and just leave Tim's change for faster loop.

request.indicesOptions(IndicesOptions.fromOptions(true, true, false, false));
assertNoIndices(request, resolveIndices(request, buildAuthorizedIndices(user, SearchAction.NAME)));
}
Expand Down