Skip to content

[Security Solution] expandable flyout - fix infinite loop in correlations#163450

Merged
PhilippeOberti merged 2 commits intomainfrom
expanded-flyout-correlation-infinite-loop
Aug 23, 2023
Merged

[Security Solution] expandable flyout - fix infinite loop in correlations#163450
PhilippeOberti merged 2 commits intomainfrom
expanded-flyout-correlation-infinite-loop

Conversation

@PhilippeOberti
Copy link
Copy Markdown
Contributor

@PhilippeOberti PhilippeOberti commented Aug 8, 2023 •
edited
Loading

Summary

QA found a nasty infinite loop bug in the correlation overview and details section of the expandable flyout. It only occurs in certain scenario, where the document has some specific data.
The problem comes from the fact that the useCorrelations hook is fetching the data in all scenarios, instead of taking into account wether or not we should actually fetch that data.

This PRs separates the useCorrelations hooks into 4 components for the right panel and another 4 components for the left panel, which each call their respective useFetch... hooks (useShowRelatedAlertsByAncestry, useShowRelatedAlertsBySameSourceEvent, useShowRelatedAlertsBySession and useShowRelatedCases). If the return value is true, the we fetch the data using the respective useFetch... hooks (useFetchRelatedAlertsByAncestry, useFetchRelatedAlertsBySameSourceEvent, useFetchRelatedAlertsBySession and useFetchRelatedCases).

Previous behavior

Screen.Recording.2023-08-15.at.12.54.26.PM.mov

New behavior

Screen.Recording.2023-08-15.at.12.56.48.PM.mov

The PR also does the following:

  • clean up a bit a couple of test_ids and translations files by making some reusable methods to generate values instead of having many individual constants
  • merge the correlations_cases_table and new related_cases files

The following file contains the a devtool console command to ingest an alert to reproduce the infinite loop
alert_infinite_loop_correlations.txt

#164394

Checklist

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-correlation-infinite-loop branch from e3318f2 to 29a7772 Compare August 8, 2023 23:09
@PhilippeOberti PhilippeOberti mentioned this pull request Aug 11, 2023
Merged
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-correlation-infinite-loop branch 8 times, most recently from b519d35 to 8488ef2 Compare August 15, 2023 21:43
@PhilippeOberti PhilippeOberti marked this pull request as ready for review August 15, 2023 21:43
@PhilippeOberti PhilippeOberti requested review from a team as code owners August 15, 2023 21:43
@PhilippeOberti PhilippeOberti added release_note:fix Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team v8.10.0 labels Aug 15, 2023
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-correlation-infinite-loop branch 2 times, most recently from debbc33 to ff81273 Compare August 16, 2023 19:22
christineweng
christineweng reviewed Aug 17, 2023
View reviewed changes
x-pack/plugins/security_solution/public/flyout/left/components/related_alerts_by_ancestry.tsx
const { loading, error, data, dataCount } = useFetchRelatedAlertsByAncestry({
dataFormattedForFieldBrowser,
scopeId,
});
Copy link
Copy Markdown
Contributor

@christineweng christineweng Aug 17, 2023

Choose a reason for hiding this comment

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

The separated components look very similar, almost identical. i understand that it's because we don't want to fetch data when we don't need to. I think we can still achieve this at a higher level by adding a skip prop to the data fetching hooks.

const showAlertsByAncestry = useShowRelatedAlertsByAncestry(...);
const { loading, error, data, dataCount } = useFetchRelatedAlertsByAncestry({
    dataFormattedForFieldBrowser,
    scopeId,
    skip: !showAlertsByAncestry
  });

Copy link
Copy Markdown
Contributor Author

@PhilippeOberti PhilippeOberti Aug 22, 2023

Choose a reason for hiding this comment

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

so I implemented this approach then finally decided against it, for 2 reasons:

  • the first one is some hooks we're using have that open but others don't. I tried to add it but then got scared of modifying somebody else's code after feature freeze and during BC time, which is super risky
  • using this skip doesn't prevent some of the code in our hooks and the hooks we're leveraging to run some of their code, which isn't super efficient

Therefore I ended up going with a route similar to what I had shown yesterday, but a bit simpler and easier to read. I hope you'll be ok with it!

@christineweng
Copy link
Copy Markdown
Contributor

christineweng commented Aug 17, 2023 •
edited
Loading

I investigated this alert as well and found the following reasons that it was stuck in infinite loop:

  1. alertsBySessionLoading is always true

    • when kibana.alert.original_event.id is not available, values fall back to an empty array. Despite being empty, it still triggers query building on this line and tries to run the dummy query.
    • Removing the fall back would fix it. same goes to this line

  2. ancestryAlertsLoading is always true

    • this line should prevent the query from running if either id or schema is null - the alert in question returns null for both id and schema
    • there is a known issue with react query v4 that query.isLoading could be true when query is not enabled, so it was stuck in this line
    • query.isFetching or (enabled && query.isLoading) equals the true isLoading

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-correlation-infinite-loop branch 3 times, most recently from 3d0c671 to d3747db Compare August 22, 2023 16:49
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-correlation-infinite-loop branch from d3747db to f0cea46 Compare August 22, 2023 18:45
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Aug 22, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4438 4444 +6

