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 Solution] [Sourcerer] Update available on legacy timelines #120022

Merged
merged 35 commits into from
Dec 7, 2021

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Nov 30, 2021

Summary

Resolves https://github.com/elastic/security-team/issues/1921

Implements an "Update available" workflow for pre-8.0 timelines and timelines that have indices that got deleted from the default data view

To test: save this json as a timelines.ndjson file and upload the timelines to the timelines page in security solution

Flow 1

Note: i paused the most in this flow so y'all can ready the copy, the rest of the flows go quicker
Legacy Timeline includes an active index pattern that is not included in the default data view, user decides to update data view. Page refresh is prompted
f1

Flow 2

Legacy Timeline includes an active index pattern that is not included in the default data view, user decides to update to the new sourcerer with only the indices that already exist in the data view
f2

Flow 3

Legacy Timeline includes an active index pattern that is not included in the default data view, user decides to reset to the new sourcerer and abandon their index pattern
f3

Flow 4

Legacy Timeline index patterns are ALL included in the default data view, user updates to the new sourcerer without needing to update the data view
f4

Flow 5

Legacy Timeline none of the index patterns in the legacy timeline match, don't allow user to upgrade data view. Forces them to reset to new sourcerer or keep a bad index pattern
f5

Flow 6

Start with a valid non-legacy timeline. Delete one of the index patterns from advanced settings. Fallsback into temporary timeline and prompts user to re-add the deleted index pattern
f6

Flow 7

Error state, not expected
f7

Checklist

@stephmilovic stephmilovic added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Threat Hunting:Explore labels Nov 30, 2021
@stephmilovic stephmilovic marked this pull request as ready for review December 6, 2021 14:11
@stephmilovic stephmilovic requested review from a team as code owners December 6, 2021 14:11
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@monina-n
Copy link

monina-n commented Dec 6, 2021

from design, overall all these look great and thanks for accounting for all the possible flows! even flow 6 (which is a very rare edge case I don't expect a lot of users to come across) is very clear in what happens.

All good from design - please remember to update the width of the time selector so it is it's default width and have the lock and refresh button moved to be beside it. It can be in this PR or in a separate one, whatever's better for you

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

It looks great! I played around and couldn't find any bug.

I still haven't taken a look at all files. I will finish it tomorrow. I left some minor comments for now.

}}
/>
</>
) as unknown as string,
Copy link
Member

Choose a reason for hiding this comment

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

Curiosity: Why double casting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS2352: Conversion of type 'Element' to type 'string' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but i need the link here... maybe ill submit a ticket to core asking this prop be updated

setPopoverIsOpen(false);
}, [onChangeDataView, dataViewId, selectedOptions]);

const handleClosePopOver = useCallback(() => {
setPopoverIsOpen(false);
setExpandAdvancedOptions(false);
}, []);
const trigger = useMemo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into its own component


const dataViewSelectOptions = useMemo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into usePickIndexPatterns

}, [selectedDataViewId]);

const tooltipContent = useMemo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into its own component along with buttonWithTooltip

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2800 2805 +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 4.6MB 4.6MB +10.9KB
timelines 202.9KB 203.1KB +148.0B
total +11.1KB

Page load bundle

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

id before after diff
securitySolution 243.7KB 244.0KB +273.0B
timelines 164.5KB 164.6KB +62.0B
total +335.0B

History

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

@angorayc
Copy link
Contributor

angorayc commented Dec 7, 2021

I found some inconstancy in the index patterns options, nothing todo with these flow though. In the first picture, the index pattern of security alert is not selectable, but we still can tick show only detection alerts to select that index pattern. Should we disable the show only detection alerts in this case?
Screenshot 2021-12-07 at 10 52 28
Screenshot 2021-12-07 at 10 53 40

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Awesome! : 🔥🔥 🚀

export const ensurePatternFormat = (patternList: string[]): string[] =>
[
...new Set(
patternList.reduce((acc: string[], pattern: string) => [...pattern.split(','), ...acc], [])
Copy link
Member

Choose a reason for hiding this comment

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

very nitpicking:

Suggested change
patternList.reduce((acc: string[], pattern: string) => [...pattern.split(','), ...acc], [])
patternList.flatMap((pattern) => pattern.split(','))

Or even more FP!

import { split } from "lodash/fp";
...
patternList.flatMap(split(','))

Oooor point-free style:

import { split, flatMap } from "lodash/fp";
...
flatMap(split(','), patternList)

...(isEmpty(selectedPatterns)
missingPatterns,
// if in timeline, allow for empty in case pattern was deleted
// need flow for this
Copy link
Member

Choose a reason for hiding this comment

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

Is it a TODO?

Copy link
Contributor Author

@stephmilovic stephmilovic Dec 7, 2021

Choose a reason for hiding this comment

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

whoops, nope thats flow 6. i need to delete this comment

@stephmilovic
Copy link
Contributor Author

Should we disable the show only detection alerts in this case?

@angorayc yes ill do that in a follow up

@stephmilovic stephmilovic merged commit 268b290 into elastic:main Dec 7, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
8.0 Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 120022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants