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.

Sign up for GitHub

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

[Alerting] Passing additional rule fields to rule executor #99819

Merged
merged 18 commits into from
May 24, 2021
Merged

[Alerting] Passing additional rule fields to rule executor #99819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5eb4ded
Passing additional rule fields to rule executor
ymao1 May 11, 2021
41bc70b
Fixing types check
ymao1 May 12, 2021
4a91d53
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 12, 2021
ca9c9ec
Api docs
ymao1 May 12, 2021
443736c
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 12, 2021
2dc4574
Adding enabled and actions
ymao1 May 12, 2021
6dee10e
Api docs
ymao1 May 12, 2021
7c98191
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 12, 2021
3395506
Encapsulating rule information into RuleConfig type
ymao1 May 12, 2021
0a9637c
Functional tests
ymao1 May 12, 2021
6d0d4fc
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 12, 2021
8fd7ffe
Functional tests
ymao1 May 12, 2021
1886751
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 12, 2021
c649270
Merge branch 'master' into alerting/pass-all-rule-params
kibanamachine May 13, 2021
bd86402
Adding producer, ruleTypeId and ruleTypeName
ymao1 May 13, 2021
2d64089
Api docs
ymao1 May 13, 2021
ad4674e
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 May 13, 2021
cb443a8
Merge branch 'master' into alerting/pass-all-rule-params
kibanamachine May 24, 2021
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
129 changes: 84 additions & 45 deletions api_docs/alerting.json
View file
Open in desktop

Large diffs are not rendered by default.

20 changes: 20 additions & 0 deletions x-pack/plugins/alerting/common/alert.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,26 @@ export interface Alert<Params extends AlertTypeParams = never> {

export type SanitizedAlert<Params extends AlertTypeParams = never> = Omit<Alert<Params>, 'apiKey'>;

export type SanitizedRuleConfig = Pick<
SanitizedAlert,
| 'name'
| 'tags'
| 'consumer'
| 'enabled'
| 'schedule'
| 'actions'
| 'createdBy'
| 'updatedBy'
| 'createdAt'
| 'updatedAt'
| 'throttle'
| 'notifyWhen'
> & {
producer: string;
ruleTypeId: string;
ruleTypeName: string;
};

export enum HealthStatus {
OK = 'ok',
Warning = 'warn',
Expand Down
46 changes: 44 additions & 2 deletions x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,13 @@ describe('Task Runner', () => {
kibanaBaseUrl: 'https://localhost:5601',
};

const mockDate = new Date('2019-02-12T21:01:22.479Z');

const mockedAlertTypeSavedObject: Alert<AlertTypeParams> = {
id: '1',
consumer: 'bar',
createdAt: new Date('2019-02-12T21:01:22.479Z'),
updatedAt: new Date('2019-02-12T21:01:22.479Z'),
createdAt: mockDate,
updatedAt: mockDate,
throttle: null,
muteAll: false,
notifyWhen: 'onActiveAlert',
Expand Down Expand Up @@ -155,6 +157,7 @@ describe('Task Runner', () => {
taskRunnerFactoryInitializerParams.actionsPlugin.renderActionParameterTemplates.mockImplementation(
(actionTypeId, actionId, params) => params
);
alertTypeRegistry.get.mockReturnValue(alertType);
});

test('successfully executes the task', async () => {
Expand Down Expand Up @@ -205,6 +208,45 @@ describe('Task Runner', () => {
expect(call.tags).toEqual(['alert-', '-tags']);
expect(call.createdBy).toBe('alert-creator');
expect(call.updatedBy).toBe('alert-updater');
expect(call.rule).not.toBe(null);
expect(call.rule.name).toBe('alert-name');
expect(call.rule.tags).toEqual(['alert-', '-tags']);
expect(call.rule.consumer).toBe('bar');
expect(call.rule.enabled).toBe(true);
expect(call.rule.schedule).toMatchInlineSnapshot(`
Object {
"interval": "10s",
}
`);
expect(call.rule.createdBy).toBe('alert-creator');
expect(call.rule.updatedBy).toBe('alert-updater');
expect(call.rule.createdAt).toBe(mockDate);
expect(call.rule.updatedAt).toBe(mockDate);
expect(call.rule.notifyWhen).toBe('onActiveAlert');
expect(call.rule.throttle).toBe(null);
expect(call.rule.producer).toBe('alerts');
expect(call.rule.ruleTypeId).toBe('test');
expect(call.rule.ruleTypeName).toBe('My test alert');
expect(call.rule.actions).toMatchInlineSnapshot(`
Copy link
Member

@pmuellr pmuellr May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ah, interesting! I missed that on my first quick review of this.

Seems like I remember some conversations with @mikecote where we were a little concerned about rules knowing a lot about their context. Some of it just to keep rule executors from having to deal with too much info, but for actions specifically, could we be leaking any info? In theory action secrets are safe in this context, since they aren't passed in here, just the params. But it is possible that someone would embed some secrets in something like a webhook header. Which would then be visible to the alert executor. Update: The case of webhook headers I mentioned isn't valid - webhook headers are part of the connector config, not params, so wouldn't be passed in here. I suspect there is no issue with "leaking secrets".

I'm trying to think of good reason to actually pass the actions in, but failing to come up with one. Thinking maybe we should remove it based on the security aspect. Update: I don't think there's a security issue here (see update above). But still would like to have @mikecote take a peek.

Copy link
Contributor

@mikecote mikecote May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

TL;DR if we don't have to, it's easier to add later than to remove later. Rule params are designed for the executor, rule object is designed for the framework.

My main concern is passing the actions enables rule types to change the execution behaviour based on what actions the user has configured. I've been pushing for rule executors to just query and flag things worth alerting on and to let the alerting framework handle the actions (notify every, mapping params, per instance, etc.). This principle facilitates future capabilities like rule simulation, alert summary, alert digest, notify after x amount of times, because of the limited context they know about the rule, forcing things to become params instead of rule properties where in the future we could call the rule executor without a rule to run a simulation or something.

I get there are workarounds to uncover the actions but I prefer hearing use cases before providing such capability to ensure it doesn't harm future platform capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this with @mikecote offline and we are ok with moving forward on this for now.

Array [
Object {
"actionTypeId": "action",
"group": "default",
"id": "1",
"params": Object {
"foo": true,
},
},
Object {
"actionTypeId": "action",
"group": "recovered",
"id": "2",
"params": Object {
"isResolved": true,
},
},
]
`);
expect(call.services.alertInstanceFactory).toBeTruthy();
expect(call.services.scopedClusterClient).toBeTruthy();
expect(call.services).toBeTruthy();
Expand Down
25 changes: 25 additions & 0 deletions x-pack/plugins/alerting/server/task_runner/task_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,9 @@ export class TaskRunner<
event: Event
): Promise<AlertTaskState> {
const {
alertTypeId,
consumer,
schedule,
throttle,
notifyWhen,
muteAll,
Expand All @@ -223,12 +226,17 @@ export class TaskRunner<
tags,
createdBy,
updatedBy,
createdAt,
updatedAt,
enabled,
actions,
} = alert;
const {
params: { alertId },
state: { alertInstances: alertRawInstances = {}, alertTypeState = {}, previousStartedAt },
} = this.taskInstance;
const namespace = this.context.spaceIdToNamespace(spaceId);
const alertType = this.alertTypeRegistry.get(alertTypeId);

const alertInstances = mapValues<
Record<string, RawAlertInstance>,
Expand Down Expand Up @@ -265,6 +273,23 @@ export class TaskRunner<
tags,
createdBy,
updatedBy,
rule: {
name,
tags,
consumer,
producer: alertType.producer,
ruleTypeId: alert.alertTypeId,
ruleTypeName: alertType.name,
enabled,
schedule,
actions,
createdBy,
updatedBy,
createdAt,
updatedAt,
throttle,
notifyWhen,
},
});
} catch (err) {
event.message = `alert execution failure: ${alertLabel}`;
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
AlertNotifyWhenType,
WithoutReservedActionGroups,
ActionVariable,
SanitizedRuleConfig,
} from '../common';
import { LicenseType } from '../../licensing/server';

Expand Down Expand Up @@ -89,6 +90,7 @@ export interface AlertExecutorOptions<
services: AlertServices<InstanceState, InstanceContext, ActionGroupIds>;
params: Params;
state: State;
rule: SanitizedRuleConfig;
spaceId: string;
namespace?: string;
name: string;
Expand Down
19 changes: 19 additions & 0 deletions x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,25 @@ const mockOptions = {
tags: [],
createdBy: null,
updatedBy: null,
rule: {
name: '',
tags: [],
consumer: '',
enabled: true,
schedule: {
interval: '1h',
},
actions: [],
createdBy: null,
updatedBy: null,
createdAt: new Date(),
updatedAt: new Date(),
throttle: null,
notifyWhen: null,
producer: '',
ruleTypeId: '',
ruleTypeName: '',
},
};

describe('The metric threshold alert type', () => {
Expand Down
19 changes: 19 additions & 0 deletions ..._solution/server/lib/detection_engine/notifications/rules_notification_alert_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ describe('rules_notification_alert_type', () => {
previousStartedAt: new Date('2019-12-13T16:40:33.400Z'),
createdBy: 'elastic',
updatedBy: 'elastic',
rule: {
name: 'name',
tags: [],
consumer: 'foo',
producer: 'foo',
ruleTypeId: 'ruleType',
ruleTypeName: 'Name of rule',
enabled: true,
schedule: {
interval: '1h',
},
actions: [],
createdBy: 'elastic',
updatedBy: 'elastic',
createdAt: new Date('2019-12-14T16:40:33.400Z'),
updatedAt: new Date('2019-12-14T16:40:33.400Z'),
throttle: null,
notifyWhen: null,
},
};

alert = rulesNotificationAlertType({
Expand Down
19 changes: 19 additions & 0 deletions ...gins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,25 @@ const getPayload = (
previousStartedAt: new Date('2019-12-13T16:40:33.400Z'),
createdBy: 'elastic',
updatedBy: 'elastic',
rule: {
name: ruleAlert.name,
tags: ruleAlert.tags,
consumer: 'foo',
producer: 'foo',
ruleTypeId: 'ruleType',
ruleTypeName: 'Name of rule',
enabled: true,
schedule: {
interval: '1h',
},
actions: [],
createdBy: 'elastic',
updatedBy: 'elastic',
createdAt: new Date('2019-12-13T16:50:33.400Z'),
updatedAt: new Date('2019-12-13T16:50:33.400Z'),
throttle: null,
notifyWhen: null,
},
});

describe('signal_rule_alert_type', () => {
Expand Down
Loading