-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] upgrade/_review
blocks main thread
#199290
Comments
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
It looks like the regex is not correct. I started looking at this because
Notes on recent regex issues: https://github.com/elastic/security-team/issues/10699 |
@dplumlee Yep, that's exactly what happens. The Also, the Our goal is to simplify the regexp as much as possible. It should break input strings into lines, not individual characters. |
…e `upgrade/_review` endpoint (elastic#199388) **Fixes elastic#199290 ## Summary The current multi-line string algorithm uses a very inefficient regex to split and analyze string fields, and exponentially increases in time complexity when the strings are long. This PR substitutes a much simpler comparison regex for far better efficiency as shown in the table below. ### Performance between different regex options using sample prebuilt rule setup guide string | | `/(\S+\|\s+)/g` (original) | `/(\s+)/g` | `/(\n)/g` | `/(\r\n\|\n\|\r)/g` | |-----------------------|---------------|----------|---------|-------------------| | Unit test speed | `986ms` | `96ms` | `1ms` | `2ms` | | FTR test with 1 rule | `3.0s` | `2.8s` | `2.0s` | `2.0s` | | FTR test with 5 rules | `11.6s` | `6.8s` | `6.1s` | | ### Performance between different regex options using intentionally long strings (25k characters) | | `/(\S+\|\s+)/g` | `/(\r\n\|\n\|\r)/g` | |----------------------|-----------------------|---------------------| | Unit test speed | `1049414ms` (17 min) | `58ms` | | FTR test with 1 rule | `>360000ms` (Timeout) | `2.1 s` | ### 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 - [ ] [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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co> (cherry picked from commit 4f6d357)
…e `upgrade/_review` endpoint (elastic#199388) **Fixes elastic#199290 ## Summary The current multi-line string algorithm uses a very inefficient regex to split and analyze string fields, and exponentially increases in time complexity when the strings are long. This PR substitutes a much simpler comparison regex for far better efficiency as shown in the table below. ### Performance between different regex options using sample prebuilt rule setup guide string | | `/(\S+\|\s+)/g` (original) | `/(\s+)/g` | `/(\n)/g` | `/(\r\n\|\n\|\r)/g` | |-----------------------|---------------|----------|---------|-------------------| | Unit test speed | `986ms` | `96ms` | `1ms` | `2ms` | | FTR test with 1 rule | `3.0s` | `2.8s` | `2.0s` | `2.0s` | | FTR test with 5 rules | `11.6s` | `6.8s` | `6.1s` | | ### Performance between different regex options using intentionally long strings (25k characters) | | `/(\S+\|\s+)/g` | `/(\r\n\|\n\|\r)/g` | |----------------------|-----------------------|---------------------| | Unit test speed | `1049414ms` (17 min) | `58ms` | | FTR test with 1 rule | `>360000ms` (Timeout) | `2.1 s` | ### 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 - [ ] [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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co> (cherry picked from commit 4f6d357)
…e `upgrade/_review` endpoint (elastic#199388) **Fixes elastic#199290 ## Summary The current multi-line string algorithm uses a very inefficient regex to split and analyze string fields, and exponentially increases in time complexity when the strings are long. This PR substitutes a much simpler comparison regex for far better efficiency as shown in the table below. ### Performance between different regex options using sample prebuilt rule setup guide string | | `/(\S+\|\s+)/g` (original) | `/(\s+)/g` | `/(\n)/g` | `/(\r\n\|\n\|\r)/g` | |-----------------------|---------------|----------|---------|-------------------| | Unit test speed | `986ms` | `96ms` | `1ms` | `2ms` | | FTR test with 1 rule | `3.0s` | `2.8s` | `2.0s` | `2.0s` | | FTR test with 5 rules | `11.6s` | `6.8s` | `6.1s` | | ### Performance between different regex options using intentionally long strings (25k characters) | | `/(\S+\|\s+)/g` | `/(\r\n\|\n\|\r)/g` | |----------------------|-----------------------|---------------------| | Unit test speed | `1049414ms` (17 min) | `58ms` | | FTR test with 1 rule | `>360000ms` (Timeout) | `2.1 s` | ### 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 - [ ] [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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
…e `upgrade/_review` endpoint (elastic#199388) **Fixes elastic#199290 ## Summary The current multi-line string algorithm uses a very inefficient regex to split and analyze string fields, and exponentially increases in time complexity when the strings are long. This PR substitutes a much simpler comparison regex for far better efficiency as shown in the table below. ### Performance between different regex options using sample prebuilt rule setup guide string | | `/(\S+\|\s+)/g` (original) | `/(\s+)/g` | `/(\n)/g` | `/(\r\n\|\n\|\r)/g` | |-----------------------|---------------|----------|---------|-------------------| | Unit test speed | `986ms` | `96ms` | `1ms` | `2ms` | | FTR test with 1 rule | `3.0s` | `2.8s` | `2.0s` | `2.0s` | | FTR test with 5 rules | `11.6s` | `6.8s` | `6.1s` | | ### Performance between different regex options using intentionally long strings (25k characters) | | `/(\S+\|\s+)/g` | `/(\r\n\|\n\|\r)/g` | |----------------------|-----------------------|---------------------| | Unit test speed | `1049414ms` (17 min) | `58ms` | | FTR test with 1 rule | `>360000ms` (Timeout) | `2.1 s` | ### 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 - [ ] [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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
Summary
With certain rule upgrade payloads, the
internal/detection_engine/prebuilt_rules/upgrade/_review
endpoint can block the main Node.js thread for an extended period. Locally, I observed a single rule blocking for over 20 seconds:Steps to Reproduce
It's not entirely clear what specific payload causes this, but I traced the issue to the
note
field in a particular rule. Specifically, the issue appears during merge version calculations here:kibana/x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/logic/diff/calculation/algorithms/multi_line_string_diff_algorithm.ts
Lines 104 to 106 in 12dc8c1
The minimal code needed to reproduce the problem is as follows, but the issue may also occur under other conditions and with different fields.
Impact
It's unclear how often rule upgrades may lead to main thread blocking, but since the
upgrade/_review
endpoint might be called relatively frequently, having even a few rules with updates that trigger the issue could result in Kibana being blocked for several minutes. For this reason, I consider this a critical issue for the Rule Customization release.The text was updated successfully, but these errors were encountered: