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

[Alerting] System action types and helpers #167871

Merged
merged 6 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export * from './rule_tags_aggregation';
export * from './iso_weekdays';
export * from './saved_objects/rules/mappings';

export { isSystemAction } from './system_actions/is_system_action';

export type {
MaintenanceWindowModificationMetadata,
DateRange,
Expand Down
24 changes: 18 additions & 6 deletions x-pack/plugins/alerting/common/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,13 @@ export interface RuleExecutionStatus {
export type RuleActionParams = SavedObjectAttributes;
export type RuleActionParam = SavedObjectAttribute;

export interface RuleActionFrequency extends SavedObjectAttributes {
export interface RuleActionFrequency {
summary: boolean;
notifyWhen: RuleNotifyWhenType;
throttle: string | null;
}

export interface AlertsFilterTimeframe extends SavedObjectAttributes {
export interface AlertsFilterTimeframe {
days: IsoWeekday[];
timezone: string;
hours: {
Expand All @@ -97,7 +97,7 @@ export interface AlertsFilterTimeframe extends SavedObjectAttributes {
};
}

export interface AlertsFilter extends SavedObjectAttributes {
export interface AlertsFilter {
query?: {
kql: string;
filters: Filter[];
Expand All @@ -121,17 +121,27 @@ export const RuleActionTypes = {

export type RuleActionTypes = typeof RuleActionTypes[keyof typeof RuleActionTypes];

export interface RuleAction {
export interface RuleDefaultAction {
uuid?: string;
group: string;
id: string;
actionTypeId: string;
params: RuleActionParams;
frequency?: RuleActionFrequency;
alertsFilter?: AlertsFilter;
type?: typeof RuleActionTypes.DEFAULT;
type: typeof RuleActionTypes.DEFAULT;
}

export interface RuleSystemAction {
uuid?: string;
guskovaue marked this conversation as resolved.
Show resolved Hide resolved
id: string;
actionTypeId: string;
params: RuleActionParams;
type: typeof RuleActionTypes.SYSTEM;
}

export type RuleAction = RuleDefaultAction | RuleSystemAction;

export interface RuleLastRun {
outcome: RuleLastRunOutcomes;
outcomeOrder?: number;
Expand Down Expand Up @@ -195,10 +205,12 @@ export interface SanitizedAlertsFilter extends AlertsFilter {
timeframe?: AlertsFilterTimeframe;
}

export type SanitizedRuleAction = Omit<RuleAction, 'alertsFilter'> & {
export type SanitizedDefaultRuleAction = Omit<RuleDefaultAction, 'alertsFilter'> & {
alertsFilter?: SanitizedAlertsFilter;
};

export type SanitizedRuleAction = SanitizedDefaultRuleAction | RuleSystemAction;

export type SanitizedRule<Params extends RuleTypeParams = never> = Omit<
Rule<Params>,
'apiKey' | 'actions'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* 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 { RuleSystemAction, RuleActionTypes, RuleDefaultAction } from '../rule';
import { isSystemAction } from './is_system_action';

describe('isSystemAction', () => {
const defaultAction: RuleDefaultAction = {
actionTypeId: '.test',
uuid: '111',
group: 'default',
id: '1',
params: {},
type: RuleActionTypes.DEFAULT,
};

const systemAction: RuleSystemAction = {
id: '1',
uuid: '123',
params: { 'not-exist': 'test' },
actionTypeId: '.test',
type: RuleActionTypes.SYSTEM,
};

it('returns true if it is a system action', () => {
expect(isSystemAction(systemAction)).toBe(true);
});

it('returns false if it is not a system action', () => {
expect(isSystemAction(defaultAction)).toBe(false);
});
});
17 changes: 17 additions & 0 deletions x-pack/plugins/alerting/common/system_actions/is_system_action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* 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 { AsApiContract } from '@kbn/actions-plugin/common';
import { RuleAction, RuleSystemAction, RuleActionTypes } from '../rule';

type GetSystemActionType<T> = T extends RuleAction
? RuleSystemAction
: AsApiContract<RuleSystemAction>;

export const isSystemAction = (
action: RuleAction | AsApiContract<RuleAction>
): action is GetSystemActionType<typeof action> => action.type === RuleActionTypes.SYSTEM;
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ interface AlertsFilterAttributes {

export interface RuleActionAttributes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we did not create here two types(system and default) instead of one?

Copy link
Member Author

@cnasikas cnasikas Oct 17, 2023

Choose a reason for hiding this comment

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

Because this type represents what is being saved in ES. The data in ES should not have the action's type so there is not need to distinguish (I don't think it is possible without the type) the two actions.

uuid: string;
group: string;
group?: string;
actionRef: string;
actionTypeId: string;
params: SavedObjectAttributes;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export {
isValidAlertIndexName,
} from './alerts_service';
export { getDataStreamAdapter } from './alerts_service/lib/data_stream_adapter';
export type { ConnectorAdapter } from './connector_adapters/types';

export const plugin = (initContext: PluginInitializerContext) => new AlertingPlugin(initContext);

Expand Down
22 changes: 18 additions & 4 deletions x-pack/plugins/alerting/server/rules_client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { EncryptedSavedObjectsClient } from '@kbn/encrypted-saved-objects-plugin
import { TaskManagerStartContract } from '@kbn/task-manager-plugin/server';
import { IEventLogClient, IEventLogger } from '@kbn/event-log-plugin/server';
import { AuditLogger } from '@kbn/security-plugin/server';
import { DistributiveOmit } from '@elastic/eui';
import { RegistryRuleType } from '../rule_type_registry';
import {
RuleTypeRegistry,
Expand All @@ -29,6 +30,8 @@ import {
SanitizedRule,
RuleSnoozeSchedule,
RawRuleAlertsFilter,
RuleSystemAction,
RuleDefaultAction,
} from '../types';
import { AlertingAuthorization } from '../authorization';
import { AlertingRulesConfig } from '../config';
Expand Down Expand Up @@ -80,18 +83,29 @@ export interface RulesClientContext {
readonly connectorAdapterRegistry: ConnectorAdapterRegistry;
readonly getAlertIndicesAlias: GetAlertIndicesAlias;
readonly alertsService: AlertsService | null;
readonly isSystemAction: (actionId: string) => boolean;
}

export type NormalizedAlertAction = Omit<RuleAction, 'actionTypeId'>;
export type NormalizedAlertAction = DistributiveOmit<RuleAction, 'actionTypeId'>;
export type NormalizedSystemAction = Omit<RuleSystemAction, 'actionTypeId'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

@cnasikas Why in one case you use DistributiveOmit and in another Omit?

Copy link
Member Author

@cnasikas cnasikas Oct 13, 2023

Choose a reason for hiding this comment

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

The reason is that the RuleAction is the union RuleDefaultAction | RuleSystemAction. Normal Omit does not work with unions. If you need to remove a property from each item in the union you need to use DistributiveOmit. For RuleSystemAction is not needed because it is not a union.


export type NormalizedAlertActionWithGeneratedValues = Omit<
NormalizedAlertAction,
'uuid' | 'alertsFilter'
export type NormalizedAlertDefaultActionWithGeneratedValues = Omit<
RuleDefaultAction,
'uuid' | 'alertsFilter' | 'actionTypeId'
> & {
uuid: string;
alertsFilter?: RawRuleAlertsFilter;
};

export type NormalizedAlertSystemActionWithGeneratedValues = Omit<
RuleSystemAction,
'uuid' | 'actionTypeId'
> & { uuid: string };

export type NormalizedAlertActionWithGeneratedValues =
| NormalizedAlertDefaultActionWithGeneratedValues
| NormalizedAlertSystemActionWithGeneratedValues;

export interface RegistryAlertTypeWithAuth extends RegistryRuleType {
authorizedConsumers: string[];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
getAlertIndicesAlias: jest.fn(),
alertsService: null,
connectorAdapterRegistry: new ConnectorAdapterRegistry(),
isSystemAction: jest.fn(),
};

// this suite consists of two suites running tests against mutable RulesClient APIs:
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/rules_client_factory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ test('creates a rules client with proper constructor arguments when security is
getAlertIndicesAlias: expect.any(Function),
alertsService: null,
connectorAdapterRegistry: expect.any(ConnectorAdapterRegistry),
isSystemAction: expect.any(Function),
});
});

Expand Down Expand Up @@ -164,6 +165,7 @@ test('creates a rules client with proper constructor arguments', async () => {
getAlertIndicesAlias: expect.any(Function),
alertsService: null,
connectorAdapterRegistry: expect.any(ConnectorAdapterRegistry),
isSystemAction: expect.any(Function),
});
});

Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/alerting/server/rules_client_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ export class RulesClientFactory {
}
return { apiKeysEnabled: false };
},
isSystemAction(actionId: string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I named it isSystemAction and not isSystemActionConnector because the checks in the alerting plugin will be done upon rule actions.

return actions.isSystemActionConnector(actionId);
},
});
}
}
24 changes: 7 additions & 17 deletions x-pack/plugins/alerting/server/task_runner/execution_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ import {
RuleTypeState,
SanitizedRule,
RuleAlertData,
RuleActionTypes,
RuleDefaultAction,
RuleSystemAction,
RuleNotifyWhen,
} from '../../common';
import {
Expand All @@ -52,6 +53,7 @@ import {
isSummaryActionThrottled,
} from './rule_action_helper';
import { ConnectorAdapter } from '../connector_adapters/types';
import { isSystemAction } from '../../common/system_actions/is_system_action';
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we use actions.isSystemActionConnector (exposed by the rule factory as isSystemAction) instead of using that helper function that checks only the type?


enum Reasons {
MUTED = 'muted',
Expand All @@ -64,7 +66,7 @@ export interface RunResult {
}

interface RunSummarizedActionArgs {
action: RuleAction;
action: RuleDefaultAction;
summarizedAlerts: CombinedSummarizedAlerts;
spaceId: string;
}
Expand All @@ -75,14 +77,14 @@ interface RunActionArgs<
ActionGroupIds extends string,
RecoveryActionGroupId extends string
> {
action: RuleAction;
action: RuleDefaultAction;
alert: Alert<State, Context, ActionGroupIds | RecoveryActionGroupId>;
ruleId: string;
spaceId: string;
}

interface RunSystemActionArgs<Params extends RuleTypeParams> {
action: RuleAction;
action: RuleSystemAction;
connectorAdapter: ConnectorAdapter;
summarizedAlerts: CombinedSummarizedAlerts;
rule: SanitizedRule<Params>;
Expand Down Expand Up @@ -615,7 +617,7 @@ export class ExecutionHandler<
action,
}: {
alert: Alert<AlertInstanceState, AlertInstanceContext, ActionGroupIds | RecoveryActionGroupId>;
action: RuleAction;
action: RuleDefaultAction;
}) {
const alertId = alert.getId();
const { rule, ruleLabel, logger } = this;
Expand Down Expand Up @@ -943,15 +945,3 @@ export class ExecutionHandler<
return bulkActions;
}
}

/**
* TODO: Substitute with a function which takes into
* account system actions.
*
* Because RuleAction has the type set as RuleActionTypes.DEFAULT
* TS produce an error as the check below will always return false.
* We need the check to be able to test.
*/

// @ts-expect-error
const isSystemAction = (action: RuleAction) => action.type === RuleActionTypes.SYSTEM;
9 changes: 8 additions & 1 deletion x-pack/plugins/alerting/server/task_runner/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
RuleLastRunOutcomeOrderMap,
RuleLastRunOutcomes,
SanitizedRule,
SanitizedRuleAction,
RuleActionTypes,
} from '../../common';
import { getDefaultMonitoring } from '../lib/monitoring';
import { UntypedNormalizedRuleType } from '../rule_type_registry';
Expand Down Expand Up @@ -43,6 +45,7 @@ export const RULE_ACTIONS = [
foo: true,
},
uuid: '111-111',
type: RuleActionTypes.DEFAULT,
},
{
actionTypeId: 'action',
Expand All @@ -52,6 +55,7 @@ export const RULE_ACTIONS = [
isResolved: true,
},
uuid: '222-222',
type: RuleActionTypes.DEFAULT,
},
];

Expand Down Expand Up @@ -192,6 +196,7 @@ export const mockedRuleTypeSavedObject: Rule<RuleTypeParams> = {
foo: true,
},
uuid: '111-111',
type: RuleActionTypes.DEFAULT,
},
{
group: RecoveredActionGroup.id,
Expand All @@ -201,6 +206,7 @@ export const mockedRuleTypeSavedObject: Rule<RuleTypeParams> = {
isResolved: true,
},
uuid: '222-222',
type: RuleActionTypes.DEFAULT,
},
],
executionStatus: {
Expand Down Expand Up @@ -283,7 +289,8 @@ export const mockedRule: SanitizedRule<typeof mockedRawRuleSO.attributes.params>
return {
...action,
id: action.uuid,
};
type: RuleActionTypes.DEFAULT,
} as SanitizedRuleAction;
}),
isSnoozedUntil: undefined,
};
Expand Down
Loading
Loading