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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Better threshold rule error checking #131088

Merged
merged 12 commits into from
May 12, 2022
Merged

[Security Solution] Better threshold rule error checking #131088

merged 12 commits into from
May 12, 2022

Conversation

madirey
Copy link
Contributor

@madirey madirey commented Apr 27, 2022 •
edited
Loading

Summary

Threshold rule configuration allows the selection of a cardinality constraint for any field. But when the cardinality field is also a field that we're aggregating on, the cardinality is guaranteed to be 1 for each bucket. This adds 2 unnecessary aggregations to the query in this case (essentially no-ops) which negatively impact performance with no benefit.

This PR adds a check to ensure the above situation never happens. It also adds some additional server-side validation for cases that we were previously only checking in the UI.

Fixes #113587

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@madirey madirey added release_note:fix Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Threshold Rule Security Solution Threshold Rule feature Team:Detection Alerts Security Detection Alerts Area Team 8.3 candidate v8.3.0 labels Apr 27, 2022
@madirey madirey requested review from a team as code owners April 27, 2022 16:22
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@madirey
Copy link
Contributor Author

madirey commented Apr 28, 2022

@elasticmachine merge upstream

@madirey madirey changed the title [Security Solution] Better threshold rule error checking [Draft][Security Solution] Better threshold rule error checking May 3, 2022
@madirey madirey changed the title [Draft][Security Solution] Better threshold rule error checking [Security Solution] Better threshold rule error checking May 3, 2022
e40pud
e40pud reviewed May 6, 2022
View reviewed changes
...ty_solution/common/detection_engine/schemas/request/add_prepackaged_rules_type_dependents.ts
@@ -96,29 +96,39 @@ export const validateTimelineTitle = (rule: AddPrepackagedRulesSchema): string[]
};

export const validateThreshold = (rule: AddPrepackagedRulesSchema): string[] => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, if there is a way to extract validateThreshold check in a separate function which we could reuse in all five cases (add, create, import, patch and update)? They look same to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e40pud Yes, that is probably a good idea!

...ty_solution/common/detection_engine/schemas/request/add_prepackaged_rules_type_dependents.ts
@@ -96,29 +96,39 @@ export const validateTimelineTitle = (rule: AddPrepackagedRulesSchema): string[]
};

export const validateThreshold = (rule: AddPrepackagedRulesSchema): string[] => {
const errors: string[] = [];
if (isThresholdRule(rule.type)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we use both isThresholdRule(rule.type) and rule.type === 'threshold'. Are those different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e40pud Yeah, those could use some refactoring. They're essentially equivalent for now, but their usages have been inconsistent.

@madirey
Copy link
Contributor Author

madirey commented May 12, 2022

@elasticmachine merge upstream

@madirey madirey enabled auto-merge (squash) May 12, 2022 14:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@madirey madirey merged commit cddd41d into elastic:main May 12, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 12, 2022
@madirey madirey deleted the threshold-cardinality branch May 13, 2022 14:34
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request May 16, 2022
@madirey @kibanamachine @Bamieh
[Security Solution] Better threshold rule error checking (elastic#131088
e447261 
)

* Better threshold rule error checking

* Add more type dependent checks

* create/import/update/add-prepackaged

* Fix tests

* whoops

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e40pud e40pud e40pud left review comments

@marshallmain marshallmain marshallmain approved these changes

Assignees
No one assigned
Labels
8.3 candidate backport:skip This commit does not require backporting Feature:Threshold Rule Security Solution Threshold Rule feature release_note:fix Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security Solution] Threshold rule performance fixes
6 participants
@madirey @elasticmachine @kibana-ci @e40pud @marshallmain @kibanamachine