[Alerting] Passing additional rule fields to rule executor

[ymao1]

Resolves #95843

Summary

Passing additional rule information to the rule executor, encapsulated in a rule object:

• consumer
• schedule
• createdAt
• updatedAt
• throttle
• notifyWhen
• enabled
• actions

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[pmuellr]

These fields all seem safe enough to me.

I'm wondering though, if this is basically all the fields in our existing alert structure(s), and so if we should encapsulate these into an alert object we pass in. That probably makes sense if there is an existing alert structure we already have that has the shape we want.

And thinking we should ping security / o11y / maps / etc to see if there are other fields they'd want, though I'd guess there's not much more we can provide.

Seems like we can add a function test for this.

It probably makes sense to add these as additional mustache variables, or at least some of them. I think maybe that's better done as a follow-on issue, although perhaps my point at the top about moving the new properties under a new alert object property would be influenced by how we decide to expose the mustache variables. I don't think there's a hard requirement that they be done the exact same way (flattened with other props, or under an alert object), but ... not sure.

[ymao1]

These fields all seem safe enough to me.

I'm wondering though, if this is basically all the fields in our existing alert structure(s), and so if we should encapsulate these into an alert object we pass in. That probably makes sense if there is an existing alert structure we already have that has the shape we want.

@pmuellr I considered doing this but we are already passing some rule fields like createdBy, updatedBy, name, and tags in as a top level field, so encapsulating all the rule information into an object would break executors that are currently using those fields. I suppose we could do it in stages? Pass all the information in as an object, inform all the rules that they need to update their executors and then remove the existing top level fields?

And thinking we should ping security / o11y / maps / etc to see if there are other fields they'd want, though I'd guess there's not much more we can provide.

I have been working with security, but you're right, at this point the only fields we arent' passing in are scheduledTaskId, apiKeyOwner, muteAll, mutedInstanceIds and executionStatus.

[marshallmain]

++ to encapsulating the fields in an Alert object - for security, it would be very convenient to have the fields passed in that way since we pass the Alert fields around to various utility functions so it would be great to pass them all together.

[ecezalp]

LGTM, thank you for adding in the required fields

[YulNaumenko]

And thinking we should ping security / o11y / maps / etc to see if there are other fields they'd want, though I'd guess there's not much more we can provide.

FYI @aaronjcaldwell

[ymao1]

@pmuellr encapsulated fields into a rule object (to follow the new terminology) in this commit: 3395506. I am leaving the existing fields as is, so for now createdBy, updatedBy, name, and tags will be available both top level and inside the rule object.

Updated the functional tests in this commit: 0a9637c

[YulNaumenko]

Code LGTM! Relying on the consumer teams on that we've provided all the needed rule data.

[ymao1]

@elasticmachine merge upstream

[dgieselaar]

@ymao we write the following data from the rule instance to the alert:

• rule.uuid: the rule instance id (options.alertId)
• tags: the tags attached to the rule instance (options.tags)
• rule.id: the rule type id
• rule.category: the rule instance name
• producer: the rule type producer

The last three are currently not available in the executor, so they need to be manually passed in. If we can add those three as well, we can get all the data we need from the executor, rather than having access to the rule type as well.

[pmuellr]

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.

[mikecote]

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.

[ymao1]

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

[pmuellr]

LGTM, but I have a question about exposing the actions that I'd like @mikecote to chime in on.

Per other comment I made in the PR, let's open an issue to remove the older top-level vars that are now in rule (I assume this is straight-forward). I also commented on whether we'd want to expose some new mustache variables for these - not completely sure, but ... let's open an issue for that too; seems like it could be useful.

[ymao1]

Per other comment I made in the PR, let's open an issue to remove the older top-level vars that are now in rule (I assume this is straight-forward). I also commented on whether we'd want to expose some new mustache variables for these - not completely sure, but ... let's open an issue for that too; seems like it could be useful.

Issue for removing older top level variables: #100115
Issue for adding context variables for new rule fields: #100116

[ecezalp]

in regards to how the actions is used, we take a look at the length of the actions array, and determine the refresh parameter for the bulkCreate method.

const refresh = actions.length ? 'wait_for' : false;

const { body: response } = await esClient.bulk({
    refresh,
    body: bulkBody,
  });

[ymao1]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: cb443a8
• Storybooks Preview
• Documentation Changes

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	211	213	+2

Unknown metric groups

API count

id	before	after	diff
alerting	211	213	+2

References to deprecated APIs

id	before	after	diff
canvas	29	25	-4
crossClusterReplication	8	6	-2
fleet	22	20	-2
globalSearch	4	2	-2
indexManagement	12	7	-5
infra	261	149	-112
lens	67	45	-22
licensing	18	15	-3
maps	286	208	-78
ml	121	115	-6
monitoring	109	56	-53
stackAlerts	101	95	-6
total			-295

History

• 💚 Build #125852 succeeded 2d64089
• 💚 Build #125853 succeeded ad4674e
• 💔 Build #125806 failed c649270
• 💚 Build #125737 succeeded 1886751
• 💔 Build #125718 failed 6d0d4fc

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

cc @ymao1

[jasonrhodes]

LGTM, thanks

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.