-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Resolver retrieve entity id of documents without field mapped #76562
[Security Solution] Resolver retrieve entity id of documents without field mapped #76562
Conversation
Pinging @elastic/endpoint-app-team (Feature:Resolver) |
Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility) |
x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/resolver/view/clickthrough.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/components/graph_overlay/index.tsx
Outdated
Show resolved
Hide resolved
import _ from 'lodash'; | ||
import { DatabaseParameters } from '../types'; | ||
|
||
export function equal(param1: DatabaseParameters, param2?: DatabaseParameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this extra abstraction layer? For the type checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since timeline now sends resolver the ID and the indices we figured it'd be a good idea to combine them in a single object. Since indices
is a string array we need some way to compare all the strings easily so Rob and I came up with this method. We might switch out the lodash equal
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was specifically talking about the abstraction of equal
over isEqual
. I understand the combined type, that makes sense. It's really more, if this equality check is going to be focused solely on checking the equality of DatabaseParameters
, we know the shape, so might as well explicitly check the equality of the individual fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you mean like, why not just use _.isEqual
instead of our equal
function? Or are you suggesting doing the comparisons directly in the places we're calling equal
? I think @oatkiller was considering removing the lodash use so we'd have to implement an array comparison in that case. We'd probably want the array comparison at a minimum to be in a helper function because we do the DatabaseParameters
check in like 3 different places I think.
I guess another option is we make DatabaseParameters
into a class and move equal
as a method of that class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that was my thought, but since it's going to be something different from isEqual
, then I would just leave it. Depending on what it is, it could still be more generalized into a util, but no need to do that here anyways
* change 'data' state shape, nesting the tree fetcher data * rename 'TreeFetcherParameters' from 'DatabaseParameters' to make it more specific to the API it works on * fix bug in 'equal' method of 'TreeFetcherParameters'` * use mockTreeFetcherParameters method in tests that need to specify a TreeFetcherParameters but when the value isn't relevant to the test * Hide Resolver if there is no databaseDocumentID * add doc comments
* change 'data' state shape, nesting the tree fetcher data * rename 'TreeFetcherParameters' from 'DatabaseParameters' to make it more specific to the API it works on * fix bug in 'equal' method of 'TreeFetcherParameters'` * use mockTreeFetcherParameters method in tests that need to specify a TreeFetcherParameters but when the value isn't relevant to the test * Hide Resolver if there is no databaseDocumentID * add doc comments
@@ -38,13 +37,6 @@ export function dataAccessLayerFactory( | |||
}); | |||
}, | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indices to use are now passed in to Resolver via props from the Security Solution app.
@@ -115,7 +115,7 @@ interface AppReceivedNewExternalProperties { | |||
/** | |||
* the `_id` of an ES document. This defines the origin of the Resolver graph. | |||
*/ | |||
databaseDocumentID?: string; | |||
databaseDocumentID: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required by ResolverProps
now should it will always be present and can always be passed to this event.
*/ | ||
databaseDocumentID: string; | ||
parameters: TreeFetcherParameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of keeping track of just the databaseDocumentID
used in the request, we now also keep track of the indices
.
|
||
const initialState: DataState = { | ||
relatedEvents: new Map(), | ||
relatedEventsReady: new Map(), | ||
resolverComponentInstanceID: undefined, | ||
tree: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data related to fetching the resolver tree is now namespaced under tree
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll likely try to move this logic into a separate nested selector in the future.
} else if (action.type === 'appAbortedResolverDataRequest') { | ||
if (action.payload === state.pendingRequestDatabaseDocumentID) { | ||
if (treeFetcherParameters.equal(action.payload, state.tree.pendingRequestParameters)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we strictly compared the databaseDocumentID
s but now we use the equal
method to compare the parameters.
}) { | ||
const dispatch = useResolverDispatch(); | ||
const locationSearch = useLocation().search; | ||
useLayoutEffect(() => { | ||
dispatch({ | ||
type: 'appReceivedNewExternalProperties', | ||
payload: { databaseDocumentID, resolverComponentInstanceID, locationSearch }, | ||
payload: { databaseDocumentID, resolverComponentInstanceID, locationSearch, indices }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now keep the redux store in sync with the Resolver indices
prop.
@@ -137,6 +139,17 @@ const GraphOverlayComponent = ({ | |||
globalFullScreen, | |||
]); | |||
|
|||
const { signalIndexName } = useSignalIndex(); | |||
const [siemDefaultIndices] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY); | |||
const indices: string[] = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concat the signal indices and the SIEM default indices and pass that to Resolver.
databaseDocumentID={graphEventId} | ||
resolverComponentInstanceID={currentTimeline.id} | ||
/> | ||
{graphEventId !== undefined && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolver cannot work without a databaseDocumentID
so we hold off rendering it unless we have one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is <Suspense>
still not ready yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot that even existed.
<StyledResolver | ||
databaseDocumentID={graphEventId} | ||
resolverComponentInstanceID={currentTimeline.id} | ||
indices={indices} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will render resolver with the SIEM default indices then once the signals index is fetched, it will rerender Resolver. This causes Resolver to start a request with just the default indices then cancel that request and re-request with the full set of indices.
@jonathan-buttner @XavierM Should we hold off rendering Resolver until the signals index has been fetched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-goldstein Also worked on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably makes sense to wait for the response from the signals
index api request 🤷
@@ -64,19 +59,6 @@ export function handleEntities(): RequestHandler<unknown, TypeOf<typeof validate | |||
values: _id, | |||
}, | |||
}, | |||
{ | |||
exists: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing these checks, Resolver should work even if the new 7.9 index mappings aren't present. @jonathan-buttner Is that the right description?
Also, how should we mark this PR in release notes? Enhancement? or bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing these checks, Resolver should work even if the new 7.9 index mappings aren't present. @jonathan-buttner Is that the right description?
That's correct.
Also, how should we mark this PR in release notes? Enhancement? or bug fix?
@benskelker @spong This PR addresses the issue where resolver wouldn't render without the steps documented by Ben. Now resolver should render if the user upgrades their stack to 7.9.2 even if they don't perform the .siem-signals
index mapping migration. Do we need to update release notes for this bug fix? If so how do I go about doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to answer the original question I think it's probably a bug fix.
if (first.length !== second.length) { | ||
return false; | ||
} | ||
const firstSet = new Set(first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ It could be a WeakSet! It's your big chance WeakSet get in there! 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha I think to use a WeakSet
the values would have to be objects though right? first
is a string[]
behind the scenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right 😢 next time, WeakSet
*/ | ||
export function resolverComponentInstanceID(state: DataState): string { | ||
return state.resolverComponentInstanceID ? state.resolverComponentInstanceID : ''; | ||
export function hadErrorLoadingTree(state: DataState): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Suggest hadErrorLoadingLastTree
to be more precise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to it. That being said, this code will only try once. If you get an error, you'll have it for good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
…field mapped (elastic#76562) * More comments * Adding tests for mapping without entity_id * Removing unnecessary comments * Fixing type errors * Removing unnecessary import * Fixups and style * change 'data' state shape, nesting the tree fetcher data * rename 'TreeFetcherParameters' from 'DatabaseParameters' to make it more specific to the API it works on * fix bug in 'equal' method of 'TreeFetcherParameters'` * use mockTreeFetcherParameters method in tests that need to specify a TreeFetcherParameters but when the value isn't relevant to the test * Hide Resolver if there is no databaseDocumentID * add doc comments * Fixing test name and adding comments * Pulling in roberts test name changes * [Resolver] Only render resolver once we have a signals index Co-authored-by: oatkiller <[email protected]>
…field mapped (#76562) (#76772) * More comments * Adding tests for mapping without entity_id * Removing unnecessary comments * Fixing type errors * Removing unnecessary import * Fixups and style * change 'data' state shape, nesting the tree fetcher data * rename 'TreeFetcherParameters' from 'DatabaseParameters' to make it more specific to the API it works on * fix bug in 'equal' method of 'TreeFetcherParameters'` * use mockTreeFetcherParameters method in tests that need to specify a TreeFetcherParameters but when the value isn't relevant to the test * Hide Resolver if there is no databaseDocumentID * add doc comments * Fixing test name and adding comments * Pulling in roberts test name changes * [Resolver] Only render resolver once we have a signals index Co-authored-by: oatkiller <[email protected]> Co-authored-by: oatkiller <[email protected]>
* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...
This PR fixes a bug in resolver and refactors a couple things:
entity
route so that the document doesn't have to haveentity_id
field mappedentity_id
exists and is not empty, we use a runtime check to avoid the mapping issue.siem-signals
index)screenshots