From 5b67f44d89365154f1d55101a3c81bbb247103c3 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Mon, 11 Nov 2024 15:56:11 -0600 Subject: [PATCH] Move calculation of rule source outside of applyRuleUpdate With the current implementation, there are instances where we call `applyRuleUpdate` but do not want/need it to calculate rule source (e.g. when called from `importRules`, which pre-calculates the rule_source for incoming rules before passing them to `importRule`. Instead of adding a flag to conditionally call `calculateRuleSource` from within `applyRuleUpdate` I've opted to separate the two functions as these seem to be logically distinct actions. The three existing calls to `applyRuleUpdate` have been updated to be functionally equivalent. The effect of this PR is that we will no longer unnecessarily call `fetchAssetsByVersion` for each individual rule being imported, which should improve performance of rule import. --- .../mergers/apply_rule_update.ts | 9 --------- .../detection_rules_client/methods/import_rule.ts | 14 +++++++++++--- .../detection_rules_client/methods/update_rule.ts | 6 +++++- .../methods/upgrade_prebuilt_rule.ts | 6 +++++- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts index b911e66a1fc45..add67888442be 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts @@ -9,18 +9,14 @@ import type { RuleResponse, RuleUpdateProps, } from '../../../../../../../common/api/detection_engine/model/rule_schema'; -import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client'; import { applyRuleDefaults } from './apply_rule_defaults'; -import { calculateRuleSource } from './rule_source/calculate_rule_source'; interface ApplyRuleUpdateProps { - prebuiltRuleAssetClient: IPrebuiltRuleAssetsClient; existingRule: RuleResponse; ruleUpdate: RuleUpdateProps; } export const applyRuleUpdate = async ({ - prebuiltRuleAssetClient, existingRule, ruleUpdate, }: ApplyRuleUpdateProps): Promise => { @@ -43,10 +39,5 @@ export const applyRuleUpdate = async ({ created_by: existingRule.created_by, }; - nextRule.rule_source = await calculateRuleSource({ - rule: nextRule, - prebuiltRuleAssetClient, - }); - return nextRule; }; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts index dd57e66c41a64..f98338f8017e1 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts @@ -19,6 +19,7 @@ import { validateMlAuth, toggleRuleEnabledOnUpdate } from '../utils'; import { createRule } from './create_rule'; import { getRuleByRuleId } from './get_rule_by_rule_id'; import { createRuleImportErrorObject } from '../../import/errors'; +import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source'; interface ImportRuleOptions { actionsClient: ActionsClient; @@ -57,12 +58,19 @@ export const importRule = async ({ if (existingRule && overwriteRules) { let ruleWithUpdates = await applyRuleUpdate({ - prebuiltRuleAssetClient, existingRule, ruleUpdate: rule, }); - // applyRuleUpdate prefers the existing rule's values for `rule_source` and `immutable`, but we want to use the importing rule's calculated values - ruleWithUpdates = { ...ruleWithUpdates, ...overrideFields }; + + // If no override fields are provided, we calculate the rule source + if (overrideFields == null) { + ruleWithUpdates.rule_source = await calculateRuleSource({ + rule: ruleWithUpdates, + prebuiltRuleAssetClient, + }); + } else { + ruleWithUpdates = { ...ruleWithUpdates, ...overrideFields }; + } const updatedRule = await rulesClient.update({ id: existingRule.id, diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts index 8fd7f7a89dcb7..6b03dc152df34 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/update_rule.ts @@ -10,6 +10,7 @@ import type { RulesClient } from '@kbn/alerting-plugin/server'; import type { RuleResponse } from '../../../../../../../common/api/detection_engine/model/rule_schema'; import type { MlAuthz } from '../../../../../machine_learning/authz'; import { applyRuleUpdate } from '../mergers/apply_rule_update'; +import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source'; import { getIdError } from '../../../utils/utils'; import { validateNonCustomizableUpdateFields } from '../../../utils/validate'; import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule'; @@ -54,10 +55,13 @@ export const updateRule = async ({ validateNonCustomizableUpdateFields(ruleUpdate, existingRule); const ruleWithUpdates = await applyRuleUpdate({ - prebuiltRuleAssetClient, existingRule, ruleUpdate, }); + ruleWithUpdates.rule_source = await calculateRuleSource({ + rule: ruleWithUpdates, + prebuiltRuleAssetClient, + }); const updatedRule = await rulesClient.update({ id: existingRule.id, diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/upgrade_prebuilt_rule.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/upgrade_prebuilt_rule.ts index 64486bed14304..497b9244db448 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/upgrade_prebuilt_rule.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/methods/upgrade_prebuilt_rule.ts @@ -15,6 +15,7 @@ import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic import { convertAlertingRuleToRuleResponse } from '../converters/convert_alerting_rule_to_rule_response'; import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule'; import { applyRuleUpdate } from '../mergers/apply_rule_update'; +import { calculateRuleSource } from '../mergers/rule_source/calculate_rule_source'; import { ClientError, validateMlAuth } from '../utils'; import { createRule } from './create_rule'; import { getRuleByRuleId } from './get_rule_by_rule_id'; @@ -70,10 +71,13 @@ export const upgradePrebuiltRule = async ({ // Else, recreate the rule from scratch with the passed payload. const updatedRule = await applyRuleUpdate({ - prebuiltRuleAssetClient, existingRule, ruleUpdate: ruleAsset, }); + updatedRule.rule_source = await calculateRuleSource({ + rule: updatedRule, + prebuiltRuleAssetClient, + }); const updatedInternalRule = await rulesClient.update({ id: existingRule.id,