[RAM] Adds auto-incrementing revision field to rules #147398

spong · 2022-12-12T22:29:39Z

Summary

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 [RAM] Adds revision to alerts schema #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)
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:

Should clone() in RulesClient reset revision to 0 as if it's a new rule, or keep the current value? (see comment)
What about snooze/un-snooze, and mute/unmute? Should we update revision on these field changes as well? (see comment)
- 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 was added for features that require explanation or tutorials
- To work with docs team
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ield-to-rules

pmuellr · 2022-12-14T20:22:33Z

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. This will include most all changes, except the enabling/disabling of the rule, or updates to metadata fields like executionStatus or monitoring.

I believe (and would need to double-check) that the updatedAt field in the rule SO has the same kind of properties, and is also monotonically increasing :-). Any chance we could just use that instead of adding a new field?

spong · 2022-12-14T20:49:00Z

I believe (and would need to double-check) that the updatedAt field in the rule SO has the same kind of properties, and is also monotonically increasing :-). Any chance we could just use that instead of adding a new field?

Good point @pmuellr! It doesn't look like this was discussed in the original issue (#136213), but I can bring it up with the team. I've been out on parental leave and just picked up this task, but from my understanding this is the first step in the larger scope of providing users the ability to have full revision control over their rules, and so having updated_at + updated_by tied to a specific revision will be of use for a swathe of features around reverting to previous revisions, seeing who modified what and when that change occurred as well.

…ield-to-rules

banderror · 2023-03-02T13:53:31Z

EventLog has not been updated to include revision along with basic rule fields currently being written. Should we?

I'd say let's do it in a separate PR and include both fields: version and revision?

banderror

This is an absolute ton of changes in so many files -- thank you for all the efforts @spong! I'm leaning towards merging this PR as soon as we can, but I also have quite a bunch of concerns related to code quality and test coverage that I think we should work more on that, probably in a follow-up PR(s).

As for the code coverage, I suggest to write a test plan for revision calculation/update logic and cover it with a set of integration tests. There are too many moving parts to not have it covered...

Let's sync up on the comments and decide what we do next.

banderror · 2023-03-02T14:02:02Z

x-pack/plugins/alerting/server/rules_client/rules_client.ts

+export const fieldsToExcludeFromRevisionUpdates: ReadonlySet<keyof RuleTypeParams> = new Set([
+  'activeSnoozes',
+  'alertTypeId',
+  'apiKey',
+  'apiKeyOwner',
+  'consumer',
+  'createdAt',
+  'createdBy',
+  'enabled',
+  'executionStatus',
+  'id',
+  'isSnoozedUntil',
+  'lastRun',
+  'monitoring',
+  'muteAll',
+  'mutedInstanceIds',
+  'nextRun',
+  'revision',
+  'running',
+  'snoozeSchedule',
+  'updatedBy',
+  'updatedAt',
+]);


When a new field is added to the rule schema, how do we make sure that the developer who adds the field checks this list and updates or leaves it as is accordingly?

Should we have a test for this similar to what Core has for SO fields?
src/core/server/integration_tests/saved_objects/migrations/group2/check_registered_types.test.ts

banderror · 2023-03-02T14:05:15Z

x-pack/plugins/alerting/server/rules_client/rules_client.ts

+export const fieldsToExcludeFromRevisionUpdates: ReadonlySet<keyof RuleTypeParams> = new Set([
+  'activeSnoozes',
+  'alertTypeId',
+  'apiKey',
+  'apiKeyOwner',
+  'consumer',
+  'createdAt',
+  'createdBy',
+  'enabled',
+  'executionStatus',
+  'id',
+  'isSnoozedUntil',
+  'lastRun',
+  'monitoring',
+  'muteAll',
+  'mutedInstanceIds',
+  'nextRun',
+  'revision',
+  'running',
+  'snoozeSchedule',
+  'updatedBy',
+  'updatedAt',
+]);


fieldsToExcludeFromRevisionUpdates is used from incrementRevision and not used from the file where it's defined. Probably worth moving closer to the usage. Side-effect would be less chances for introducing circular dependencies between files.

banderror · 2023-03-02T14:11:17Z

x-pack/plugins/alerting/server/rules_client/lib/increment_revision.ts

+  currentRule: SavedObject<RawRule>,
+  { data }: UpdateOptions<Params>,
+  updatedParams: RuleTypeParams


At the first glance, these parameters are obscure and it would be helpful to add JSDoc comments clarifying what they exactly mean.

For instance, updatedParams: RuleTypeParams -- is this the same as data.params?

banderror · 2023-03-02T14:12:36Z

x-pack/plugins/alerting/server/rules_client/lib/increment_revision.ts

