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][Rule Registry] Improve the API of RuleDataService and RuleDataClient #106421

Closed
5 of 6 tasks
Tracked by #101016
banderror opened this issue Jul 21, 2021 · 5 comments
Closed
5 of 6 tasks
Tracked by #101016
Labels
Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Theme: rac label obsolete

Comments

@banderror
Copy link
Contributor

banderror commented Jul 21, 2021

Parent ticket: #101016

Summary

We'd like to improve DX for those who need to work with RuleDataService and RuleDataClient.

On a high level, if we compare the two implementations we have in rule_registry, and what we'd like to achieve:

Feature RuleDataService AS IS EventLogService RuleDataService TO BE
Schema Implicit, schema-less (indexing function accepts documents as any) Explicit schema for documents Explicit schema for documents
Dependency on FieldMap No hard dependency Hard dependency No hard dependency
Component templates Yes No Yes
Index management API Imperative Declarative Declarative
Index management encapsulation Low-level methods are exposed to clients, clients are responsible for correct implementation of index management Low-level methods are encapsulated, machanism is responsible for index management Low-level methods are encapsulated, machanism is responsible for index management

Specifically, let's do the following:

  • Make it simple and declarative to specify TypeScript schema for RuleDataClient.

    • Dev should be able to define it manually or convert a FieldMap to it. This is basically what was already implemented in [RAC] Build a consolidated implementation in Rule Registry (Draft) #101453
    • Schema should be optional.
    • Out of scope (maybe later): generating schema from Elasticsearch mappings (so we could have templates as a single source of truth); validation on write; normalization on read.
  • Make it simple and declarative to specify Elasticsearch resources needed for an instance of RuleDataClient (and its corresponding indices, e.g. .alerts-security.alerts-{namespace}): ILM policy, component templates, index template, aliases, etc.

    • Solution plugins should not have to know about internal details of index bootstrapping (creating/updating all the mentioned resources and the concrete index itself)
    • We shouldn't have to create a ready signal in solution plugins and pass it into the rule data client constructor
    • Devs should be clearly guided away from inefficient patterns even if they don't know the internal details of why it's inefficient. e.g. with the rollover check in getWriter, a single writer should be used for the duration of a rule executor. getWriter should not be called repeatedly. We should make this clear in documentation, and also rename getWriter to something that sounds more appropriate like initializeWriter - this has a better chance of letting devs know that initializeWriter is not something you want to call repeatedly.
  • Use flexible approach to component templates, but enforce anything that is critical to correct work of this implementation. For example, a solution dev should be able to specify any number of component templates for a log (e.g. 3 custom component templates for security.alerts carrying something that makes sense for security detection alerts), but the implementation must include references to the common templates (e.g. with "technical fields") under the hood.

    • We're not going to standardize templates like proposed in [RAC][Meta] Consolidate the two indexing implementations in rule_registry plugin #101016 (comment)
    • All component templates should be namespace-agnostic, so shared between all concrete logs. We don't need to support namespace-aware component templates.
    • We're probably not going to introduce a user component template, because if the user updates mappings while Kibana is running, we will need to check "if we need to rollover the index" before each write (we'd rather do that at the start and only once). Instead, as far as we understand at this point, the recommendation is to use runtime fields.
    • We would like to leverage component templates by default and avoid merging mappings in the code where possible. So that could be our idiomatic way of combining mappings and settings.
  • Index bootstrapping logic should be encapsulated. RuleDataService should provide a method that would implement bootstrapping; it would accept a log definition object and return a RuleDataClient. Invariants should be enforced by this logic, race conditions and errors handled correctly.

  • Enforce allowed values for the datasetSuffix on the API level. In the index naming convention .alerts-{registrationContext}.{datasetSuffix}-{namespace}, datasetSuffix can only be alerts or events. It would make sense to restrict it in the RuleDataService interface to make it safer and more explicit for developers.

  • Provide different default values for namespace, which seem a bit safer defaults:

    • in getWriter set namespace = 'default' by default
    • in getReader maybe set namespace = '*' by default

Details

  • RuleDataPluginService should provide a function that takes a "log definition" in some form, creates the appropriate component and index templates, and returns a Promise<RuleDataClient> for that log. Rule executors can then await this promise to ensure that they don't write to the log until it is properly bootstrapped. This removes the need for solution plugins to define a "ready signal" to pass into the RuleDataClient that signals when the templates are created, instead the rule registry handles all the synchronization.
    • "log definition" implementation TBD - initially the log definition could be as simple as an array of component templates and an index template that would be passed to RuleDataPluginService.createOrUpdateComponentTemplate and RuleDataPluginService.createOrUpdateIndexTemplate. A schema for the log can be passed in as an optional parameter alongside the log definition to get static typing for the documents written by the RuleDataClient writer.
    • In the context of the current security solution RuleDataPluginService usage, we'd take the templates here and pass them in to a single function like RuleDataPluginService.bootstrapSolutionTemplates(templates): Promise<RuleDataClient> instead of imperatively creating the templates and later directly instantiating a RuleDataClient.
    • The RuleDataPluginService can automatically add the technicalComponentTemplate as the final component template in the index template, ensuring that the technical fields will be included and mapped correctly.
    • The log definition should be simple and flexible, allowing solutions to define arbitrary component templates
    • A more advanced log definition could be defined as a set of FieldMaps that the RuleDataPluginService would transform into the appropriate component and index templates. This would enable the RuleDataPluginService to build the TS types for the log automatically. However, this could also be done as an additional refactor after starting with a simpler implementation.
  • The technical component template is the only required template for .alerts indices. The rule registry may provide other component templates that solutions can share for ease of use, e.g. the full ECS component template.

This approach should adopt the declarative style from the event_log. The main difference is it aims to be a lighter-weight implementation that encapsulates template bootstrapping details while providing some flexibility in log definitions. Rather than caching objects for repeated retrieval from the service, the RuleDataPluginService will simply create templates and return a Promise<RuleDataClient>. Solution plugins will be responsible for passing the RuleDataClient around to the appropriate places where it is needed.

@botelastic botelastic bot added the needs-team Issues missing a team label label Jul 21, 2021
@banderror banderror added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team Theme: rac label obsolete and removed needs-team Issues missing a team label labels Jul 21, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror banderror self-assigned this Jul 21, 2021
banderror added a commit that referenced this issue Aug 15, 2021
…ing implementation (#108115)

**Addresses:** #106421, #106428, #102089, #106433

## Summary

This PR focuses on consolidation of indexing implementations in `rule_registry` (#101016). It addresses some of the sub-tasks of the parent ticket.

- [x] Encapsulate index bootstrapping logic in a new improved API exposed by `RuleDataService`.
- [x] Enforce allowed values for the `datasetSuffix` on the API level.
- [x] Migrate plugins using the existing `RuleDataService` API to the improved one.
- [x] Make sure index names comply with design architecture.
    - #102089
- [x] Improve the API of `RuleDataClient`.
- [x] Enhance index bootstrapping: support custom ILM policy per index (`{registrationContext}.{datasetSuffix}`).
- [x] Enhance index bootstrapping: create index template per namespace and support rollovers properly
    - based on #107700
- [x] Enhance index bootstrapping: support secondary aliases
    - based on #107700
- [x] Remove `EventLogService` implementation
    - #106433

This will be addressed in follow-up PRs:

- [ ] Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
- [ ] Enhance index bootstrapping: implement upgrades of existing index templates.
- [ ] Make index bootstrapping logic more robust. This _is partially addressed_ in this PR, but more improvements are needed.
- [ ] Change the way index prefix works.
- [ ] Add support for optional TS schema (static typing).
- [ ] Update `README` in `rule_registry`.

### Checklist

- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
- [ ] [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 issue Aug 15, 2021
…ing implementation (elastic#108115)

**Addresses:** elastic#106421, elastic#106428, elastic#102089, elastic#106433

## Summary

This PR focuses on consolidation of indexing implementations in `rule_registry` (elastic#101016). It addresses some of the sub-tasks of the parent ticket.

- [x] Encapsulate index bootstrapping logic in a new improved API exposed by `RuleDataService`.
- [x] Enforce allowed values for the `datasetSuffix` on the API level.
- [x] Migrate plugins using the existing `RuleDataService` API to the improved one.
- [x] Make sure index names comply with design architecture.
    - elastic#102089
- [x] Improve the API of `RuleDataClient`.
- [x] Enhance index bootstrapping: support custom ILM policy per index (`{registrationContext}.{datasetSuffix}`).
- [x] Enhance index bootstrapping: create index template per namespace and support rollovers properly
    - based on elastic#107700
- [x] Enhance index bootstrapping: support secondary aliases
    - based on elastic#107700
- [x] Remove `EventLogService` implementation
    - elastic#106433

This will be addressed in follow-up PRs:

- [ ] Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
- [ ] Enhance index bootstrapping: implement upgrades of existing index templates.
- [ ] Make index bootstrapping logic more robust. This _is partially addressed_ in this PR, but more improvements are needed.
- [ ] Change the way index prefix works.
- [ ] Add support for optional TS schema (static typing).
- [ ] Update `README` in `rule_registry`.

### Checklist

- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
- [ ] [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 issue Aug 15, 2021
…ing implementation (#108115) (#108638)

**Addresses:** #106421, #106428, #102089, #106433

## Summary

This PR focuses on consolidation of indexing implementations in `rule_registry` (#101016). It addresses some of the sub-tasks of the parent ticket.

- [x] Encapsulate index bootstrapping logic in a new improved API exposed by `RuleDataService`.
- [x] Enforce allowed values for the `datasetSuffix` on the API level.
- [x] Migrate plugins using the existing `RuleDataService` API to the improved one.
- [x] Make sure index names comply with design architecture.
    - #102089
- [x] Improve the API of `RuleDataClient`.
- [x] Enhance index bootstrapping: support custom ILM policy per index (`{registrationContext}.{datasetSuffix}`).
- [x] Enhance index bootstrapping: create index template per namespace and support rollovers properly
    - based on #107700
- [x] Enhance index bootstrapping: support secondary aliases
    - based on #107700
- [x] Remove `EventLogService` implementation
    - #106433

This will be addressed in follow-up PRs:

- [ ] Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
- [ ] Enhance index bootstrapping: implement upgrades of existing index templates.
- [ ] Make index bootstrapping logic more robust. This _is partially addressed_ in this PR, but more improvements are needed.
- [ ] Change the way index prefix works.
- [ ] Add support for optional TS schema (static typing).
- [ ] Update `README` in `rule_registry`.

### Checklist

- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
- [ ] [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
Copy link
Contributor Author

Partially addressed in #108115

@banderror banderror removed their assignment Sep 1, 2021
@banderror banderror changed the title [RAC] Improve the API of RuleDataService and RuleDataClient [RAC][Rule Registry] Improve the API of RuleDataService and RuleDataClient Sep 1, 2021
@banderror banderror added the Team:Detection Alerts Security Detection Alerts Area Team label Oct 11, 2021
@banderror
Copy link
Contributor Author

Hey everyone, FYI ownership of this ticket and other tickets related to rule_registry (like #101016) now goes to the Detection Alerts area (Team:Detection Alerts label). Please ping @peluja1012 and @marshallmain if you have any questions.

@marshallmain
Copy link
Contributor

Major work on this issue is all completed (see #108115), and other approaches are being taken for document schemas for Detection Alerts - see #127218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Theme: rac label obsolete
Projects
None yet
Development

No branches or pull requests

3 participants