Remove mute/unmute logic when bulk updating rule actions

[jpdjere]

Intro

This PR modifies the logic of bulk updating rule actions, in preparation for https://github.com/elastic/kibana/pull/137430

Summary

• Removes the mute/unmute logic for bulk updating rule actions
• Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when updating rules individually)
• Also corrects bulk update rule actions flyout, so that:
  • available actions are always displayed
  • copy referring to using "Perform No Actions" to mute all selected rules is no longer displayed.

Screenshots

Removed unwanted copy and "On each rule execution" selected as default

"Perform No Action" option no longer available

Checklist

Delete any items that are not applicable to this PR.

• [ ] Documentation was added for features that require explanation or tutorials
• [ ] Unit or functional tests were updated or added to match the most common scenarios
• [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• [ ] This was checked for cross-browser compatibility

For maintainers

• [ ] This was checked for breaking API changes and was labeled appropriately

[jpdjere]

@jethr0null @banderror

One additional thing to take into account is that, if we remove the Perform no actions option from the throttle dropdown for bulk rule actions, then we also lose the feature of "deleting all rules for all selected actions". As the explanation on the screenshot says, a user could do that by choosing Perform no actions and checking the Overwrite all selected rules actions checkbox.

Are we OK with that?

[jpdjere]

Can we remove muteAll related code from our create and update rule endpoints?

CREATING A RULE

We don’t seem to need to mute the rule on creation: the rule is only muted during creation if throttle is set to NOTIFICATION_THROTTLE_NO_ACTIONS (no_actions), which doesn’t make much sense because there are no actions to perform anyway.

Params of the rules upon creating it, with the current logic:

{
  "throttle": null,
  "notifyWhen": "onActiveAlert",
  "actions": [],
  "muteAll": true,
}

And NO actions are executed

Params of the rules upon creating it, if we delete the code the mutes the rule on create:

{
  "throttle": null,
  "notifyWhen": "onActiveAlert",
  "actions": [],
  "muteAll": false,
}

And NO actions are executed

Only difference in the UI here is that in Stack Management, if you check in the Rules Detail page, a UI icon informs if the rule is muted or not. It will show that the rule is unmuted immediately upon creation, if no actions are added (while with the present code, all rules with no actions show up as muted):

If rule IS muted on creation (current logic):

If rule is NOT muted on creation:

---

UPDATING A RULE

When updating a rule, we execute the maybeMute function which mutes the rule if it is currently not muted and the updated throttle value is NOTIFICATION_THROTTLE_NO_ACTIONS.

Params of the rule upon editing it, with the current logic:

1. Add some action to be run "On Each Rule" execution
{
  "throttle": null,
  "notifyWhen": "onActiveAlert",
  "muteAll": false,
  "actions": [
    {
      "actionTypeId": ".webhook",
      "params": {
        "body": "New Rule 1 test"
      },
      "actionRef": "action_0",
      "group": "default"
    }
  ],
}

**Actions are executed**

2. Set Action options back to "Perform No actions"

"throttle": null,
"notifyWhen": "onActiveAlert",
"muteAll": true,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "New Rule 1 test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

**Actions are NOT executed**

Which is the expected behaviour.

Params of the rule upon updating it, if rule is NOT muted:

1. Add some action to be run "On Each Rule" execution

"throttle": null,
"notifyWhen": "onActiveAlert",
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "New Rule 2 - test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

Actions are executed


2. Set Action options back to "Perform No actions"

"throttle": null,
"notifyWhen": "onActiveAlert",
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "New Rule 2 - test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

Actions are **STILL** executed

Which is, of course, not the expected behaviour.

Also in the UI, in the Actions tab still shows that the actions run “On each rule execution” instead of “Perform No Actions”. Looks like the UI depends on the muteAll param instead of throttle or notifyWhen .

---

Conclusion and Possible steps

The muteAll parameter is not orthogonal and completely uncoupled with the throttle and notifyWhen parameter. They still depend on each other for the actions functionality to work as expected.

This is why we cannot remove muteAll related logic from rule update endpoints without breaking the actions logic. (We could remove it, though, from the create rule endpoint, which would imply only a minor UI change)

It seems that if we want to make the logic uncoupled we would need to add an extra possible value to notifyWhen such as never (that should be its default value) and should be checked independently from muteAll in order to make the decision if to execute the rule actions or not. This way we could use muteAll to temporarily mute rules, independently of the value of notifyWhen .

[banderror]

One additional thing to take into account is that, if we remove the Perform no actions option from the throttle dropdown for bulk rule actions, then we also lose the feature of "deleting all rules for all selected actions". As the explanation on the screenshot says, a user could do that by choosing Perform no actions and checking the Overwrite all selected rules actions checkbox.

Are we OK with that?

@jpdjere Yea we're ok with that. We need to remove this sentence from the callout.

[banderror]

@jpdjere Thank you for the detailed explanation, it's super clear and helpful for understanding how it all works.

Can we achieve the following behavior when updating a rule?

Params of the rule upon updating it, if rule is NOT muted:

1. Add some action to be run "On Each Rule" execution

"throttle": null,
"notifyWhen": "onActiveAlert",
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "New Rule 2 - test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

Actions are executed

Alternative 1:

2. Set Action options back to "Perform No actions"

"throttle": null,
"notifyWhen": null,
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "New Rule 2 - test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

Actions are **NOT** executed

Alternative 2:

2. Set Action options back to "Perform No actions"

"throttle": null,
"notifyWhen": null,
"muteAll": false,
"actions": [],

Actions are **NOT** executed

Then, on the Rule Editing page, we would not need to check muteAll whatsoever, and muting would be orthogonal to actions and their frequency.

[jpdjere]

@banderror

Alternative 1

It looks I've found the reason why muteAll had to be used as a "workaround" for effectively stopping actions from being executed: with the current implementation, if we try to update notifyWhen the value does not get updated in the saved object. There is even a comment from Frank explicitly mentioning this:

export const transformToNotifyWhen = (
  throttle: string | null | undefined
): RuleNotifyWhenType | null => {
  if (throttle == null || throttle === NOTIFICATION_THROTTLE_NO_ACTIONS) {
    return null; // Although I return null, this does not change the value of the "notifyWhen" and it keeps the current value of "notifyWhen"
  } else if (throttle === NOTIFICATION_THROTTLE_RULE) {
    return 'onActiveAlert';
  } else {
    return 'onThrottleInterval';
  }
};

transformToNotifyWhen is used in the update_rules handler.

(Is this related to the model behind the alert data model as saved object?)

We can however, set it to never (or any other string) and the saved object will effectively update. However, if we do this and do not mute the rule, the actions will continue to be executed. (So it still looks as the execution of the actions does not depend on notifyWhen being onActiveAlert).

Alternative 2

This is possible and it indeeds stops the execution of the actions (which makes sense because there are no actions related to that rule anymore).

The only issue here is that it implies a small UX degradation: since all actions are removed, when you try to add them back they are no longer there.

The current behaviour, when switching from having actions -> Perform No Actions -> back to actions: So the actions the user had are maintained, and don't have to be recreated.

If we delete all actions, they need to be recreated:

But at least the connectors for each action are still there:

[jpdjere]

@banderror

Had a call with @XavierM today and discussed the possibility of working on Alternative 1, setting notifyWhen to null and having that prevent the actions from being sent by the Alerting Framework.

Xavier explained his position against working to fix that in that way, and that the implementation for muting rule actions should work exclusively with the muteAll property, since the logic with notifyWhen and throttle was kind of a hack to get it work initially.

We discussed the following:

• finishing this PR as it is now, removing the option for “Perform no actions” from the bulk update rule actions dropdown options ONLY.
• releasing 8.5 with the current usage of muteAll to mute individual rules when the Perform No Actions options is selected. In 8.5 the snooze functionality that Xavier worked on won't be enabled for Security Rules, only Observability and Stack rules, so the new functionality won't break the current logic (the use of muteAll to prevent actions from being executed).
• Start work to get to 8.6 with the following logic:
  • muting rule actions should work exclusively with the muteAll property and the new snooze functionality
  • the default value for actions is "On each rule execution" and we get rid of "Perform no actions"

Given all of the above, do you think there is still the need to work on Alternative 2 for 8.5?

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[jpdjere]

@joepeeples @banderror

This PR introduces changes to the Bulk Editing of Rule Actions that might need some updating of docs to what was already discussed in the PR that added them originally: https://github.com/elastic/kibana/pull/138900

This PR already removes some copy that is made redundant (details above), but could you please take a look?

Here's a link to a cloud deployment: https://kibana-pr-140626.kb.us-west2.gcp.elastic-cloud.com:9243/

Please ping me for the password

[jpdjere]

Decisions taken on sync with @banderror and @xcrzx :

• Correct PR to reintroduce logic that unmutes rule when bulk actions are set (otherwise previously muted rules will not execute actions when they are bulk added to them)
• Introduce tests for coverage on behaviour of rules which are edited individually and then edited on bulk
• Test behaviour of Alerting Framework when notifyWhen is written to null by modifying current logic
• Open issue for Response-Ops to have Alerting Framework stop executing actions when notifyWhen is set to null.

[jpdjere]

@banderror @xcrzx

Tested locally correcting the logic so that the notifyWhen param can be set to null instead of being corrected to onActiveAlert.

A rule with the following params:

"throttle": null,
"notifyWhen": null,
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "Second Test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

still executes the actions, so it looks like the Alerting Framework decides whether to execute the actions exclusively by reading the muteAll param and ignores notifyWhen and throttle.

[banderror]

A rule with the following params:

"throttle": null,
"notifyWhen": null,
"muteAll": false,
"actions": [
  {
    "actionTypeId": ".webhook",
    "params": {
      "body": "Second Test"
    },
    "actionRef": "action_0",
    "group": "default"
  }
],

still executes the actions, so it looks like the Alerting Framework decides whether to execute the actions exclusively by reading the muteAll param and ignores notifyWhen and throttle.

Oh that's sad. Alright, thank you so much for testing this @jpdjere and clarifying the Framework's behavior. I will write up a bug for the ResponseOps team about it.

[jpdjere]

Thanks for the review @banderror . Addressed your comments and rebased.

The bug you mentioned above (the UI displaying "Perform No Actions" even if there are actions for the rules) is indeed fixed by unmuting the rules when bulk adding rule actions, as @xcrzx proposed.

[xcrzx]

I don't think it is related to this PR but found that the displayed number of rules to edit is wrong:

I'll create a separate issue for that.

[xcrzx]

I receive a Bag Request error when bulk adding empty actions. Not sure what should be the correct behavior in that case, though. Probably, a form validation error saying that at least one action should be added would suffice.

https://user-images.githubusercontent.com/1938181/191938062-2966b5e4-422a-4e32-bbc7-98df1a5222ba.mov

The error message is too vague to understand what is the root cause:

[request body]: Invalid value "edit" supplied to "action",Invalid value "add_rule_actions" supplied to "edit,type",Invalid value "{"actions":[],"throttle":"1h"}" supplied to "edit,value",Invalid value "undefined" supplied to "edit,value,timeline_id",Invalid value "undefined" supplied to "edit,value,timeline_title",Invalid value "1h" supplied to "edit,value,throttle",Invalid value "undefined" supplied to "edit,value,interval",Invalid value "undefined" supplied to "edit,value,lookback" (400)

[jpdjere]

@

I receive a Bag Request error when bulk adding actions:

Screen.Recording.2022-09-23.at.12.04.08.mov Error message:

[request body]: Invalid value "edit" supplied to "action",Invalid value "add_rule_actions" supplied to "edit,type",Invalid value "{"actions":[],"throttle":"1h"}" supplied to "edit,value",Invalid value "undefined" supplied to "edit,value,timeline_id",Invalid value "undefined" supplied to "edit,value,timeline_title",Invalid value "1h" supplied to "edit,value,throttle",Invalid value "undefined" supplied to "edit,value,interval",Invalid value "undefined" supplied to "edit,value,lookback" (400)

@xcrzx Did you pull the most recent changes? Sorry, I was still fixing the types, but it should work now.

[xcrzx]

Did you pull the most recent changes?

I'm testing on 5af4ac1dc41, and it looks like it is the latest commit.

[jpdjere]

For reviewers: please rebuild the packages if you pull the latest version, the latest changes edit the io-ts types in the packages. Otherwise requests will fail.

[banderror]

receive a Bag Request error when bulk adding empty actions. Not sure what should be the correct behavior in that case, though. Probably, a form validation error saying that at least one action should be added would suffice.

Bulk adding rule actions with an empty array of actions should work because that's how the user can change the frequency.

[xcrzx]

Bulk adding rule actions with an empty array of actions should work because that's how the user can change the frequency.

Yes, that's right. I was just confused by the validation error. The problem was that the package with schema didn't rebuild for some reason, so after rebuilding it manually, the error was gone.

[jpdjere]

@banderror Fixed the TimeDuration type to make its time units configurable. Want to take one final look?

Also, I'm about to put up for review this PR with the fixes and added test coverage for the Bulk Edit Schedules, and it includes an additional change to the type, to throw error on decimal numbers. If we merge this PR I'll rebase to main and fix the conflicts

[vitaliidm]

@jpdjere, @banderror what's the reasoning behind?

Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when creating or editing rules individually)

This would limit our users' ability to edit rule actions in bulk.

[jpdjere]

@jpdjere, @banderror what's the reasoning behind?

Remove option for “Perform no actions” from the bulk update rule actions dropdown options ONLY (option still available when creating or editing rules individually)

This would limit our users' ability to edit rule actions in bulk.

This PR changes are in preparation for https://github.com/elastic/kibana/pull/137430. We want to remove all logic related to the muteAll param for rule actions, which should only depend on throttle and notifyWhen.

In the PR linked above, Xavier worked on a POC to introduce the snooze functionality, which should depend only on the muteAll param, and be completely uncoupled from the rules actions parameters. This PR is a first step to that. We could not remove all related muteAll logic without breaking the functionality as it works today, but we removed it where we could, starting with removing the feature to bulk update rule actions to "Perform No Actions".

Next steps would be to have rule actions not triggered when notifyWhen is set to null (there is an upcoming ticket for this), in order to clear the way to remove all remaining muteAll logic related to rule actions.

[banderror]

Files by Code Owner

elastic/security-detections-response-rules

• x-pack/plugins/security_solution/common/detection_engine/schemas/request/perform_bulk_action_schema.ts
• x-pack/plugins/security_solution/public/detections/components/rules/description_step/throttle_description.tsx
• x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx
• x-pack/plugins/security_solution/public/detections/components/rules/throttle_select_field/index.tsx
• x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/bulk_actions/forms/rule_actions_form.tsx
• x-pack/plugins/security_solution/server/lib/detection_engine/rules/bulk_edit_rules.ts

elastic/security-solution

• x-pack/plugins/security_solution/common/detection_engine/schemas/request/perform_bulk_action_schema.ts
• x-pack/plugins/security_solution/public/detections/components/rules/description_step/throttle_description.tsx
• x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx
• x-pack/plugins/security_solution/public/detections/components/rules/throttle_select_field/index.tsx
• x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/bulk_actions/forms/rule_actions_form.tsx
• x-pack/plugins/security_solution/server/lib/detection_engine/rules/bulk_edit_rules.ts
• x-pack/test/detection_engine_api_integration/security_and_spaces/group10/perform_bulk_action.ts

elastic/security-solution-platform

• packages/kbn-securitysolution-io-ts-alerting-types/index.ts
• packages/kbn-securitysolution-io-ts-alerting-types/src/default_throttle_null/index.test.ts
• packages/kbn-securitysolution-io-ts-alerting-types/src/default_throttle_null/index.ts
• packages/kbn-securitysolution-io-ts-alerting-types/src/throttle/index.ts
• packages/kbn-securitysolution-io-ts-types/src/time_duration/index.test.ts
• packages/kbn-securitysolution-io-ts-types/src/time_duration/index.ts

[vitaliidm]

In the PR linked above, Xavier worked on a POC to introduce the snooze functionality, which should depend only on the muteAll param, and be completely uncoupled from the rules actions parameters. This PR is a first step to that. We could not remove all related muteAll logic without breaking the functionality as it works today, but we removed it where we could, starting with removing the feature to bulk update rule actions to "Perform No Actions".

Are we going to remove "perform no actions" for single rule update as well in future?

[banderror]

Are we going to remove "perform no actions" for single rule update as well in future?

@vitaliidm This is TBD and will depend on the UI design of the Actions step.

[kibana-ci]

:yellow_heart: Build succeeded, but was flaky

• Buildkite Build
• Commit: eeee4319afe3e5ed4555156823d4f1d8e0ef8a34
• Cloud Deployment

Failed CI Steps

• Security Solution Tests #2

Test Failures

• [job] [logs] Security Solution Tests #2 / Alerts detection rules table auto-refresh should disable auto refresh when any rule selected and enable it after rules unselected

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	3153	3152	-1

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
@kbn/securitysolution-io-ts-alerting-types	129	127	-2
@kbn/securitysolution-io-ts-types	33	36	+3
total			+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lists	152.9KB	152.6KB	-352.0B
securitySolution	6.6MB	6.6MB	-686.0B
total			-1.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
securitySolution	263.3KB	263.6KB	+357.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/securitysolution-io-ts-alerting-types	148	145	-3
@kbn/securitysolution-io-ts-types	63	65	+2
total			-1

History

• :green_heart: Build #76019 succeeded 036abb733b3a7fe6a63eb09829dfd64c1b20d371
• :broken_heart: Build #75806 failed 42b29f050979102f0fd290179ea89b402b9fc2dc
• :broken_heart: Build #75780 failed b9ba0df12a775b7a5cce2a9da302b45dbe9cbb88
• :broken_heart: Build #75341 failed 4014894f50f16c33cb64c9fd568a56d705c8e186
• :broken_heart: Build #75322 failed 95f03234c3aeb299429185c73f7ed20b0344f601

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

cc @jpdjere

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation