[Security Solution][Detections] Handle dupes when processing threshold rules #83062

madirey · 2020-11-10T14:53:26Z

Summary

This PR makes changes to processing of threshold rules in order to prevent duplicates when evaluating thresholds over long lookback intervals. To do so, we query for any signals that have already been created on the interval, and exclude those buckets from the search during the timespan that the signal(s) already covered.

This builds upon the work done in #82444.

To address in next PR

Handle merging of signal subfields (see [Security Solution][Detections] Handle dupes when processing threshold rules #83062 (comment) and [Security Solution][Detections] Handle dupes when processing threshold rules #83062 (comment))
Move monstrous chunk of threshold bucket processing code to a separate function (see [Security Solution][Detections] Handle dupes when processing threshold rules #83062 (comment))

Checklist

Unit or functional tests were updated or added to match the most common scenarios

There are currently very few tests for threshold rules. More tests coming in follow-up PR.

For maintainers

This was checked for breaking API changes and was labeled appropriately

madirey · 2020-11-16T14:45:27Z

@marshallmain Is there any way we can force this new mapping to get applied, or are we dependent on the migration work? <----- @rylnd

Also, any opinions on this new approach? Thanks!

marshallmain · 2020-11-16T14:53:47Z

It'll be applied automatically if you bump the version number in https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts#L10. I think adding the new fields is fine with a version bump and no reindex - I would just leave the original threshold_count field as well so we don't risk any mapping complaints from ES.

x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/signals_mapping.json

rylnd

I had a few architectural questions but nothing that should necessarily block this being merged. Code looks good and we've talked through the logic a few times, but I'd definitely appreciate @marshallmain taking another pass here too if possible 😉 .

rylnd · 2020-11-25T22:23:48Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/build_bulk_body.ts

@@ -71,6 +71,7 @@ export const buildBulkBody = ({
    ...buildSignal([doc], rule),
    ...additionalSignalFields(doc),
  };
+  delete doc._source.threshold_result;


Does this merit a comment about preventing the "meta signals pollution"? Or should we make this more implicit by moving this into a simple filtering function, for now?

This is really about persisting values that belong to the signal before the the signal object has been created yet. I like the idea of using a filter, or even maybe architecting this a little differently so that the signal fields can be merged into the signal object, rather than having this function create the signal object. Will look at this in a follow-up PR.

rylnd · 2020-11-25T22:47:23Z

...ugins/security_solution/server/lib/detection_engine/signals/bulk_create_threshold_signals.ts

@@ -176,7 +179,10 @@ const getTransformedHits = (

        const source = {
          '@timestamp': get(timestampOverride ?? '@timestamp', hit._source),
-          threshold_count: docCount,
+          threshold_result: {


My understanding is that these "temporary" fields (@timestamp, threshold_result) are not intended to be persisted in this format to the final signal, but instead are used to pass context from the query to the signal generation. Had you considered making this process more explicit by nesting these fields in some declarative key like searchContext ?

This seems indicative of some broader architectural issues: getThresholdSignalQueryFields seems analogous to additionalSignalFields except that it exists at a different level, but I could absolutely see it being moved up as additionalSourceFields and similarly taking from _source.searchContext.

I know this isn't new code so definitely a NIT on the above; just curious what your thoughts are here.

So I think I can address this with the signal properties merging code discussed above. getThresholdSignalQueryFields is going away, per our discussion last week. Those fields are just copies of the data that can be reconstructed using signal.rule.query and signal.rule.filter combined with the "searchContext" contained in threshold_result. It may be cumbersome to get them all into one searchContext object, but I'll take a look at it. The query and filter already exist in the signal.rule section of the signal, while threshold_result belongs to the signal itself (it's computed after the data are aggregated). I think this can be cleaned up a bit though, so I'll see what I can do, and then run it by you. Thanks!

rylnd · 2020-11-25T22:51:28Z

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts

+            buildRuleMessage,
+          });
+
+          previousSignals.aggregations.threshold.buckets.forEach((bucket: ThresholdQueryBucket) => {


Could we move this logic to a function? We've got similar filter-generating functions and this file is already chonky.

madirey · 2020-11-30T01:18:06Z

@elasticmachine merge upstream

kibanamachine · 2020-11-30T03:00:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a855a19

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	43133	43134	+1

History

💚 Build #90327 succeeded 2c61c26
💚 Build #90315 succeeded fe956ad
💚 Build #89989 succeeded e5f2593
💚 Build #89848 succeeded 1e8d122
💔 Build #89835 failed 23e4ec3

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

…d rules (elastic#83062) * Fix threshold rule synthetic signal generation * Use top_hits aggregation * Find signals and aggregate over search terms * Exclude dupes * Fixes to algorithm * Sync timestamps with events/signals * Add timestampOverride * Revert changes in signal creation * Simplify query, return 10k buckets * Account for when threshold.field is not supplied * Ensure we're getting the last event when threshold.field is not provided * Add missing import * Handle case where threshold field not supplied * Fix type errors * Handle non-ECS fields * Regorganize * Address comments * Fix type error * Add unit test for buildBulkBody on threshold results * Add threshold_count back to mapping (and deprecate) * Timestamp fixes Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…d rules (#83062) (#84466) * Fix threshold rule synthetic signal generation * Use top_hits aggregation * Find signals and aggregate over search terms * Exclude dupes * Fixes to algorithm * Sync timestamps with events/signals * Add timestampOverride * Revert changes in signal creation * Simplify query, return 10k buckets * Account for when threshold.field is not supplied * Ensure we're getting the last event when threshold.field is not provided * Add missing import * Handle case where threshold field not supplied * Fix type errors * Handle non-ECS fields * Regorganize * Address comments * Fix type error * Add unit test for buildBulkBody on threshold results * Add threshold_count back to mapping (and deprecate) * Timestamp fixes Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: [Security Solution] Exceptions Cypress tests (elastic#81759) [ML] Fix spaces job ID check (elastic#84404) [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062) skip flaky suite (elastic#84440) skip flaky suite (elastic#84445) [APM] Fix missing `service.node.name` (elastic#84269) Upgrade fp-ts to 2.8.6 (elastic#83866) Added data streams privileges to better control delete actions in UI (elastic#83573) Improve short-url redirect validation (elastic#84366) TSVB offsets (elastic#83051) [Discover] Fix navigating back when changing index pattern (elastic#84061) [Logs UI] Polish the UI for the log entry examples in the anomaly table (elastic#82139) [Logs UI] Limit the height of the "view in context" container (elastic#83178) [Application Usage] Update `schema` with new `fleet` rename (elastic#84327) fix identation in list (elastic#84301)

…bana into add-metadata-to-node-details * 'add-metadata-to-node-details' of github.com:phillipb/kibana: [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended (elastic#84415) Support for painless language autocomplete within monaco (elastic#80577) [Lens] Time scale ui (elastic#83904) removing beta callouts (elastic#84510) [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493) Trusted Apps signer API. (elastic#83661) increase stdout max listeners for legacy logging (elastic#84497) [APM] Service overview: Add throughput chart (elastic#84439) [Discover] Unskip main functional tests (elastic#84300) Uptime overview overhaul (elastic#83406) [APM] Adjust time formats based on the difference between start and end (elastic#84470) [ML] Renaming saved object repair to sync (elastic#84311) [UsageCollection] Remove `formatBulkUpload` and other unused APIs (elastic#84313) [Visualizations] Adds visConfig.title and uiState to build pipeline function (elastic#84456) [Elasticsearch Migration] Update docs re UsageCollection (elastic#84322) TSVB field list performance issue on using annotations (elastic#84407) [Security Solution] Exceptions Cypress tests (elastic#81759) [ML] Fix spaces job ID check (elastic#84404) [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062)

madirey added 9 commits November 3, 2020 15:13

Fix threshold rule synthetic signal generation

7ac4f7d

Merge branch 'master' of github.com:elastic/kibana into cidr-house-rules

fba2636

Merge branch 'master' of github.com:elastic/kibana into cidr-house-rules

352a139

Use top_hits aggregation

0c9d5e3

Find signals and aggregate over search terms

2f61b30

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

56e558a

Exclude dupes

4610643

Fixes to algorithm

3fa0e69

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

0c3c1e7

madirey added release_note:enhancement v8.0.0 Feature:Detection Rules Security Solution rules and Detection Engine Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. 7.11.0 labels Nov 10, 2020

madirey requested review from a team as code owners November 10, 2020 14:53

madirey mentioned this pull request Nov 10, 2020

[Security Solution][Detections] Handle dupes when processing threshold rules #81798

Closed

8 tasks

madirey added 13 commits November 10, 2020 15:17

Sync timestamps with events/signals

35e2119

Merge branch 'master' of github.com:elastic/kibana into cidr-house-rules

29641b6

Add timestampOverride

5914e99

Merge branch 'cidr-house-rules' into threshold-dupes

26c7c0d

Revert changes in signal creation

7bbd12a

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

399d886

Simplify query, return 10k buckets

960ed9b

Account for when threshold.field is not supplied

8591886

Merge branch 'master' of github.com:elastic/kibana into cidr-house-rules

a90af11

Ensure we're getting the last event when threshold.field is not provided

bf910ab

Merge branch 'cidr-house-rules' into threshold-dupes

2396190

Add missing import

83f81cb

Merge branch 'cidr-house-rules' into threshold-dupes

3052975

madirey added 3 commits November 12, 2020 15:44

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

925acc7

Handle non-ECS fields

ae24022

Regorganize

2c8dcfa

spong requested a review from a team November 13, 2020 03:17

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

8a7c84a

madirey added 7 commits November 20, 2020 18:08

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

49044a6

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

974a5a3

Address comments

23e4ec3

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

2bc904f

Fix type error

1e8d122

Add unit test for buildBulkBody on threshold results

3a54495

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

e5f2593

rylnd reviewed Nov 24, 2020

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/routes/index/signals_mapping.json Show resolved Hide resolved

madirey added 3 commits November 25, 2020 19:50

Add threshold_count back to mapping (and deprecate)

80a582a

Merge branch 'master' of github.com:elastic/kibana into threshold-dupes

fe956ad

Timestamp fixes

2c61c26

rylnd approved these changes Nov 25, 2020

View reviewed changes

Merge branch 'master' into threshold-dupes

a855a19

madirey merged commit bdf7b88 into elastic:master Nov 30, 2020

madirey deleted the threshold-dupes branch November 30, 2020 03:10

madirey mentioned this pull request Nov 30, 2020

[7.x] [Security Solution][Detections] Handle dupes when processing threshold rules (#83062) #84466

Merged

spong added the Feature:Threshold Rule Security Solution Threshold rule type label Dec 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Handle dupes when processing threshold rules #83062

[Security Solution][Detections] Handle dupes when processing threshold rules #83062

madirey commented Nov 10, 2020 •

edited

Loading

madirey commented Nov 16, 2020

marshallmain commented Nov 16, 2020

rylnd left a comment

rylnd Nov 25, 2020

madirey Nov 30, 2020

rylnd Nov 25, 2020

rylnd Nov 25, 2020

madirey Nov 30, 2020

rylnd Nov 25, 2020

madirey Nov 30, 2020

madirey commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

[Security Solution][Detections] Handle dupes when processing threshold rules #83062

[Security Solution][Detections] Handle dupes when processing threshold rules #83062

Conversation

madirey commented Nov 10, 2020 • edited Loading

Summary

To address in next PR

Checklist

For maintainers

madirey commented Nov 16, 2020

marshallmain commented Nov 16, 2020

rylnd left a comment

Choose a reason for hiding this comment

rylnd Nov 25, 2020

Choose a reason for hiding this comment

madirey Nov 30, 2020

Choose a reason for hiding this comment

rylnd Nov 25, 2020

Choose a reason for hiding this comment

rylnd Nov 25, 2020

Choose a reason for hiding this comment

madirey Nov 30, 2020

Choose a reason for hiding this comment

rylnd Nov 25, 2020

Choose a reason for hiding this comment

madirey Nov 30, 2020

Choose a reason for hiding this comment

madirey commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

madirey commented Nov 10, 2020 •

edited

Loading