[RAC] Get o11y alerts in alerts table by XavierM · Pull Request #109346 · elastic/kibana

XavierM · 2021-08-19T21:14:49Z

Summary

@dhurley14 and I pair to bring back o11y alerts in the table. Now, we are passing directly the indexes name in our alerts search strategy and count on the filter of the authorization to show the right alerts. The index names are saved in memory inside of the ruleServiceData.

elasticmachine · 2021-08-19T21:14:51Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

weltenwort

Thanks for jumping on this so quickly!

The fact that we're using the ruleDataClient here leads to a few problems (that existed before, but are solidified through these changes):

The field list is read using the internal user's permissions and not the current user's permissions.
The addition of the indexNames argument to getReader breaks the abstraction, because the whole point of the RuleDataClient is to delegate the index name generation to the RuleDataService.

How about the following:

The dynamic_index_pattern route turns a list of registrationContexts and a namespace into an index name pattern (e.g. ['.alerts-observability.logs.alerts-default', '.alerts-observability.metrics.alerts-default']). On the alerts page we then use Kibana's IndexPatternService to request getFieldsForWildcard to get the field list for use as a dynamicIndexPattern.

Does that make sense?

x-pack/plugins/observability/server/routes/rules.ts

weltenwort · 2021-08-20T10:27:08Z

x-pack/plugins/rule_registry/server/rule_data_client/rule_data_client.ts

  }