+import { RawRule, RuleTypeParams } from '../../types';
+import { fieldsToExcludeFromRevisionUpdates, UpdateOptions } from '..';
+
+export function incrementRevision<Params extends RuleTypeParams>(


Nit: maybeIncrementRevision or getNextRevision would be more precise since it doesn't always increment it

banderror · 2023-03-02T14:18:40Z

x-pack/plugins/alerting/server/rules_client/lib/increment_revision.ts

+import { RawRule, RuleTypeParams } from '../../types';
+import { fieldsToExcludeFromRevisionUpdates, UpdateOptions } from '..';
+
+export function incrementRevision<Params extends RuleTypeParams>(


Why is it a generic function? Can we use just the RuleTypeParams type?

banderror · 2023-03-02T15:00:11Z

x-pack/plugins/alerting/server/rules_client/tests/bulk_edit.test.ts

@@ -251,6 +253,7 @@ describe('bulkEdit()', () => {
            type: 'alert',
            attributes: expect.objectContaining({
              tags: ['foo', 'test-1'],
+              revision: 1,


So this is the issue with a lot of our tests: while the test says should add new tag it also verifies that the revision gets incremented. This has a few very important downsides that make it much harder to maintain them when there are a lot of them:

It makes testing (for the revision update logic) implicit and buried under all the other testing logic.

It makes tests more brittle and prone to false positive errors: when the revision update logic changes, it will make tests like should add new tag red although the logic of updating the tags themselves still works correctly.

I think this should be a separate test, and along with other tests, a whole suite of tests checking in what conditions and cases the revision gets incremented and in what cases it doesn't.

Let's please extract tests for revision either into a separate test suite, or just have separate it blocks for them in every relevant test suite under rules_client/tests, and be very specific about what we check in them.

banderror · 2023-03-02T15:07:08Z

x-pack/plugins/alerting/server/rules_client/tests/bulk_edit.test.ts

@@ -302,6 +306,7 @@ describe('bulkEdit()', () => {
            type: 'alert',
            attributes: expect.objectContaining({
              tags: [],
+              revision: 1,


This is a part of the test should delete tag.

I don't get why the revision gets incremented here from 0 to 1, while the test doesn't seem to be deleting a tag that actually exists in a rule.

banderror · 2023-03-02T15:11:54Z

...ins/security_solution/common/detection_engine/rule_management/model/import/rule_to_import.ts

@@ -44,6 +44,7 @@ export const RuleToImport = t.intersection([
      created_by,
      related_integrations: RelatedIntegrationArray,
      required_fields: RequiredFieldArray,
+      revision,


What if we just ignore it and don't add to the schema? Will then a ndjson file with a revision be accepted, but the revision will be stripped out?

banderror · 2023-03-02T15:15:05Z

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts

@@ -544,6 +544,7 @@ export const performBulkActionRoute = (
                      exceptionsList: exceptions,
                    },
                  },
+                  shouldIncrementRevision: () => false,


Looks like a hack 🙂 The only reason is that we need to update the rule the second time with the newly created exception list?

I guess this is ok but could represent some problem in the future when we need to implement rule history -- which is going to be based on revisions.

banderror · 2023-03-02T15:19:46Z

...rity_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts

@@ -141,6 +141,7 @@ export const importRules = async ({
                      exceptions_list: [...exceptions],
                    },
                    allowMissingConnectorSecrets,
+                    shouldIncrementRevision: false,


Why exactly it is false here?
When testing import I noticed a bug and I suspect this might be causing it

banderror

We had a chat with @spong on the next steps and I'm leaning towards fixing leftover bugs within this PR, merging it, and addressing the rest of the comments separately.

…ield-to-rules

kibana-ci · 2023-03-10T21:47:20Z

💚 Build Succeeded

Buildkite Build
Commit: d2366cf

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	515	518	+3

Async chunks

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

id	before	after	diff
`securitySolution`	15.7MB	15.7MB	+412.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`alerting`	40	41	+1

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id	before	after	diff
`alert`	100	101	+1

Unknown metric groups

API count

id	before	after	diff
`alerting`	531	534	+3

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💔 Build #113414 failed a575eb8
💔 Build #113365 failed 7896ea9
💔 Build #113158 failed 2028524
💔 Build #113056 failed eef57f6
💔 Build #112904 failed adf18cc
💔 Build #112853 failed 41d32b3

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

cc @spong

@XavierM

## 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 · 2023-03-11T00:19:49Z

@banderror -- here's the issue for tracking the remaining feedback: #153144

I will resolve your above comments with links to the specific commits they're addressed in 👍

## 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

## Summary Follow up from #147398, which adds `revision` to the alerts schema so the rule's current revision is included when creating alerts. In Security Solution: <img width="500" src="https://user-images.githubusercontent.com/2946766/227386305-c8afe295-b79b-4b28-838a-cc3bed0f3eda.png" /> In Observability: <img width="500" src="https://user-images.githubusercontent.com/2946766/227577019-05307860-e0e3-4e1e-b4cf-604bdb52afdf.png" /> 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 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~ * Base docs to be added for #147398 - [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

## Summary Follow up from elastic#147398, which adds `revision` to the alerts schema so the rule's current revision is included when creating alerts. In Security Solution: <img width="500" src="https://user-images.githubusercontent.com/2946766/227386305-c8afe295-b79b-4b28-838a-cc3bed0f3eda.png" /> In Observability: <img width="500" src="https://user-images.githubusercontent.com/2946766/227577019-05307860-e0e3-4e1e-b4cf-604bdb52afdf.png" /> Note: this was originally a branched off elastic#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 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~ * Base docs to be added for elastic#147398 - [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

## Summary Follow on from #151388 & #147398, which includes the rule's current `revision` when writing to the kibana event-log. Note: Added as `kibana.alert.rule.revision` instead of as ECS field `rule.version` as the [ECS docs](https://www.elastic.co/guide/en/ecs/current/ecs-rule.html#field-rule-version) conflate `version` & `revision` and figured it was best to be explicit. If we do indeed want to use `rule.version` I'll make the change. <img width="500" src="https://user-images.githubusercontent.com/2946766/233216775-f371f412-dcf6-4ef7-a396-84ec853eebbb.png" /> ### 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

Initial plumbing of revision through alerting and security

542bf21

spong self-assigned this Dec 12, 2022

spong added 11 commits December 12, 2022 18:25

Adding revision to FTRs

57c17dc

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

04033c8

…ield-to-rules

Fixing typechecks and more FTRs

6d3aa3d

More typecheck fixes throughout triggers and actions ui

9b28134

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

ef64bb1

…ield-to-rules

More typecheck fixes throughout security

96f386b

Additional typecheck and FTR fixes

31b143a

Jest fixes and detections FTR fixes

bdd5285

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

f70b505

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

9371731

…ield-to-rules

Fixes additional detections FTRs

396ccc1

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

82feace

…ield-to-rules

spong mentioned this pull request Dec 15, 2022

[Security Solution] Add skipped rules to Bulk Edit rules API response #147345

Merged

9 tasks

spong added 5 commits December 16, 2022 11:12

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

3087e63

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

d71e752

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

3120bae

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

b33d3e4

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

6d63ae4

…ield-to-rules

banderror reviewed Mar 2, 2023

View reviewed changes

banderror approved these changes Mar 6, 2023

View reviewed changes

spong added 10 commits March 7, 2023 12:12

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

78834e3

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

8ce5da6

…ield-to-rules

Updating SO hash

41d32b3

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

adf18cc

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

eef57f6

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

2028524

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

7896ea9

…ield-to-rules

Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…

646c2c2

…ield-to-rules

Skips synthetics MonitorSummaryTab flakey test

a575eb8

Updates new export tests to exclude revision

d2366cf

spong merged commit 15f1f64 into elastic:main Mar 10, 2023

spong deleted the adds-revision-field-to-rules branch March 10, 2023 23:19

spong mentioned this pull request Mar 11, 2023

[Security Solution] Rule revision remaining feedback #153144

Open

spong mentioned this pull request Mar 24, 2023

[RAM] Adds revision to event-log #153716

Merged

1 task

banderror mentioned this pull request Mar 31, 2023

[DOCS] Document the new revision rule field elastic/security-docs#3116

Closed

4 tasks

vitaliidm mentioned this pull request Apr 19, 2023

[RAM][Security Solution][Alerts] moves legacy actions migration to rulesClient #153101

Merged

2 tasks

jmikell821 mentioned this pull request May 5, 2023

Add new revision field to alert schema elastic/security-docs#3237

Closed

Mikaayenson mentioned this pull request May 9, 2024

[FR] Add limited support and validation for version and revision elastic/detection-rules#3657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAM] Adds auto-incrementing revision field to rules #147398

[RAM] Adds auto-incrementing revision field to rules #147398

spong commented Dec 12, 2022 •

edited

Loading

pmuellr commented Dec 14, 2022

spong commented Dec 14, 2022

banderror commented Mar 2, 2023

banderror left a comment

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror Mar 2, 2023

banderror left a comment

kibana-ci commented Mar 10, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

spong commented Mar 11, 2023

[RAM] Adds auto-incrementing revision field to rules #147398

[RAM] Adds auto-incrementing revision field to rules #147398

Conversation

spong commented Dec 12, 2022 • edited Loading

Summary

Details

Migrations

Downstream Index Updates

Reference Fields

RuleClient Updates

Open questions:

Checklist

For maintainers

pmuellr commented Dec 14, 2022

spong commented Dec 14, 2022

banderror commented Mar 2, 2023

banderror left a 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

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

kibana-ci commented Mar 10, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

Saved Objects .kibana field count

API count

ESLint disabled line counts

Total ESLint disabled count

History

spong commented Mar 11, 2023

spong commented Dec 12, 2022 •

edited

Loading