[RAC] [RBAC] working find route for alerts as data client #107982

dhurley14 · 2021-08-09T22:53:20Z

Summary

Addition of a find api to the alerts client to authorize requests using RBAC, updates alerts histograms to use new API on alerts page, updates new alerts aggs data table on alerts page, and updates alerts histogram on overview page.

To test (no need for any rule registry env vars to be turned on)

Just turn on auditbeat etc..
Write a rule to query *:*
View data

Pages without data...

Overview page - no data

Alerts page - no data

Pages with data...

Overview page with data

Alerts page with data

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ts aggs, copied from what saved objects aggs types are allowed

…sts need to be worked through more

…d api when rule registry feature flag is enabled

…d_api

…lerts aggs table, adds integration tests

…d_api

…ty hits when querying alert index the user is not authorized to query, so I added an extra check to determine if the user is querying the appropriate index and if they are authorized to even execute queries against the provided index

elasticmachine · 2021-08-15T02:58:28Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

machadoum · 2021-08-16T12:20:25Z

Alerts trends graph and count table are working perfectly with the new API! Thank you for updating the API call. 👏

madirey · 2021-08-17T18:44:19Z

Somehow I got into a state where the alerts table refreshed, but the histogram didn't... is that expected?

madirey · 2021-08-17T19:33:23Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+    aggs,
+    _source,
+    // eslint-disable-next-line @typescript-eslint/naming-convention
+    track_total_hits,


Nit: Probably should prefer track_total_hits: trackTotalHits here, instead of the eslint exception?

…elds, uses recursive validation with io-ts at the find route level, adds tests for when nested aggs are present and tests for when nested aggs have scripts field

…r with track total hits, make extra params optional so we are not adding undefined everywhere in the code

banderror

I took a shallow high-level look and posted a few silly questions.
Sorry about that, my head is not working anymore.
I will approve the PR, and hope to be able to take a deeper look tomorrow.
If anyone could do a proper in-depth review, it would be great.

banderror · 2021-08-17T23:32:42Z

x-pack/plugins/rule_registry/common/types.ts

+export type PutIndexTemplateRequest = estypes.IndicesPutIndexTemplateRequest & {
+  body?: { composed_of?: string[] };
+};


Can be deleted, estypes.IndicesPutIndexTemplateRequest contains composed_of:

export interface IndicesPutIndexTemplateRequest extends RequestBase { name: Name body?: { index_patterns?: Indices composed_of?: Name[] template?: IndicesPutIndexTemplateIndexTemplateMapping data_stream?: EmptyObject priority?: integer version?: VersionNumber _meta?: Metadata } }

ah good catch. I copied some of these types from elsewhere in kibana so didn't look too deeply at the types but I will update this. Thanks!

banderror · 2021-08-17T23:43:21Z

x-pack/plugins/rule_registry/server/routes/find.ts

+        if (alerts == null) {
+          return response.notFound({
+            body: { message: `alerts with query and index ${index} not found` },
+          });
+        }


What's the use case for 404, could it be 200 with an empty array if nothing is found? Less cases for handling on the client side imho.

If the alerts have an empty hits array we will respond with that (200 + the Elasticsearch response), but if for some reason the response from Elasticsearch is null (either because of an error or something else) then I figured a 404 is better than throwing a 500.

But yes in general when we execute a search and no hits are found (but we receive a 200 from Elasticsearch) we will respond with exactly that, an empty array.

banderror · 2021-08-17T23:47:33Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+  public async find<Params extends AlertTypeParams = never>({
+    query,
+    aggs,
+    _source,
+    // eslint-disable-next-line @typescript-eslint/naming-convention
+    track_total_hits,
+    size,
+    index,
+  }: {
+    query?: object | undefined;
+    aggs?: object | undefined;
+    index: string | undefined;
+    track_total_hits?: boolean | undefined;
+    _source?: string[] | undefined;
+    size?: number | undefined;
+  }) {


How does this find() method compare to RuleDataClient.getReader().search(), when should we use which one, etc? Do we even need RuleDataClient.getReader() in the current form?

…re aggs.terms.missing field could be a string or a number

XavierM

Great job, thank you for adding the test around the aggs and script. Now I feel it is golden!!!

kibanamachine · 2021-08-18T02:53:58Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	6.5MB	6.5MB	+648.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	211.8KB	211.9KB	+145.0B

History

💚 Build #146442 succeeded 5de23b5
💚 Build #146411 succeeded a8188db
💚 Build #146382 succeeded 862b5ad
💚 Build #146208 succeeded 628cd86
💚 Build #145506 succeeded 4d89998

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dhurley14

…7982) Addition of a find api to the alerts client to authorize requests using RBAC, updates alerts histograms to use new API on alerts page, updates new alerts aggs data table on alerts page, and updates alerts histogram on overview page.

kibanamachine · 2021-08-18T02:57:26Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…109034) Addition of a find api to the alerts client to authorize requests using RBAC, updates alerts histograms to use new API on alerts page, updates new alerts aggs data table on alerts page, and updates alerts histogram on overview page. Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>

dhurley14 added 20 commits August 9, 2021 18:52

working find route

3c18dba

adds types find to prevent usage of aggs except for metrics and bucke…

ba914ec

…ts aggs, copied from what saved objects aggs types are allowed

Merge branch 'master' into alerts_client_find_api

7808549

adds jest tests, partially working integration tests, unauthorized te…

a435c70

…sts need to be worked through more

fixes integration tests for find api

d6fa4e2

updates alerts count panel UI to use rule registry alerts as data fin…

68d0f41

…d api when rule registry feature flag is enabled

Merge remote-tracking branch 'upstream/master' into alerts_client_fin…

50200d1

…d_api

fix some newly renamed things after merge with master

6d826b3

Merge remote-tracking branch 'upstream/master' into alerts_client_fin…

ff5226e

…d_api

do not 404 when there are empty hits, just return empty hits, fixes a…

de44247

…lerts aggs table, adds integration tests

Merge remote-tracking branch 'upstream/master' into alerts_client_fin…

e69493d

…d_api

update histograms on overview page

21f27f6

little cleanup

e00a071

remove logs and undo unnecessary changes in timeline

85ea8ed

fix describe in the integration test

a442929

fix merge conflicts after merge with master

9596697

fix other failing integration tests

e08c2cd

remove commented out code

42f5a94

updates alerts client docs

4d89998

dhurley14 self-assigned this Aug 15, 2021

dhurley14 marked this pull request as ready for review August 15, 2021 02:58

dhurley14 requested a review from a team as a code owner August 15, 2021 02:58

dhurley14 added release_note:feature Makes this part of the condensed release notes release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels Aug 15, 2021

peluja1012 mentioned this pull request Aug 16, 2021

[RAC] [Alerts] [RBAC] #95721

Closed

13 tasks

merge with master

628cd86

madirey reviewed Aug 17, 2021

View reviewed changes

allow nested aggs but disallow scripts fields and other extraneous fi…

862b5ad

…elds, uses recursive validation with io-ts at the find route level, adds tests for when nested aggs are present and tests for when nested aggs have scripts field

madirey approved these changes Aug 17, 2021

View reviewed changes

fixes bug where date_histogram was disallowed, removes ts-expect-erro…

a8188db

…r with track total hits, make extra params optional so we are not adding undefined everywhere in the code

banderror approved these changes Aug 17, 2021

View reviewed changes

removes debug log, removes unnecessary ts expect error, fixes bug whe…

5de23b5

…re aggs.terms.missing field could be a string or a number

XavierM approved these changes Aug 18, 2021

View reviewed changes

adds integration test for histogram query on rule details page

4602b35

dhurley14 enabled auto-merge (squash) August 18, 2021 00:27

dhurley14 merged commit c3ccda9 into elastic:master Aug 18, 2021

kibanamachine mentioned this pull request Aug 18, 2021

[7.x] [RAC] [RBAC] working find route for alerts as data client (#107982) #109034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAC] [RBAC] working find route for alerts as data client #107982

[RAC] [RBAC] working find route for alerts as data client #107982

dhurley14 commented Aug 9, 2021 •

edited

Loading

elasticmachine commented Aug 15, 2021

machadoum commented Aug 16, 2021

madirey commented Aug 17, 2021

madirey Aug 17, 2021

banderror left a comment

banderror Aug 17, 2021

dhurley14 Aug 18, 2021

banderror Aug 17, 2021

dhurley14 Aug 18, 2021

dhurley14 Aug 18, 2021

banderror Aug 17, 2021 •

edited

Loading

XavierM left a comment

kibanamachine commented Aug 18, 2021

kibanamachine commented Aug 18, 2021

[RAC] [RBAC] working find route for alerts as data client #107982

[RAC] [RBAC] working find route for alerts as data client #107982

Conversation

dhurley14 commented Aug 9, 2021 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 15, 2021

machadoum commented Aug 16, 2021

madirey commented Aug 17, 2021

madirey Aug 17, 2021

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

banderror Aug 17, 2021

Choose a reason for hiding this comment

dhurley14 Aug 18, 2021

Choose a reason for hiding this comment

banderror Aug 17, 2021

Choose a reason for hiding this comment

dhurley14 Aug 18, 2021

Choose a reason for hiding this comment

dhurley14 Aug 18, 2021

Choose a reason for hiding this comment

banderror Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

XavierM left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 18, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kibanamachine commented Aug 18, 2021

💚 Backport successful

dhurley14 commented Aug 9, 2021 •

edited

Loading

banderror Aug 17, 2021 •

edited

Loading