-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Actions] Add support for system actions in Rules APIs #161726
[Actions] Add support for system actions in Rules APIs #161726
Conversation
…ana into system_actions_registration
@@ -38,7 +38,7 @@ const ruleSnoozeScheduleSchemaWithValidation = schema.object( | |||
); | |||
|
|||
const ruleActionSchema = schema.object({ | |||
group: schema.string(), | |||
group: schema.maybe(schema.string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System actions do not have a group. Changing this to optional to allow system actions to be created without a group.
type: typeof RuleActionTypes.SYSTEM; | ||
} | ||
|
||
export type RuleAction = RuleDefaultAction | RuleSystemAction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is used internally by the rules client.
@@ -16,4 +16,4 @@ import { SanitizedRule } from '../../common'; | |||
* originally registered to {@link PluginSetupContract.registerNavigation}. | |||
* | |||
*/ | |||
export type AlertNavigationHandler = (rule: SanitizedRule) => string; | |||
export type AlertNavigationHandler = (rule: SanitizedRuleResponse) => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The responses from the alerting APIs do not contain the type of action.
@@ -115,8 +118,15 @@ export async function bulkEditRules<Params extends RuleParams>( | |||
context: RulesClientContext, | |||
options: BulkEditOptions<Params> | |||
): Promise<BulkEditResult<Params>> { | |||
try { | |||
bulkEditOperationsSchema.validate(options.operations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation of the schema was missing from the method. The schema requires at least one operation to be set.
@@ -458,20 +472,15 @@ async function updateRuleAttributesAndParamsInMemory<Params extends RuleParams>( | |||
rule.references = migratedActions.resultedReferences; | |||
} | |||
|
|||
const ruleActions = injectReferencesIntoActions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformRuleAttributesToRuleDomain
calls transformRawActionsToDomainActions
which in turn injects the references into the actions. No need to do it twice.
@@ -49,7 +59,7 @@ const bulkEditTagSchema = schema.object({ | |||
const bulkEditActionsSchema = schema.object({ | |||
operation: schema.oneOf([schema.literal('add'), schema.literal('set')]), | |||
field: schema.literal('actions'), | |||
value: schema.arrayOf(bulkEditActionSchema), | |||
value: schema.arrayOf(schema.oneOf([bulkEditDefaultActionSchema, bulkEditSystemActionSchema])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule methods accept two types of actions.
type: schema.literal(RuleActionTypes.DEFAULT), | ||
}); | ||
|
||
export const bulkEditSystemActionSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System actions are not allowed to contain: group
, alertsFilter
, and frequency
.
references?: SavedObjectReference[]; | ||
} | ||
|
||
export const transformRawActionsToDomainActions = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function adds the id from the references and the type
in all actions. The type
is not persisted in ES.
@@ -149,13 +149,13 @@ export const transformRuleAttributesToRuleDomain = <Params extends RuleParams = | |||
})?.toISOString() | |||
: null; | |||
|
|||
let actions = esRule.actions | |||
? injectReferencesIntoActions(id, esRule.actions as RawRule['actions'], references || []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic moved inside transformRawActionsToDomainActions
.
} | ||
|
||
const connectorAdapter = connectorAdapterRegistry.get(connectorTypeId); | ||
const schema = connectorAdapter.ruleActionParamsSchema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the schema of the system action params persisted in the rule.
data: { ...alert, notifyWhen }, | ||
data: { | ||
...alert, | ||
actions: rewriteActionsReq(alert.actions, (connectorId: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds the type
to the actions.
return res.ok({ | ||
body: alertRes, | ||
body: { ...alertRes, actions: rewriteActionsResLegacy(alertRes.actions) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes the type
from the actions.
return []; | ||
} | ||
|
||
return actions.map((action) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds the type
to the actions.
: {}), | ||
})); | ||
|
||
return actions.map((action) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes the type
from the actions.
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Page load bundle
History
To update your PR or re-run it, just comment with: cc @cnasikas |
@@ -27,6 +34,7 @@ function transformAction(input: AsApiContract<RuleAction>): RuleAction { | |||
}, | |||
} | |||
: {}), | |||
...(alertsFilter && { alertsFilter }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the code and left some comments.
I will run and test the PR locally then approve.
@@ -586,6 +587,12 @@ export class ActionsPlugin implements Plugin<PluginSetupContract, PluginStartCon | |||
inMemoryConnectors: this.inMemoryConnectors, | |||
renderActionParameterTemplates: (...args) => | |||
renderActionParameterTemplates(actionTypeRegistry, ...args), | |||
isSystemActionConnector: (connectorId: string): boolean => { | |||
return !!this.inMemoryConnectors.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: inMemoryConnectors.some could be better here.
throw error; | ||
} | ||
|
||
const systemActions = context.inMemoryConnectors.filter((connector) => connector.isSystemAction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are connectors not actions (I know isSystemAction is misleading :) )
Even the return type is FindConnectorResult
@@ -25,8 +25,12 @@ import { | |||
import { MONITORING_HISTORY_LIMIT, RuleTypeParams } from '../../common'; | |||
import { AlertingEventLogger } from '../lib/alerting_event_logger/alerting_event_logger'; | |||
|
|||
export interface RuleData<Params extends RuleTypeParams> extends LoadedIndirectParams<RawRule> { | |||
indirectParams: RawRule; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think indirectParams has been removed by mistake
Closing the PR in favor of smaller PRs to aid the review process |
Summary
In this PR we allow the usage of system actions through the Rules APIs. Major decisions:
type
property only inside the methods of the rule client.type
of the action. The framework will detect this automatically.type
to the actions before passing the request to the rule client.type
.type
from the actions before returning the response to the client.group
is now optional because system actions do not support it.type
of the action will not be persisted to ES.To keep the size of the PR small, and because it will be merged into a feature branch, I decided to work in the Create Rule, Update Rule, and Bulk Edit Rules APIs. I also follow TS errors and failed tests and update some of the other routes/client methods. Another PR will follow to handle the rest of the APIs and the CI errors produced by these APIs and another PR will follow to handle the APIs of Security Solution.
Issue: #160367
Depends on: #165671
Checklist
Delete any items that are not applicable to this PR.
For maintainers