[Security Solution][Detections] Fix flaky indicator enrichment tests #94241

rylnd · 2021-03-09T23:58:01Z

Summary

Addresses #93152.

Due to the fact that we use named queries to determine matches, and the fact that the order in which named queries are returned is undefined, (as observed in the output of the test runs here), we cannot guarantee a consistent ordering of enrichments if a given event matches multiple named queries.

Because the ordering is not in itself important to enrichment, in order to assert the multi-match functionality we must make the assertions order independent.

This robustness is verified via this build, where enrichment tests were run 20 times without failure. Prior to this change, ~50% of those runs would fail.

Checklist

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

Due to the fact that we use named queries to determine matches, and the fact that the order in which named queries are returned is undefined, we cannot guarantee a consistent ordering of enrichments if a given event matches multiple named queries. Because the ordering is not in itself important to enrichment, in order to assert the multi-match functionality we must make the assertions order independent.

elasticmachine · 2021-03-10T02:11:31Z

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

elasticmachine · 2021-03-10T02:11:32Z

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

banderror

Thank you for making our tests better!
LGTM, I just left 2 small comments which shouldn't block this PR though. 👍

banderror · 2021-03-10T14:23:45Z

...ck/test/detection_engine_api_integration/security_and_spaces/tests/create_threat_matching.ts

