[Security Solution][Detections] Performance enhancement for readRules function #76398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...lugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.test.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/basic/tests/import_rules.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts
Outdated
Show resolved
Hide resolved
dhurley14
force-pushed
the
remove-file-validation-import-rule
branch
2 times, most recently
from
bdd9e31 to
f352b89
Compare
dhurley14
commented
Sep 9, 2020
| @@ -12,7 +12,7 @@ export const getFilter = (filter: string | null | undefined) => { | |||
| if (filter == null) { | |||
| return `alert.attributes.alertTypeId: ${SIGNALS_ID}`; | |||
| } else { | |||
| return `alert.attributes.alertTypeId: ${SIGNALS_ID} AND ${filter}`; | |||
| return `alert.attributes.alertTypeId: ${SIGNALS_ID} AND (${filter})`; | |||
There was a problem hiding this comment.
adding parenthesis due to OR joins in KQL passed to alerts client find here -> https://github.com/elastic/kibana/pull/76398/files#diff-a8b6548cd6fceb9a3bb78453de9e2d17R44-R45
dhurley14
commented
Sep 9, 2020
Comment on lines
19
to
+23
| export const readRules = async ({ | ||
| alertsClient, | ||
| id, | ||
| ruleId, | ||
| }: ReadRuleOptions): Promise<SanitizedAlert | null> => { | ||
| ruleIds, | ||
| }: ReadRuleOptions): Promise<RuleAlertType[] | null> => { |
There was a problem hiding this comment.
readRules currently takes either a single id which is used on the alerts client get api or an array of ruleIds which are used on the find api. I'm wondering if there is impetus for re-working the function to also accept an array of ids in addition to this array of ruleIds?
There was a problem hiding this comment.
That makes sense to me. Has it just not yet been the case to need to pass in an array of ids?
There was a problem hiding this comment.
I'll work on refactoring to take in an array of ids as well. I wasn't sure if that would result in me touching too much stuff in this PR. But I think it'll help to have the same interface between the two parameters.
There was a problem hiding this comment.
since the bug was only related to the kql filter in the alerts client find api and not their get api I don't think it's worth also refactoring the readRules function to accept an array of ids. This is probably tech debt now that should be resolved in 7.10.
dhurley14
commented
Sep 9, 2020
| ruleFromFind.data.length > 0 && | ||
| rules.every((rule): rule is RuleAlertType => isAlertType(rule)) | ||
| ) { | ||
| return rules; |
There was a problem hiding this comment.
I also debated returning an array of found rules or an object of found rules where the keys are the ruleIds. Open to hearing opinions on this too.
There was a problem hiding this comment.
Looking through these changes it seems like returning an array makes sense. Seems like we're either a) passing in a bunch of ids and using all the rules returned or b) just accessing the first item in the array on none bulk actions. I could see the object being useful if we were having to constantly loop through the rules to find x rule, which I don't think is the case? Just saying that based off this PR not having looked at all the various use cases, so could be totally wrong! What tradeoffs were you considering for each?
There was a problem hiding this comment.
I was just thinking since we do iterate over the array returned by the readRules function pretty heavily it might make things faster if each .find was replaced with foundRuleObject[myruleId] which would result in faster checking for the existence of the rule returned by this function. I think that would be cleaner.
|
Pinging @elastic/siem (Team:SIEM) |
madirey
reviewed
Sep 12, 2020
| if (rule != null) { | ||
| if ( | ||
| foundRules != null && | ||
| foundRules.length > 0 && |
There was a problem hiding this comment.
Super nit, but I think this length check is unnecessary?
madirey
reviewed
Sep 12, 2020
| @@ -187,7 +196,11 @@ export const importRulesRoute = (router: IRouter, config: ConfigType, ml: SetupP | |||
|
|
|||
| throwHttpError(await mlAuthz.validateRuleType(type)); | |||
|
|
|||
| const rule = await readRules({ alertsClient, ruleId, id: undefined }); | |||
| let rule; | |||
There was a problem hiding this comment.
You could get rid of this let by doing something like: const rule = rules?.find((aRule) => aRule.params.ruleId === ruleId);
madirey
reviewed
Sep 12, 2020
| const existingRule = await readRules({ alertsClient, ruleId, id }); | ||
| if (existingRule?.params.type) { | ||
| const foundRule = foundRules?.find((rule) => rule.params.ruleId === ruleId); | ||
| let existingRule = foundRule != null ? [foundRule] : null; |
There was a problem hiding this comment.
You could also combine these statements to get rid of the let ...
There was a problem hiding this comment.
Do we even need this check here since .find will return undefined if none found which would match up agains our == null or != null checks?
There was a problem hiding this comment.
We do need the check there since the type would be RuleAlertType | RuleAlertType[] | null which would cause the below checks to increase in complexity since we'd have to check if it's an array first before attempting to access etc.. But thanks @madirey for the suggestion! I will combine the two.
yctercero
approved these changes
Sep 14, 2020
There was a problem hiding this comment.
Thanks so much for these fixes/updates! I pulled down and tested basic CRUD with rules, importing 200 rules, deleting 200 rules - it all looked good to me. The speeds were significantly faster on 7.9 cloud instance, but @dhurley14 pointed out that this is pre new rbac changes. Between pulling PR down and comparing it to master locally, times were pretty consistent. Just left some super nit comments, nothing that would block PR from going in.
| if ( | ||
| foundRules != null && | ||
| foundRules.length > 0 && | ||
| foundRules.find((rule) => rule.params.ruleId === ruleId) |
There was a problem hiding this comment.
Super duper nit: would .some() fit in a bit better here since it explicitly returns a boolean?
| @@ -135,8 +140,11 @@ export const createRulesBulkRoute = (router: IRouter, ml: SetupPlugins['ml']) => | |||
| }); | |||
| } | |||
| if (ruleId != null) { | |||
| const rule = await readRules({ alertsClient, ruleId, id: undefined }); | |||
| if (rule != null) { | |||
| if ( | |||
There was a problem hiding this comment.
Super nit: can the if blocks on 142 and 143 be merged?
| const existingRule = await readRules({ alertsClient, ruleId, id }); | ||
| if (existingRule?.params.type) { | ||
| const foundRule = foundRules?.find((rule) => rule.params.ruleId === ruleId); | ||
| let existingRule = foundRule != null ? [foundRule] : null; |
There was a problem hiding this comment.
Do we even need this check here since .find will return undefined if none found which would match up agains our == null or != null checks?
| @@ -104,10 +104,14 @@ export const patchRulesRoute = (router: IRouter, ml: SetupPlugins['ml']) => { | |||
| throwHttpError(await mlAuthz.validateRuleType(type)); | |||
| } | |||
|
|
|||
| const existingRule = await readRules({ alertsClient, ruleId, id }); | |||
| if (existingRule?.params.type) { | |||
| const existingRules = await readRules({ | |||
There was a problem hiding this comment.
Super nit: seems like theres some repeated logic being used in the routes, is it worth pulling out somewhere? Maybe not since they're small chunks.
There was a problem hiding this comment.
yeah I think we could pull out the logic for rejecting unauthorized modification of an ML rule into a little function. That's a good idea. Not sure if that refactor would be a good fit for a minor release like this PR but that would clean up the code nicely. I'll take a look.
Comment on lines
19
to
+23
| export const readRules = async ({ | ||
| alertsClient, | ||
| id, | ||
| ruleId, | ||
| }: ReadRuleOptions): Promise<SanitizedAlert | null> => { | ||
| ruleIds, | ||
| }: ReadRuleOptions): Promise<RuleAlertType[] | null> => { |
There was a problem hiding this comment.
That makes sense to me. Has it just not yet been the case to need to pass in an array of ids?
| ruleFromFind.data.length > 0 && | ||
| rules.every((rule): rule is RuleAlertType => isAlertType(rule)) | ||
| ) { | ||
| return rules; |
There was a problem hiding this comment.
Looking through these changes it seems like returning an array makes sense. Seems like we're either a) passing in a bunch of ids and using all the rules returned or b) just accessing the first item in the array on none bulk actions. I could see the object being useful if we were having to constantly loop through the rules to find x rule, which I don't think is the case? Just saying that based off this PR not having looked at all the various use cases, so could be totally wrong! What tradeoffs were you considering for each?
madirey
approved these changes
Sep 15, 2020
|
Hi @dhurley14 We have gone through the ticket and observed it required DEV_Validation to test the ticket . thanks !! |
|
@elasticmachine merge upstream |
dhurley14
force-pushed
the
remove-file-validation-import-rule
branch
2 times, most recently
from
35c3bb7 to
ec5ce0c
Compare
|
@elasticmachine merge upstream |
|
jenkins test this |
…quest, adds new functional tests and unit tests to make sure we can reach and don't go past bounds. These tests would have helped uncover performance issues io-ts gave us with validating the import rules file object
…param defaulted to false
dhurley14
force-pushed
the
remove-file-validation-import-rule
branch
from
4a0e8dc to
377b543
Compare
💛 Build succeeded, but was flaky
Test FailuresX-Pack Endpoint Functional Tests.x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list·ts.endpoint endpoint list when there is data, when the hostname is clicked on, navigates to ingest fleet when the Reassign Policy link is clickedStandard OutStack TraceBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
2 tasks
dhurley14
changed the title
|
Closing since #77040 fixes the underlying performances issues. Tested this with importing 200+ rules and they imported just as fast as on 7.9 so I don't think these changes are needed right now. Maybe in the future this PR will help clean up the code and reduce network calls but for now I will be closing it to continue on higher priority items for the next release. |
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
UPDATE: splitting these changes into two PR's. The second bug reported below is only a problem in 7.10.0 which has not been released yet. I will separate out the validation bug from the changes to
readRulesso as to not touch too many things at once since we need the validation bug backported to 7.9.2.Fixes two bugs.
io-ts validation on the rule import file object was creating performance issues. We replace this validation with kbn schema.any() since the validation on the rules occurs later in the route handler.This is now fixed in [Security Solution] [Detections] Remove file validation on import route #77770The second bug was related to performance issues discovered in the find api from the alerts client (https://github.com/elastic/security-team/issues/190, KQL expression parsing is slow #76811). The suggested resolution was to pass in many
ruleIdsinto the alerts client find api to create a kind of "bulk find". This implements that suggestion and creates the necessary changes to the other routes that utilize the readRules function (which uses find under-the-hood.) Without this change I couldn't even run the functional test where we create 1000 rules, as Elasticsearch would throw a timeout error. With the "batching" of these ruleIds into a single find call, we can now import 1000 rules in about 11 seconds.Checklist
Delete any items that are not applicable to this PR.
For maintainers