Fix indices shown in _cat/indices by tlrx · Pull Request #43286 · elastic/elasticsearch

tlrx · 2019-06-17T15:33:27Z

After two recent changes (#38824 and #33888), the _cat/indices API no longer report information for active recovering indices and non-replicated closed indices. It also misreport replicated closed indices that are potentially not authorized for the user.

This commit changes how the cat action works by first using the Get Settings API in order to resolve authorized indices. It then uses the Cluster State, Cluster Health and Indices Stats APIs to retrieve information about the indices.

Since the health and the stats of the indices are not guaranteed to exist (ex: recovering indices have a health status but no index stats; non-replicated closed indices have no health and no stats) they are shown only if they are present.

Closes #39933

elasticmachine · 2019-06-17T15:33:30Z

Pinging @elastic/es-core-features

tlrx · 2019-06-18T07:26:21Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

tlrx · 2019-06-18T08:47:11Z

Note that this API now requires an additional indices:admin/get privilege.

This is a breaking change but I don't come with a better solution to correctly resolved authorized indices before retrieving other information.

tlrx · 2019-06-19T07:29:46Z

@elasticmachine update branch

ywelsch · 2019-06-20T14:56:16Z

Can we use the cluster health request to resolve the authorized indices instead of using the newly added Get Index request?

ywelsch · 2019-06-20T14:24:20Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

+                sendIndicesStatsRequest(concreteIndices, indicesOptions, includeUnloadedSegments, client, step3);
+            }, listener::onFailure);
+
+            final StepListener<Map<String, IndexMetaData>> step4 = new StepListener<>();


could step 2, 3 and 4 run in parallel?

Yes, I used a GroupedActionListener for this.

tlrx · 2019-06-21T08:54:51Z

Can we use the cluster health request to resolve the authorized indices instead of using the newly added Get Index request?

The way the Cluster Health API works prevent us from using it to resolve the authorized indices: when using the Cat Indices API, the user requires the appropriate privilege to get the cluster state (cluster:monitor/state) and the cluster health (cluster:monitor/health). Those Cluster APIs provide a cluster-wide view and don't re-filter indices given index privileges (because the Cluster API itself requires a "higher" privilege), so a user that has the privilege to access the cluster state (or the cluster health) can access the state (or health) of all indices. Then the _cat/indices tries to be smart and filters out indices as the Indices Stats API would do but even if both APIs provides a similar output I don't think we should mix them.

I'm not a big fan of adding another privilege and call for this _cat/indices but I don't have a better idea for now. I'm waiting for some ideas @albertzaharovits :)

albertzaharovits · 2019-06-21T12:15:40Z

Hi @tlrx ,
I think we can use the get index settings API (TransportGetSettingsAction) to get the set of authorized indices, because, as you said, cluster health and cluster state are "cluster-wide" and are not confined by index permissions and that get index stats is also not suitable, because we wish to display indices that are not returned by this API (recovering indices and non-replicated closed indices). If we learn of some indices that are not returned by the get index settings API, then we can simply list all of them irrespective of index privileges. My reasoning is that if the user has privileges to run this API (_cat/indices) at all, it can also call the cluster state on its own and "see" the indices there, so the hiding we're fighting with here is just a "nice to have".

This reverts commit 3c28876

This reverts commit 99e8170

tlrx · 2019-06-24T12:56:59Z

@elasticmachine run elasticsearch-ci/1

tlrx · 2019-06-24T15:22:39Z

@albertzaharovits Thanks for your suggestion, it seems to work nicely :). Can you have a look please?

albertzaharovits

I believe we should keep the original behavior with respect to wildcards that are expanded with the same options by all APIs. This prevents errors when the wildcard expands to indices that are subsequently removed before the other calls. This is my only objection and in case I'm not mistaken can be rectified easily.

I'll LGTM for now.
Thank you Tanguy for tackling fallouts of my changes!

albertzaharovits · 2019-06-24T18:01:49Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

+
+                    sendIndicesStatsRequest(concreteIndices, indicesOptions, includeUnloadedSegments, client, groupedListener);
+                    sendClusterStateRequest(concreteIndices, indicesOptions, local, masterNodeTimeout, client, groupedListener);
+                    sendClusterHealthRequest(concreteIndices, indicesOptions, local, masterNodeTimeout, client, groupedListener);


Why do we use the concreteIndices rather than indices with indicesOptions. There is a comment in the original code:

... // This behavior can be ensured by letting the cluster health and indices // stats requests re-resolve the index names with the same indices options // that we used for the initial cluster state request (strictExpand).

that I believe still applies (if wildcards expand to indices that are subsequently deleted, we should not error).

Yes, this is not good. I restored the original comment and behavior in 4eb76f5

albertzaharovits · 2019-06-24T18:02:11Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

+            public void onFailure(final Exception e) {
+                listener.onFailure(e);
+            }
+        });


this can be simplified.

albertzaharovits · 2019-06-24T18:02:51Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

+
+            @Override
+            public void onFailure(final Exception e) {
+                listener.onFailure(e);


this can be simplified to client.admin().cluster().health(request, listener)
there are 3 occurrences of this.

There's still a need to downcast - I pushed ff35567

ywelsch

LGTM

ywelsch · 2019-06-25T09:59:21Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestIndicesAction.java

+        };
+    }
+
+    private void sendGetSettingsRequest(final String[] indices,


can you add a comment why we're using getSettings request here? i.e. that it is used to resolve the indices.

I pushed d93c087

tlrx · 2019-06-25T11:20:50Z

@elasticmachine run elasticsearch-ci/1

After two recent changes (elastic#38824 and elastic#33888), the _cat/indices API no longer report information for active recovering indices and non-replicated closed indices. It also misreport replicated closed indices that are potentially not authorized for the user. This commit changes how the cat action works by first using the Get Settings API in order to resolve authorized indices. It then uses the Cluster State, Cluster Health and Indices Stats APIs to retrieve information about the indices. Closes elastic#39933

After two recent changes (#38824 and #33888), the _cat/indices API no longer report information for active recovering indices and non-replicated closed indices. It also misreport replicated closed indices that are potentially not authorized for the user. This commit changes how the cat action works by first using the Get Settings API in order to resolve authorized indices. It then uses the Cluster State, Cluster Health and Indices Stats APIs to retrieve information about the indices. Closes #39933

After two recent changes (elastic#38824 and elastic#33888), the _cat/indices API no longer report information for active recovering indices and non-replicated closed indices. It also misreport replicated closed indices that are potentially not authorized for the user. This commit changes how the cat action works by first using the Get Settings API in order to resolve authorized indices. It then uses the Cluster State, Cluster Health and Indices Stats APIs to retrieve information about the indices. Closes elastic#39933

After two recent changes (#38824 and #33888), the _cat/indices API no longer report information for active recovering indices and non-replicated closed indices. It also misreport replicated closed indices that are potentially not authorized for the user. This commit changes how the cat action works by first using the Get Settings API in order to resolve authorized indices. It then uses the Cluster State, Cluster Health and Indices Stats APIs to retrieve information about the indices. Closes #39933

Fix indices shown in _cat/indices

5b4ea1b

tlrx added >bug :Core/Infra/CAT APIs Text APIs behind /_cat v8.0.0 v7.3.0 labels Jun 17, 2019

tlrx requested review from albertzaharovits and ywelsch June 17, 2019 15:33

tlrx added 2 commits June 18, 2019 10:47

Adapt privileges in MultipleIndicesPermissionsTests

3c28876

Adapt privileges in EnableSecurityOnBasicLicenseIT

99e8170

Merge branch 'master' into cat-indices

0a7ae0f

ywelsch reviewed Jun 20, 2019

View reviewed changes

tlrx added 5 commits June 24, 2019 09:52

Revert "Adapt privileges in MultipleIndicesPermissionsTests"

db151cf

This reverts commit 3c28876

Revert "Adapt privileges in EnableSecurityOnBasicLicenseIT"

b02a79c

This reverts commit 99e8170

Use Get Settings action instead of Get Indices

9360793

Merge branch 'master' into cat-indices

001b08b

Restore randomIndicesStatsResponse

c27246b

tlrx requested a review from ywelsch June 24, 2019 15:22

albertzaharovits approved these changes Jun 24, 2019

View reviewed changes

tlrx added 2 commits June 25, 2019 09:51

Re-resolve indices

4eb76f5

Wrap listeners

ff35567

tlrx requested a review from albertzaharovits June 25, 2019 08:21

ywelsch approved these changes Jun 25, 2019

View reviewed changes

add comment

d93c087

tlrx merged commit ba07eb4 into elastic:master Jun 25, 2019

tlrx deleted the cat-indices branch June 25, 2019 14:42

tlrx added the backport pending label Jun 25, 2019

tlrx added v7.2.1 and removed backport pending labels Jun 26, 2019

codebrain mentioned this pull request Aug 5, 2019

[meta] 7.2 Release elastic/elasticsearch-net#3980

Closed

37 tasks

benwtrent mentioned this pull request Aug 29, 2019

[CI] MlJobIT.testDeleteJob and testDeleteJobAsync fail with index_not_found_exception #45652

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

tlrx commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 17, 2019

Uh oh!

tlrx commented Jun 18, 2019

Uh oh!

tlrx commented Jun 18, 2019

Uh oh!

tlrx commented Jun 19, 2019

Uh oh!

ywelsch commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 21, 2019

Uh oh!

albertzaharovits commented Jun 21, 2019

Uh oh!

tlrx commented Jun 24, 2019

Uh oh!

tlrx commented Jun 24, 2019

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tlrx commented Jun 17, 2019 •

edited

Loading

tlrx Jun 25, 2019 •

edited

Loading