[Security Solution] Adds diff algorithm and unit tests for `query` fields #190179

dplumlee · 2024-08-08T19:04:16Z

Summary

Related ticket: #187658

Adds diff algorithm for all the grouped query fields we have in the DiffableRule type and prebuilt rule upgrade workflow. These include kql_query (both inline and saved query), eql_query, and esql_query. Also adds unit tests for all three algorithms.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-08-08T19:04:19Z

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

elasticmachine · 2024-08-08T19:04:19Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

elasticmachine · 2024-08-08T19:04:19Z

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

jpdjere · 2024-08-09T13:52:17Z

@dplumlee I'm gonna continue reviewing next week cause I gotta handle some SDHs today but:

the query field shouldn't be mergeable. We should treat it as a single line string field.
since filters is an array, I was thinking that we might be able to apply some more clever merging logic, to have better UX in a case, for example, where a user just adds a filter to a list of existing filters; and therefore not throwing a conflict but just merging the current and target versions. However:
- more complex scenarios will be increasingly difficult to decide what to do, since each filter is a really complex object
- there's only 15 prebuilt rules that have filters in their definitions. It looks it doesn't make a lot of sense to put a lot of effort into this right now.

So let's just use the simple diffing algorithm for the filters field for now, as you are already doing. I created a ticket so we can revisit this in the future.

jpdjere · 2024-09-09T19:38:49Z

...ion_engine/prebuilt_rules/logic/diff/calculation/algorithms/kql_query_diff_algorithm.test.ts

+      it('returns target_version as merged output if all versions are the same', () => {
+        const mockVersions: ThreeVersionsOf<RuleKqlQuery> = {
+          base_version: MissingVersion,
+          current_version: {
+            type: KqlQueryType.inline_query,
+            query: 'query string = true',
+            language: KqlQueryLanguageEnum.kuery,
+            filters: [],
+          },
+          target_version: {
+            type: KqlQueryType.inline_query,
+            query: 'query string = false',
+            language: KqlQueryLanguageEnum.kuery,
+            filters: [],
+          },
+        };
+
+        const result = kqlQueryDiffAlgorithm(mockVersions);
+
+        expect(result).toEqual(
+          expect.objectContaining({
+            has_base_version: false,
+            base_version: undefined,
+            merged_version: mockVersions.target_version,
+            diff_outcome: ThreeWayDiffOutcome.MissingBaseCanUpdate,
+            merge_outcome: ThreeWayMergeOutcome.Target,
+            conflict: ThreeWayDiffConflict.SOLVABLE,
+          })
+        );
+      });
+
+      it('returns target_version as merged output if all versions are different', () => {


Not sure I understand these two scenarios as described here:

returns target_version as merged output if all versions are the same: Do you mean here if current and target versions have the same type?

returns target_version as merged output if all versions are different: Did you mean here if current and target versions have different type? In that case it doesn't match the input data, which are both of saved_query type.

Yeah I meant if both versions had the same/different type fields, I'll switch it over 👍

jpdjere · 2024-09-09T19:39:23Z

...tion/server/lib/detection_engine/prebuilt_rules/logic/diff/calculation/algorithms/helpers.ts

+export const determineIfAllVersionsAreEqual = <T>(
+  baseVersion: T,
+  currentVersion: T,
+  targetVersion: T
+): boolean => {
+  const baseEqlCurrent = isEqual(baseVersion, currentVersion);
+  const baseEqlTarget = isEqual(baseVersion, targetVersion);
+  const currentEqlTarget = isEqual(currentVersion, targetVersion);
+
+  return baseEqlCurrent && baseEqlTarget && currentEqlTarget;
+};


This is unused now, right? Let's delete it for now then.

jpdjere

Leaving my approval because overall LGTM ✅ but please take a look at the two comments I left before merging.

kibana-ci · 2024-09-09T21:19:10Z

💚 Build Succeeded

Buildkite Build
Commit: 8263b1a

Metrics [docs]

✅ unchanged

History

💚 Build #230202 succeeded d5dadcd
💛 Build #227472 was flaky 2ef6cb5
💔 Build #227451 failed f34a643
💛 Build #226643 was flaky e90c66e

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

cc @dplumlee

) ## Summary Related ticket: #187658 Adds test plan for diff algorithm for arrays of scalar values implemented here: #190179 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

…tic#192529) ## Summary Related ticket: elastic#187658 Adds test plan for diff algorithm for arrays of scalar values implemented here: elastic#190179 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 6680f35)

… algorithm (#192529) (#193062) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Test plan for `query` fields diff algorithm (#192529)](#192529)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>

…92655) ## Summary Completes #187658 Switches `kql_query`, `eql_query`, and `esql_query` fields to use the implemented diff algorithms assigned to them in #190179 Adds integration tests in accordance to #192529 for the `upgrade/_review` API endpoint for the `query` field diff algorithms. ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

…astic#192655) ## Summary Completes elastic#187658 Switches `kql_query`, `eql_query`, and `esql_query` fields to use the implemented diff algorithms assigned to them in elastic#190179 Adds integration tests in accordance to elastic#192529 for the `upgrade/_review` API endpoint for the `query` field diff algorithms. ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit ceb1b1a)

…f algorithms (#192655) (#193108) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Integration tests for `query` diff algorithms (#192655)](#192655)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>

dplumlee added 4 commits August 8, 2024 11:55

adds kql query algorithm

4af2cc5

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

0d279b0

adds eql query diff algorithm

1f6e217

adds esql query diff algorithm

bf6e356

dplumlee self-assigned this Aug 8, 2024

dplumlee requested a review from a team as a code owner August 8, 2024 19:04

dplumlee requested a review from xcrzx August 8, 2024 19:04

dplumlee requested review from jpdjere and removed request for xcrzx August 8, 2024 19:04

dplumlee mentioned this pull request Aug 8, 2024

[Security Solution] Implement query fields diff algorithms #187658

Closed

8 tasks

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

e90c66e

jpdjere mentioned this pull request Aug 9, 2024

[Security Solution] Implement query filters diff algorithm #190241

Open

2 tasks

dplumlee added 5 commits August 9, 2024 16:30

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

ab012ba

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

4162edd

removes the mergeable logic in algorithms

f34a643

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

2ef6cb5

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

d5dadcd

jpdjere reviewed Sep 9, 2024

View reviewed changes

jpdjere approved these changes Sep 9, 2024

View reviewed changes

dplumlee added 2 commits September 9, 2024 15:46

Merge remote-tracking branch 'upstream/main' into query-diff-algorithm

337fd94

addresses comments

8263b1a

dplumlee force-pushed the query-diff-algorithm branch from eb1d612 to 8263b1a Compare September 9, 2024 19:51

dplumlee enabled auto-merge (squash) September 9, 2024 19:54

dplumlee merged commit f3be719 into elastic:main Sep 9, 2024
39 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Sep 9, 2024

dplumlee deleted the query-diff-algorithm branch September 9, 2024 21:22

This was referenced Sep 10, 2024

[Security Solution] Test plan for query fields diff algorithm #192529

Merged

[Security Solution] Integration tests for query diff algorithms #192655

Merged

szaffarano mentioned this pull request Sep 13, 2024

[Telemetry][Security Solution] Fix integration tests on branch 9.0 #192842

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Adds diff algorithm and unit tests for `query` fields #190179

[Security Solution] Adds diff algorithm and unit tests for `query` fields #190179

dplumlee commented Aug 8, 2024 •

edited

Loading

elasticmachine commented Aug 8, 2024

elasticmachine commented Aug 8, 2024

elasticmachine commented Aug 8, 2024

jpdjere commented Aug 9, 2024 •

edited

Loading

jpdjere Sep 9, 2024

dplumlee Sep 9, 2024

jpdjere Sep 9, 2024

jpdjere left a comment

kibana-ci commented Sep 9, 2024

[Security Solution] Adds diff algorithm and unit tests for query fields #190179

[Security Solution] Adds diff algorithm and unit tests for query fields #190179

Conversation

dplumlee commented Aug 8, 2024 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 8, 2024

elasticmachine commented Aug 8, 2024

elasticmachine commented Aug 8, 2024

jpdjere commented Aug 9, 2024 • edited Loading

jpdjere Sep 9, 2024

Choose a reason for hiding this comment

dplumlee Sep 9, 2024

Choose a reason for hiding this comment

jpdjere Sep 9, 2024

Choose a reason for hiding this comment

jpdjere left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 9, 2024

💚 Build Succeeded

Metrics [docs]

History

[Security Solution] Adds diff algorithm and unit tests for `query` fields #190179

[Security Solution] Adds diff algorithm and unit tests for `query` fields #190179

dplumlee commented Aug 8, 2024 •

edited

Loading

jpdjere commented Aug 9, 2024 •

edited

Loading