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.

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

Filter original indices in shard level request #78508

Merged
merged 23 commits into from
Oct 14, 2021

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Sep 30, 2021

Today the search action send the full list of original indices on every shard request.
This change restricts the list to the concrete index of the shard or the alias that was used to resolve it.

Relates #78314

This change removes the transient OriginalIndices from the ShardSearchTarget class.
It is replaced by an an helper function in the SearchPhaseContext to access the
OriginalIndices per cluster alias.
@jimczi jimczi added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.16.0 labels Sep 30, 2021
@jimczi jimczi requested a review from ywelsch September 30, 2021 10:37
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 30, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Thank you, I like this much better!

@jimczi jimczi changed the title Centralize access to the original indices in the search action Filter original indices in shard level request Oct 1, 2021
@jimczi
Copy link
Contributor Author

jimczi commented Oct 1, 2021

I took another approach and reduced the original indices to the concrete index or aliases that is targeted. We don't need the full list on these per shard request so that should help to speed up authorization too. I updated the PR title and description accordingly, @ywelsch could you take another look ?

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Implementation looks good. There seem to be test failures though.

@@ -99,4 +102,68 @@ public void testResolveWildcardsRegexs() throws Exception {
assertThat((String) getResponse.getSource().get("field3"), equalTo("value3"));
}

public void testSearchResolveWildcardsRegexs() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This new test looks good to me, i.e. it can fail without the latest update for a similar scenario as described in previous discussion. Thanks!

@albertzaharovits albertzaharovits self-requested a review October 12, 2021 08:01
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

The change to the TransportSearchAction LGTM.

Though I believe the alias name will not be included in the ShardSearchRequest if the alias points to a datastream backing index (the alias points to the index, not to the datastream). Consequently DLS/FLS permissions defined on such aliases will not apply when the search uses these names.
But I'm not even sure we should allow aliases to backing indices of datastreams,
and even then this PR is big enough.

Thanks for the integ test.
I now realize we don't have similar tests for Security with Datastreams....

@jimczi
Copy link
Contributor Author

jimczi commented Oct 12, 2021

I now realize we don't have similar tests for Security with Datastreams....

I added IndicesPermissionsWithAliasesWildcardsAndRegexsTests#testSearchResolveDataStreams just now. I had to change the original indices to include the original index name if the data stream is explicitly requested. It is needed to handle the test case.

@jimczi
Copy link
Contributor Author

jimczi commented Oct 13, 2021

@elasticmachine run elasticsearch-ci/part-2

@jimczi jimczi merged commit f2ac26f into elastic:master Oct 14, 2021
@jimczi jimczi deleted the original_indices_cleanup branch October 14, 2021 07:00
jimczi added a commit that referenced this pull request Oct 14, 2021
Today the search action send the full list of original indices on every shard request.
This change restricts the list to the concrete index of the shard or the alias that was used to resolve it.

Relates #78314
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.16.0 v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants