[Security Solution] Remove logic that bumps rule version on every change #137168
Closed
3 tasks
Assignees
Labels
8.8 candidate
Feature:Prebuilt Detection Rules
Security Solution Prebuilt Detection Rules area
Feature:Rule Management
Security Solution Detection Rule Management area
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.8.0
Comments
xcrzx
added
Team:Detections and Resp
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
2 tasks
banderror
added
the
Feature:Rule Management
7 tasks
banderror
added
Feature:Prebuilt Detection Rules
banderror
changed the title
2 tasks
spong
added a commit
that referenced
this issue
Mar 10, 2023
## Summary Resolves #137164. This PR adds a new `revision` field (number) to Alerting Rules that auto-increments when relevant content changes have been made via the `Rules Client`. _Relevant content changes_ are defined as any content change that the user may either want to be notified about, or have the option to later revert to. This will include most all changes, except the enabling/disabling of the rule, or updates to metadata fields like `executionStatus` or `monitoring`. This `revision` field is not intended to be user editable, and should be ignored if ever provided via the API. See #136213 for additional details. To be followed-up by #137168, which will remove the version bump logic from security solution. ## Details ### Migrations Includes SO migration to default `revision` to `0` when upgrading a cluster, or when importing `pre-8.8.0` rules via the SO Management UI. For consistency, security rule import follows the same logic as SO Management and will not reset the `revision` to `0` when overriding or creating a new rule. ### Downstream Index Updates * EventLog _has not_ been updated to include `revision` along with basic rule fields currently being written. Should we? * AAD Schema will be updated in #151388 (as this one is getting pretty big) to include `revision` so alerts written will include which specific revision of the rule created the alert. ### Reference Fields Any creation of or modification to `actions` will result in a revision increment. More typical reference fields like `exception lists` on the security side will only result in a revision increment when the list is initially associated/deleted from the rule (as subsequent updates will be done directly against the list). ### RuleClient Updates The following methods within the RuleClient have been updated to support incrementing revision when relevant field changes have been detected: * `clone()` - resets to 0 currently (see open question) * `update()` - increments `revision` so long a change has been made to relevant fields (fields not in [ignore list](https://github.com/elastic/kibana/pull/147398/files#diff-6736e143ede2dc06e825bddcdc23b4d088a6620805751db4eddc5900d586c4dfR69-R85)) * `bulkEdit()` - increments `revision` for relevant fields (all current bulk edit fields minus api key/snooze/mute) Mutation methods not updated to include revision log: * `snooze()` * `unsnooze()` * `clearExpiredSnoozes()` * `muteAll()` * `unmuteAll()` * `muteInstance()` * `unmuteInstance()` * `updateApiKey()` - increments revision as rule functionality could be impacted ## Open questions: - [X] Should `clone()` in RulesClient reset revision to 0 as if it's a new rule, or keep the current value? (see [comment](https://github.com/elastic/kibana/pull/147398/files#r1106484105)) - [X] What about snooze/un-snooze, and mute/unmute? Should we update revision on these field changes as well? (see [comment](https://github.com/elastic/kibana/pull/147398/files#r1106431966)) - Discussed with @XavierM and determined to not update on snooze/mute/API key changes as this actions could be plentiful and don't necessarily represent a version of the rule a user would want to revert to, thus polluting the revision history. - [ ] Should we write `revision` to EventLog? --- ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] To work with docs team - [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 ### For maintainers - [X] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
bmorelli25
pushed a commit
to bmorelli25/kibana
that referenced
this issue
Mar 10, 2023
## Summary Resolves elastic#137164. This PR adds a new `revision` field (number) to Alerting Rules that auto-increments when relevant content changes have been made via the `Rules Client`. _Relevant content changes_ are defined as any content change that the user may either want to be notified about, or have the option to later revert to. This will include most all changes, except the enabling/disabling of the rule, or updates to metadata fields like `executionStatus` or `monitoring`. This `revision` field is not intended to be user editable, and should be ignored if ever provided via the API. See elastic#136213 for additional details. To be followed-up by elastic#137168, which will remove the version bump logic from security solution. ## Details ### Migrations Includes SO migration to default `revision` to `0` when upgrading a cluster, or when importing `pre-8.8.0` rules via the SO Management UI. For consistency, security rule import follows the same logic as SO Management and will not reset the `revision` to `0` when overriding or creating a new rule. ### Downstream Index Updates * EventLog _has not_ been updated to include `revision` along with basic rule fields currently being written. Should we? * AAD Schema will be updated in elastic#151388 (as this one is getting pretty big) to include `revision` so alerts written will include which specific revision of the rule created the alert. ### Reference Fields Any creation of or modification to `actions` will result in a revision increment. More typical reference fields like `exception lists` on the security side will only result in a revision increment when the list is initially associated/deleted from the rule (as subsequent updates will be done directly against the list). ### RuleClient Updates The following methods within the RuleClient have been updated to support incrementing revision when relevant field changes have been detected: * `clone()` - resets to 0 currently (see open question) * `update()` - increments `revision` so long a change has been made to relevant fields (fields not in [ignore list](https://github.com/elastic/kibana/pull/147398/files#diff-6736e143ede2dc06e825bddcdc23b4d088a6620805751db4eddc5900d586c4dfR69-R85)) * `bulkEdit()` - increments `revision` for relevant fields (all current bulk edit fields minus api key/snooze/mute) Mutation methods not updated to include revision log: * `snooze()` * `unsnooze()` * `clearExpiredSnoozes()` * `muteAll()` * `unmuteAll()` * `muteInstance()` * `unmuteInstance()` * `updateApiKey()` - increments revision as rule functionality could be impacted ## Open questions: - [X] Should `clone()` in RulesClient reset revision to 0 as if it's a new rule, or keep the current value? (see [comment](https://github.com/elastic/kibana/pull/147398/files#r1106484105)) - [X] What about snooze/un-snooze, and mute/unmute? Should we update revision on these field changes as well? (see [comment](https://github.com/elastic/kibana/pull/147398/files#r1106431966)) - Discussed with @XavierM and determined to not update on snooze/mute/API key changes as this actions could be plentiful and don't necessarily represent a version of the rule a user would want to revert to, thus polluting the revision history. - [ ] Should we write `revision` to EventLog? --- ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] To work with docs team - [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 ### For maintainers - [X] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
spong
added a commit
that referenced
this issue
Mar 24, 2023
## Summary Resolves #137168 by removing the logic to bump the `version` of security rules. Also adds an SO migration to set the initial `revision` as the current `version`. Note: this was originally a branched off #147398, so the large commit list is resulting from there as Github doesn't seem to re-write after after a rebase w/ `main` and a force push. ### Checklist ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ * Found no reference with regards to the auto-incrementing version logic in [current docs](https://www.elastic.co/guide/en/security/current/detection-engine-overview.html). - [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
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Parent ticket: #136213
Summary
After #137164 is implemented, we don't need to bump rule
versionin Security Solution anymore. So we need to remove it, as it will conflict with the rule upgrade logic for prebuilt rules.Consider also migrating the
versionfield of current rules over torevision.Todo
versionon the Security Solution side.revisionfield (write a migration for it in thealertingplugin). For custom rules:revision = version; version = version;. For prebuilt rules:revision = 0; version = version;.revisionto rules returned from the Security Solution's API endpoints.The text was updated successfully, but these errors were encountered: