[Security Solution] `DetectionRulesClient`: return `RuleResponse` from all methods #186179

nikitaindik · 2024-06-13T12:04:58Z

Partially addresses: #184364

Summary

This PR is a follow-up to PR #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 · 2024-06-13T12:05:06Z

/ci

nikitaindik · 2024-06-14T20:31:04Z

/ci

nikitaindik · 2024-06-16T16:07:12Z

/ci

…ce it won't do anything

nikitaindik · 2024-06-18T15:58:00Z

/ci

elasticmachine · 2024-06-18T21:31:19Z

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

elasticmachine · 2024-06-18T21:31:20Z

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

elasticmachine · 2024-06-18T21:31:21Z

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

xcrzx

Thank you for the refactoring, Nikita 👍 The changes overall look good to me. I have a couple of minor comments to discuss. Let's sync on them over Zoom once you get a chance.

xcrzx · 2024-06-19T11:44:57Z

...solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts

    clients.rulesClient.find.mockResolvedValue(getEmptyFindResult());
    clients.rulesClient.update.mockResolvedValue(importedRule);


As rulesClient becomes an internal implementation detail. I think it would be best to start mocking the detectionRulesClient instead.

xcrzx · 2024-06-19T11:46:42Z

...solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts

-    clients.detectionRulesClient.importRule.mockResolvedValue(importedRule);
+    clients.detectionRulesClient.importRule.mockResolvedValue({
+      ...getRulesSchemaMock(),
+      rule_id: importedRule.params.ruleId,


importedRule.params is an internal data representation of the rules client. Is it possible to avoid relying on it in tests and use our RuleResponse representation?

xcrzx · 2024-06-19T12:06:52Z

...rver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/patch_rule.ts

@@ -39,16 +45,26 @@ export const patchRule = async (

  const patchedRule = convertPatchAPIToInternalSchema(nextParams, existingRule);


Out of the scope of this PR, but we need to split convertPatchAPIToInternalSchema into two methods:

Merge patch params with the existing rule

Convert the rule to internal Alerting representation

xcrzx · 2024-06-19T13:53:27Z

...ver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts

    });
  }
+
+  /* Trying to convert an internal rule to a RuleResponse object */
+  const parseResult = RuleResponse.safeParse(transform(importedInternalRule));


I think we need to use internalRuleToAPIResponse instead of transform here. If a rule has an incorrect rule type, the validation will fail at the RuleResponse parse stage, but the error would be more cryptic. I suggest reviewing all cases where transform is used and replacing them with internalRuleToAPIResponse.

nikitaindik · 2024-06-20T15:00:55Z

Thanks @xcrzx! I have addressed the comments. The PR can be reviewed again.

banderror

Quickly checked the diff, left a couple of minor comments. LGTM!

banderror · 2024-06-20T17:17:18Z

...rver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/patch_rule.ts

+  const { enabled } = await toggleRuleEnabledOnUpdate(
+    rulesClient,
+    existingRule,
+    nextParams.enabled
+  );


Can this function throw exceptions? If yes, I'd suggest to think about if we should add error handling to it so it's safe to call.

The case I'm thinking about is: if the await rulesClient.update call above was successful, how should we handle a failure of the subsequent enable/disable call?

banderror · 2024-06-20T17:21:37Z

...ver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts

+  /* Trying to convert an internal rule to a RuleResponse object */
+  const parseResult = RuleResponse.safeParse(internalRuleToAPIResponse(importedInternalRule));
+
+  if (!parseResult.success) {
+    throw new RuleResponseValidationError({
+      message: stringifyZodError(parseResult.error),
+      ruleId: importedInternalRule.params.ruleId,
+    });
+  }
+
+  return parseResult.data;


Now when this is duplicated in all of the methods, I think it's time to extract this into a function.

xcrzx

LGTM 👍 Thank you, @nikitaindik, for addressing my comments

kibana-ci · 2024-06-21T11:20:31Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 39dd3e9

Failed CI Steps

Metrics [docs]

✅ unchanged

History

💛 Build #216933 was flaky 463cb0e
💛 Build #216460 was flaky 8d3fe01
💔 Build #215958 failed bf56a9ad9b74f6f62acc796c73e8408c95b80f95
💔 Build #215946 failed c838747606e784aa0a31f8c51088eae1074b98c3
💔 Build #215755 failed 43e7ccfb83b51456adc149993a7db3d24777e7ce

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

cc @nikitaindik

nikitaindik · 2024-06-21T12:13:47Z

Hey @banderror! I went ahead and merged the PR to let @xcrzx build his changes on top of it more easily. I have added both your suggestions to the checklist in the issue. Will address them in the next PR.

…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 self-assigned this Jun 13, 2024

nikitaindik changed the title ~~[Security Solution] DetectionRulesClient: return RuleResponse from all methods~~ [Security Solution] DetectionRulesClient: return RuleResponse from all methods Jun 14, 2024

nikitaindik mentioned this pull request Jun 14, 2024

[Security Solution] DetectionRulesClient refactoring. Part 2 #184364

Closed

17 tasks

nikitaindik force-pushed the detection-rules-client-pr-5 branch from 2b9f24a to c838747 Compare June 14, 2024 20:29

nikitaindik force-pushed the detection-rules-client-pr-5 branch from c838747 to bf56a9a Compare June 16, 2024 16:06

banderror removed the backport:skip This commit does not require backporting label Jun 17, 2024

nikitaindik added 6 commits June 18, 2024 17:46

Make updateRule method return RuleResponse

713512d

Make patchRule method return RuleResponse

c53db9c

Make upgradePrebuiltRule method return RuleResponse

b6293f6

Make importRule method return RuleResponse

c8f9519

Fix failing tests

bfcac78

Do not call toggleRuleEnabledOnUpdate in upgradePrebuiltRule, sin…

8d3fe01

…ce it won't do anything

nikitaindik force-pushed the detection-rules-client-pr-5 branch from 9294d6c to 8d3fe01 Compare June 18, 2024 15:57

nikitaindik marked this pull request as ready for review June 18, 2024 21:31

nikitaindik requested a review from a team as a code owner June 18, 2024 21:31

nikitaindik requested a review from xcrzx June 18, 2024 21:31

xcrzx reviewed Jun 19, 2024

View reviewed changes

Address code review feedback

463cb0e

nikitaindik requested a review from xcrzx June 20, 2024 15:00

banderror reviewed Jun 20, 2024

View reviewed changes

xcrzx approved these changes Jun 21, 2024

View reviewed changes

Merge branch 'main' into detection-rules-client-pr-5

39dd3e9

nikitaindik merged commit 55687dd into elastic:main Jun 21, 2024
39 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2024

banderror mentioned this pull request Jul 5, 2024

[Security Solution] DetectionRulesClient refactoring. Part 3 #187656

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] `DetectionRulesClient`: return `RuleResponse` from all methods #186179

[Security Solution] `DetectionRulesClient`: return `RuleResponse` from all methods #186179

nikitaindik commented Jun 13, 2024 •

edited

Loading

nikitaindik commented Jun 13, 2024

nikitaindik commented Jun 14, 2024

nikitaindik commented Jun 16, 2024

nikitaindik commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

xcrzx left a comment

xcrzx Jun 19, 2024

xcrzx Jun 19, 2024

xcrzx Jun 19, 2024

xcrzx Jun 19, 2024

nikitaindik commented Jun 20, 2024

banderror left a comment

banderror Jun 20, 2024

banderror Jun 20, 2024

xcrzx left a comment

kibana-ci commented Jun 21, 2024

nikitaindik commented Jun 21, 2024

		clients.rulesClient.find.mockResolvedValue(getEmptyFindResult());
		clients.rulesClient.update.mockResolvedValue(importedRule);

		@@ -39,16 +45,26 @@ export const patchRule = async (

		const patchedRule = convertPatchAPIToInternalSchema(nextParams, existingRule);

[Security Solution] DetectionRulesClient: return RuleResponse from all methods #186179

[Security Solution] DetectionRulesClient: return RuleResponse from all methods #186179

Conversation

nikitaindik commented Jun 13, 2024 • edited Loading

Summary

nikitaindik commented Jun 13, 2024

nikitaindik commented Jun 14, 2024

nikitaindik commented Jun 16, 2024

nikitaindik commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

elasticmachine commented Jun 18, 2024

xcrzx left a comment

Choose a reason for hiding this comment

xcrzx Jun 19, 2024

Choose a reason for hiding this comment

xcrzx Jun 19, 2024

Choose a reason for hiding this comment

xcrzx Jun 19, 2024

Choose a reason for hiding this comment

xcrzx Jun 19, 2024

Choose a reason for hiding this comment

nikitaindik commented Jun 20, 2024

banderror left a comment

Choose a reason for hiding this comment

banderror Jun 20, 2024

Choose a reason for hiding this comment

banderror Jun 20, 2024

Choose a reason for hiding this comment

xcrzx left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 21, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

nikitaindik commented Jun 21, 2024

[Security Solution] `DetectionRulesClient`: return `RuleResponse` from all methods #186179

[Security Solution] `DetectionRulesClient`: return `RuleResponse` from all methods #186179

nikitaindik commented Jun 13, 2024 •

edited

Loading