[Security Solution] Extend the `/upgrade/_review` endpoint contract and functionality by jpdjere · Pull Request #187770 · elastic/kibana

jpdjere · 2024-07-08T16:19:43Z

Resolves: #180153
Resolves: #188277

Summary

Extend the POST /upgrade/_review API endpoint's contract and functionality
Changes has_conflict property within each rule field's ThreeWayDiff from boolean to enum with possible values:
- NONE: no conflicts in three way diff
- SOLVABLE: conflict detected but was successfully resolved by our algorithms
- NON_SOLVABLE: conflict detected and could not be resolved by our algorithms.
Adds has_base_version boolean field within each field diff calculation. Has values:
- true: the base version of the field was found and is either defined or undefined
- false: the base version of the field was not found
The possible values for has_conflict for each concrete diff algorithm are:
- single line strings: NO, NON_SOLVABLE
- multi line strings: NO, SOLVABLE, NON_SOLVABLE
- numbers: NO, NON_SOLVABLE
- array of scalar values: NO, SOLVABLE
Adds new logic to handle [Security Solution] Updates diff outcome logic to return CustomizedValueCanUpdate on scenario -AB #186435 (comment)

For maintainers

This was checked for breaking API changes and was labeled appropriately

jpdjere · 2024-07-08T17:14:53Z

/ci

jpdjere · 2024-07-09T14:33:40Z

/ci

jpdjere · 2024-07-10T13:19:47Z

/ci

jpdjere · 2024-07-12T15:51:31Z

/ci

elasticmachine · 2024-07-15T10:14:24Z

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

elasticmachine · 2024-07-15T10:14:25Z

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

elasticmachine · 2024-07-15T10:14:26Z

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

dplumlee

This looks good for the most part @jpdjere! Just left one comment about naming and we can figure out how to sync with #188022

dplumlee · 2024-07-16T18:50:13Z

...ution/common/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_diff.ts

Should this be something like NO_CONFLICT? I guess NO makes sense in the conflict: no assignment but it does look a bit weird standalone

Updated the enum for clarity

jpdjere · 2024-07-22T15:35:49Z

@banderror @nikitaindik @dplumlee

Thanks for the review. I've addressed your feedback, and included in the changes of this PR the handling of scenarios with a missing base version, with the "compromise" solution we had decided here.

Please take a look when you can.

nikitaindik

Thanks for the updates to PR @jpdjere! I have only reviewed changes related to MissingVersion - they look good to me!

dplumlee

The new changes look great to me @jpdjere, thanks for incorporating all this into the existing diff work! I do like the idea you mentioned here, not sure if there's a reason we didn't incorporate this before other than not really having any special logic needs for those cases with the initial implementation

dplumlee · 2024-07-23T19:20:35Z

...ne/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.test.ts

      expect(result).toEqual(
        expect.objectContaining({
+          has_base_version: false,
+          base_version: undefined,


dplumlee · 2024-07-23T19:21:48Z

..._engine/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.ts

+    case ThreeWayDiffOutcome.StockValueNoUpdate: // Scenarios AAA and -AA
+    case ThreeWayDiffOutcome.CustomizedValueNoUpdate: // Scenario ABA
+    case ThreeWayDiffOutcome.CustomizedValueSameUpdate: // Scenario ABB


Nice idea to put these scenarios in here, I think I'll incorporate this into some other diff files as well

@dplumlee I ended up removing these, since now each scenario has its own ThreeWayDiffOutcome and there's no case like // Scenarios AAA and -AA anymore.

...mon/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_merge_outcome.ts

…arios to ThreeWayDiffOutcome enum

jpdjere · 2024-07-24T13:59:50Z

@banderror Reintroduced ThreeWayMergeOutcome enum as we discussed.

@dplumlee While introducing/modifying the integration tests for scenarios MissingBaseNoUpdate and MissingBaseCanUpdate, I realised that doing assertions on the whole reviewResponse.rules[0].diff.fields object was confusing, since version is always present and we need to describe its behaviour in all unrelated tests.

For example, the assertion in the case of the -AA or MissingBaseNoUpdate scenario was:

              const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest);
              expect(reviewResponse.rules[0].diff.fields).toEqual({
                version: {
                  current_version: 1,
                  target_version: 2,
                  merged_version: 2,
                  diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
                  merge_outcome: ThreeWayMergeOutcome.Target,
                  has_conflict: false,
                  has_update: true,
                },
              });

which in my opinion creates cognitive dissonance because the reader of the test is expecting to see diff_outcome: ThreeWayDiffOutcome.MissingBaseNoUpdate but actually sees StockValueCanUpdate. It might take a while to realize that StockValueCanUpdate refers only to version and that the actual field being tested actually does have has a merge_outcome of ThreeWayDiffOutcome.MissingBaseNoUpdate, but it's just not returned as part of the diff object by the endpoint.

So I went ahead and simplified all integration tests, now doing assertions only on the field that should/shouldn't be part of the diff object - so that if the scenario excludes the field from the diff object, then the assertion is just expect(reviewResponse.rules[0].diff.fields.name).toBeUndefined().

So now the focus of each test is exclusively on the field that corresponds to the algorithm type.

banderror

Thanks @jpdjere, I left a couple more comments. Approving in advance 👍

banderror · 2024-07-24T19:07:48Z

...ction_engine/prebuilt_rules/logic/diff/calculation/algorithms/scalar_array_diff_algorithm.ts

-      }
-
-      const addedCurrent = difference(dedupedCurrentVersion, dedupedBaseVersion);
+      const addedCurrent = difference(dedupedCurrentVersion, dedupedBaseVersion as TValue[]);


We always try to avoid as casting in production code, can we try to rewrite this algorithm in a type-safe way?

banderror · 2024-07-24T19:07:57Z

...ction_engine/prebuilt_rules/logic/diff/calculation/algorithms/scalar_array_diff_algorithm.ts

      const removedCurrent = difference(dedupedBaseVersion, dedupedCurrentVersion);

-      const addedTarget = difference(dedupedTargetVersion, dedupedBaseVersion);
+      const addedTarget = difference(dedupedTargetVersion, dedupedBaseVersion as TValue[]);


banderror · 2024-07-24T19:10:06Z

..._engine/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.ts

-      const mergedVersion = merge(currentVersion, baseVersion, targetVersion, {
+      // TS does not realize that in ABC scenario, baseVersion cannot be missing
+      // Missing baseVersion scenarios were handled as -AA and -AB.
+      const mergedVersion = merge(currentVersion, baseVersion as string, targetVersion, {


kibana-ci · 2024-07-25T14:45:59Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 61ce4de

Failed CI Steps

Jest Tests #2

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5611	5612	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	20.4MB	20.4MB	+568.0B

History

💛 Build #223564 was flaky 63262b0
💚 Build #223505 succeeded b2de7e0
💔 Build #223468 failed 7851d36
💛 Build #223171 was flaky 82c7779
💔 Build #222991 failed 18279f9
💔 Build #222959 failed e1d3311

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

cc @jpdjere

jpdjere added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team 8.16 candidate labels Jul 8, 2024

jpdjere self-assigned this Jul 8, 2024

jpdjere added release_note:skip Skip the PR/issue when compiling release notes Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.16.0 and removed 8.16 candidate labels Jul 8, 2024

jpdjere mentioned this pull request Jul 12, 2024

[Security Solution] Adds diff algorithm and unit tests for multi-line string fields #188022

Merged

2 tasks

jpdjere force-pushed the extend-upgrade-review-endpoint branch from b83a88c to 7654ea7 Compare July 12, 2024 15:18

jpdjere marked this pull request as ready for review July 15, 2024 10:14

jpdjere requested a review from a team as a code owner July 15, 2024 10:14

jpdjere requested a review from dplumlee July 15, 2024 10:14

dplumlee reviewed Jul 16, 2024

View reviewed changes

jpdjere requested a review from a team as a code owner July 17, 2024 09:36

dplumlee approved these changes Jul 17, 2024

View reviewed changes

jpdjere added 6 commits July 19, 2024 09:17

Create ConflictResolution enum

b31b957

Update prop name

6f783dc

Fixes

a6e11b7

Endpoint updates

bf00ce7

Replaced enum

32c1fae

Fix tests

3c5c858

jpdjere requested a review from a team as a code owner July 22, 2024 11:26

jpdjere requested a review from nkhristinin July 22, 2024 11:26

jpdjere added 3 commits July 22, 2024 16:30

Handled missing base version scenarios

e1d3311

Updated integration tests

8529241

Remove changes to rfc

c700ac1

jpdjere removed request for a team and nkhristinin July 22, 2024 15:42

jpdjere added 4 commits July 22, 2024 17:44

Added link to RFC

1aa243c

Remove consolelog

18279f9

remove index

1464706

Fix

82c7779

nikitaindik approved these changes Jul 23, 2024

View reviewed changes

jpdjere mentioned this pull request Jul 23, 2024

[Security Solution] Integration tests for multi line string field diff algorithm #188688

Merged

3 tasks

dplumlee approved these changes Jul 23, 2024

View reviewed changes

banderror requested changes Jul 24, 2024

View reviewed changes

...mon/api/detection_engine/prebuilt_rules/model/diff/three_way_diff/three_way_merge_outcome.ts Show resolved Hide resolved

jpdjere added 4 commits July 24, 2024 14:21

Readded ThreeWayMergeOutcome enum and added missing base version scen…

7851d36

…arios to ThreeWayDiffOutcome enum

Simplified single line strings integration tests

0bf6e12

Simplified integration tests

23189ec

Merge branch 'main' into extend-upgrade-review-endpoint

b2de7e0

jpdjere requested a review from banderror July 24, 2024 13:53

Merge branch 'main' into extend-upgrade-review-endpoint

63262b0

banderror approved these changes Jul 24, 2024

View reviewed changes

jpdjere added 2 commits July 25, 2024 14:39

Remove type casting

a5b2628

Merge branch 'main' into extend-upgrade-review-endpoint

61ce4de

jpdjere merged commit fb82b0e into elastic:main Jul 25, 2024

kibanamachine added the backport:skip This PR does not require backporting label Jul 25, 2024

jpdjere mentioned this pull request Jul 25, 2024

[Security Solution] Updates diff outcome logic to return CustomizedValueCanUpdate on scenario -AB #186435

Closed

3 tasks

Conversation

jpdjere commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

For maintainers

Uh oh!

jpdjere commented Jul 8, 2024

Uh oh!

jpdjere commented Jul 9, 2024

Uh oh!

jpdjere commented Jul 10, 2024

Uh oh!

jpdjere commented Jul 12, 2024

Uh oh!

elasticmachine commented Jul 15, 2024

Uh oh!

elasticmachine commented Jul 15, 2024

Uh oh!

elasticmachine commented Jul 15, 2024

Uh oh!

dplumlee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpdjere commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikitaindik left a comment

Choose a reason for hiding this comment

Uh oh!

dplumlee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpdjere commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Jul 25, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jpdjere commented Jul 8, 2024 •

edited

Loading

jpdjere commented Jul 22, 2024 •

edited

Loading

dplumlee left a comment •

edited

Loading

jpdjere commented Jul 24, 2024 •

edited

Loading