[Security Solution] adds WrapSequences method (RAC) by ecezalp · Pull Request #102106 · elastic/kibana

ecezalp · 2021-06-14T17:42:10Z

Summary

Adds a wrapSequencesFactory method for the EQL rule implementation.
Passes isRuleRegistryEnabled flag from plugin to signalRuleAlertType.

Checklist

Delete any items that are not applicable to this PR.

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

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

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-06-14T19:15:16Z

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

ecezalp · 2021-06-15T22:31:05Z

@elasticmachine merge upstream

FrankHassanabad · 2021-06-16T15:00:51Z

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

-) => Array<BaseHit<{ '@timestamp': string }>>;
+export type SimpleHit = BaseHit<{ '@timestamp': string }>;
+
+export type WrapHits = (hits: Array<estypes.SearchHit<unknown>>) => SimpleHit[];


Can this unknown be typed and then we avoid the as casting below.

_source: buildBulkBody(ruleSO, doc as SignalSourceHit),

FrankHassanabad · 2021-06-16T15:01:23Z

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

+        doc._id,
+        doc._version ? doc._version.toString() : '',
+        ruleSO.attributes.params.ruleId ?? ''
+      ),


Is it possible to type the line below in the file above and then we avoid this as cast?

_source: buildBulkBody(ruleSO, doc as SignalSourceHit),

FrankHassanabad · 2021-06-16T15:04:13Z

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

+}): WrapSequences => (sequences) => {
+  const wrappedDocs = sequences.reduce(
+    (acc: WrappedSignalHit[], sequence) =>
+      acc.concat(buildSignalGroupFromSequence(sequence, ruleSO, signalsIndex)),


nit: can we do a splat and avoid the concat here? Something like this? (warning I did not test this, but this is usually the format I see in the code base).

[...acc, buildSignalGroupFromSequence(sequence, ruleSO, signalsIndex)]

FrankHassanabad · 2021-06-16T15:17:04Z

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

+      _id: generateId(
+        doc._index,
+        doc._id,
+        doc._version ? doc._version.toString() : '',


fwiw a lot of people use the constructor variant such as String(doc._version) because it's safer if someone changes this code and accidentally pushes in a non string.

One other thing is that I do not think the doc._version is the best usage of identifying unique document changes anymore within elastic.

In Saved Objects and other places if we are trying to identify if a document has changed and key off of that it's better to use _seq_no and _primary_term

Ref: https://www.elastic.co/guide/en/elasticsearch/reference/current/optimistic-concurrency-control.html

The caveat being we have to ensure we are passing the correct flags to return these two fields with our search results which you would have to check. You could fall back on doc._version if you do not see them being set to double check things or write an e2e test to ensure we do return them as expected as well at some point.

However, overall I think we have older deprecated patterns of using doc._version instead of using those two other fields and should prefer using those.

Let me know if that doesn't sound right.

just pushed some changes and updated it as String(doc._version) for this moment. It looks like we would need to use the seq_no_primary_term flag, I haven't found out exactly how just yet, looking into it.

created #102395

spalger · 2021-06-16T20:22:37Z

jenkins, test this

(restarting due to jenkins upgrade)

ecezalp · 2021-06-17T01:37:17Z

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/generating_signals.ts

            {
              rule: signalNoRule.parents[0].rule, // rule id is always changing so skip testing it
-              id: 'b63bcc90b9393f94899991397a3c2df2f3f5c6ebf56440434500f1e1419df7c9',
+              id: signalNoRule.parents[0].id,


I wasn't exactly sure why this started braking after pulling in master - noticing the comment a line above, I used the same strategy, please confirm if this is an appropriate fix

@ecezalp Did the ID change because the _id calculation in wrap_hits_factory changed to include the version? I suspect that's the case. If so, we probably want to hard-code the new calculated id here, as it should be stable.

uhh that makes sense. I updated the ids. locally I got a 409 error which I thought was strange considering that I was just updating the comparison object used in the assertion, I pushed the change anyway, hopefully it should work as expected.

kibanamachine · 2021-06-17T20:52:36Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #132127 succeeded efa0b3f
💔 Build #132042 failed 65bcd0f
💔 Build #131935 failed c1b57f6
💔 Build #131865 failed 02056f3
💔 Build #131821 failed 8081fa0

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

cc @ecezalp

kibanamachine · 2021-06-17T20:59:30Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 102106

…ets-tab * 'master' of github.com:elastic/kibana: (93 commits) [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506) [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384) [ML] Anomaly detection job custom_settings improvements (elastic#102099) [Cases] Route: Get all alerts attach to a case (elastic#101878) Fixes wrong list exception type when creating endpoint event filters list (elastic#102522) remove search bar that's not working yet (elastic#102550) Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409) [Maps] clean up feature editing name space to avoid conflicts with layer settings editing (elastic#102516) [canvas] Refactor Storybook from bespoke to standard configuration (elastic#101962) [Security Solution] adds wrapSequences method (RAC) (elastic#102106) [FTR] Stabilize SSLP functional tests (elastic#102553) [K8] Added `Inter` font files for new theme (elastic#102359) [Workplace Search] Convert Groups pages to new page template (elastic#102449) [DOC] Add experimental disclaimer to rollup jobs (elastic#95624) [Security Solution][Endpoint] Suppress some of the jest console.error noise created by endpoint list middelware (elastic#102535) [Fleet] Improve performance of Fleet setup (elastic#102219) [Alerting] Add event log entry when a rule starts executing (elastic#102001) [Fleet] Update docker image of registry used in integration tests (elastic#101911) [Asset Management] Osquery telemetry updates (elastic#100754) Converts saved object tagging to new management layout (elastic#102284) ... # Conflicts: # x-pack/plugins/fleet/kibana.json

kibanamachine · 2021-06-21T20:58:16Z