Skip to content

resolve indices for prefixed _all expressions #137330

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

Merged
merged 11 commits into from
Nov 6, 2025
Merged

resolve indices for prefixed _all expressions #137330

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9c7e04b
resolve indices for prefixed _all expressions
richard-dennehy Oct 29, 2025
2ecdc40
add test for origin prefix _all expression
richard-dennehy Oct 29, 2025
b19ec68
[CI] Auto commit changes from spotless
Oct 29, 2025
84ea19d
use splitIndexName instead of rewriteIndexExpression
richard-dennehy Oct 30, 2025
8bbfd8f
Merge branch 'main' into remote-project-prefixed-all-expressions
richard-dennehy Oct 30, 2025
aaea6cf
use rewriteIndexExpression again
richard-dennehy Oct 31, 2025
7812375
Merge remote-tracking branch 'upstream/main' into remote-project-pref…
richard-dennehy Nov 5, 2025
ef0417d
extend shouldExcludeLocalResolution check
richard-dennehy Nov 5, 2025
13ceafc
rename
richard-dennehy Nov 5, 2025
d3470c9
Merge branch 'main' into remote-project-prefixed-all-expressions
richard-dennehy Nov 6, 2025
9730e5e
review comment
richard-dennehy Nov 6, 2025
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
17 changes: 13 additions & 4 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 @@ -335,10 +335,19 @@ ResolvedIndices resolveIndicesAndAliases(
String allIndicesPatternSelector = null;
if (indicesRequest.indices() != null && indicesRequest.indices().length > 0) {
// Always parse selectors, but do so lazily so that we don't spend a lot of time splitting strings each resolution
isAllIndices = IndexNameExpressionResolver.isAllIndices(
indicesList(indicesRequest.indices()),
(expr) -> IndexNameExpressionResolver.splitSelectorExpression(expr).v1()
);
isAllIndices = crossProjectModeDecider.resolvesCrossProject(replaceable)
? IndexNameExpressionResolver.isAllIndices(
indicesList(indicesRequest.indices()),
(expr) -> CrossProjectIndexExpressionsRewriter.rewriteIndexExpression(
expr,
authorizedProjects.originProjectAlias(),
authorizedProjects.allProjectAliases()
).localExpression()
)
: IndexNameExpressionResolver.isAllIndices(
indicesList(indicesRequest.indices()),
(expr) -> IndexNameExpressionResolver.splitSelectorExpression(expr).v1()
);
Copy link
Member

@ywangd ywangd Oct 30, 2025

Choose a reason for hiding this comment

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

This matches what I was thinking, i.e. let cases like *:_all go through the all indices handling. I suggest we use RemoteClusterAware#splitIndexName instead of rewriteIndexExpression. It is lighter and we don't care about resolving projects at this point. It also needs to handle syntax like selector. I'd prefer we don't error out at isAllIndices check. The unsupported syntax will still be caught later when we call rewriteIndexExpression. So overall I am suggeting something like

IndexNameExpressionResolver.isAllIndices(
    indicesList(indicesRequest.indices()), 
    expr -> {
        IndexNameExpressionResolver.splitSelectorExpression(RemoteClusterAware.splitIndexName(expr)[1]).v1()
    }
)

Copy link
Contributor Author

@richard-dennehy richard-dennehy Oct 31, 2025

Choose a reason for hiding this comment

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

It appears this isn't enough, as this means any expression targeting _all will resolve all indices on the local project, even if that expression is e.g. some_remote_project:_all.

We need to check if the expression is _all, and that the prefix includes the local project. It looks to me like rewriteIndexExpression handles all the edge cases here, even if it does more than what's necessary here.

Copy link
Member

@ywangd ywangd Nov 5, 2025

Choose a reason for hiding this comment

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

Yes we will resolve local indices unnecessarily in this case. But I think local indices should still be excluded because of this code. I was going to leave the unnecessary expansion for a future optimization.

Even if we use rewriteIndexExpression here, it is not sufficient due to project_routing, e.g. we will still expand local indices unnecessarily when the request has expression _all and project routing some_remote_project. If you are keen to avoid local expansion, I'd rather prefer we solve this more comprehensively to cover this TODO as well.

What I have in mind is something like the follows:

  1. Still use the splitIndexName and splitSelectorExpression for checking isAllIndices which IMO should be interpreted as "is it an all indices pattern for any project".
  2. Before expanding local indices, check crossProjectModeDecider.resolvesCrossProject and only expand locally if the final resolved projects include the origin project.

Copy link
Member

@ywangd ywangd Nov 5, 2025

Choose a reason for hiding this comment

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

But I think local indices should still be excluded because of this code.

It appears that this logic only handles project routing filtering but not the qualified projects in index expression. So it does not work for linked_project:_all.

I am still a little suspicious on the latest change. IIUC, it means an expression like linked_project:_all goes through the else branch of the isAllIndices check? While expressions like *:_all or _all with project routing linked_project still go through the if branch. The later is functionally equivalent to linked_project:_all and it would great if they follow the same path. Maybe we just need to resolve the project routing before invoking rewriteIndexExpression? If we take this path, I think we should also rename the isAllIndices variable to something like containsLocalAllIndices?

Copy link
Contributor Author

@richard-dennehy richard-dennehy Nov 5, 2025

Choose a reason for hiding this comment

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

I've switched back to using splitIndexName again, and extended the shouldExcludeLocalResolution check to check if localExpression is set, and this seems to work (including the REST test). Does this align with what you were thinking?

I could also take a look at moving the condition earlier to see if we can skip the redundant local expansion as well.

Copy link
Member

@ywangd ywangd Nov 6, 2025

Choose a reason for hiding this comment

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

That works for me as well. We can have separate PR to address the TODO for skipping redundant local expansion.

if (isAllIndices) {
// This parses the single all-indices expression for a second time in this conditional branch, but this is better than
// parsing a potentially big list of indices on every request.
Expand Down
85 changes: 85 additions & 0 deletions .../src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2956,6 +2956,91 @@ public void testCrossProjectSearchSelectorsNotAllowed() {
assertThat(exception.getMessage(), equalTo("Selectors are not currently supported but was found in the expression [_all::data]"));
}

public void testResolveAllWithWildcardRemotePrefix() {
when(crossProjectModeDecider.resolvesCrossProject(any(IndicesRequest.Replaceable.class))).thenReturn(true);

var request = new SearchRequest().indices("*:_all");
request.indicesOptions(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), true, true));
var resolvedIndices = defaultIndicesResolver.resolveIndicesAndAliases(
"indices:/" + randomAlphaOfLength(8),
request,
projectMetadata,
buildAuthorizedIndices(user, TransportSearchAction.TYPE.name()),
new TargetProjects(
createRandomProjectWithAlias("local"),
List.of(createRandomProjectWithAlias("P1"), createRandomProjectWithAlias("P2"), createRandomProjectWithAlias("P3"))
)
);

var expectedIndices = new String[] { "bar", "foobarfoo", "bar-closed", "foofoobar", "foofoo-closed", "foofoo" };

assertThat(resolvedIndices.getLocal(), contains(expectedIndices));
assertThat(resolvedIndices.getRemote(), containsInAnyOrder("P1:_all", "P2:_all", "P3:_all"));

final var resolved = request.getResolvedIndexExpressions();
assertThat(resolved, is(notNullValue()));
assertThat(
resolved.expressions(),
contains(resolvedIndexExpression("*:_all", Set.of(expectedIndices), SUCCESS, Set.of("P1:_all", "P2:_all", "P3:_all")))
);
}

public void testResolveAllWithRemotePrefix() {
when(crossProjectModeDecider.resolvesCrossProject(any(IndicesRequest.Replaceable.class))).thenReturn(true);

var expression = randomBoolean() ? "local:_all" : "_origin:_all";
var request = new SearchRequest().indices(expression);
request.indicesOptions(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), true, true));
var resolvedIndices = defaultIndicesResolver.resolveIndicesAndAliases(
"indices:/" + randomAlphaOfLength(8),
request,
projectMetadata,
buildAuthorizedIndices(user, TransportSearchAction.TYPE.name()),
new TargetProjects(
createRandomProjectWithAlias("local"),
List.of(createRandomProjectWithAlias("P1"), createRandomProjectWithAlias("P2"), createRandomProjectWithAlias("P3"))
)
);