+// Asserts that each expected value is included in the subject, independent of
+// ordering. Uses _.isEqual for value comparison.
+const assertContains = (subject: unknown[], expected: unknown[]) =>
+  expected.map((expectedValue) =>
+    expect(subject.some((value) => isEqual(value, expectedValue))).to.eql(
+      true,
+      `expected ${format(subject)} to contain ${format(expectedValue)}`
+    )


Super nit: expected.map could be replaced by expected.forEach

banderror · 2021-03-10T14:30:15Z

...ck/test/detection_engine_api_integration/security_and_spaces/tests/create_threat_matching.ts

+          assertContains(threat.indicator, [
            {
-              indicator: [
-                {
-                  description: 'this should match auditbeat/hosts on both port and ip',
-                  first_seen: '2021-01-26T11:06:03.000Z',
-                  ip: '45.115.45.3',
-                  matched: {
-                    atomic: '45.115.45.3',
-                    id: '978785',
-                    index: 'filebeat-8.0.0-2021.01.26-000001',
-                    field: 'source.ip',
-                    type: 'url',
-                  },
-                  port: 57324,
-                  provider: 'geenensp',
-                  type: 'url',
-                },
-                {
-                  description: 'this should match auditbeat/hosts on ip',
-                  first_seen: '2021-01-26T11:06:03.000Z',
-                  ip: '45.115.45.3',
-                  matched: {
-                    atomic: '45.115.45.3',
-                    id: '978787',
-                    index: 'filebeat-8.0.0-2021.01.26-000001',
-                    field: 'source.ip',
-                    type: 'ip',
-                  },
-                  provider: 'other_provider',
-                  type: 'ip',
-                },
-              ],
+              description: 'this should match auditbeat/hosts on both port and ip',
+              first_seen: '2021-01-26T11:06:03.000Z',
+              ip: '45.115.45.3',
+              matched: {
+                atomic: '45.115.45.3',
+                id: '978785',
+                index: 'filebeat-8.0.0-2021.01.26-000001',
+                field: 'source.ip',
+                type: 'url',
+              },
+              port: 57324,
+              provider: 'geenensp',
+              type: 'url',
+            },
+            {
+              description: 'this should match auditbeat/hosts on ip',
+              first_seen: '2021-01-26T11:06:03.000Z',
+              ip: '45.115.45.3',
+              matched: {
+                atomic: '45.115.45.3',
+                id: '978787',
+                index: 'filebeat-8.0.0-2021.01.26-000001',
+                field: 'source.ip',
+                type: 'ip',
+              },
+              provider: 'other_provider',
+              type: 'ip',


If I got it right, these tests load some events from auditbeat/hosts ES archive, create a rule, generate signals and then check that signals have certain fields (set by the enrichment process in the rule?) like matched.*.

I think my question is: how stable or brittle these tests are in terms of input data (events being tested)? How likely it is if someone updates this ES archive with new events, and these tests will break with false negatives although the enrichment functionality stays correctly implemented?

Good question! My understanding is that those archives are stable, and that for e.g. auditbeat-7.13 one would create a new archive/tests rather than update the existing one.

The tests are definitely brittle in terms of these data changing, but I believe that most if not all of our event-based integration tests share this property. Most of the other "create rule, assert on alerts" tests like this are reliant on the same archives, it just happened that these tests were brittle due to the nondeterministic ordering of named queries.

My understanding is that those archives are stable, and that for e.g. auditbeat-7.13 one would create a new archive/tests rather than update the existing one.

Oh sorry, I was assuming they could be updated from time to time. :) Good to know that it's not the case. Thanks for your explanation @rylnd 👍

* Since we're only looping for side effects, prefer forEach to map for more idiomatic FP.

kibanamachine · 2021-03-10T20:56:27Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #112327 succeeded 09050ce

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

cc @rylnd

…lastic#94241) * Make indicator enrichment tests order-independent Due to the fact that we use named queries to determine matches, and the fact that the order in which named queries are returned is undefined, we cannot guarantee a consistent ordering of enrichments if a given event matches multiple named queries. Because the ordering is not in itself important to enrichment, in order to assert the multi-match functionality we must make the assertions order independent. * PR feedback * Since we're only looping for side effects, prefer forEach to map for more idiomatic FP.

…94241) (#94374) * Make indicator enrichment tests order-independent Due to the fact that we use named queries to determine matches, and the fact that the order in which named queries are returned is undefined, we cannot guarantee a consistent ordering of enrichments if a given event matches multiple named queries. Because the ordering is not in itself important to enrichment, in order to assert the multi-match functionality we must make the assertions order independent. * PR feedback * Since we're only looping for side effects, prefer forEach to map for more idiomatic FP.

…94241) (#94375) * Make indicator enrichment tests order-independent Due to the fact that we use named queries to determine matches, and the fact that the order in which named queries are returned is undefined, we cannot guarantee a consistent ordering of enrichments if a given event matches multiple named queries. Because the ordering is not in itself important to enrichment, in order to assert the multi-match functionality we must make the assertions order independent. * PR feedback * Since we're only looping for side effects, prefer forEach to map for more idiomatic FP.

…-action * 'master' of github.com:elastic/kibana: (43 commits) [Console] Update copy when showing warnings in response headers (#94270) [TSVB] Type public code. Step 1 (#93231) [ML] Functional tests - stabilize slider value selection (#94313) skip another suite blocking es promotion (#94367) [Security Solution] Eliminates a redundant external link icon (#94194) skip another suite blocking es promotion (#94367) [App Search] Role mappings migration part 1 (#94346) [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241) [Workplace Search] Deduplicate icons (#94359) [ML] Add latest transform to intro text (#94039) skip test failing es promotion (#94367) [Maps] convert elasticsearch_utils to TS (#93984) [Security_Solution][Telemetry] - Update endpoint usage to use agentService (#93829) [Security Solution][Exceptions] Fixes OS adding method for exception enrichment (#94343) [ILM] Add support for frozen phase (#93068) [App Search] Fixed 2 relevance tuning bugs (#94312) remove `try` auth mode (#94287) Removing resolver functional tests (#94331) migrate warning mixin to core (#94273) [App Search] Add routes for Role Mappings (#94221) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_issues_context.tsx

rylnd added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Mar 9, 2021

rylnd self-assigned this Mar 9, 2021

rylnd marked this pull request as ready for review March 10, 2021 02:11

rylnd requested a review from a team as a code owner March 10, 2021 02:11

banderror approved these changes Mar 10, 2021

View reviewed changes

PR feedback

091050d

* Since we're only looping for side effects, prefer forEach to map for more idiomatic FP.

rylnd merged commit 5c352ca into elastic:master Mar 10, 2021

rylnd deleted the fix_indicator_match_tests branch March 10, 2021 22:00

rylnd mentioned this pull request Mar 10, 2021

[7.x] [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241) #94374

Merged

rylnd mentioned this pull request Mar 10, 2021

[7.12] [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241) #94375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Fix flaky indicator enrichment tests #94241

[Security Solution][Detections] Fix flaky indicator enrichment tests #94241

rylnd commented Mar 9, 2021

elasticmachine commented Mar 10, 2021

elasticmachine commented Mar 10, 2021

banderror left a comment

banderror Mar 10, 2021

banderror Mar 10, 2021

rylnd Mar 10, 2021

banderror Mar 11, 2021

kibanamachine commented Mar 10, 2021

[Security Solution][Detections] Fix flaky indicator enrichment tests #94241

[Security Solution][Detections] Fix flaky indicator enrichment tests #94241

Conversation

rylnd commented Mar 9, 2021

Summary

Checklist

For maintainers

elasticmachine commented Mar 10, 2021

elasticmachine commented Mar 10, 2021

banderror left a comment

Choose a reason for hiding this comment

banderror Mar 10, 2021

Choose a reason for hiding this comment

banderror Mar 10, 2021

Choose a reason for hiding this comment

rylnd Mar 10, 2021

Choose a reason for hiding this comment

banderror Mar 11, 2021

Choose a reason for hiding this comment

kibanamachine commented Mar 10, 2021

💚 Build Succeeded

Metrics [docs]

History