Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Auto-merge ignores incoming rule changes for rule query fields #202185

Open
Tracked by #201502
xcrzx opened this issue Nov 28, 2024 · 4 comments
Open
Tracked by #201502
Assignees
Labels
8.18 candidate bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.18.0

Comments

@xcrzx
Copy link
Contributor

xcrzx commented Nov 28, 2024

Summary

When rule customization is enabled, during a rule upgrade, incoming changes to a field are ignored if the same field has been modified in the current version.

Steps to Reproduce

  1. Enable rule customization.
  2. Find a rule with an update to a multiline string field (e.g., ESQL).
  3. Make a local change to the field (e.g., modify any line).
  4. Open the update flyout to review the rule update.

Expected Result

The merge algorithm should respect incoming changes for the rule field. The proposed version should include both local and remote changes. For example:

  • The target version includes a new line added to the query.
  • The current version has a different line modified.

There’s no actual conflict, as different lines were touched. Both edits should be preset in the final version:

image

Actual Result

The change introduced by Elastic is missing in the final version:

image

@xcrzx xcrzx added 8.18 candidate bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team labels Nov 28, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Nov 28, 2024
@banderror banderror self-assigned this Nov 28, 2024
@nikitaindik nikitaindik assigned maximpn and unassigned banderror Nov 29, 2024
@banderror banderror changed the title [Security Solution] Auto-merge ignores incoming rule changes [Security Solution] Auto-merge ignores incoming rule changes for rule query fields Dec 6, 2024
@maximpn
Copy link
Contributor

maximpn commented Dec 17, 2024

This ticket was discussed at out product meeting and it was decided to make the following in Milestone 3 scope

  • Change field with conflicts header's text to make it explicit users have to merge current value with Elastic update manually
    Image

  • Show Update from Elastic Diff view by default


Solution for Milestone 4 would require more elaborated approach. Rule query diff algorithm should be changed to merge rule query changes made by users with update from Elastic. In general case it might be really tricky when changes are made to the same lines. It'd require producing AST Trees and merging them together. We should start with a simpler option merging changes made to different lines. Though result query may be invalid. We can run query validation to make sure the result query is valid. In case the algorithm is unable to merge or the result query is incorrect it should fallback to the current value.

As a potential pitfall such changes may cause performance degradation. It should be verified such changes won't reduce _review API endpoint performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.18 candidate bug Fixes for quality problems that affect the customer experience Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.18.0
Projects
None yet
Development

No branches or pull requests

4 participants