-  public getReader(options: { namespace?: string } = {}): IRuleDataReader {
+  public getReader(options: { namespace?: string; indexNames?: string[] } = {}): IRuleDataReader {


Doesn't this break the abstraction layer the RuleDataClient represents?

weltenwort · 2021-08-20T10:28:31Z

x-pack/plugins/rule_registry/server/rule_data_plugin_service/rule_data_plugin_service.ts

  }
+
+  /**
+   * Initializes alerts-as-data index and starts index bootstrapping right away.


How does this comment correlate to the actual function?

weltenwort · 2021-08-20T12:09:03Z

@XavierM I took the liberty of removing the RuleDataClient usage. Let me know what you think.

dhurley14 · 2021-08-20T13:09:08Z

x-pack/plugins/observability/public/pages/alerts/index.tsx

      endpoint: 'GET /api/observability/rules/alerts/dynamic_index_pattern',
      params: {
        query: {
+          namespace: 'default',


@weltenwort Is there a way to get the space id from the UI? This may not always be default, no?

We were pulling it off of the alerting framework rulesClient in the rules/alerts/dynamic_index_pattern route so we could ensure we acquired the correct space id on the request.. If observability doesn't plan on segmenting by space id I could understand why this would be hard coded to default then. Just looking for clarification here..

This is intentionally fixed as default, because in observability we're not using the space id in the namespace. The filtering by space happens as part of the RBAC filter if I saw that correctly.

…s-in-o11y

banderror

Looks good to me overall, I just left a few comments. Thank you @XavierM @dhurley14 @weltenwort

Also, mapConsumerToIndexName is still used by AlertsClient (RBAC), right?

banderror · 2021-08-20T14:13:52Z

x-pack/plugins/rule_registry/server/rule_data_plugin_service/rule_data_plugin_service.ts

      indexOptions,
    });

+    this.registeredIndices.set(indexOptions.registrationContext, indexInfo);


The full index name (indexInfo.baseName) is defined not only by registrationContext, but also by dataset (alerts or events).

I guess the proper key for this map would be ${indexOptions.registrationContext}.${indexOptions.dataset}, and getRegisteredIndexInfo would need to accept dataset as well.

True, this only works because we assume alerts as a dataset implicitly. Would it be ok to add that as a follow-up to unblock this ASAP?

Yep, of course 👍
TODO in the follow-up PR: let getRegisteredIndexInfo to accept dataset as well, generate keys of the registeredIndices map correctly.

x-pack/plugins/observability/server/routes/rules.ts

x-pack/plugins/observability/server/plugin.ts

banderror · 2021-08-20T14:24:50Z

x-pack/plugins/observability/public/pages/alerts/index.tsx

+          registrationContexts: [
+            'observability.apm',
+            'observability.logs',
+            'observability.infrastructure',


observability.infrastructure registration context doesn't exist in the code. What plugin is supposed to be defining it?

I think this should be observability.metrics. The app is called infrastructure for historical reasons, but not the registration context.

TODO in the follow-up PR: remove observability.infrastructure and make sure observability.metrics is there.

XavierM

@weltenwort, I agree with what you did and to be honest I was not really familiar with all the work and discussion around ruleDataClient or ruleDataService. So I am glad you fix it and make it good for everybody.

banderror

Approving to unblock and merge this PR ASAP.

I'll try to address these things in a follow-up PR:

weltenwort

LGTM, thanks everyone for the collaboration.

Known issues are the wrong observability.infrastructure registration context and the lack of dataset in the registered index info map.

michaelolo24

Looks good! LGTM!!

kibanamachine · 2021-08-20T16:52:31Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	256	259	+3

Async chunks

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

id	before	after	diff
`cases`	582.8KB	582.6KB	-204.0B
`observability`	489.6KB	492.1KB	+2.6KB
`securitySolution`	6.5MB	6.5MB	-702.0B
`timelines`	437.2KB	437.2KB	-60.0B
total			+1.6KB

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	134	136	+2
`timelines`	961	960	-1
total			+1

API count missing comments

id	before	after	diff
`ruleRegistry`	113	114	+1
`timelines`	841	840	-1
total			-0

Non-exported public API item count

id	before	after	diff
`ruleRegistry`	6	7	+1

References to deprecated APIs

id	before	after	diff
`observability`	41	37	-4

History

💚 Build #147319 succeeded c494512
💚 Build #147274 succeeded 3b18c32
💚 Build #147175 succeeded dcb6c9d
💔 Build #147153 failed 80bc75c
💔 Build #147130 failed bc3527a

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

* get back index names in o11y * testing and integration * fix types * Avoid using the rule data client for field list * Remove left-over index argument * no needs of alert consumer anymore Co-authored-by: Felix Stürmer <stuermer@weltenwort.de>

kibanamachine · 2021-08-20T16:55:56Z

💔 Backport failed

Status	Branch	Result
✅	7.15
❌	7.x	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 109346

weltenwort · 2021-08-20T17:23:34Z

The failed backport is probably due a chain of other backports failing. I'll keep track of those any retry later today:

* get back index names in o11y * testing and integration * fix types * Avoid using the rule data client for field list * Remove left-over index argument * no needs of alert consumer anymore Co-authored-by: Felix Stürmer <stuermer@weltenwort.de> Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Felix Stürmer <stuermer@weltenwort.de>

* get back index names in o11y * testing and integration * fix types * Avoid using the rule data client for field list * Remove left-over index argument * no needs of alert consumer anymore Co-authored-by: Felix Stürmer <stuermer@weltenwort.de> # Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx # x-pack/plugins/observability/public/pages/alerts/index.tsx

* get back index names in o11y * testing and integration * fix types * Avoid using the rule data client for field list * Remove left-over index argument * no needs of alert consumer anymore Co-authored-by: Felix Stürmer <stuermer@weltenwort.de> # Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx # x-pack/plugins/observability/public/pages/alerts/index.tsx Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com>

…atures to index names (#109567) **Ticket:** #102089 🚨 **This PR is critical for Observability 7.15** 🚨 ## Summary This PR introduces changes that fix the usage of alerts-as-data index naming in RBAC. It builds on top of #109346 and replaces #108872. TODO: - [x] Address #109346 (review) - [x] Make changes to `AlertsClient.getAuthorizedAlertsIndices()` so it starts using `RuleDataService` to get index names by feature ids. - [x] Delete the hardcoded `mapConsumerToIndexName` where we had incorrect index names. - [x] Close #108872 ### Checklist Delete any items that are not applicable to this PR. - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…atures to index names (elastic#109567) **Ticket:** elastic#102089 🚨 **This PR is critical for Observability 7.15** 🚨 ## Summary This PR introduces changes that fix the usage of alerts-as-data index naming in RBAC. It builds on top of elastic#109346 and replaces elastic#108872. TODO: - [x] Address elastic#109346 (review) - [x] Make changes to `AlertsClient.getAuthorizedAlertsIndices()` so it starts using `RuleDataService` to get index names by feature ids. - [x] Delete the hardcoded `mapConsumerToIndexName` where we had incorrect index names. - [x] Close elastic#108872 ### Checklist Delete any items that are not applicable to this PR. - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…atures to index names (#109567) (#110068) **Ticket:** #102089 🚨 **This PR is critical for Observability 7.15** 🚨 ## Summary This PR introduces changes that fix the usage of alerts-as-data index naming in RBAC. It builds on top of #109346 and replaces #108872. TODO: - [x] Address #109346 (review) - [x] Make changes to `AlertsClient.getAuthorizedAlertsIndices()` so it starts using `RuleDataService` to get index names by feature ids. - [x] Delete the hardcoded `mapConsumerToIndexName` where we had incorrect index names. - [x] Close #108872 ### Checklist Delete any items that are not applicable to this PR. - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>

…atures to index names (#109567) (#110067) **Ticket:** #102089 🚨 **This PR is critical for Observability 7.15** 🚨 ## Summary This PR introduces changes that fix the usage of alerts-as-data index naming in RBAC. It builds on top of #109346 and replaces #108872. TODO: - [x] Address #109346 (review) - [x] Make changes to `AlertsClient.getAuthorizedAlertsIndices()` so it starts using `RuleDataService` to get index names by feature ids. - [x] Delete the hardcoded `mapConsumerToIndexName` where we had incorrect index names. - [x] Close #108872 ### Checklist Delete any items that are not applicable to this PR. - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>

get back index names in o11y

bc3527a

XavierM requested review from dhurley14 and weltenwort August 19, 2021 21:14

XavierM requested a review from a team as a code owner August 19, 2021 21:14

XavierM added the release_note:skip Skip the PR/issue when compiling release notes label Aug 19, 2021

XavierM added 3 commits August 19, 2021 18:25

testing and integration

80bc75c

fix types

f9c3928

Merge branch 'master' of github.com:elastic/kibana into alerts-in-o11y

dcb6c9d

weltenwort reviewed Aug 20, 2021

View reviewed changes

Avoid using the rule data client for field list

3b18c32

dhurley14 reviewed Aug 20, 2021

View reviewed changes

weltenwort and others added 3 commits August 20, 2021 15:24

Remove left-over index argument

c494512

Merge branch 'master' of github.com:elastic/kibana into alerts-in-o11y

53ab296

Merge branch 'alerts-in-o11y' of github.com:XavierM/kibana into alert…

63f710d

…s-in-o11y

weltenwort mentioned this pull request Aug 20, 2021

[RAC] 7.15 Observability Checklist #107933

Closed

52 tasks

no needs of alert consumer anymore

f3f8aec

banderror reviewed Aug 20, 2021

View reviewed changes

XavierM commented Aug 20, 2021

View reviewed changes

banderror approved these changes Aug 20, 2021

View reviewed changes

weltenwort approved these changes Aug 20, 2021

View reviewed changes

XavierM added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 20, 2021

XavierM enabled auto-merge (squash) August 20, 2021 15:57

michaelolo24 approved these changes Aug 20, 2021

View reviewed changes

XavierM merged commit e8e53e3 into elastic:master Aug 20, 2021

kibanamachine mentioned this pull request Aug 20, 2021

[7.15] [RAC] Get o11y alerts in alerts table (#109346) #109517

Merged

banderror mentioned this pull request Aug 21, 2021

[RAC] Fix index names used by RBAC, delete hardcoded map of Kibana features to index names #109567

Merged

5 tasks

weltenwort mentioned this pull request Aug 23, 2021

[7.x] [RAC] Get o11y alerts in alerts table (#109346) #109705

Merged

weltenwort mentioned this pull request Sep 8, 2021

[RAC][Rule Registry] Implement RuleDataService.getReader() that skips index bootstrapping #111173

Closed

Conversation

XavierM commented Aug 19, 2021 • edited by mgiota Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Aug 19, 2021

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltenwort commented Aug 20, 2021

Uh oh!

dhurley14 Aug 20, 2021 • 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!

banderror 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltenwort Aug 20, 2021 • 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!

XavierM left a comment

Choose a reason for hiding this comment

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

michaelolo24 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 20, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

API count

API count missing comments

Non-exported public API item count

References to deprecated APIs

History

Uh oh!

kibanamachine commented Aug 20, 2021

💔 Backport failed

Uh oh!

weltenwort commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

XavierM commented Aug 19, 2021 •

edited by mgiota

Loading

dhurley14 Aug 20, 2021 •

edited

Loading

weltenwort Aug 20, 2021 •

edited

Loading

weltenwort commented Aug 20, 2021 •

edited

Loading