Async chunks

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

id before after diff
securitySolution 15.7MB 15.7MB -3.6KB

History

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

christineweng
christineweng approved these changes Aug 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@christineweng christineweng left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Tested against the trouble alert and it works as expected.

One suggestion I have is to consolidate the 4 show.. hooks into 1. Most of the checks are very simple. Then we can have a boolean that checks if any insights can be shown, similar to this line

const canShowAtLeastOneInsight =
      showRelatedCases ||
      showSameSourceEvents ||
      showAlertsByAncestry ||
      showAlertsBySession;

@PhilippeOberti PhilippeOberti merged commit a95f4f8 into main Aug 23, 2023
@PhilippeOberti PhilippeOberti deleted the expanded-flyout-correlation-infinite-loop branch August 23, 2023 07:17
@PhilippeOberti
Copy link
Copy Markdown
Contributor Author

PhilippeOberti commented Aug 23, 2023

One suggestion I have is to consolidate the 4 show.. hooks into 1. Most of the checks are very simple. Then we can have a boolean that checks if any insights can be shown, similar to this line

Good idea, I'm implementing this in my next PR to save some build time and save time on rebase for the next 2-3 PRs!

@kibanamachine kibanamachine mentioned this pull request Aug 23, 2023
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 23, 2023

💚 All backports created successfully

Status Branch Result
✅ 8.10

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 23, 2023
@PhilippeOberti
[Security Solution] expandable flyout - fix infinite loop in correlat…
e981eb6 
…ions (elastic#163450)

(cherry picked from commit a95f4f8)
kibanamachine added a commit that referenced this pull request Aug 23, 2023
@kibanamachine @PhilippeOberti
[8.10] [Security Solution] expandable flyout - fix infinite loop in c…
238a764 
…orrelations (#163450) (#164527)

# Backport

This will backport the following commits from `main` to `8.10`:
- [[Security Solution] expandable flyout - fix infinite loop in
correlations (#163450)](#163450)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Philippe
Oberti","email":"philippe.oberti@elastic.co"},"sourceCommit":{"committedDate":"2023-08-23T07:17:02Z","message":"[Security
Solution] expandable flyout - fix infinite loop in correlations
(#163450)","sha":"a95f4f8fca1ceb0fe5ab6db522a6bcf229db7c9d","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Threat
Hunting:Investigations","v8.10.0","v8.11.0"],"number":163450,"url":"https://github.com/elastic/kibana/pull/163450","mergeCommit":{"message":"[Security
Solution] expandable flyout - fix infinite loop in correlations
(#163450)","sha":"a95f4f8fca1ceb0fe5ab6db522a6bcf229db7c9d"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163450","number":163450,"mergeCommit":{"message":"[Security
Solution] expandable flyout - fix infinite loop in correlations
(#163450)","sha":"a95f4f8fca1ceb0fe5ab6db522a6bcf229db7c9d"}}]}]
BACKPORT-->

Co-authored-by: Philippe Oberti <philippe.oberti@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 23, 2023
@jloleysens
Merge branch 'main' into zdt-test-harness
fab26f8 
* main: (150 commits)
  Fixes unnecessary autocompletes on HTTP methods (elastic#163233)
  [Defend Workflows] Convert filterQuery to kql  (elastic#161806)
  [Fleet] copy `inactivity_timeout` when duplicating agent policy (elastic#164544)
  Fix 7.17 forward compatibility with 8.2+ (elastic#164274)
  [ML] Fixes dark mode in flyouts and modals (elastic#164399)
  [Defend Workflows]Changes to policy settings are not persistent until a refresh (elastic#164403)
  [Security Solution][Endpoint] Fixes kibana crash when going back to policy details page (elastic#164329)
  Prepare the Security domain HTTP APIs for Serverless (elastic#162087)
  skip failing test suite (elastic#160986)
  [Security Solution] Fix flaky Event Filters test (elastic#164473)
  [EDR workflows] Osquery serverless tests (elastic#163795)
  [Fleet] Only show agent dashboard links if there is more than one non-server agent and if the dashboards exist (elastic#164469)
  [Chrome UI] Fix background color in serverless (elastic#164419)
  [DOCS] Saved objects - resolve import errors API (elastic#162825)
  Remove 'Create Rule' button from Rule Group page (elastic#164167)
  [Security Solution] expandable flyout - fix infinite loop in correlations (elastic#163450)
  [Remote Clusters] Update copy about port help text (elastic#164442)
  [api-docs] 2023-08-23 Daily api_docs build (elastic#164524)
  [data views] Disable scripted fields in serverless environment (elastic#163228)
  [Reporting] Fix - show diagnostic only when image reporting is enabled (elastic#164336)
  ...
@PhilippeOberti PhilippeOberti mentioned this pull request Aug 25, 2023
Closed
This was referenced Oct 1, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christineweng christineweng christineweng approved these changes

Assignees

No one assigned

Labels

release_note:fix Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team v8.10.0 v8.11.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PhilippeOberti @christineweng @kibana-ci @kibanamachine