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

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

Merged
merged 80 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 49 commits
Commits
Show all changes
80 commits
Select commit Hold shift + click to select a range
542bf21
Initial plumbing of revision through alerting and security
spong Dec 12, 2022
57c17dc
Adding revision to FTRs
spong Dec 13, 2022
04033c8
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 13, 2022
6d3aa3d
Fixing typechecks and more FTRs
spong Dec 13, 2022
9b28134
More typecheck fixes throughout triggers and actions ui
spong Dec 13, 2022
ef64bb1
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 13, 2022
96f386b
More typecheck fixes throughout security
spong Dec 13, 2022
31b143a
Additional typecheck and FTR fixes
spong Dec 13, 2022
bdd5285
Jest fixes and detections FTR fixes
spong Dec 14, 2022
f70b505
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 14, 2022
9371731
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 14, 2022
396ccc1
Fixes additional detections FTRs
spong Dec 14, 2022
82feace
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 15, 2022
3087e63
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Dec 16, 2022
d71e752
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 4, 2023
3120bae
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 23, 2023
b33d3e4
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 25, 2023
6d63ae4
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 25, 2023
7f5d57f
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 26, 2023
1f5ce9f
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 27, 2023
98b57a3
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Jan 30, 2023
daf5099
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 7, 2023
98098b5
Reuse incrementVersion in update()
spong Feb 7, 2023
e6fef49
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 7, 2023
40fa9b0
Adds migration and bulk edit support
spong Feb 7, 2023
e122adb
Adds incrementRevision logic and fixing tests
spong Feb 8, 2023
673d1c5
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 8, 2023
e4279f4
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 8, 2023
4a80747
Resolving remaining test failures
spong Feb 8, 2023
bd87b0d
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 8, 2023
f0f5c32
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 9, 2023
60abd10
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 9, 2023
827ecb3
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 10, 2023
e8f40c6
Update alert SO hash
spong Feb 10, 2023
937b2ed
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 10, 2023
e1ff58a
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 13, 2023
61dc2c0
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 13, 2023
8270e78
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 13, 2023
f7dbf41
Merge branch 'main' into adds-revision-field-to-rules
spong Feb 14, 2023
d162cd8
Remove specific version check from increment logic
spong Feb 14, 2023
ac244db
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 14, 2023
a71a309
Adds migration tests, updates bulk edit snooze tests, update_api_key …
spong Feb 15, 2023
4f971d6
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 15, 2023
2a567c1
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 15, 2023
a1a21e6
Fixes updateAPIKey tests and reset SO registered type hash
spong Feb 15, 2023
88a18f8
Removes revision update on snooze/mute/apiKeyUpdate and updates bulkE…
spong Feb 15, 2023
cc1926b
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 15, 2023
9b7e948
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 16, 2023
440d424
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 16, 2023
cbd1bed
Comments from review and expanding tests
spong Feb 22, 2023
4a39c4d
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 22, 2023
65ce48b
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 22, 2023
2b3ea72
Updates SO hash since last update from merge from main
spong Feb 22, 2023
1784b14
Updating new uuid tests to include revision
spong Feb 22, 2023
c8697d2
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 22, 2023
1a7da2f
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 22, 2023
5619b02
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 23, 2023
ef66c8d
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 23, 2023
62ca943
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 25, 2023
f949386
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 25, 2023
f3b5c4e
Adds shouldIncrementRevision bypass, fixes imports, and clarifies map…
spong Feb 27, 2023
152d53c
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 27, 2023
53a5941
Updating alert SO hash and fixing basic import test
spong Feb 27, 2023
9bd4ffd
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 27, 2023
e6f5a7b
Removing remaining todo's
spong Feb 28, 2023
73ccb27
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Feb 28, 2023
320c706
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 1, 2023
24eae01
Addressing PR comments
spong Mar 1, 2023
d894736
Fixes multi increment when adding bulk actions
spong Mar 2, 2023
640526e
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 2, 2023
78834e3
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 7, 2023
8ce5da6
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 8, 2023
41d32b3
Updating SO hash
spong Mar 8, 2023
adf18cc
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 8, 2023
eef57f6
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 9, 2023
2028524
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 9, 2023
7896ea9
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 10, 2023
646c2c2
Merge branch 'main' of github.com:elastic/kibana into adds-revision-f…
spong Mar 10, 2023
a575eb8
Skips synthetics MonitorSummaryTab flakey test
spong Mar 10, 2023
d2366cf
Updates new export tests to exclude revision
spong Mar 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
Object {
"action": "6cfc277ed3211639e37546ac625f4a68f2494215",
"action_task_params": "db2afea7d78e00e725486b791554d0d4e81956ef",
"alert": "f81ad957a7936522482e4539c7a96a963ebdbc3e",
"alert": "38213efd0ec097dcf9a0d9d608f957ae8724e387",
"api_key_pending_invalidation": "16e7bcf8e78764102d7f525542d5b616809a21ee",
"apm-indices": "d19dd7fb51f2d2cbc1f8769481721e0953f9a6d2",
"apm-server-schema": "1d42f17eff9ec6c16d3a9324d9539e2d123d0a9a",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const mockRule: RuleTableItem = {
ruleType: 'Test Rule Type',
isEditable: true,
enabledInLicense: true,
revision: 0,
};

export const RulesListNotifyBadgeSandbox = ({ triggersActionsUi }: SandboxProps) => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/common/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export interface Rule<Params extends RuleTypeParams = never> {
isSnoozedUntil?: Date | null;
lastRun?: RuleLastRun | null;
nextRun?: Date | null;
revision: number;
running?: boolean | null;
}

Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/alerting/public/alert_api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ describe('loadRule', () => {
"params": Object {
"x": 42,
},
"revision": 0,
"schedule": Object {
"interval": "1s",
},
Expand Down Expand Up @@ -267,6 +268,7 @@ function getApiRule() {
updated_by: '2889684073',
mute_all: false,
muted_alert_ids: [],
revision: 0,
schedule: {
interval: '1s',
},
Expand Down Expand Up @@ -330,5 +332,6 @@ function getRule(): Rule<{ x: number }> {
lastExecutionDate: RuleExecuteDate,
lastDuration: 1194,
},
revision: 0,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ describe('common_transformations', () => {
notify_when: 'onActiveAlert',
mute_all: false,
muted_alert_ids: ['bob', 'jim'],
revision: 0,
execution_status: {
last_execution_date: dateExecuted.toISOString(),
last_duration: 42,
Expand Down Expand Up @@ -183,6 +184,7 @@ describe('common_transformations', () => {
],
},
},
"revision": 0,
"schedule": Object {
"interval": "1s",
},
Expand Down Expand Up @@ -226,6 +228,7 @@ describe('common_transformations', () => {
notify_when: 'onActiveAlert',
mute_all: false,
muted_alert_ids: ['bob', 'jim'],
revision: 0,
execution_status: {
last_execution_date: dateExecuted.toISOString(),
status: 'error',
Expand Down Expand Up @@ -340,6 +343,7 @@ describe('common_transformations', () => {
"nextRun": 2021-12-15T12:34:55.789Z,
"notifyWhen": "onActiveAlert",
"params": Object {},
"revision": 0,
"schedule": Object {
"interval": "1s",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -784,4 +784,5 @@ const BaseRule: SanitizedRule<{ bar: boolean }> = {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ describe('bulkEditInternalRulesRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

const mockedAlerts: Array<SanitizedRule<{}>> = [mockedAlert];
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/clone_rule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe('cloneRuleRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

const ruleToClone: AsApiContract<CreateOptions<{ bar: boolean }>['data']> = {
Expand All @@ -89,6 +90,7 @@ describe('cloneRuleRoute', () => {
created_at: mockedRule.createdAt,
updated_at: mockedRule.updatedAt,
id: mockedRule.id,
revision: 0, // TODO: Finalize clone rule behavior. Clone or reset revision?
execution_status: {
status: mockedRule.executionStatus.status,
last_execution_date: mockedRule.executionStatus.lastExecutionDate,
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/create_rule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('createRuleRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

const ruleToCreate: AsApiContract<CreateOptions<{ bar: boolean }>['data']> = {
Expand All @@ -92,6 +93,7 @@ describe('createRuleRoute', () => {
created_at: mockedAlert.createdAt,
updated_at: mockedAlert.updatedAt,
id: mockedAlert.id,
revision: mockedAlert.revision,
execution_status: {
status: mockedAlert.executionStatus.status,
last_execution_date: mockedAlert.executionStatus.lastExecutionDate,
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/get_rule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ describe('getRuleRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

const getResult: AsApiContract<SanitizedRule<{ bar: boolean }>> = {
Expand All @@ -75,6 +76,7 @@ describe('getRuleRoute', () => {
created_at: mockedAlert.createdAt,
updated_at: mockedAlert.updatedAt,
id: mockedAlert.id,
revision: mockedAlert.revision,
execution_status: {
status: mockedAlert.executionStatus.status,
last_execution_date: mockedAlert.executionStatus.lastExecutionDate,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ describe('createAlertRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

it('creates an alert with proper parameters', async () => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/routes/legacy/get.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ describe('getAlertRoute', () => {
status: 'unknown',
lastExecutionDate: new Date('2020-08-20T19:23:38Z'),
},
revision: 0,
};

it('gets an alert with proper parameters', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const sampleRule: SanitizedRule<RuleTypeParams> & { activeSnoozes?: string[] } =
},
},
nextRun: DATE_2020,
revision: 0,
};

describe('rewriteRule', () => {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/routes/resolve_rule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ describe('resolveRuleRoute', () => {
},
outcome: 'aliasMatch',
alias_target_id: '2',
revision: 0,
};

const resolveResult: AsApiContract<ResolvedSanitizedRule<{ bar: boolean }>> = {
Expand All @@ -87,6 +88,7 @@ describe('resolveRuleRoute', () => {
created_at: mockedRule.createdAt,
updated_at: mockedRule.updatedAt,
id: mockedRule.id,
revision: mockedRule.revision,
execution_status: {
status: mockedRule.executionStatus.status,
last_execution_date: mockedRule.executionStatus.lastExecutionDate,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { SavedObject } from '@kbn/core/server';
import { get, isEqual } from 'lodash';
import { RawRule, RuleTypeParams } from '../../types';
import { fieldsToExcludeFromRevisionUpdates, UpdateOptions } from '..';

export function incrementRevision<Params extends RuleTypeParams>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

currentRule: SavedObject<RawRule>,
{ data }: UpdateOptions<Params>,
updatedParams: RuleTypeParams
Comment on lines +14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

): number {
// Diff root level fields
for (const [field, value] of Object.entries(data).filter(([key]) => key !== 'params')) {
if (!fieldsToExcludeFromRevisionUpdates.map(toString).includes(field)) {
if (!isEqual(value, get(currentRule.attributes, field))) {
return currentRule.attributes.revision + 1;
}
}
}

// Diff rule params
for (const [field, value] of Object.entries(updatedParams)) {
// TODO: Should RuleTypes have a way to declare fields that should be revision skipped as well?
if (!fieldsToExcludeFromRevisionUpdates.map(toString).includes(field)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need to improve the alignment of types here. The fieldsToExcludeFromRevisionUpdates variable is an array of top-level Rule fields with the type Array<keyof Rule>. However, the field variable contains RuleTypeParams fields, which do not intersect with the top-level fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, types got interesting here between rule/attrs/params and their normalized variants, and looks like I still got it wrong, so appreciate the comment 😅.

I ended up changing fieldsToExcludeFromRevisionUpdates to be a Set<keyof RuleTypeParams>, which isn't the best since RuleTypeParams is just Record<string, unknown>, but is at least accurate for updatedParams and data.params off of UpdateOptions. If you see a better way of managing this please let me know.

if (!isEqual(value, get(currentRule.attributes.params, field))) {
return currentRule.attributes.revision + 1;
}
}
}
return currentRule.attributes.revision;
}
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/rules_client/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ export { checkAuthorizationAndGetTotal } from './check_authorization_and_get_tot
export { scheduleTask } from './schedule_task';
export { createNewAPIKeySet } from './create_new_api_key_set';
export { recoverRuleAlerts } from './recover_rule_alerts';
export { incrementRevision } from './increment_revision';
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ async function getUpdatedAttributesFromOperations(
if (isAttributeModified) {
ruleActions = modifiedAttributes;
isAttributesUpdateSkipped = false;
attributes.revision = attributes.revision + 1;
}

// TODO https://github.com/elastic/kibana/issues/148414
Expand Down Expand Up @@ -631,6 +632,7 @@ async function getUpdatedAttributesFromOperations(
attributes = {
...attributes,
...modifiedAttributes,
revision: attributes.revision + 1,
};
isAttributesUpdateSkipped = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export async function clone<Params extends RuleTypeParams = never>(
mutedInstanceIds: [],
executionStatus: getRuleExecutionStatusPending(lastRunTimestamp.toISOString()),
monitoring: getDefaultMonitoring(lastRunTimestamp.toISOString()),
revision: 0, // TODO: Clarify if we're resetting revision since it's a new rule, or carrying over from previous rule (existing security solution behavior)
Copy link
Member Author

@spong spong Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: should a rule clone/duplicate reset revision to 0 or keep the current value? Historic Security behavior has been to keep the current value, but we should align here for consistency (no strong opinion on the security side, but makes sense to me to reset to 0 as if creating a new rule).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When creating new rules, it's logical to set the revision number to 0 because revisions are used to track the history of changes, and a new rule doesn't have any previous history. Moreover, suppose we were to display the revision history to the user through a dropdown menu, allowing them to select which historical revision to revert to. In that case, it could be counterintuitive if the revision numbers started from a value other than zero, which might look like a bug. Like there are some historical versions I don't have access to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing locally, I noticed duplicated rules (through bulk actions) have revision set to 1. So it seems like their revision gets immediately bumped after creation. When I create rules one by one they have revision set to 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, suppose we were to display the revision history to the user through a dropdown menu, allowing them to select which historical revision to revert to. In that case, it could be counterintuitive if the revision numbers started from a value other than zero, which might look like a bug. Like there are some historical versions I don't have access to.

Really good point -- I'll keep it at 0 and remove these TODO's.

While testing locally, I noticed duplicated rules (through bulk actions) have revision set to 1. So it seems like their revision gets immediately bumped after creation. When I create rules one by one they have revision set to 0.

Hmmm, were these rules being duplicated with additional reference objects like actions or exceptions? I came across this in some of the tests, but thought I had it working for the raw duplicate. Let me debug and see what I can do to keep it at 0 when duplicating regardless of if there are references or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okie, looks like it's not specific to the reference scenario, although that'll need to be fixed too (though only applicable for exceptions).

There's a call to update() after create() when duplicating that's supposed to handle the adding of the exceptions portion:

const updatedRule = await rulesClient.update({
id: createdRule.id,
data: {
...duplicateRuleToCreate,
params: {
...duplicateRuleToCreate.params,
exceptionsList: exceptions,
},
},
});

However it looks like we're passing in a slightly different riskScoreMapping (risk_score: undefined) here which is resulting in the revision being incremented:

I'll get a fix in place for this and be sure to add a test on the Security Solution side for this duplication logic. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty, fix is in place as of f3b5c4e. I ended up adding a shouldIncrementRevision callback for consumers as we discussed in the other thread. I didn't fix this risk_score_mapping issue just yet as I want to check with @banderror to see why it may've been typed this way. For now duplicates/clones will work as expected, but updates without changing anything will still result in an unexpected revision update until we resolve this.

scheduledTaskId: null,
running: false,
};
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/rules_client/methods/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export interface CreateOptions<Params extends RuleTypeParams> {
| 'isSnoozedUntil'
| 'lastRun'
| 'nextRun'
| 'revision'
> & { actions: NormalizedAlertAction[] };
options?: SavedObjectOptions;
allowMissingConnectorSecrets?: boolean;
Expand Down Expand Up @@ -153,6 +154,7 @@ export async function create<Params extends RuleTypeParams = never>(
throttle,
executionStatus: getRuleExecutionStatusPending(lastRunTimestamp.toISOString()),
monitoring: getDefaultMonitoring(lastRunTimestamp.toISOString()),
revision: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use resetRevision() here

running: false,
};

Expand Down
19 changes: 17 additions & 2 deletions x-pack/plugins/alerting/server/rules_client/methods/update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import { bulkMarkApiKeysForInvalidation } from '../../invalidate_pending_api_key
import { ruleAuditEvent, RuleAuditAction } from '../common/audit_events';
import { getMappedParams } from '../common/mapped_params_utils';
import { NormalizedAlertAction, RulesClientContext } from '../types';
import { validateActions, extractReferences, updateMeta, getPartialRuleFromRaw } from '../lib';
import {
validateActions,
extractReferences,
updateMeta,
getPartialRuleFromRaw,
incrementRevision,
} from '../lib';
import { generateAPIKeyName, apiKeyAsAlertAttributes } from '../common';

export interface UpdateOptions<Params extends RuleTypeParams> {
Expand Down Expand Up @@ -144,8 +150,9 @@ async function updateWithOCC<Params extends RuleTypeParams>(
async function updateAlert<Params extends RuleTypeParams>(
context: RulesClientContext,
{ id, data, allowMissingConnectorSecrets }: UpdateOptions<Params>,
{ attributes, version }: SavedObject<RawRule>
currentRule: SavedObject<RawRule>
): Promise<PartialRule<Params>> {
const { attributes, version } = currentRule;
const ruleType = context.ruleTypeRegistry.get(attributes.alertTypeId);

// TODO https://github.com/elastic/kibana/issues/148414
Expand Down Expand Up @@ -195,6 +202,13 @@ async function updateAlert<Params extends RuleTypeParams>(
const apiKeyAttributes = apiKeyAsAlertAttributes(createdAPIKey, username);
const notifyWhen = getRuleNotifyWhenType(data.notifyWhen ?? null, data.throttle ?? null);

// Increment revision if applicable field has changed
const revision = incrementRevision<Params>(
currentRule,
{ id, data, allowMissingConnectorSecrets },
updatedParams
);

let updatedObject: SavedObject<RawRule>;
const createAttributes = updateMeta(context, {
...attributes,
Expand All @@ -203,6 +217,7 @@ async function updateAlert<Params extends RuleTypeParams>(
params: updatedParams as RawRule['params'],
actions,
notifyWhen,
revision,
updatedBy: username,
updatedAt: new Date().toISOString(),
});
Expand Down
26 changes: 25 additions & 1 deletion x-pack/plugins/alerting/server/rules_client/rules_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { SanitizedRule, RuleTypeParams } from '../types';
import { SanitizedRule, RuleTypeParams, Rule } from '../types';
import { parseDuration } from '../../common/parse_duration';
import { RulesClientContext, BulkOptions, MuteOptions } from './types';

Expand Down Expand Up @@ -66,6 +66,30 @@ const fieldsToExcludeFromPublicApi: Array<keyof SanitizedRule> = [
'activeSnoozes',
];

export const fieldsToExcludeFromRevisionUpdates: Array<keyof Rule> = [
'activeSnoozes',
'alertTypeId',
'apiKey',
'apiKeyOwner',
'consumer',
'createdAt',
'createdBy',
'enabled',
'executionStatus',
'id',
'isSnoozedUntil',
'lastRun',
'monitoring',
'muteAll',
'mutedInstanceIds',
'nextRun',
'revision',
'running',
'snoozeSchedule',
'updatedBy',
'updatedAt',
];

export class RulesClient {
private readonly context: RulesClientContext;

Expand Down
Loading