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] DetectionRulesClient refactoring. Part 2 #184364

Closed
17 tasks done
Tracked by #174168
xcrzx opened this issue May 28, 2024 · 6 comments · Fixed by #186988
Closed
17 tasks done
Tracked by #174168

[Security Solution] DetectionRulesClient refactoring. Part 2 #184364

xcrzx opened this issue May 28, 2024 · 6 comments · Fixed by #186988
Assignees
Labels
8.16 candidate Feature:Rule Management Security Solution Detection Rule Management area 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 v8.16.0

Comments

@xcrzx
Copy link
Contributor

xcrzx commented May 28, 2024

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

Summary

We need to finalize the DetectionRulesClient (formerly known as RulesManagementClient) refactoring and address the remaining comments left after the initial PR: #182802.

PRs

PR 1 (merged)

  • Come up with a better client name (see this comment). Consider DetectionRulesClient

PR 2 (merged)

  • Wrap all public client methods with withSecuritySpan (comment)
  • Move non-public client methods (starting with _) outside of the main client implementation for better code readability.

PR 3 (merged)

  • Rename DetectionRulesClient containing directory from rule_management to detection_rules_client
  • Move DetectionRulesClient methods into the detection_rules_client/methods dir
  • Move the TS interface of DetectionRulesClient into a separate file detection_rules_client_interface.ts
  • Simplify importRule method parameters
  • Add memoization to getDetectionRulesClient

PR 4 (merged)

  • Replace PrebuiltRuleAsset type with RuleCreateProps and RulePatchProps in upgradePrebuiltRule and createPrebuiltRule
  • Do not return the internal RuleAlertType from RulesManagementClient (see this comment). Transition to returning RuleResponse for these methods: createCustomRule and createPrebuiltRule

PR 5 (merged)

  • Do not return the internal RuleAlertType from RulesManagementClient (see this comment). Transition to returning RuleResponse for the remaining methods: updateRule, patchRule, deleteRule, upgradePrebuiltRule and importRule.
  • Make toggleRuleEnabledOnUpdate return enabled and then use it in return value
  • Check upgradePrebuiltRule enable behaviour - there might be a bug (place in code). Check if we need to explicitly toggle "enabled" on upgrade.

PR 6 (in review)

  • Colocate rule converters (rule_management/normalization/rule_converters) inside the new rules management client. Check how the converters are used outside the client to see if we can encapsulate them inside the client.
  • Refactor the converters. Split them into multiple functions, each should have a single responsibility.
  • Extract duplicated piece of code that converts to RuleResponse and throws an error into a function (comment)
  • Extract "transform and validate" code into a function (comment). Consider doing this together with refactoring converters.

Left to do

Leftovers moved to #187656.

@xcrzx xcrzx added refactoring Feature:Detection Rules Security Solution rules and Detection Engine 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 8.15 candidate labels May 28, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx
Copy link
Contributor Author

xcrzx commented May 31, 2024

@nikitaindik I also noticed that we don't provide correct APM spans inside our client. We need to wrap every client method in withSecuritySpan with a corresponding name. See how that's implemented in the prebuilt rule assets client:

fetchAssetsByVersion: (versions: RuleVersionSpecifier[]): Promise<PrebuiltRuleAsset[]> => {
return withSecuritySpan('IPrebuiltRuleAssetsClient.fetchAssetsByVersion', async () => {

@nikitaindik
Copy link
Contributor

Opened a PR that renames rulesManagementClient to detectionRulesClient.

nikitaindik added a commit that referenced this issue Jun 3, 2024
…Client` (#184598)

**Partially addresses: #184364

## Summary
As a first step in refactoring our newly added `rulesManagementClient`,
we are renaming it from `rulesManagementClient` to
`detectionRulesClient`.

`rulesManagementClient` looks and sounds too similar to a long existing
`rulesClient`, so it can be confusing to new devs or anyone quickly
digging through the code.

This PR just updates the name. Other refactorings will be arriving in
the upcoming PRs.
@nikitaindik nikitaindik changed the title [Security Solution] RulesManagementClient refactoring. Part 2 [Security Solution] DetectionRulesClient refactoring. Part 2 Jun 3, 2024
nikitaindik added a commit that referenced this issue Jun 6, 2024
…nd add APM spans (#184820)

**Partially addresses: #184364

## Summary
This PR is second step in refactoring our newly added
`detectionRulesClient`.

Changes in this PR:
 - every public method was extracted into its own file for readability
- `_createRule`, `_updateRule`, `_patchRule` and
`_upgradePrebuiltRuleWithTypeChange` private methods were removed, their
code inlined into the public methods
- `toggleRuleEnabledOnUpdate`, `validateMlAuth` and `ClientError` were
moved to `utils.ts`
- methods are now wrapped in `withSecuritySpan` to report perf stats to
APM
- renamed `*.rules_management_client.test.ts` ->
`*.detection_rules_client.test.ts`
- now using the whole `detectionRulesClient` in tests, not just separate
methods
- simplified parameters of `createDetectionRulesClient`. Now 2
parameters are needed instead of 5,

**DetectionRulesClient method showing up in APM**
<img width="918" alt="Scherm­afbeelding 2024-06-05 om 14 00 36"
src="https://github.com/elastic/kibana/assets/15949146/c8b469f7-9d0b-4534-a1c9-f35327ec2c4c">

**Extracted methods**
Upon reviewing the private methods in `detection_rules_client.ts`, it
became apparent that extracting these methods into separate files may
not be the most effective approach to improve readability. The primary
reason is that these private methods do not provide clear abstractions,
making them difficult to name appropriately.

Take `_updateRule` as an example. This method combines an existing rule
with a rule update to create an InternalRuleUpdate object, which is then
passed to `rulesClient.update`. If we were to extract this into a
separate file, we would need to import it for use in the public
`updateRule` method. This would result in an `updateRule` method that
calls `_updateRule`, creating confusion about what the inner
`_updateRule` does.

Also, extracting only private methods does not significantly improve
readability, as these methods do not contain a large amount of code.

So I ended up inlining the code from most of these private methods
directly into the public methods.
banderror added a commit that referenced this issue Jun 7, 2024
…4954)

**Partially addresses: #184364

## Summary
This PR contains various smaller-scale refactorings for the recently
added `DetectionsRuleClient`.

**Changes**:
- Renamed `DetectionRulesClient` containing directory from
`rule_management` to `detection_rules_client`
- Moved `DetectionRulesClient` methods into the
`detection_rules_client/methods` dir
- Moved the TS interface of `DetectionRulesClient` into a separate file
`detection_rules_client_interface.ts`
- Simplified `importRule` method parameters
- Added memoization to `getDetectionRulesClient`

---------

Co-authored-by: Georgii Gorbachev <[email protected]>
eokoneyo pushed a commit to eokoneyo/kibana that referenced this issue Jun 13, 2024
…stic#184954)

**Partially addresses: elastic#184364

## Summary
This PR contains various smaller-scale refactorings for the recently
added `DetectionsRuleClient`.

**Changes**:
- Renamed `DetectionRulesClient` containing directory from
`rule_management` to `detection_rules_client`
- Moved `DetectionRulesClient` methods into the
`detection_rules_client/methods` dir
- Moved the TS interface of `DetectionRulesClient` into a separate file
`detection_rules_client_interface.ts`
- Simplified `importRule` method parameters
- Added memoization to `getDetectionRulesClient`

---------

Co-authored-by: Georgii Gorbachev <[email protected]>
nikitaindik added a commit that referenced this issue Jun 18, 2024
…m `createCustomRule` and `createPrebuiltRule` (#185748)

**Partially addresses: #184364

## Summary
This PR changes `createCustomRule` and `createPrebuiltRule` methods to
return `RuleResponse` instead of `RuleAlertType`.

This is a continuation of the effort to improve `DetectionRulesClient`.
As a part of it we want to make the client return `RuleResponse` from
all its public methods. This is good because it lets us hide rule's
internal structure and centralise conversions in one place – in the
client. So in this and upcoming PRs we are going to convert
`RuleAlertType` to `RuleResponse` within the client's methods and return
the `RuleResponse` object.
bhapas pushed a commit to bhapas/kibana that referenced this issue Jun 18, 2024
…m `createCustomRule` and `createPrebuiltRule` (elastic#185748)

**Partially addresses: elastic#184364

## Summary
This PR changes `createCustomRule` and `createPrebuiltRule` methods to
return `RuleResponse` instead of `RuleAlertType`.

This is a continuation of the effort to improve `DetectionRulesClient`.
As a part of it we want to make the client return `RuleResponse` from
all its public methods. This is good because it lets us hide rule's
internal structure and centralise conversions in one place – in the
client. So in this and upcoming PRs we are going to convert
`RuleAlertType` to `RuleResponse` within the client's methods and return
the `RuleResponse` object.
nikitaindik added a commit that referenced this issue Jun 21, 2024
…m all methods (#186179)

**Partially addresses: #184364

## Summary

This PR is a follow-up to [PR
#185748](#185748) and it converts
the remaining `DetectionRulesClient` methods to return `RuleResponse`.

Changes in this PR:
- These methods now return `RuleResponse` instead of internal
`RuleAlertType` type:
  - `updateRule`
  - `patchRule`
  - `upgradePrebuiltRule`
  - `importRule`
bhapas pushed a commit to bhapas/kibana that referenced this issue Jun 24, 2024
…m all methods (elastic#186179)

**Partially addresses: elastic#184364

## Summary

This PR is a follow-up to [PR
elastic#185748](elastic#185748) and it converts
the remaining `DetectionRulesClient` methods to return `RuleResponse`.

Changes in this PR:
- These methods now return `RuleResponse` instead of internal
`RuleAlertType` type:
  - `updateRule`
  - `patchRule`
  - `upgradePrebuiltRule`
  - `importRule`
@nikitaindik nikitaindik assigned xcrzx and unassigned nikitaindik Jun 24, 2024
@nikitaindik
Copy link
Contributor

Hey @xcrzx! I've just changed the assignee to you since you are taking over the leftover work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.16 candidate Feature:Rule Management Security Solution Detection Rule Management area 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 v8.16.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants