[Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents #112257

banderror · 2021-09-15T12:28:34Z

Ticket: #109815

Summary

Background: siem-detection-engine-rule-status documents stores the lastFailureMessage a string which is indexed as type: "text" but some failure messages are so large that these documents are up to 26MB. These large documents cause migrations to fail because a batch of 1000 documents easily exceed Elasticsearch's http.max_content_length which defaults to 100mb.

This PR truncates lastFailureMessage and lastSuccessMessage in the following cases:

When we write new or update existing status SOs:
- The lists of errors/warnings are deduped -> truncated to max 20 items -> joined to a string
- The resulting strings are truncated to max 10240 characters
When we migrate siem-detection-engine-rule-status SOs to 7.15.2:
- The two message fields are truncated to max 10240 characters

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

elasticmachine · 2021-09-15T12:52:35Z

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

elasticmachine · 2021-09-15T12:52:37Z

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

x-pack/plugins/security_solution/server/lib/detection_engine/rules/saved_object_mappings.ts

...gins/security_solution/server/lib/detection_engine/rule_execution_log/utils/normalization.ts

...ecurity_solution/server/lib/detection_engine/rule_execution_log/rule_execution_log_client.ts

...ecurity_solution/server/lib/detection_engine/rule_types/create_security_rule_type_factory.ts

x-pack/plugins/security_solution/server/lib/detection_engine/rules/saved_object_mappings.ts

…uments

banderror · 2021-10-14T11:56:33Z

@rudolf @spong @xcrzx this fix is ready for the 2nd review! 🙏

kibanamachine · 2021-10-14T13:01:25Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #160783 failed 8946be27914d37e4a7062702050d212ebb1a942c
💚 Build #158500 succeeded 5b25da2c384a37a5adcdeb7a13bb6b9dd177a74d
💚 Build #153517 succeeded cd02610ceb83c1eecdcb4d5d762ea35a55573e6b

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

cc @banderror

rudolf · 2021-10-14T13:23:45Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/saved_object_mappings.ts

@@ -47,11 +48,28 @@ export const ruleStatusSavedObjectMappings: SavedObjectsType['mappings'] = {
  },
 };

+const truncateMessageFields: SavedObjectMigrationFn<Record<string, unknown>> = (doc) => {


Using Record<string, unknown> doesn't really add any type safety. Could you use IRuleStatusSOAttributes https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/server/lib/detection_engine/rules/types.ts#L113-L125

Don't have to block on this, but I think this is a good pattern to adopt for migrations.

In general IRuleStatusSOAttributes would correspond to the current (target for this migration) interface, not to the previous interface (prior to migration). Although in this particular case no changes to IRuleStatusSOAttributes have been made, it doesn't mean we wouldn't change it in the future. Any substantial change to the TS interface in a future version would keep this function statically typed but it would not be safe anymore - we'd need to adjust the code of the migration function to conform to the changed interface, thus making the function prone to bugs. Maintaining history of these interfaces (like IRuleStatusSOAttributes_v15_1 etc) would help keeping it fully statically typed and correct, but we don't do this and effort/value ratio of this seems to be too high. Specifying target SO attributes (MigratedAttributes) as IRuleStatusSOAttributes is actually prone to the same issues.

I think unless we start maintaining a history of TS interfaces for the SO attributes, introducing type safety here actually means introducing risk of making migrations incorrect in the future.

Also, we'll likely get rid of this SO type very soon.

xcrzx

Thank you for the changes, Georgii👍
We could address outstanding questions separately, this PR LGTM!

…detection-engine-rule-status documents (elastic#112257) **Ticket:** elastic#109815 ## Summary **Background:** `siem-detection-engine-rule-status` documents stores the `lastFailureMessage` a string which is indexed as `type: "text"` but some failure messages are so large that these documents are up to 26MB. These large documents cause migrations to fail because a batch of 1000 documents easily exceed Elasticsearch's `http.max_content_length` which defaults to 100mb. This PR truncates `lastFailureMessage` and `lastSuccessMessage` in the following cases: 1. When we write new or update existing status SOs: - The lists of errors/warnings are deduped -> truncated to max `20` items -> joined to a string - The resulting strings are truncated to max `10240` characters 2. When we migrate `siem-detection-engine-rule-status` SOs to 7.15.2: - The two message fields are truncated to max `10240` characters ### Checklist Delete any items that are not applicable to this PR. - [ ] [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

kibanamachine · 2021-10-14T15:43:55Z

💔 Backport failed

Status	Branch	Result
✅	7.x
❌	7.15	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 112257

banderror · 2021-10-14T16:41:29Z

I 👀 the failed backport, will manually make it.

UPD: #115166

…detection-engine-rule-status documents (#112257) (#115038) **Ticket:** #109815 ## Summary **Background:** `siem-detection-engine-rule-status` documents stores the `lastFailureMessage` a string which is indexed as `type: "text"` but some failure messages are so large that these documents are up to 26MB. These large documents cause migrations to fail because a batch of 1000 documents easily exceed Elasticsearch's `http.max_content_length` which defaults to 100mb. This PR truncates `lastFailureMessage` and `lastSuccessMessage` in the following cases: 1. When we write new or update existing status SOs: - The lists of errors/warnings are deduped -> truncated to max `20` items -> joined to a string - The resulting strings are truncated to max `10240` characters 2. When we migrate `siem-detection-engine-rule-status` SOs to 7.15.2: - The two message fields are truncated to max `10240` characters ### Checklist Delete any items that are not applicable to this PR. - [ ] [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 Co-authored-by: Georgii Gorbachev <[email protected]>

…r siem-detection-engine-rule-status documents (elastic#112257) **Ticket:** elastic#109815 **Background:** `siem-detection-engine-rule-status` documents stores the `lastFailureMessage` a string which is indexed as `type: "text"` but some failure messages are so large that these documents are up to 26MB. These large documents cause migrations to fail because a batch of 1000 documents easily exceed Elasticsearch's `http.max_content_length` which defaults to 100mb. This PR truncates `lastFailureMessage` and `lastSuccessMessage` in the following cases: 1. When we write new or update existing status SOs: - The lists of errors/warnings are deduped -> truncated to max `20` items -> joined to a string - The resulting strings are truncated to max `10240` characters 2. When we migrate `siem-detection-engine-rule-status` SOs to 7.15.2: - The two message fields are truncated to max `10240` characters Delete any items that are not applicable to this PR. - [ ] [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

…r siem-detection-engine-rule-status documents (#112257) (#115166) **Ticket:** #109815 **Background:** `siem-detection-engine-rule-status` documents stores the `lastFailureMessage` a string which is indexed as `type: "text"` but some failure messages are so large that these documents are up to 26MB. These large documents cause migrations to fail because a batch of 1000 documents easily exceed Elasticsearch's `http.max_content_length` which defaults to 100mb. This PR truncates `lastFailureMessage` and `lastSuccessMessage` in the following cases: 1. When we write new or update existing status SOs: - The lists of errors/warnings are deduped -> truncated to max `20` items -> joined to a string - The resulting strings are truncated to max `10240` characters 2. When we migrate `siem-detection-engine-rule-status` SOs to 7.15.2: - The two message fields are truncated to max `10240` characters Delete any items that are not applicable to this PR. - [ ] [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

banderror self-assigned this Sep 15, 2021

banderror requested review from xcrzx and spong September 15, 2021 12:47

banderror marked this pull request as ready for review September 15, 2021 12:52

banderror requested a review from a team as a code owner September 15, 2021 12:52

banderror added release_note:fix v7.14.2 labels Sep 15, 2021

banderror commented Sep 15, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/saved_object_mappings.ts Outdated Show resolved Hide resolved

xcrzx reviewed Sep 15, 2021

View reviewed changes

...ecurity_solution/server/lib/detection_engine/rule_types/create_security_rule_type_factory.ts Outdated Show resolved Hide resolved

spong added v7.15.1 and removed v7.14.2 labels Sep 15, 2021

rudolf reviewed Sep 16, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/saved_object_mappings.ts Outdated Show resolved Hide resolved

banderror added v7.16.0 v8.0.0 labels Oct 6, 2021

banderror force-pushed the truncate-siem-detection-engine-rule-status branch from cd02610 to 5b25da2 Compare October 6, 2021 14:36

banderror marked this pull request as draft October 6, 2021 14:36

Truncate lastFailureMessage for siem-detection-engine-rule-status doc…

b2e018d

…uments

banderror added v7.15.2 and removed v7.15.1 labels Oct 14, 2021

banderror force-pushed the truncate-siem-detection-engine-rule-status branch from 5b25da2 to 8946be2 Compare October 14, 2021 06:17

Address comments

236009c

banderror force-pushed the truncate-siem-detection-engine-rule-status branch from 8946be2 to 236009c Compare October 14, 2021 10:55

banderror marked this pull request as ready for review October 14, 2021 11:10

banderror added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 14, 2021

banderror requested review from xcrzx and rudolf October 14, 2021 11:52

rudolf reviewed Oct 14, 2021

View reviewed changes

xcrzx approved these changes Oct 14, 2021

View reviewed changes

banderror merged commit b304c1c into elastic:master Oct 14, 2021

banderror deleted the truncate-siem-detection-engine-rule-status branch October 14, 2021 15:40

kibanamachine mentioned this pull request Oct 14, 2021

[7.x] [Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents (#112257) #115038

Merged

spong mentioned this pull request Oct 14, 2021

[Security Solution] Migrates siem-detection-engine-rule-status alertId to saved object references array #114585

Merged

3 tasks

banderror mentioned this pull request Oct 15, 2021

[7.15] [Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents (#112257) #115166

Merged

banderror linked an issue Oct 15, 2021 that may be closed by this pull request

Truncate lastFailureMessage for siem-detection-engine-rule-status documents #109815

Closed

banderror mentioned this pull request Nov 11, 2021

[Security Solution] Rule Execution Log - technical debt #118324

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents #112257

[Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents #112257

banderror commented Sep 15, 2021 •

edited

Loading

elasticmachine commented Sep 15, 2021

elasticmachine commented Sep 15, 2021

banderror commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

rudolf Oct 14, 2021

banderror Oct 14, 2021 •

edited

Loading

xcrzx left a comment

kibanamachine commented Oct 14, 2021

banderror commented Oct 14, 2021 •

edited

Loading

[Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents #112257

[Security Solution][Detections] Truncate lastFailureMessage for siem-detection-engine-rule-status documents #112257

Conversation

banderror commented Sep 15, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Sep 15, 2021

elasticmachine commented Sep 15, 2021

banderror commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

💚 Build Succeeded

Metrics [docs]

History

rudolf Oct 14, 2021

Choose a reason for hiding this comment

banderror Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

xcrzx left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 14, 2021

💔 Backport failed

banderror commented Oct 14, 2021 • edited Loading

banderror commented Sep 15, 2021 •

edited

Loading

banderror Oct 14, 2021 •

edited

Loading

banderror commented Oct 14, 2021 •

edited

Loading