-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Endpoint] add resolver middleware #58288
[Endpoint] add resolver middleware #58288
Conversation
x-pack/plugins/endpoint/public/embeddables/resolver/store/middleware.ts
Outdated
Show resolved
Hide resolved
Pinging @elastic/endpoint-app-team (Feature:Resolver) |
@@ -138,7 +142,7 @@ export interface LegacyEndpointEvent { | |||
} | |||
|
|||
export interface EndpointEvent { | |||
'@timestamp': Date; | |||
'@timestamp': 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.
❔ Give the vagaries of dealing with time that you are especially familiar with, it might benefit the reader to have some comment describing what format is expected here.
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.
TBH I'm not sure "string" is right. Does anyone know how dates get serialized into JSON by default through hapi
? The underlying value is a date equivalent in Elasticsearch, which according to the docs is stored as a 64-bit integer, so I would imagine that what we're more likely to see is that this is a number
unless the elasticsearch
node client/hapi
combo is doing some number
-->Date
-->string
conversion under the hood.
Then again, on second thought, this could be a string | number
if we're working with the source document and just shoving it back to the client...
@@ -155,6 +159,20 @@ export interface EndpointEvent { | |||
agent: { | |||
type: string; | |||
}; | |||
endgame: { | |||
pid: number; |
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.
❔ Since we're bringing these things in, it might brook a short explanation in comments (Like why is a pid not the same as a unique_pid? - things we know, but maybe are not obvious to someone who hasn't worked with us before that might be expected to read and understand this.)
if (columnId === 'alert_type') { | ||
return ( | ||
<Link | ||
data-testid="alertTypeCellLink" | ||
to={urlFromQueryParams({ ...queryParams, selected_alert: 'TODO' })} | ||
to={urlFromQueryParams({ ...queryParams, selected_alert: row.event.id })} |
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.
❔ row could resolve to alertListData[NaN]
(Div by 0) and throw if the selector ever returned 0 for pageSize. Unlikely? Or maybe use the new chaining syntax?
height: 100%; | ||
width: 100%; | ||
display: flex; | ||
flex-grow: 1; |
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.
❔ just flex: 1
in case it also needed a shrink?
coreStart, | ||
}: { | ||
className?: string; | ||
selectedEvent: object; |
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.
❔ 2 question: is selectedEvent being used (or will it)? can it be a ResolverEvent type?
@@ -5,15 +5,16 @@ | |||
*/ | |||
|
|||
import { uniquePidForProcess, uniqueParentPidForProcess } from './process_event'; | |||
import { IndexedProcessTree, ProcessEvent } from '../types'; | |||
import { IndexedProcessTree } from '../types'; | |||
import { EndpointEvent } from '../../../../common/types'; |
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.
❔ per @andrewstucki we will end up with two types
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.
Yep, looks like this interface is currently describing legacy endgame events, while our focus should be on the ECS-specified elastic-native events. Ideally we can do both to support both the legacy platform and new endpoint product through resolver, and just union their types together--but yeah, separate PR for that sometime in the future is most welcome.
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.
Yeah maybe eventually importing ResolverEvent
and doing some type guard to narrow the type to either a LegacyEndpointEvent
or EndpointEvent
.
/** | ||
* The time (since epoch in milliseconds) when the action was dispatched. | ||
*/ | ||
readonly time: number; | ||
}; | ||
} | ||
|
||
export type ResolverAction = CameraAction | DataAction | UserBroughtProcessIntoView; | ||
interface UserChangedSelectedEvent { |
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.
🔴 Follow the pattern in the file of describing actions with comments
} = coreStart; | ||
//const uniquePid = action.payload.selectedEvent?.value?.source.endgame.data.pid; | ||
const uniquePid = '3096'; | ||
const { lifecycle } = await http.get(`/api/endpoint/resolver/${uniquePid}`, { |
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.
❔ Looks like you could cut a little time out by doing these requests in parallel rather than in a series of await
s since they don't seem to depend on the previous one. (Or maybe that will change?)
<StyledPanel /> | ||
<StyledGraphControls /> | ||
{isLoading ? ( | ||
<div className="loading-container"> |
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.
❔ May want to check with a11y re: putting a live role 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.
👍 Call for a thumb when you do the revisions. I had one 🔴 about putting better comments in actions, everything else is ❔
@@ -33,6 +33,7 @@ export function renderApp(coreStart: CoreStart, { appBasePath, element }: AppMou | |||
interface RouterProps { | |||
basename: string; | |||
store: Store; | |||
coreStart: CoreStart; |
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.
@kqualters-elastic - maybe not for this PR, but we should consider using the kibana-react components for sharing coreStart
in the app. I think the use of hooks rather than to force coreStart
down to each route's View would be better DX. If we did not want to use the kibana-react components, we could just create our own react Context
and set of hooks.
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.
agreed, updated to do just that.
@@ -155,6 +161,20 @@ export interface EndpointEvent { | |||
agent: { | |||
type: string; | |||
}; | |||
endgame: { |
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 interface is for the new elastic endpoint events which will not have any references to endgame
. I think these belong in LegacyEndpointEvent
.
@@ -5,15 +5,16 @@ | |||
*/ | |||
|
|||
import { uniquePidForProcess, uniqueParentPidForProcess } from './process_event'; | |||
import { IndexedProcessTree, ProcessEvent } from '../types'; | |||
import { IndexedProcessTree } from '../types'; | |||
import { EndpointEvent } from '../../../../common/types'; |
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.
Yeah maybe eventually importing ResolverEvent
and doing some type guard to narrow the type to either a LegacyEndpointEvent
or EndpointEvent
.
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 need to coordinate merging with this PR #58412, as there is overlapping code . We could base one on the other and make sure they play well together, then merge in sync.
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.
do it
({ selected_alert: selectedAlert }, alertList) => { | ||
return (alertList.find( | ||
alert => alert.event.id === selectedAlert | ||
) as unknown) as LegacyEndpointEvent; |
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.
TODO stop it
@@ -19,7 +19,7 @@ export class ResolverEmbeddableFactory extends EmbeddableFactory { | |||
return true; | |||
} | |||
|
|||
public async create(initialInput: EmbeddableInput, parent?: IContainer) { | |||
public async create(initialInput: EmbeddableInput, parent: IContainer) { |
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.
?
export type ResolverAction = CameraAction | DataAction | UserBroughtProcessIntoView; | ||
interface UserChangedSelectedEvent { | ||
readonly type: 'userChangedSelectedEvent'; | ||
readonly payload: { |
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.
/**
* Used when the alert list selects an alert and the flyout shows resolver.
*/
interface UserChangedSelectedEvent { | ||
readonly type: 'userChangedSelectedEvent'; | ||
readonly payload: { | ||
selectedEvent?: LegacyEndpointEvent; |
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.
/**
* Optional because they could have unselected the event.
*/
} | ||
|
||
interface AppRequestedResolverData { | ||
readonly type: 'appRequestedResolverData'; |
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.
/**
* Triggered by middleware when the data for resolver needs to be loaded. Used to set state in redux to 'loading'.
*/
|
||
useEffect(() => { |
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.
try useLayoutEffect
. This way, if the resolver is rendered with a selected event initially, it'll dispatch the event and rerender before being flushed to the DOM. Otherwise you'll have a flash of uninitialized resolver DOM.
// At this time, processes are provided via mock data. In the future, this test will have to provide those mocks. | ||
const processes: ProcessEvent[] = [ | ||
const serverResponseAction: ResolverAction = { | ||
type: 'serverReturnedResolverData', |
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 not use the existing mock factory?
@@ -17,6 +17,12 @@ export interface EndpointPluginSetupDependencies { | |||
|
|||
export interface EndpointPluginStartDependencies {} // eslint-disable-line @typescript-eslint/no-empty-interface | |||
|
|||
export interface EndpointPluginServices extends Partial<CoreStart> { |
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.
TODO doc comment
@@ -8,7 +8,7 @@ import { EndpointAppConstants } from '../../../../common/types'; | |||
|
|||
describe('children events query', () => { | |||
it('generates the correct legacy queries', () => { | |||
const timestamp = new Date(); | |||
const timestamp = new Date().getTime(); |
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.
consider reverting this file
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.
going to keep it as is, the ChildrenQuery function uses the PaginationParams interface, which has timestamp as a number https://github.com/elastic/kibana/pull/58288/files/0626d65f70f9164285fce3659c7643625a2ee085#diff-530e696bd60363b2b701f88bbb9bc6feR14
@@ -8,7 +8,7 @@ import { EndpointAppConstants } from '../../../../common/types'; | |||
|
|||
describe('related events query', () => { | |||
it('generates the correct legacy queries', () => { | |||
const timestamp = new Date(); | |||
const timestamp = new Date().getTime(); |
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.
revert?
if (columnId === 'alert_type') { | ||
return ( | ||
<Link | ||
data-testid="alertTypeCellLink" | ||
to={urlFromQueryParams({ ...queryParams, selected_alert: 'TODO' })} | ||
to={urlFromQueryParams({ ...queryParams, selected_alert: row.event.id })} |
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.
@kqualters-elastic We use row.id
in the rest of the alert details flyout. We use row.id
to fetch the details of the alert.
uiQueryParams, | ||
alertListData, | ||
({ selected_alert: selectedAlert }, alertList) => { | ||
const found = alertList.find(alert => alert.event.id === selectedAlert); |
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.
These will never equal each other based on how the rest of the details flyout is implemented. We use row.id
for selectedAlert
.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (26 commits) [Endpoint] Alert Details Overview (elastic#58412) Service map language icons (elastic#58633) [SIEM] [Case] Comments to case view (elastic#58315) Remove appBasePath from docs + add mock for AppMountParameters (elastic#58775) [kbn/optimizer] fix ui/* url rewrites in dist (elastic#58627) Dashboard a11y tests (elastic#58122) Downgrade "setting up plugin" log to debug (elastic#58776) [CI] Pipeline refactoring (elastic#56447) [Advanced Settings] Fix a11y of unsaved indicator (elastic#58511) put params into short url instead of behind it (elastic#58846) show timepicker in timelion and tsvb (elastic#58857) improve graph missing workspace error message (elastic#58876) [Maps] direct Discover "visualize" to open Maps application (elastic#58549) Disallow duplicate percentiles (elastic#57444) (elastic#58299) removing references to visTypes uiExports (elastic#58337) [SIEM] Default the Timeline events filter to show All events (elastic#58953) [Remote clusters] Add indexManagement as required plugin (elastic#58915) [DOCS] Rework of main get started page (elastic#58260) [Endpoint] [Tests] fixes elastic#57946 flaky endpoint policy list test (elastic#58348) [Endpoint] add resolver middleware (elastic#58288) ...
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* Add resolver middleware * Update types to match events, use sample events in useCamera tests * add predicate to convert alertdata to legacy endpoint event * Use mock event generator in tests * Guard against events not having agent or endpoint fields Co-authored-by: Robert Austin <robert.austin@elastic.co>
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
Summary
First pass at creating a middleware for resolver, that loads data from the recently added resolver apis. Context is passed to a new middleware for the separate resolver store that uses kibana http services to load events from the 3 recently added resolver apis.
Related issues:
https://github.com/elastic/endpoint-app-team/issues/25
https://github.com/elastic/endpoint-app-team/issues/202
Checklist
non goals
Delete any items that are not applicable to this PR.
For maintainers