Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Aug 17, 2021

Addresses: #102089

🚨 This PR is critical for Observability 7.15 🚨

Summary

This PR changes the mapConsumerToIndexName used by RAC RBAC and tests containing references to those indices.

  • observability-apm renamed to observability.apm
  • .alerts suffix was added, e.g. .alerts-observability.apm.alerts and .alerts-observability.metrics.alerts
  • observability: '.alerts-observability' is not changed, does it make sense to make it observability: '.alerts-observability.*.alerts'?

Checklist

Delete any items that are not applicable to this PR.

@banderror banderror added bug Fixes for quality problems that affect the customer experience v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team auto-backport Deprecated - use backport:version if exact versions are needed Theme: rac label obsolete v7.15.0 labels Aug 17, 2021
@banderror banderror self-assigned this Aug 17, 2021
@banderror banderror requested a review from a team as a code owner August 17, 2021 11:38
@banderror banderror requested review from a team August 17, 2021 11:38
@elasticmachine
Copy link
Contributor

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

@banderror banderror added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Aug 17, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

infrastructure: '.alerts-observability.metrics',
apm: '.alerts-observability.apm.alerts',
logs: '.alerts-observability.logs.alerts',
infrastructure: '.alerts-observability.metrics.alerts',
observability: '.alerts-observability',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to change it to .alerts-observability.*.alerts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This object should be updated to append the space id in the index name as well as reading from the kibana config for observability's usage of custom prefixes. Not sure if these are still happening @peluja1012 ?

@banderror It would be ideal to implement this data structure as a saved object, this way we provide the customer with some way to mutate the index names without having to push out a new release of kibana.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom index prefix setting has been removed.

Do we need this data-structure at all at this point? Can't we use the registration context as a further (non-security-relevant) filter? (not for this PR, but in future iterations)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't index evaluation docs the result would be the same, but the change also couldn't hurt while we're at it.

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty straightforward change.. Looks good. If you have the time I think it is worth it to consider moving this into a saved object. Better to store data in Elasticsearch than in kibana..

infrastructure: '.alerts-observability.metrics',
apm: '.alerts-observability.apm.alerts',
logs: '.alerts-observability.logs.alerts',
infrastructure: '.alerts-observability.metrics.alerts',
observability: '.alerts-observability',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This object should be updated to append the space id in the index name as well as reading from the kibana config for observability's usage of custom prefixes. Not sure if these are still happening @peluja1012 ?

@banderror It would be ideal to implement this data structure as a saved object, this way we provide the customer with some way to mutate the index names without having to push out a new release of kibana.

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, looks simple enough. Thank you!

Ideally we wouldn't need that mapping at all in the future. 🤔

infrastructure: '.alerts-observability.metrics',
apm: '.alerts-observability.apm.alerts',
logs: '.alerts-observability.logs.alerts',
infrastructure: '.alerts-observability.metrics.alerts',
observability: '.alerts-observability',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom index prefix setting has been removed.

Do we need this data-structure at all at this point? Can't we use the registration context as a further (non-security-relevant) filter? (not for this PR, but in future iterations)

@weltenwort weltenwort requested review from a team and dominiqueclarke August 18, 2021 15:41
@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Looks like Jest snapshots need to be updated.

@banderror banderror force-pushed the rule-registry-index-names-compliance-with-design branch 2 times, most recently from b7b0a0d to 070a7d6 Compare August 19, 2021 10:22
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we either have to specify the namespace suffix in the mapConsumerToIndexName mapping or dynamically add it in the code paths that access the alerts? I think that's not happening right now and causes the alerts table query to target the wrong indices.

@yctercero are there any pending PRs that add the namespace suffix during data access?

@banderror banderror force-pushed the rule-registry-index-names-compliance-with-design branch from 070a7d6 to f526378 Compare August 19, 2021 17:21
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the -* on short notice. It doesn't fix everything (such as the alerts table field selector) but it unblock some other development work. 🙇

@banderror banderror force-pushed the rule-registry-index-names-compliance-with-design branch from f526378 to f2b0a22 Compare August 20, 2021 11:40
@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

  • 💔 Build #147003 failed f52637847ffdef77534c1a0067bf6ad2fdf320f9
  • 💔 Build #146862 failed 070a7d67b80c4455b84b1fc99848a56b1650a5af
  • 💔 Build #146806 failed b7b0a0d11d552cbcc7247d732bf1e866800350c3
  • 💔 Build #146109 failed bb23f87dac490472c43799341fc4ddde25f63dc9

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

cc @banderror

@banderror
Copy link
Contributor Author

This PR is going to be replaced by #109567 and closed.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uptime changes LGTM

@banderror
Copy link
Contributor Author

I'm closing this one, the same goal (updating index names that RAC RBAC uses - in order to comply with the index naming scheme) is addressed in #109567 in a better way.

@banderror banderror closed this Aug 24, 2021
@banderror banderror deleted the rule-registry-index-names-compliance-with-design branch August 24, 2021 21:20
banderror added a commit that referenced this pull request Aug 25, 2021
…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
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 25, 2021
…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 pushed a commit to kibanamachine/kibana that referenced this pull request Aug 25, 2021
…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 added a commit that referenced this pull request Aug 25, 2021
…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 <[email protected]>
kibanamachine added a commit that referenced this pull request Aug 25, 2021
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Theme: rac label obsolete v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants