[Security Solution] Integration tests for simple diff algorithm #184889

dplumlee · 2024-06-06T02:51:01Z

Summary

Completes related tickets: #180160 and #180158

Switches fields to use the diff algorithms assigned to them in the related tickets

Adds integration tests in accordance to #184484 for the upgrade/_review API endpoint for the simple diff algorithm.

Also changes logic in the upgrade/_review API endpoint to return user customized fields in the diffs even if there was not an update for that field. This new logic is described in #180154. We filter out the fields that fall under this new logic so that they are only returned from the API but not displayed in the per-field rule diff flyout as the current UI cannot support them. They are utilized in testing logic and will be implemented in the UI at a later date

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
Flaky Test Runner was used on any tests changed

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-06-06T02:51:04Z

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

elasticmachine · 2024-06-06T02:51:04Z

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

elasticmachine · 2024-06-06T02:51:04Z

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

…tion-tests

kibanamachine · 2024-06-06T05:21:53Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6224

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.
[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/configs/serverless.config.ts: 100/100 tests passed.

see run history

…tion-tests

dplumlee · 2024-06-10T20:14:58Z

...rity_solution/public/detection_engine/rule_creation_ui/components/step_about_rule/schema.tsx

@@ -217,7 +217,7 @@ export const schema: FormSchema<AboutStepRule> = {
    label: i18n.translate(
      'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.fieldMitreThreatLabel',
      {
-        defaultMessage: 'MITRE ATT&CK\\u2122',
+        defaultMessage: 'MITRE ATT&CK\u2122',


When I was doing some testing, I noticed this was returning with the literal string both in the diffing UI and in the rule forms, probably caused by the big i18n update that got merged in a couple weeks ago. This fixes the unicode to display correctly.

…tion-tests

nikitaindik

Thanks @dplumlee! I have taken a look. Left some comments.

...ement/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.ts

...ver/lib/detection_engine/prebuilt_rules/api/review_rule_upgrade/review_rule_upgrade_route.ts

...ement/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.ts

nikitaindik · 2024-06-11T15:43:32Z

...ement/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.ts

+                name: {
+                  current_version: 'B',
+                  diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
+                  has_conflict: false,


Should this actually be has_conflict: true? The test plan mentions that there should be a conflict, and I think it makes sense.

Let's say you have a rule with a customized field value, then the base version gets removed somehow. In such case I think we shouldn't automatically choose the target version, but force the user to make this choice.

Yeah I agree, I'll change the logic of what we're returning to match this output

I think this poses another question actually. Currently, whenever the base_version is missing, we assume the rule is not customized giving us the diff outcomes of StockValueNoUpdate for the -AA situation and StockValueCanUpdate for the -AB situation. Here is where that logic occurs in the code

Should we be instead assuming both are customized and the fallback for an undefined base_version would be to return the field and let users decide if they want to keep or update the field?

In this scenario we would return CustomizedValueSameUpdate for the now assumed -BB outcome and CustomizedValueCanUpdate for the -BC outcome.

The only downside I can think of for this would be we'd return all the fields for an entire rule no matter what if the base_version was unavailable which could be overwhelming.

cc: @banderror @nikitaindik

I think it's okay to return a bunch of fields in this edge case, since it should only happen once. Base version would get created if you upgrade a rule, right? Otherwise we run the risk of removing user's custom value with no way to return to it.

This was my line of thinking:
If we have the -AA situation this could mean one of:

AAA -> no conflict, pick target

ABB -> no conflict, pick target
Since we don't know which case it is, we can assume the most complex scenario (ABB) and return ThreeWayDiffOutcome.CustomizedValueSameUpdate. Then indeed we'll return every field in response.

If we have the -AB situation this could mean one of:

AAB -> no conflict, pick target

ABC -> has conflict, user has to choose
Again, I would assume the "worst" (ABC) and return CustomizedValueCanUpdate. Then the field would also have has_conflict: true and the user would have to make a choice.

I think we can merge this as is for now and wait until folks come back from vacations to discuss with them. Looking at the comment here, perhaps there was an idea behind the current behaviour 🤷 .

I have read the thread and agree with the compromise logic option. Does it make sense to inform the user about missing the base version when this happens?

@approksiu I think it's probably a good idea, not sure if that's in the UI plans yet or not

We'll discuss with @ARWNightingale.

Thanks for the feedback @approksiu, we'll go with option 3 then.

Does it make sense to inform the user about missing the base version when this happens?
not sure if that's in the UI plans yet or not

It would be a rare edge case to handle. It's not planned. I think it might be useful eventually, especially when we reach that 15k limit, but at this point I would not include this into Milestone 3. I opened a #186880 that we can consider adding to Milestone 4.

cc @dplumlee @jpdjere @xcrzx @nikitaindik

Agreed, definitely in Milestone 3. Thanks for adding the issue!

...ement/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.ts

...ver/lib/detection_engine/prebuilt_rules/api/review_rule_upgrade/review_rule_upgrade_route.ts

banderror

Left my thoughts on the -AA and -AB question.

I'd like to take a deeper look at the added tests next week. Please merge w/o waiting for me though when the PR is reviewed and ready, I can take a look post-merge.

banderror · 2024-06-12T12:59:08Z

...ement/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.ts

+                name: {
+                  current_version: 'B',
+                  diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
+                  has_conflict: false,


@nikitaindik @dplumlee This is a great catch, thank you both!

We have a technical and a UX sides of the question.

Technically, your above comments make sense that if we treat -AA as ABB and -AB as ABC, we would need to adjust the returned ThreeWayDiffOutcome values accordingly.

From the UX perspective, we have a trade-off:

1st option is to treat -AA as ABB and -AB as ABC. The trade-off is safety at the cost of usability. In practice, all -AA and -AB fields would be shown in the rule upgrade flyout, with -AB fields shown as conflicting. This might look confusing, dangerous and overwhelming, especially if the user has never customized their installed rules and just lost some of the prebuilt rule assets.

2nd option is to treat -AA as AAA and -AB as AAB. The trade-off is usability at the cost of safety. In practice, all -AA fields would not be shown in the rule upgrade flyout, and -AB fields would be shown in the flyout as auto-accepted, thus significantly saving the time to upgrade the rules. This might be dangerous for those rules that had been customized by the user, then Elastic published a new package with updates to those rules, and the user lost some of the prebuilt rule assets (-AB situation).

There can be a few ways to loose rule assets partially:

By manually deleting some of them using a script, or dev tools, etc. I think chances are very low, and that would be the shooting yourself in the foot type of case.

By installing a new package version where some of the historical rule versions will be missing. We have a hard limit of 15000 rule versions per package. We haven't reached it yet, but reaching it is a matter of time. When the TRaDE team reaches it, they will need to come up with an algorithm for evicting old rule versions from the package. Our upgrade logic should work well regardless of existence or non-existence of certain historical versions. What are the chances for the user to find themselves in this situation? I think it will depend on this eviction algorithm in the future, but currently I think it's 0.

The 1st option would be a fool-proof and safe way of handling the two situations above, the 2nd option would be a quick-and-easy way.

Probably there can be a 3rd, compromise option:

Treat -AA as AAA and NO_CONFLICT for increased usability.

Treat -AB as ABC and SOLVABLE_CONFLICT for safety. In this case, we would successfully auto-merge the change and return a merged_version: 'C', but mark it as SOLVABLE_CONFLICT according to the idea of solvable and non-solvable conflicts described in the RFC and this ticket. In this case, users would need to manually accept the auto-merged value in the upgrade flyout.

I agree that we should hold off changing the current logic until all people are back from vacations 👍

…tion-tests

nikitaindik

Thanks for addressing my comments @dplumlee! The PR LGTM now!

dplumlee · 2024-06-13T15:35:12Z

@elasticmachine merge upstream

kibana-ci · 2024-06-13T17:01:45Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a161e10

Failed CI Steps

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	13.6MB	13.6MB	+2.0KB

History

💔 Build #215640 failed 2828693
💔 Build #215605 failed 76f398c
💔 Build #215477 failed edd3b03
💛 Build #215263 was flaky ffb2aa0
💔 Build #215077 failed 073455b

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

cc @dplumlee

vitaliidm

Detection Engine changes LGTM

adds simple diff tests

9a357c9

dplumlee self-assigned this Jun 6, 2024

dplumlee requested a review from a team as a code owner June 6, 2024 02:51

dplumlee requested a review from nikitaindik June 6, 2024 02:51

updates diff fields

0c319e7

This was referenced Jun 6, 2024

[Security Solution] Implement number diff algorithm #180160

Closed

[Security Solution] Implement single-line string diff algorithm #180158

Closed

Merge remote-tracking branch 'upstream/main' into simple-diff-integra…

abe4351

…tion-tests

banderror mentioned this pull request Jun 7, 2024

[Security Solution] Simple diff algorithm test plan #184484

Merged

dplumlee added 3 commits June 7, 2024 13:01

update imports

66e9a62

Merge remote-tracking branch 'upstream/main' into simple-diff-integra…

fdb1f20

…tion-tests

matches updated test plan

be1cfbb

dplumlee mentioned this pull request Jun 10, 2024

[Security Solution] Return user-customized fields from the POST /prebuilt_rules/upgrade/_review API endpoint even if they haven't been updated by Elastic in the target version #180154

Closed

dplumlee added 2 commits June 10, 2024 16:11

Merge remote-tracking branch 'upstream/main' into simple-diff-integra…

8e6d519

…tion-tests

fix translation bugs

073455b

dplumlee requested a review from a team as a code owner June 10, 2024 20:12

dplumlee requested a review from dhurley14 June 10, 2024 20:12

dplumlee commented Jun 10, 2024

View reviewed changes

adds filter for unsupported display type

2cb9535

Merge remote-tracking branch 'upstream/main' into simple-diff-integra…

ffb2aa0

…tion-tests

nikitaindik reviewed Jun 11, 2024

View reviewed changes

nikitaindik reviewed Jun 12, 2024

View reviewed changes

...ver/lib/detection_engine/prebuilt_rules/api/review_rule_upgrade/review_rule_upgrade_route.ts Outdated Show resolved Hide resolved

banderror reviewed Jun 12, 2024

View reviewed changes

dplumlee added 3 commits June 12, 2024 10:59

addresses changes

edd3b03

Merge remote-tracking branch 'upstream/main' into simple-diff-integra…

76f398c

…tion-tests

addresses comments

2828693

nikitaindik self-requested a review June 13, 2024 09:52

nikitaindik approved these changes Jun 13, 2024

View reviewed changes

Merge branch 'main' into simple-diff-integration-tests

a161e10

vitaliidm approved these changes Jun 13, 2024

View reviewed changes

dplumlee merged commit 0082f45 into elastic:main Jun 13, 2024
42 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jun 13, 2024

dplumlee deleted the simple-diff-integration-tests branch June 13, 2024 18:06

dplumlee mentioned this pull request Jun 19, 2024

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

Closed

3 tasks

banderror mentioned this pull request Jun 25, 2024

[Security Solution] Inform users about missing base versions during prebuilt rule upgrade #186880

Open

jpdjere mentioned this pull request Jul 9, 2024

[Security Solution] Integration tests for scalar array diff algorithm #187778

Merged

3 tasks

jpdjere mentioned this pull request Jul 22, 2024

[Security Solution] Extend the /upgrade/_review endpoint contract and functionality #187770

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Integration tests for simple diff algorithm #184889

[Security Solution] Integration tests for simple diff algorithm #184889

dplumlee commented Jun 6, 2024 •

edited

Loading

elasticmachine commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

kibanamachine commented Jun 6, 2024

dplumlee Jun 10, 2024 •

edited

Loading

nikitaindik left a comment

nikitaindik Jun 11, 2024 •

edited

Loading

dplumlee Jun 11, 2024

dplumlee Jun 11, 2024 •

edited

Loading

nikitaindik Jun 12, 2024

nikitaindik Jun 12, 2024

approksiu Jun 24, 2024

dplumlee Jun 25, 2024

approksiu Jun 25, 2024

banderror Jun 25, 2024

approksiu Jun 25, 2024

banderror left a comment

banderror Jun 12, 2024 •

edited

Loading

nikitaindik left a comment

dplumlee commented Jun 13, 2024

kibana-ci commented Jun 13, 2024

vitaliidm left a comment

[Security Solution] Integration tests for simple diff algorithm #184889

[Security Solution] Integration tests for simple diff algorithm #184889

Conversation

dplumlee commented Jun 6, 2024 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

elasticmachine commented Jun 6, 2024

kibanamachine commented Jun 6, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6224

dplumlee Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

nikitaindik left a comment

Choose a reason for hiding this comment

nikitaindik Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplumlee Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

banderror Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

nikitaindik left a comment

Choose a reason for hiding this comment

dplumlee commented Jun 13, 2024

kibana-ci commented Jun 13, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

vitaliidm left a comment

Choose a reason for hiding this comment

dplumlee commented Jun 6, 2024 •

edited

Loading

dplumlee Jun 10, 2024 •

edited

Loading

nikitaindik Jun 11, 2024 •

edited

Loading

dplumlee Jun 11, 2024 •

edited

Loading

banderror Jun 12, 2024 •

edited

Loading