var expectedIndices = new String[] { "bar", "foobarfoo", "bar-closed", "foofoobar", "foofoo-closed", "foofoo" };

assertThat(resolvedIndices.getLocal(), contains(expectedIndices));
assertThat(resolvedIndices.getRemote(), is(empty()));

final var resolved = request.getResolvedIndexExpressions();
assertThat(resolved, is(notNullValue()));
assertThat(resolved.expressions(), contains(resolvedIndexExpression(expression, Set.of(expectedIndices), SUCCESS, Set.of())));
}

public void testResolveIndexWithRemotePrefix() {
Copy link
Contributor Author

@richard-dennehy richard-dennehy Oct 29, 2025

Choose a reason for hiding this comment

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

I noticed that I could just set all cross project requests to go down the "all indices" path and all the tests would pass, even though this is obviously wrong; I've added this test which fails under that scenario

Copy link
Member

@ywangd ywangd Oct 30, 2025

Choose a reason for hiding this comment

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

all the tests would pass

Do you mean all tests in this test class? I am surprised unless all existing tests are effectively resolving to all accessible indices. Is that the case? Also, I assume tests in other places, e.g. the serverless REST test, should fail?

Copy link
Contributor Author

@richard-dennehy richard-dennehy Oct 30, 2025

Choose a reason for hiding this comment

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

I think it's because the majority of the other tests in this file have CPS mode disabled; I'll see if the REST test guards against this, but I don't want it to be the only test that would fail - it makes the feedback loop too long

Copy link
Contributor Author

@richard-dennehy richard-dennehy Oct 30, 2025

Choose a reason for hiding this comment

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

Confirmed that the REST test fails when this functionality is implemented incorrectly

when(crossProjectModeDecider.resolvesCrossProject(any(IndicesRequest.Replaceable.class))).thenReturn(true);

var request = new SearchRequest().indices("*:bar");
request.indicesOptions(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), true, true));
var resolvedIndices = defaultIndicesResolver.resolveIndicesAndAliases(
"indices:/" + randomAlphaOfLength(8),
request,
projectMetadata,
buildAuthorizedIndices(user, TransportSearchAction.TYPE.name()),
new TargetProjects(
createRandomProjectWithAlias("local"),
List.of(createRandomProjectWithAlias("P1"), createRandomProjectWithAlias("P2"), createRandomProjectWithAlias("P3"))
)
);

var expectedIndices = new String[] { "bar" };

assertThat(resolvedIndices.getLocal(), contains(expectedIndices));
assertThat(resolvedIndices.getRemote(), containsInAnyOrder("P1:bar", "P2:bar", "P3:bar"));

final var resolved = request.getResolvedIndexExpressions();
assertThat(resolved, is(notNullValue()));
assertThat(
resolved.expressions(),
contains(resolvedIndexExpression("*:bar", Set.of(expectedIndices), SUCCESS, Set.of("P1:bar", "P2:bar", "P3:bar")))
);
}

private void assertIndicesMatch(IndicesRequest.Replaceable request, String expression, List<String> indices, String[] expectedIndices) {
assertThat(indices, hasSize(expectedIndices.length));
assertThat(request.indices().length, equalTo(expectedIndices.length));
Expand Down