[Security Solution] `DetectionRulesClient` refactoring. Part 2

[xcrzx]

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

Summary

We need to finalize the DetectionRulesClient (formerly known as RulesManagementClient) refactoring and address the remaining comments left after the initial PR: https://github.com/elastic/kibana/pull/182802.

PR 1 (merged)

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

PR 2 (merged)

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

PR 3 (merged)

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

PR 4 (merged)

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

PR 5 (merged)

• [x] 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.
• [x] Make toggleRuleEnabledOnUpdate return enabled and then use it in return value
• [x] Check upgradePrebuiltRule enable behaviour - there might be a bug (place in code). Check if we need to explicitly toggle "enabled" on upgrade.

High priority

• [ ] Add methods for fetching multiple rules to the client and refactor these utilities:
  • [ ] get_existing_prepackaged_rules.ts
  • [ ] find_rules.ts
• [ ] Add methods for fetching a single rule and refactor these utilities:
  • [ ] fetch_rule_by_id.ts
  • [ ] readRules
  • [ ] Check if it's possible to split this into two utilities
• [ ] 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.

Low priority

• [ ] Consider adding a method for exporting rules to the client. Check if this refactoring is overly complex. If it is, add it to a separate ticket.
• [ ] Replace usage of getIdError in DetectionRulesClient. Perhaps findRuleById can be used instead.
• [ ] Refactor throwing an error created with createBulkErrorObject in importRule. There shouldn't be a dependency on createBulkErrorObject since we don't deal with bulk operations in the client.
  • [ ] Wrap every method in a try-catch and throw a method-specific error (check how it's done it rule monitoring)
  • [ ] Check in Git history why is there a check for removed rule at the end of upgradePrebuiltRule
  • [ ] Add duplicateRule method?
  • [ ] Extract "transform and validate" code into a function (comment). Consider doing this together with refactoring converters.
  • [ ] Add error handling to toggleRuleEnabledOnUpdate (comment)
  • [ ] Extract duplicated piece of code that converts to RuleResponse and throws an error into a function (comment)

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

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

[xcrzx]

@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:

https://github.com/elastic/kibana/blob/71ea578bcba8814decd109264b4322984e65ec92/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client.ts#L127-L128

[nikitaindik]

Opened a PR that renames rulesManagementClient to detectionRulesClient.

[nikitaindik]

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