elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.77k stars 8.17k forks source link

[Cases][Alerting] Validate actions using additional fields beyond params #116006

Open jonathan-buttner opened 2 years ago

jonathan-buttner commented 2 years ago

Background

We've introduced the concept of a "legacy/deprecated" connector in https://github.com/elastic/kibana/pull/105440. Currently we've added a deprecated icon to the connectors list page

Deprecated Connectors List Page ![image](https://user-images.githubusercontent.com/56361221/138340262-c5234bd3-f428-430e-92ae-4f755f78d487.png)

We'd like to show the deprecation message within the rule edit/creation UI like so:

Deprecated Callout in edit/create UI ![image](https://user-images.githubusercontent.com/56361221/138340940-5accdb59-fe06-4099-91b6-87da037da544.png)

The Cases plugin currently prevents a user from creating a new case when they've selected a deprecated connector for the case. Cases also does not allow fields within the connector to be updated (like the priority field) if it is deprecated. Users can still make changes to the case, add comments, etc, and push the case to service now even if the connector is deprecated.

The problem

We'd like to prevent new rules from being created if the selected connector is deprecated. Ideally we wouldn't prevent editing a rule that contains a deprecated connector. Allowing a user to edit a rule will still give them the flexibility to make updates to their rules after upgrading to 7.16 even if they haven't been able to install the elastic service now application.

Currently the field that controls whether a connector is deprecated is stored in the config section. As far as I can tell the config section is not used to validate an action during the alert creation/editing flow:

https://github.com/elastic/kibana/blob/master/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx#L134

The alert parameter contains the params but not the config portion from a connector.

Potential Solutions

Add state bucket

I believe the component that displays the available connectors uses a callback to set the chosen id of the connector and stores it in the AlertAction fields:

https://github.com/elastic/kibana/blob/master/x-pack/plugins/alerting/common/alert.ts#L50

One option would be to add an additional field that can be set by the child component that is also accessible by AlertAdd and AlertEdit so it can be passed to a validation function managed by each connector.

I think this solution has a lot of overlap with this issue which talks about adding another bucket of values (state): https://github.com/elastic/kibana/issues/114148

Store the configs in the upper AlertAdd and AlertEdit components

Another solution would be to add another reducer here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_add.tsx#L68

which the child component could update using dispatch like it does for setting the id of the connector. The child component would dispatch an action to set the config of the selected connectors. This solution might avoid needing an additional bucket of state.

Add the deprecation flag to params

This is probably more of a hack. We could add the deprecation flag into the params field as well. We already have access to the config section within the service now params components: https://github.com/elastic/kibana/blob/master/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/servicenow/servicenow_sir_params.tsx#L46

We could then set the deprecation flag in a useEffect so it wouldn't be dependent on any user filled field in the params component.

We could then add additional validation logic to service nows validateParams function so that it would return an error if the deprecation field is ever set to true. The downside here is that this won't satisfy allowing users to edit rules if they are already using a deprecated connector (it being marked deprecated after the 7.16 upgrade migration).

Another downside to this solution is that I believe all the params are set during an _execute call. We'd need to edit the service now action plugin's backend implementation to strip off that field before making requests to service now since it's not a valid service now API field.

Allowing edits to a rule with a deprecated connector

To allow users to edit existing rules that now have a deprecated connector we'll need additional validation logic.

I think if we an additional state bucket, we could add a new validateState function that is optional in the ActionTypeModel https://github.com/elastic/kibana/blob/master/x-pack/plugins/triggers_actions_ui/public/types.ts#L128

During the rule creation/edit flow we could use validateState if it was defined. validateState could take an operation parameter so that for service now we could allow rule edits event for a deprecated connector but prevent creating new rules.

If we went with adding the deprecation flag to the params, we could also add a new parameter to the validateParams so that we could figure out if the operation was an edit or create during validation.

Related Issues

Copy update: https://github.com/elastic/security-team/issues/1938 New state bucket: https://github.com/elastic/kibana/issues/114148 Original bug: https://github.com/elastic/kibana/issues/114097

elasticmachine commented 2 years ago

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

elasticmachine commented 2 years ago

Pinging @elastic/security-threat-hunting-cases (Team:Threat Hunting:Cases)

jonathan-buttner commented 2 years ago

cc: @YulNaumenko @monina-n

gmmorris commented 2 years ago

We'd like to prevent new rules from being created if the selected connector is deprecated.

@arisonl Do you feel this is something we want to enforce at platform level?

arisonl commented 2 years ago

@jonathan-buttner would it be possible to share more on the product thinking around this? E.g. this may force users to operate with a mix of rules using the deprecated and new connector until they "migrate" all rules to the new connector, right? Is there any chance that this may cause workflow/operational friction? Thoughts on the alternative of not prohibiting but rather strongly discouraging through the UX? cc @paulewing @cnasikas @vinaychandrasekhar @gmmorris

jonathan-buttner commented 2 years ago

would it be possible to share more on the product thinking around this?

Yeah @paulewing @cnasikas @alexfrancoeur could probably speak to that better. With the certification of service now I think we're trying to encourage users to update their connectors and use the elastic service now application. I can definitely see your point about how it could be frustrating to lose some functionality suddenly when they upgrade though.

E.g. this may force users to operate with a mix of rules using the deprecated and new connector until they "migrate" all rules to the new connector, right?

I think the hope is that they'd update all their connectors (which is hopefully not that many) rather than all their rules. A user who had many rules using one or two different service now connectors would only need to update their connectors to the latest version (which includes installing the elastic service now application on their service now instance).

Thoughts on the alternative of not prohibiting but rather strongly discouraging through the UX?

@paulewing @cnasikas should we change cases to allow creating cases with the deprecated connectors and allow changing the deprecated connectors that are already associated with a case?

cnasikas commented 2 years ago

As @jonathan-buttner said, I think the reason behind this decision for cases is to enforce users to migrate to the new connector.

@paulewing @cnasikas should we change cases to allow creating cases with the deprecated connectors and allow changing the deprecated connectors that are already associated with a case?

Maybe we should revise. Let's talk about it on the sync.

elasticmachine commented 2 years ago

Pinging @elastic/response-ops-cases (Feature:Cases)