elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.71k stars 24.67k forks source link

Return warning if _terms_enum results are empty due to DLS #88321

Open Dosant opened 2 years ago

Dosant commented 2 years ago

Description

_terms_enum returns empty results for indices with document level security https://github.com/elastic/elasticsearch/issues/83632

Kibana uses _terms_enum for autocomplete experience in multiple places and currently there is no indication for user if there is really no results of if they hit a DLS limitation. https://github.com/elastic/kibana/issues/132162

There is a workaround in Kibana that requires a config change: users can switch back to slower terms aggregation implementation. We would like to improve this experience, possibly by pointing users to the documentation when they hit empty results because of DLS or maybe even by filling back to term aggregation automatically.

For this, we would like elasticsearch to return a warning in case the user hits empty results due to DLS limitation.

We are also open to other suggestion on how to improve this in Kibana.

cc @lukasolson @sixstringcode

elasticmachine commented 2 years ago

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

javanna commented 2 years ago

Thanks for reporthing this @Dosant . Do you mean that you would like to see a warning returned as part of the http response headers? Or would you like to see the API return an error instead of the empty results?

Dosant commented 2 years ago

Do you mean that you would like to see a warning returned as part of the http response headers? Or would you like to see the API return an error instead of the empty results?

I thought of the warning as part of the http response, that would work for us. But if the team decides that here it actually makes more sense to return an error, then this would also work for us

javanna commented 1 year ago

We discussed this with the ES security team and we think that it would be a good idea to throw a clear error here, rather than returning a warning.

cbuescher commented 1 year ago

@Dosant I looked into this today and opened a PR with a suggested solution. We would still return an empty "terms" array but in addition flag the response as "incomplete" (that field should already exist) and also include the error details as a shard failure under the "_shards" section. This way the change should not be breaking existing usage, but you can use that additional information to make the right choices on the client side. Please take a look at the example response in the PR and let me know if that would work for you.

Dosant commented 1 year ago

@cbuescher, thanks for the info!

Please take a look at the example response in the PR and let me know if that would work for you.

@lukasolson, could you take a look please 🙏

lukasolson commented 1 year ago

Yes, I believe this should work for us @cbuescher, but to be sure can you provide a sample response with the warning?

cbuescher commented 1 year ago

@lukasolson there's one here in the PR

cbuescher commented 1 year ago

I'm reopening this issue since we are planning to revert #91720 for a few reasons, some of which are

Alternative approaches might be:

elasticsearchmachine commented 2 months ago

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