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

banderror · 2021-08-21T01:33:00Z

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:

Address [RAC] Get o11y alerts in alerts table #109346 (review)
Make changes to AlertsClient.getAuthorizedAlertsIndices() so it starts using RuleDataService to get index names by feature ids.
Delete the hardcoded mapConsumerToIndexName where we had incorrect index names.
Close [RAC] Update RBAC mapping of features to index names to comply with the index naming scheme #108872

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-08-21T01:33:03Z

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

elasticmachine · 2021-08-21T01:33:03Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2021-08-23T14:18:12Z

Pinging @elastic/uptime (Team:uptime)

dhurley14 · 2021-08-23T16:54:18Z

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

+          // TODO: Remove space id from the index name and make sure the app works well.
+          // We should not include space id into the index name, because a
+          // namespace goes into the index name, and it's user-defined in general.
+          // The user can set a custom namespace per rule instance which could be != space id.


namespaces can have whitespace chars in them, correct? That could be a reason to use space id over namespace. Not sure we would want whitespace chars in index names or maybe those get hyphenated?

We had a chat with @dhurley14 on this comment. For visibility I will post a summary here (Devin is on PTO right now).

My point was: namespaces, according to the design document, are supposed to be user-defined. But in Security they will be equal to the space id by default. But still, the user will be able to set a custom namespace for a single rule instance.

if we include a space id in the index name when reading data, we will omit the alerts generated by this rule instance

namespaces should not include whitespace characters I guess, but they can contain dashes; basically they can contain any characters a space id can

when reading from RAC indices, we should omit the namespace part from the queries to ES

Devin's point is:

that all makes sense except for the last bullet point:

when reading from RAC indices, we should omit the namespace part from the queries to ES

We are doing that as part of the authorization filter. This requirement came early on from Brandon I think so that would need to be discussed further.
I'm fine with whichever decision is made going forward (sounds like it'll be namespaces > space ids) but I just want to make sure that change is also reflected in places we use space id for authorization in the alerts as data client.

Im not sure what should be done in this PR regarding that, to be honest. Maybe I just don’t understand why this function (getAuthorizedAlertsIndices) needs to include space id to the index name. I don’t know how the HTTP endpoint that calls this function is actually used in the app. Where the index names from the endpoint's response are being propagated to, etc. Maybe I'm just missing something.

I will remove this TODO comment to avoid confusion (especially if it's misleading) and try to figure out the usage of space id in this function later.

weltenwort

This looks fine, but I don't know enough about the AlertsClient to speak with confidence about the changes therein.

weltenwort · 2021-08-24T10:40:35Z

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

+    const indicesAssociatedWithFeature = this.indicesByFeatureId.get(indexOptions.feature) ?? [];
+    this.indicesByFeatureId.set(indexOptions.feature, [...indicesAssociatedWithFeature, indexInfo]);
+    this.indicesByBaseName.set(indexInfo.baseName, indexInfo);


This works fine, but it makes me wonder if we shouldn't just keep a Set() of IndexInfos (with added feature ids) and .filter() it when we need to query by some property. 🤔 That would be a much simpler datastructure. Definitely not a high priority for this PR, though.

This is not obvious to me right away, I'll revisit this comment tomorrow.

weltenwort · 2021-08-24T11:26:18Z

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

 import { AlertsClient } from './alerts_client';

 export interface AlertsClientFactoryProps {
  logger: Logger;
  esClient: ElasticsearchClient;
  getAlertingAuthorization: (request: KibanaRequest) => PublicMethodsOf<AlertingAuthorization>;
  securityPluginSetup: SecurityPluginSetup | undefined;
+  ruleDataService: RuleDataPluginService | null;


Why do we allow null here? Doesn't this together with the assertion in private ruleDataService! break type safety?

I'll revisit this comment tomorrow.

banderror · 2021-08-24T18:07:41Z

packages/kbn-rule-data-utils/src/alerts_as_data_rbac.ts

-export const mapConsumerToIndexName: Record<AlertConsumers, string | string[]> = {
-  apm: '.alerts-observability-apm',
-  logs: '.alerts-observability.logs',
-  infrastructure: '.alerts-observability.metrics',
-  observability: '.alerts-observability',
-  siem: '.alerts-security.alerts',
-  uptime: '.alerts-observability.uptime',
-};
-export type ValidFeatureId = keyof typeof mapConsumerToIndexName;
+export type ValidFeatureId = AlertConsumers;

-export const validFeatureIds = Object.keys(mapConsumerToIndexName);


Besides deleting the map itself, this PR actually fixes the names of the indices, particularly .alerts-observability-apm -> .alerts-observability.apm.alerts.
The current rule data client in the master branch writes to .alerts-observability.apm.alerts.

dominiqueclarke

Uptime changes LGTM

…te arguments

banderror · 2021-08-25T12:13:50Z

Thank you @dhurley14 @weltenwort @dominiqueclarke for your reviews 🙏

kibanamachine · 2021-08-25T14:27:15Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`ruleRegistry`	114	117	+3

Async chunks

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

id	before	after	diff
`apm`	4.4MB	4.4MB	-296.0B
`observability`	568.4KB	568.1KB	-327.0B
`securitySolution`	6.5MB	6.5MB	-296.0B
`timelines`	442.4KB	441.3KB	-1.2KB
total			-2.1KB

Page load bundle

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

id	before	after	diff
`infra`	149.5KB	149.2KB	-296.0B
`uptime`	36.4KB	36.2KB	-296.0B
total			-592.0B

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	136	140	+4

History

💚 Build #148460 succeeded 213d269
💚 Build #148049 succeeded e9014a6
💚 Build #147765 succeeded 42d995e
💔 Build #147551 failed 6b46e84
💔 Build #147548 failed 25c10b7

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

cc @banderror

…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

kibanamachine · 2021-08-25T14:32:13Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

…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>

banderror requested review from XavierM, dhurley14 and weltenwort August 21, 2021 01:33

banderror self-assigned this Aug 21, 2021

banderror mentioned this pull request Aug 21, 2021

[RAC] Update RBAC mapping of features to index names to comply with the index naming scheme #108872

Closed

1 task

banderror force-pushed the rule-registry-rbac-mapping-to-index-names branch from 6b46e84 to 42d995e Compare August 23, 2021 14:18

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Aug 23, 2021

dhurley14 reviewed Aug 23, 2021

View reviewed changes

banderror force-pushed the rule-registry-rbac-mapping-to-index-names branch from 42d995e to e9014a6 Compare August 24, 2021 07:49

banderror requested a review from a team as a code owner August 24, 2021 07:49

banderror requested review from a team August 24, 2021 07:49

banderror requested a review from a team as a code owner August 24, 2021 07:49

banderror mentioned this pull request Aug 24, 2021

[RAC] [APM] removes hardcoded alerts as data index from apm integration test #109715

Merged

9 tasks

weltenwort approved these changes Aug 24, 2021

View reviewed changes

banderror commented Aug 24, 2021

View reviewed changes

dominiqueclarke approved these changes Aug 24, 2021

View reviewed changes

banderror force-pushed the rule-registry-rbac-mapping-to-index-names branch from e9014a6 to 213d269 Compare August 25, 2021 08:27

banderror added 6 commits August 25, 2021 14:02

Remove observability.infrastructure registration context from the rou…

e11180c

…te arguments

Fix mappings to IndexInfo in RuleDataPluginService

f650dff

AlertsClient.getAuthorizedAlertsIndices() starts using RuleDataService

86d47e0

Delete mapConsumerToIndexName

1e6037a

Fix tests

a480916

Address comments

c3e2a8f

banderror force-pushed the rule-registry-rbac-mapping-to-index-names branch from 213d269 to c3e2a8f Compare August 25, 2021 12:10

banderror merged commit 8ce1d10 into elastic:master Aug 25, 2021

banderror deleted the rule-registry-rbac-mapping-to-index-names branch August 25, 2021 14:29

kibanamachine mentioned this pull request Aug 25, 2021

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

Merged

kibanamachine mentioned this pull request Aug 25, 2021

[7.x] [RAC] Fix index names used by RBAC, delete hardcoded map of Kibana features to index names (#109567) #110068

Merged

banderror mentioned this pull request Aug 25, 2021

[RAC] Make sure index names comply with design architecture #102089

Closed

This was referenced Sep 3, 2021

[RAC][Rule Registry] Investigate why getAuthorizedAlertsIndices includes Kibana space id into the index name #111154

Closed

[RAC][Rule Registry] Small clean-ups #111470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

banderror commented Aug 21, 2021 •

edited

Loading

elasticmachine commented Aug 21, 2021

elasticmachine commented Aug 21, 2021

elasticmachine commented Aug 23, 2021

dhurley14 Aug 23, 2021 •

edited

Loading

banderror Aug 25, 2021 •

edited

Loading

weltenwort left a comment

weltenwort Aug 24, 2021

banderror Aug 25, 2021

weltenwort Aug 24, 2021

banderror Aug 25, 2021

banderror Aug 24, 2021

dominiqueclarke left a comment

banderror commented Aug 25, 2021

kibanamachine commented Aug 25, 2021

API count

kibanamachine commented Aug 25, 2021

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

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

Conversation

banderror commented Aug 21, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Aug 21, 2021

elasticmachine commented Aug 21, 2021

elasticmachine commented Aug 23, 2021

dhurley14 Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

banderror Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Aug 24, 2021

Choose a reason for hiding this comment

banderror Aug 25, 2021

Choose a reason for hiding this comment

weltenwort Aug 24, 2021

Choose a reason for hiding this comment

banderror Aug 25, 2021

Choose a reason for hiding this comment

banderror Aug 24, 2021

Choose a reason for hiding this comment

dominiqueclarke left a comment

Choose a reason for hiding this comment

banderror commented Aug 25, 2021

kibanamachine commented Aug 25, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Aug 25, 2021

💚 Backport successful

banderror commented Aug 21, 2021 •

edited

Loading

dhurley14 Aug 23, 2021 •

edited

Loading

banderror Aug 25, 2021 •

edited

Loading