Skip to content
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

[POC] Integrate Alert summary inside of security solution rule page #151916 #153113

Closed

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Mar 10, 2023

Summary

These changes covers the features mentioned in the main ticket. Here are the details:

  • Integrate new action's frequency attribute which will allow us to specify per-action notifications
  • Updates action UI to allow per-action intervals (instead of former rule level throttle and notifyWhen attributes)

@e40pud e40pud requested review from XavierM and vitaliidm March 10, 2023 15:58
@e40pud e40pud self-assigned this Mar 10, 2023
@e40pud e40pud added release_note:enhancement ci:cloud-deploy Create or update a Cloud deployment labels Mar 10, 2023
@e40pud
Copy link
Contributor Author

e40pud commented Mar 21, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Mar 23, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Mar 27, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Mar 28, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Mar 29, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Mar 30, 2023

@elasticmachine merge upstream

e40pud added a commit that referenced this pull request Apr 13, 2023
…ty solution needs (#154531) (#154526)

## Summary

These changes customise action form UI for security solution needs.


![image](https://user-images.githubusercontent.com/2700761/230332242-20cbd4a1-8f76-4e1d-835e-cd0525cc530c.png)

To see how new design works in security solution, you can use this
[draft PR](#153113) which consists
of all the changes. I split that big PR into smaller independent pieces
for easier review process.

Fixes #154531

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@e40pud
Copy link
Contributor Author

e40pud commented Apr 14, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 14, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 14, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 14, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 15, 2023

@elasticmachine merge upstream

@e40pud e40pud changed the title [Security Solution][Alerts] - Integrate Alert summary inside of security solution rule page #151916 [POC] Integrate Alert summary inside of security solution rule page #151916 Apr 15, 2023
@e40pud
Copy link
Contributor Author

e40pud commented Apr 18, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Apr 18, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3826 3827 +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 122 127 +5

Async chunks

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

id before after diff
securitySolution 16.0MB 16.0MB -4.3KB

Page load bundle

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

id before after diff
securitySolution 58.3KB 58.4KB +116.0B
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-io-ts-alerting-types 141 147 +6

ESLint disabled line counts

id before after diff
securitySolution 432 436 +4

Total ESLint disabled count

id before after diff
securitySolution 512 516 +4

History

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

cc @e40pud

vitaliidm added a commit that referenced this pull request Apr 19, 2023
…lesClient (#153101)

## Summary
- this PR is the first part of work related to conditional logic
actions. The rest of PRs, will be merged after this one. As they depend
on a work implemented here. [List of
tickets](elastic/security-team#2894 (comment))
- addresses #151919
- moves code related to legacy actions migration from D&R to
RulesClient,
[details](#151919 (comment))
- similarly to D&R part, now rulesClient read APIs, would return legacy
actions within rule
- similarly, every mutation API in rulesClient, would migrate legacy
actions, and remove sidecar SO
- each migrated legacy action will have also [`frequency`
object](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/types.ts#L234-L238),
that would allow to have notifyWhen/throttle on action level once
#151916 is implemented, which is
targeted in 8.8, right after this PR.
But before it's merged, `frequency` is getting removed in
[update/bulk_edit/create
APIs](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/rules_client/methods/update.ts#L151-L160).
Hence it's not reflected in most of the tests at this point.
- cleanup of legacy actions related code in D&R
- adds unit tests for RulesClient
- keeps functional/e2e tests in D&R

Changes in behaviour, introduced in this PR:
- since, migration happens within single rulesClient API call, revision
in migrated rule will increment by `1` only.
- legacy actions from sidecar SO, now will be merged with rules actions,
if there any.
Before, in the previous implementation, there was inconsistency in a way
how legacy and rules actions were treated.
- On read: actions existing in rule, [would take precedence over legacy
ones
](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts#L94-L114)
- On migration: SO actions [only
saved](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/rule_actions/legacy_action_migration.ts#L114).
If any actions present in rule, they will be lost. Here is an example
video from **main** branch
      <details>
<summary>Here is an example video from MAIN branch, where action in rule
is overwritten by legacy action</summary>
      

https://user-images.githubusercontent.com/92328789/230397535-d3fcd644-7cf9-4970-a573-18fd8c9f2235.mov

      </details>

So, depends on sequence of events, different actions could be saved for
identical use case: rule has both legacy and existing action.
- if rule migrated through update API, existing in rule action will be
saved
- if rule migrated through enable/disable API, bulk edit, legacy action
will be saved

In this implementation, both existing in rule and legacy actions will be
merged, to prevent loss of actions
      <details>
<summary>Here is an example video from this PR branch, where actions are
merged</summary>
<video
src="https://user-images.githubusercontent.com/92328789/230405569-f1da38e9-4e36-46a8-9654-f664e0a31063.mov"
/>
      </details>
      
- when duplicating rule, we don't migrate source rule anymore. It can
lead to unwanted API key regeneration, with possible loss of privileges,
earlier associated with the source rule. As part of UX, when duplicating
any entity, users would not be expecting source entity to be changed
  
TODO:
- performance improvement issue for future
#154438
- currently, in main branch, when migration is performed through rule
enabling, actions not showing anymore in UI.
Relevant ticket is #154567
I haven't fixed it in this PR, as in[ the next one
](#153113), we will display
notifyWhen/throttle on action level in UI, rather than on rule level.
So, once that PR is merged, actions should be displayed in new UI
  
### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
@e40pud e40pud closed this Apr 23, 2023
@e40pud e40pud deleted the security/feature/actions-integration branch October 10, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants