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

[Security team: AWP] Session view: Alert details tab #127500

Merged
merged 27 commits into from
Mar 23, 2022

Conversation

mitodrummer
Copy link
Contributor

@mitodrummer mitodrummer commented Mar 10, 2022

Summary

Issue: #126065

Implementation complete for the Alerts tab in the Session View details panel. I've also broken the loading of alerts and process events apart into their own routes. The new alerts_route will load a fixed number of alerts for the session (currently 1000). This is done so we don't need to coordinate loading pages of process events and alert events in order to show status in process tree.

The alerts tab will show all alerts for the session with the ability to focus on an investigatedAlert (e.g when loading session view from the alerts page). This alert appears sticky at the top of the scroll area with special UX treament.

The alerts tab also supports the ability to group alerts by rule.

The following actions can be taken on each alert in the list:

image

image

image

Checklist

Delete any items that are not applicable to this PR.

@mitodrummer mitodrummer added Team: AWP: Platform Adaptive Workload Protection Platform team from Security Solution v8.2.0 labels Mar 10, 2022
@mitodrummer mitodrummer added the release_note:feature Makes this part of the condensed release notes label Mar 10, 2022
Copy link
Contributor

@zizhouW zizhouW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@mitodrummer mitodrummer added backport:skip This commit does not require backporting release_note:enhancement release_note:skip Skip the PR/issue when compiling release notes release_note:feature Makes this part of the condensed release notes and removed release_note:feature Makes this part of the condensed release notes release_note:enhancement release_note:skip Skip the PR/issue when compiling release notes labels Mar 16, 2022
export const registerAlertsRoute = (router: IRouter) => {
router.get(
{
path: ALERTS_ROUTE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing this directly is probably kinda dangerous, and there's some hidden nuance in this index I think, with rbac and building block alerts and things like that. Probably what should be done is make the session_view dependent on the ruleRegistry plugin, and make use of the AlertsClient exported by that plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples of this would be appreciated, not sure how to go about accessing AlertsClient after making rule_registry a dep. I assume it's not as simple as just importing AlertsClient from rule_registry/server/index.ts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitodrummer you can use the rule_registry plugin start contract RuleRegistryPluginStartContract for accessing AlertsClient, by calling the method getRacClientWithRequest.
I didn't find the code for AlertsClient , but here is the similar usage of RulesClient: you get it by calling getRulesClientWithRequest, code x-pack/plugins/monitoring/server/plugin.ts

https://github.com/elastic/kibana/blob/main/x-pack/plugins/monitoring/server/plugin.ts#L333-L340

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rule_registry is added as a dep and I'm making use of getRacClientWithRequest in alerts_route.ts. Let me know if the plugin setup looks good.

const alertsQuery = useFetchSessionViewAlerts(sessionEntityId);
const { data: alerts, error: alertsError, isFetching: alertsFetching } = alertsQuery;

const hasData = alerts && data && data.pages?.[0].events.length > 0;
Copy link
Contributor

@zizhouW zizhouW Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is alerts here in case fetching alerts ran into error? Because I imagine if we don't have alerts, it will just be a truthy []. If it is for errors, would the alertsError be enough for error handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to avoid passing an undefined to ProcessTree as the prop expects an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alerts is initially undefined until the react-query fetch completes.

@mitodrummer mitodrummer requested a review from a team as a code owner March 21, 2022 23:31
);
});

describe('doSearch(client, sessionEntityId)', () => {
Copy link
Contributor

@YulNaumenko YulNaumenko Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is great, but it tests the function executed inside and it doesn't test the route itself. Please check the examples here.
As the long term vision I suggest to split the routes and its internal logic to the library files or SessionViewClient, if you want to expose this logic not only with the routs.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Suggesting to add the routes tests, could be a follow up PR and very useful.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default Accessibility Tests / Maps app meets ally validations single cancel modal

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
sessionView 88 94 +6

Async chunks

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

id before after diff
sessionView 39.1KB 48.1KB +9.0KB
Unknown metric groups

ESLint disabled line counts

id before after diff
sessionView 7 5 -2

Total ESLint disabled count

id before after diff
sessionView 7 5 -2

History

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

@mitodrummer mitodrummer merged commit c55bb91 into elastic:main Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team: AWP: Platform Adaptive Workload Protection Platform team from Security Solution v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants