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

[Security Solution] RulesManagementClient refactoring. Part 1 #180128

Closed
Tracked by #174168
jpdjere opened this issue Apr 5, 2024 · 4 comments
Closed
Tracked by #174168

[Security Solution] RulesManagementClient refactoring. Part 1 #180128

jpdjere opened this issue Apr 5, 2024 · 4 comments
Assignees
Labels
8.15 candidate Feature:Rule Management Security Solution Detection Rule Management refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.15.0

Comments

@jpdjere
Copy link
Contributor

jpdjere commented Apr 5, 2024

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168

‼️ Part of critical path ‼️

Summary

As part of the preparatory changes for Prebuilt Rules Customization Milestone 3, we want to refactor rule management CRUD utilities into a new abstraction with tentative name RuleManagementClient.

Currently, the CRUD utilities that our endpoints use suffer from the problem of tightly coupled logic, which leads them to having unnecessary complex interfaces and logic in order to handle all the different use cases for which they are used.

We aim for the new abstraction to deliver two main improvements:

  1. Separation of concerns that reduces tightly-coupled logic: rule management utilities should be use-case specific instead of generic. For example, the current createRules utility is used for creating custom rules, importing rules and upgrading rules. Instead, create specific utilities for each use case.
  2. Provide an abstraction where we can centralize migration on write and normalization on read logic

Background

### Problem with tightly coupled logic in our endpoints
All endpoints belonging to Detection Rules Management that create and update -including upgrade of prebuilt rules to new version- use three CRUD methods under the hood:
- [`createRules`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/crud/create_rules.ts)
- [`patchRules`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/crud/patch_rules.ts)
- [`updateRules`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/crud/update_rules.ts)
This "overuse" of these 3 methods for a variety of user actions makes their logic tightly coupled and creates a considerable amount of complexity to CRUD functions that should remain logically simple.
For example: the `createRules` method is used in 3 use cases:
1. when creating custom rules with our Rule Creation endpoints
2. when importing rules, when the imported `rule_id` does not already exist in Kibana
3. when upgrading rules, if a rule undergoes a `type` change, the existing rule is deleted a new one is created.
The same happens with `patchRules`. It is used:
1. when patching rules via our Rule Patch endpoints
3. when upgrading rules, if a rule maintains its `type`.
This causes these 3 CRUD functions to have an unnecessary complex interfaces and logic in order to handle all these different use cases.
As part of the epic, we should move away from this practice and create new CRUD methods that handle the different cases mentioned above specifically. This will help simplify our existing methods, keep logic in all of our CRUD methods simple and logically uncoupled from one another.

@jpdjere jpdjere added triage_needed Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team labels Apr 5, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@elasticmachine
Copy link
Contributor

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

@banderror banderror changed the title [Security Solution] Refactor rule management utilities behind a new abstraction of RuleManagementClient [Security Solution] Refactor rule management utilities behind a new abstraction of RuleManagementClient (DRAFT) Apr 17, 2024
@jpdjere jpdjere changed the title [Security Solution] Refactor rule management utilities behind a new abstraction of RuleManagementClient (DRAFT) [Security Solution] Refactor rule management utilities behind a new abstraction of RuleManagementClient Apr 18, 2024
@banderror banderror added refactoring Feature:Rule Management Security Solution Detection Rule Management 8.15 candidate v8.15.0 and removed triage_needed Feature:Detection Rules Anything related to Security Solution's Detection Rules labels May 12, 2024
xcrzx added a commit that referenced this issue May 28, 2024
…ation (#182802)

**Partially addresses: #180128

## Summary

- Creates `RulesManagementClient`, which centralizes CRUD utilites for
rules, at
`x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/crud/rules_management_client.ts`
    - Move ML Auth validation from endpoints to the new client utils.
- Adds client to `SecuritySolution` API requests context.
- Deletes `createRules`, `deletesRules`,`updateRules` and `patchRules`
utils and replaces them with new client methods.
- **Testing**:
- Creates individual test files for each "public" method of the
RulesManagementClient.
    - Creates importable mock of the client

## To-Do:

- Replace `readRules` method for a new public method within the API
(left out of this PR to keep scope and size manageable)

## Flaky Test Runner

- [FTR Exceptions -
ESS](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6080)
🟢
- [FTR Exceptions -
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6081)
🟢
- [FTR - Prebuilt Rules - Bundled Prebuilt Rules Package - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6083)
🟢
- [FTR - Prebuilt Rules - Large Prebuilt Rules Package - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6084)
🟢
- [FTR - Prebuilt Rules - Management - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6085)
🟢
- [FTR - Prebuilt Rules - Update Prebuilt Rules Package - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6085)
🟢
- [FTR - Rules Management - Rule Bulk Actions - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6087)
🟢
- [FTR - Rules Management - Rule Creation - Basic License Essentials
Tier - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6088)
🟢
- [FTR - Rules Management - Rule Creation - Trial License Complete Tier
- ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6089)
🟢
- [FTR - Rules Management - Rule Deletion - Basic License Essentials
Tier - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6090)
(name is wrong in the FTR UI, but runs the correct tests) 🟢
- [FTR - Rules Management - Rule Deletion - Trial License Complete Tier
- ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6091)
🟢
- [FTR - Rules Management - Rule Import and Export - Basic License
Essentials Tier - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6092)
🟢
- [FTR - Rules Management - Rule Import and Export - Trial License
Complete Tier - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6093)
🟢
- [FTR - Rules Management - Rule Patch - Basic License Essentials Tier -
ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6095)
🟢
- [FTR - Rules Management - Rule Patch - Trial License Complete Tier -
ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6096)
🟢
- [FTR - Rules Management - Rule Update - Basic License Essentials Tier
- ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6097)
🟢
- [FTR - Rules Management - Rule Update - Trial License Complete Tier -
ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6098)
🟢
- [Cypress - Security Solution Rule Management - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6099)
🟢
- [Cypress - Security Solution Rule Management - Prebuilt Rules - ESS
and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6100)
🟢
- [Cypress - Security Solution Detection Engine - Exceptions - ESS and
Serverless](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6101)
🟢

### 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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Dmitrii Shevchenko <[email protected]>
@xcrzx
Copy link
Contributor

xcrzx commented May 28, 2024

I've listed all remaining work in this follow-up ticket: #184364.

@xcrzx xcrzx closed this as completed May 28, 2024
@banderror banderror changed the title [Security Solution] Refactor rule management utilities behind a new abstraction of RuleManagementClient [Security Solution] RulesManagementClient refactoring. Part 1 May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.15 candidate Feature:Rule Management Security Solution Detection Rule Management refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.15.0
Projects
None yet
Development

No branches or pull requests

5 participants