[Security Solution] Allow users to edit max_signals field for custom rules

[dplumlee]

Resolves: https://github.com/elastic/kibana/issues/173593 Fixes: https://github.com/elastic/kibana/issues/164234

Summary

Adds a number component in the create and edit rule forms so that users are able to customize the max_signals value for custom rules from the UI. Also adds validations to the rule API's for invalid values being passed in.

This PR also exposes the xpack.alerting.rules.run.alerts.max config setting from the alerting framework to the frontend and backend so that we can validate against it as it supersedes our own max_signals value.

Flaky test run (internal)

Screenshots

Form component

Details Page

Error state

Warning state

Checklist

Delete any items that are not applicable to this PR.

• [x] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• [x] Documentation was added for features that require explanation or tutorials
• [x] Unit or functional tests were updated or added to match the most common scenarios
• [x] Flaky Test Runner was used on any tests changed
• [x] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

For maintainers

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

[dplumlee]

How do we feel about selecting the name for max_signals to be "Max signals"? Obviously that's the field name but seeing as this is the first time (other than maybe this) we're exposing this value to users in the UI and not just the API, do we want to call it as such? The field was created back when we were still using the signals language instead of alerts. cc @joepeeples @approksiu

[joepeeples]

How do we feel about selecting the name for max_signals to be "Max signals"? Obviously that's the field name but seeing as this is the first time (other than maybe this) we're exposing this value to users in the UI and not just the API, do we want to call it as such? The field was created back when we were still using the signals language instead of alerts. cc @joepeeples @approksiu

Haha, jinx, we both asked the same thing just now. 😜 I think "Max alerts" is a more meaningful label to users — I assume we can't also rename the field itself (due to breaking changes, etc)? It might be a little odd to have different names in the UI and in the data, but I don't know if users are likely to notice, if max_signals isn't directly exposed to them.

[dplumlee]

@joepeeples yeah, we can't easily rename the field without a lot of problems but in the one prior example we have of referring to the value in the UI, we use "max alerts" so I reckon that's probably a good enough solution. It won't break anything for API users anyways so I don't think we would run into too much confusion adding it to the rule forms. I'll go ahead and do that then unless @approksiu had another idea in mind.

[dplumlee]

/ci

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

[marshallmain]

For the UI label, I think we should consider adding the timeframe to the name as well - Max alerts per run. Otherwise it may appear to users that the rule can only generate some number of alerts ever.

[approksiu]

Thanks @dplumlee!

Could you please fix the offset after this new field? It is larger than for the other components:

Would it make sense to mention allowed values in the note (Choose a value between 1 and __)?

[ARWNightingale]

@dplumlee Could we reduce the width of the input so it does not cover the whole width, could it match other numerical inputs, I think they may be around 200px width. Also could we make sure the padding below matches the rest it seems to be slightly larger. Thanks

[jpdjere]

Hi @dplumlee Thanks for working on this!

I have a major doubt related to the xpack.alerting.rules.run.alerts.max config setting, and maybe @banderror @marshallmain and @approksiu can chime in from a tech and UX point of view.

What Davis implemented here is what's originally in the ticket as acceptance criteria: the global setting has a higher "prio" than the rule, so that the individual rules' max_signals field cannot be set to anything higher than the value of the setting. And we validate against it and prevent the rule from being saved:

However, when I discussed this with @banderror, -correct me here if I'm wrong- he was of the opinion that the behaviour of global setting should be that it should be overridable on a narrower scope. In this case, that would mean that the rule should be able to have a higher value than what's set in the global config if the user sets it manually.

What I'm trying to understand here is how this global setting currently behaves: if the value in the global setting is lower than the max_signals value in the rule, what is the actual max number of alerts produced by the Detection Engine (or is it the Alerting Framework)? In other words, does the rule currently override the global setting? Do you know @marshallmain?

And, if the rule setting overrides the global setting, what is the use of having this global setting? The global setting is not the default, it is a separate value, so that's not its function.

(Also: from what I see, xpack.alerting.rules.run.alerts.max is only settable via the kibana.yml config, is that right? I do not see it in the global Kibana setting in the UI)

[banderror]

However, when I discussed this with @banderror, -correct me here if I'm wrong- he was of the opinion that the behaviour of global setting should be that it should be overridable on a narrower scope. In this case, that would mean that the rule should be able to have a higher value than what's set in the global config if the user sets it manually.

@jpdjere Great questions! I don't exactly remember our conversation, but my current thinking would be:

• If xpack.alerting.rules.run.alerts.max defines a hard maximum value, then it means no rules should generate more than this number regardless of their own settings.
• If xpack.alerting.rules.run.alerts.max defines a default for the max_signals parameter for each rule, then it should be possible to set the number higher than that, as long as it is lower than the hard maximum value, which seems to be 1000.
• Also, what happens when, for example, xpack.alerting.rules.run.alerts.max gets updated from, let's say, 1000 to 50, while an existing rule has max_signals set to 100. What should be the behavior of rules during execution and how validation should work on the Rule Editing page?

[dplumlee]

@jpdjere you are correct in thinking that xpack.alerting.rules.run.alerts.max is a "hard maximum" in terms of alert writing and will overwrite our max_signals value in terms of a limit. For instance if max_signals is set to the default of 100 and xpack.alerting.rules.run.alerts.max is set to 10, the rule will error on run - but will still write the first 10 alerts. This is the error that bubbles up from the rules client:

So I believe we should use the xpack.alerting.rules.run.alerts.max as a hard cap no matter what we select for max_signals. You are also correct that it's only set via the .yml file (or also a command I think you can run from terminal) so it would require the user to go out of their way to change it.

Also, what happens when, for example, xpack.alerting.rules.run.alerts.max gets updated from, let's say, 1000 to 50, while an existing rule has max_signals set to 100. What should be the behavior of rules during execution and how validation should work on the Rule Editing page?

@banderror The current behavior would have our rules error out when the config max setting for alerts was hit. I think that's a "good" outcome and a user shouldn't start changing server side if they're not aware of the impacts elsewhere, especially as this is the behavior we have currently. Essentially this cap we are putting on the UI input is the behavior/restrictions that we've implicitly been following so far, we're just exposing those limits to the users now as well.

[dplumlee]

The one thing I could see us doing instead is defaulting max_signals to 100 unless xpack.alerting.rules.run.alerts.max is less than 100 - then we default to whatever the config value is instead? Currently if xpack.alerting.rules.run.alerts.max is set to something like 10, you can't create a rule without going in and editing the initial value of 100.

[banderror]

@dplumlee @approksiu I agree with the suggestion above + I would also suggest to change the validation for max_signals on the page in such a way that:

• It shouldn't be possible to set max_signals to a value < MIN_VALUE. Should MIN_VALUE be 0 or 1?
• If user sets max_signals to a value < MIN_VALUE:
  • API endpoints should return an error
  • UI should show a validation error and it should NOT be possible to save the rule
• If user sets max_signals to a value > xpack.alerting.rules.run.alerts.max:
  • API endpoints should successfully create or update the rule
  • UI should show a validation warning near/under the form input, but it should be possible to save the rule. The warning should explain that during rule execution xpack.alerting.rules.run.alerts.max will be used as the limit instead of the value set for this specific rule, because the global setting is lower.

This will allow users to update a rule even after Kibana admin updates xpack.alerting.rules.run.alerts.max to a lower value, which instead would block rule updates unless the user adjusted the max_signals value accordingly.

Also, this should be in line with https://github.com/elastic/kibana/discussions/178867 (just replace "source data" with "kibana config"). cc @jpdjere

[jpdjere]

@dplumlee Thanks for the explanation!

I agree with everything proposed above by you and @banderror.

It shouldn't be possible to set max_signals to a value < MIN_VALUE. Should MIN_VALUE be 0 or 1?

I feel this should be 1. Allowing 0 could lead to a lot of confusion and make unsuspecting users think their rules are not producing alerts at all. But let's hear @approksiu opinion.

[approksiu]

It shouldn't be possible to set max_signals to a value < MIN_VALUE. Should MIN_VALUE be 0 or 1?

I feel this should be 1. Allowing 0 could lead to a lot of confusion and make unsuspecting users think their rules are not producing alerts at all. But let's hear @approksiu opinion.

Agree, we should not have 0 - it would not make sense in terms of results for the user. Thanks all!

[marshallmain]

The current behavior would have our rules error out when the config max setting for alerts was hit. I think that's a "good" outcome and a user shouldn't start changing server side if they're not aware of the impacts elsewhere, especially as this is the behavior we have currently. Essentially this cap we are putting on the UI input is the behavior/restrictions that we've implicitly been following so far, we're just exposing those limits to the users now as well.

If we find that max signals is less than xpack.alerting.rules.run.alerts.max at rule execution time, ideally we would write a warning status to notify the user - even if the rule execution doesn't try to write that many alerts. It would also be nice if hitting xpack.alerting.rules.run.alerts.max writes a warning rather than an error, similar to the warning for max signals for consistency, but I don't know if we have control over the error message.

Maybe we can just set real_max_signals = min(max_signals, xpack.alerting.rules.run.alerts.max) at the beginning of the rule execution and use that value to limit the number of alerts we try to write? And write a warning if real_max_signals != max_signals.

This change is not required IMO for this PR, as the problem exists independently from the work here to expose max signals in the UI.

[jpdjere]

Hi @dplumlee.

This is not related to this PR but to the setup field PR.

I just realized that that PR does not remove the setup field from the security-rule asset schema in x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/model/rule_assets/prebuilt_rule_asset.ts.

Since now we've added setup to the BaseDefaultableFields, which is part of BaseCreateProps it doesn't need to be added on top of it:

export const PrebuiltRuleAsset = BaseCreateProps.and(TypeSpecificCreateProps).and(
  z.object({
    rule_id: RuleSignatureId,
    version: RuleVersion,
    related_integrations: RelatedIntegrationArray.optional(),
    required_fields: RequiredFieldArray.optional(),
    // setup: SetupGuide.optional(), <----------------- this one can be removed
  })
);

Let's create a follow up PR to fix that.

[dplumlee]

@jpdjere I'll put it in the follow-up test PR I already opened up

[dplumlee]

@joepeeples when you get a chance, could you look through the language we're using in this PR in a few specific places?

Right now we have this warning message that notifies a user that, while they can enter in this value for max_signals, the rule will only run with the config value of xpack.rules.run.alerts.max - in this case 1000.

We also have the invalid state for when there is a lower number than 1.

And finally the detection engine warning we generate here for when the rule's max_signals value is again over the xpack.rules.run.alerts.max config maximum and we're using that value instead during our rule execution.

max_signals value set above limit set in xpack.rule.run.alerts.max, will only write a maximum of ${maxAlertsAllowed} per rule execution

In all of these (particularly the alerting config related ones), I'm not entirely sure how best to convey the meaning without diving into too much technical detail. Perhaps it wouldn't be the worst thing to list out the config value (xpack.rules.run.alerts.max) where this is set in both the UI and the docs, but also I'd imagine its the type of thing users won't be changing often in their workflows so maybe it's better just left to the docs page.

[joepeeples]

@joepeeples when you get a chance, could you look through the language we're using in this PR in a few specific places?

@dplumlee Here's what I came up with. I wanted to distinguish between the rule's setting and the "Kibana alerting" setting. I also tried to explain the consequence of having a higher max_signals value: possibility of missed alerts. But I'm not certain if "missed alerts" is the best description, maybe more like "fewer alerts than expected"? Feel free to omit this part if you think it clouds the issue or could cause unnecessary alarm.

1. UI warning

   Kibana alerting only allows a maximum of 1000 alerts per rule run. Setting a higher value here could result in missed alerts.

2. UI error — LGTM, no change:

   Max alerts must be greater than 0.

3. Logged error

   The rule’s max_signals value (${MAX_SIGNALS}) is greater than the Kibana alerting limit set in xpack.alerting.rules.run.alerts.max, which could result in missed alerts. The rule will only write a maximum of ${maxAlertsAllowed} per rule run.

   • Is it feasible to inject the configured max_signals value into the log message? Then the user doesn’t have to look around the rest of the rule details page for this info. Added some pseudocode for this.
   • The Kibana docs have a slightly different name for the alerting config setting, so I used that in the message above: xpack.alerting.rules.run.alerts.max

[dplumlee]

Thanks @joepeeples, this looks great!

But I'm not certain if "missed alerts" is the best description, maybe more like "fewer alerts than expected"?

I think that's an accurate description of what could happen and is similar to the language in the partial failure we log when max_signals is hit currently.

For the Logged error point, it's definitely possible to add the max_signals value to the log message 👍. And you're correct, the config value is xpack.alerting.rules.run.alerts.max.

[dplumlee]

Actually after seeing it in use, perhaps "fewer alerts than expected" is a more accurate description of what is being warned about. As the user won't be missing alerts if they're explicitly warned about the lower value being used when defining the rule, I think

Kibana alerting only allows a maximum of 1000 alerts per rule run. Setting a higher value here could result in fewer alerts than expected.

notifies the user that it's possible to miss alerts based on the value they're setting. What do you think @joepeeples?

[joepeeples]

@dplumlee I agree, the more I think about it, the less certain I am. 🙃 How about omitting that part for a cleaner, simpler message — it seems pretty clear to users that if Kibana alerting only allows 1000 alerts, they shouldn't expect 1001+ alerts. So:

1. UI warning

   Kibana alerting only allows a maximum of 1000 alerts per rule run.

2. UI error — no change:

   Max alerts must be greater than 0.

3. Logged error

   The rule’s max_signals value (${MAX_SIGNALS}) is greater than the Kibana alerting limit set in xpack.alerting.rules.run.alerts.max. The rule will only write a maximum of ${maxAlertsAllowed} per rule run.

[dplumlee]

To summarize the design meeting we had offline, we are now moving forward with the warning state being represented by the append EUI attribute with a warning icon across the new rule form UI components (notably also in https://github.com/elastic/kibana/pull/180682). This will give some consistency in the way we display warnings and has been approved by @ARWNightingale for design specs.

[xcrzx]

If a rule has max_signals set to a value higher than xpack.alerting.rules.run.alerts.max, it finishes execution with an empty message. We need to pass the correct warning message to the final rule status. Here are examples of the issue:

[banderror]

@dplumlee @xcrzx I'm wondering why does a rule log a warning status when max_signals > xpack.alerting.rules.run.alerts.max? There's nothing dangerous in this situation, and user doesn't have to necessarily fix anything. I think we should use a min(max_signals, xpack.alerting.rules.run.alerts.max) as the final limit in the rule execution logic, and it shouldn't affect rule statuses. We can optionally log a debug message that max_signals has this value, and xpack.alerting.rules.run.alerts.max has that value, and that one will be used as the final limit.

[xcrzx]

@dplumlee @xcrzx I'm wondering why does a rule log a warning status when max_signals > xpack.alerting.rules.run.alerts.max? There's nothing dangerous in this situation, and user doesn't have to necessarily fix anything.

Users might assume they have overridden the Alerting setting when configuring the rule, but in reality, the rule will use the lower setting and generate fewer alerts than expected.

I'm not entirely sure if this is a frequent scenario, though. It appears that a primary use case for this feature would be to set the max signals setting to a lower value to make a rule less noisy.

[dplumlee]

@banderror Dmitrii basically summed it up, the rule doesn't break but will potentially produce different results than the rule would if it were run with the parameters it's defined with. This behavior kind of matches the existing logic we use when throwing the warning for max_signals being hit - where it's possible that the rest of the alerts found in the rule execution over max_signals could be duplicates/exceptioned on, but it's still possible there are missing alerts for the rule run. We define that with a partial failure as the rule is not running as it's defined to, whether or not it actually impacts the result given.