-
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
[Endpoint] Basic Functionality Alert List #55800
Changes from 8 commits
e5a6278
ff349af
806dc49
3ec2901
d05a80b
d8d5475
919e304
1063eda
49312ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,24 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
/** | ||
* A deep readonly type that will make all children of a given object readonly recursively | ||
*/ | ||
export type Immutable<T> = T extends undefined | null | boolean | string | number | ||
? T | ||
: T extends Array<infer U> | ||
? ImmutableArray<U> | ||
: T extends Map<infer K, infer V> | ||
? ImmutableMap<K, V> | ||
: T extends Set<infer M> | ||
? ImmutableSet<M> | ||
: ImmutableObject<T>; | ||
|
||
export type ImmutableArray<T> = ReadonlyArray<Immutable<T>>; | ||
export type ImmutableMap<K, V> = ReadonlyMap<Immutable<K>, Immutable<V>>; | ||
export type ImmutableSet<T> = ReadonlySet<Immutable<T>>; | ||
export type ImmutableObject<T> = { readonly [K in keyof T]: Immutable<T[K]> }; | ||
|
||
export class EndpointAppConstants { | ||
static ENDPOINT_INDEX_NAME = 'endpoint-agent*'; | ||
} | ||
|
@@ -44,3 +62,37 @@ export interface EndpointMetadata { | |
}; | ||
}; | ||
} | ||
|
||
export type AlertData = Immutable<{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be one section of the alert data that is mutable... otherwise, it should all be immutable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment (in jsdoc style) to each field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This data will change very soon to conform to ECS. We will document more once the format is closer to the final format. Right now we are using outdated fake data. |
||
value: { | ||
source: { | ||
endgame: { | ||
data: { | ||
file_operation: string; | ||
malware_classification: { | ||
score: number; | ||
}; | ||
}; | ||
metadata: { | ||
key: string; | ||
}; | ||
timestamp_utc: Date; | ||
}; | ||
labels: { | ||
endpoint_id: string; | ||
}; | ||
host: { | ||
hostname: string; | ||
ip: string; | ||
os: { | ||
name: string; | ||
}; | ||
}; | ||
}; | ||
}; | ||
}>; | ||
|
||
/** | ||
* The PageId type is used for the payload when firing userNavigatedToPage actions | ||
*/ | ||
export type PageId = 'alertsPage' | 'endpointListPage'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { EndpointListAction } from './endpoint_list'; | ||
import { AlertAction } from './alerts'; | ||
import { RoutingAction } from './routing'; | ||
|
||
export type AppAction = EndpointListAction | AlertAction | RoutingAction; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AlertData } from '../../../../../common/types'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts about setting a I'm not sure if we are going to be able to maintain our own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexk307 We are currently not using our own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI - I seem remember a discussion from the Kibana platform team that suggests this is something they want to look at and possibly introduce aliases that will simplify the import paths. Found discussion on Kibana issues (look at Issue 40446). |
||
|
||
interface ServerReturnedAlertsData { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would be a better place for // maybe
interface ServerReturnedAlertsData {
readonly type: 'serverReturnedAlertsData';
readonly payload: readonly AlertData[];
}
// or
type ServerReturnedAlertsData = Immutable<{
type: 'serverReturnedAlertsData';
payload: AlertData[];
}>; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
readonly type: 'serverReturnedAlertsData'; | ||
|
||
readonly payload: AlertData[]; | ||
} | ||
|
||
export type AlertAction = ServerReturnedAlertsData; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { alertListReducer } from './reducer'; | ||
export { AlertAction } from './action'; | ||
export * from './types'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using a simple middleware to handle side-effects here, as opposed to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with this approach as well - much simpler There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peluja1012 what are some of the main differences here? It looks like we're still using Let me know what I'm missing @oatkiller @paul-tavares @parkiino I'm all for simplicity, just wanna better understand what we're proposing that we remove. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we're proposing to remove anything. Just that the alerting feature will (initially anyway) forgo The logic consists of:
Redux middleware, by default, provides the ability to run code on each action. |
||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Dispatch, MiddlewareAPI } from 'redux'; | ||
import { CoreStart } from 'kibana/public'; | ||
import { AlertData } from '../../../../../common/types'; | ||
import { GlobalState } from '../reducer'; | ||
import { AppAction } from '../action'; | ||
|
||
// TODO, move this somewhere | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DO IT then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
type MiddlewareFactory = ( | ||
coreStart: CoreStart | ||
) => ( | ||
api: MiddlewareAPI<Dispatch<AppAction>, GlobalState> | ||
) => (next: Dispatch<AppAction>) => (action: AppAction) => unknown; | ||
|
||
export const alertMiddlewareFactory: MiddlewareFactory = coreStart => { | ||
return store => next => async (action: AppAction) => { | ||
next(action); | ||
if (action.type === 'userNavigatedToPage' && action.payload === 'alertsPage') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming by your above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexk307 We define actions in files like this one https://github.com/elastic/kibana/pull/55800/files#diff-c971622c4d5a45ad624ef764513a0131. The type checker will throw and error if you use an invalid action type in this if-statement. So if we were to do |
||
const response: AlertData[] = await coreStart.http.get('/api/endpoint/alerts'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be read only ya? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
store.dispatch({ type: 'serverReturnedAlertsData', payload: response }); | ||
} | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Reducer } from 'redux'; | ||
import { AlertListState } from './types'; | ||
import { AppAction } from '../action'; | ||
|
||
const initialState = (): AlertListState => { | ||
return { | ||
alerts: [], | ||
}; | ||
}; | ||
|
||
export const alertListReducer: Reducer<AlertListState, AppAction> = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you using Some suggestions: The feature (aka concern) is called 'alerting' and the directory takes this name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will address this in the next PR where I add a stub alert detail |
||
state = initialState(), | ||
action | ||
) => { | ||
if (action.type === 'serverReturnedAlertsData') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to consider (in the future) "reseting" the state when a user leaves the Alerts list so that data stored here does not take up memory in the browser. |
||
return { | ||
...state, | ||
alerts: action.payload, | ||
}; | ||
} | ||
|
||
return state; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AlertListState } from './types'; | ||
|
||
export const alertListData = (state: AlertListState) => state.alerts; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider naming this type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will address this in the next PR where I add a stub alert detail |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The types in this file would move the main There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the topic of "top level types" and given my prior comment re: PR #54772 - These types here (and in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paul-tavares Moved the types to |
||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Immutable, AlertData } from '../../../../../common/types'; | ||
|
||
export type AlertListState = Immutable<{ | ||
alerts: AlertData[]; | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
}>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,9 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Reducer } from 'redux'; | ||
import { EndpointListState } from './types'; | ||
import { EndpointListAction } from './action'; | ||
import { AppAction } from '../action'; | ||
|
||
const initialState = (): EndpointListState => { | ||
return { | ||
|
@@ -16,7 +17,10 @@ const initialState = (): EndpointListState => { | |
}; | ||
}; | ||
|
||
export const endpointListReducer = (state = initialState(), action: EndpointListAction) => { | ||
export const endpointListReducer: Reducer<EndpointListState, AppAction> = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah Ok - but were you getting TS errors? I think the benefit was that types shown here would be narrowed down to only the ones this concern cares about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We weren't getting errors but I think to your previous point it might be good to take advantage of being able to use other concern's or generic/global app actions in the future and this would keep that ability. Kind of like how we have the page navigated actions now. I do see what you're saying though, i don't necessarily disagree with it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even if we filter out non-concern specific actions in the future, they aren't yet filtered out, so this type is more correct |
||
state = initialState(), | ||
action | ||
) => { | ||
if (action.type === 'serverReturnedEndpointList') { | ||
return { | ||
...state, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { PageId } from '../../../../../common/types'; | ||
|
||
interface UserNavigatedToPage { | ||
readonly type: 'userNavigatedToPage'; | ||
readonly payload: PageId; | ||
} | ||
|
||
export type RoutingAction = UserNavigatedToPage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { GlobalState } from './reducer'; | ||
import * as alertListSelectors from './alerts/selectors'; | ||
|
||
export const alertListData = composeSelectors( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI. File: export function useEndpointListSelector<TSelected>(
selector: (state: EndpointListState) => TSelected
) {
return useSelector(function(state: GlobalState) {
return selector(state.endpointList);
});
} The idea is that each of the concerns will have an associated hook. To use this in a component, you would: import { endpointListData } from "../store/endpoint_list/selectors;
import { useEndpointListSelector } from "../store/hooks;
const EndpointsListPage = () => {
const listData = useEndpointListSelector(endpointListData);
return (<div>...</div>)
} (Credit to @oatkiller for the idea during a recent discussion 😃 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paul-tavares If we use this approach, how would a middleware (or saga) use the selector if needed to grab some data from state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peluja1012 - From a saga or middleware we have access directly to the When we discussed this, the goal was to avoid the need to define all concerns selectors a second time in order for us to continue to use |
||
alertListStateSelector, | ||
alertListSelectors.alertListData | ||
); | ||
|
||
/** | ||
* Returns the alert list state from within Global State | ||
*/ | ||
function alertListStateSelector(state: GlobalState) { | ||
return state.alertList; | ||
} | ||
|
||
/** | ||
* Calls the `secondSelector` with the result of the `selector`. Use this when re-exporting a | ||
* concern-specific selector. `selector` should return the concern-specific state. | ||
*/ | ||
function composeSelectors<OuterState, InnerState, ReturnValue>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
selector: (state: OuterState) => InnerState, | ||
secondSelector: (state: InnerState) => ReturnValue | ||
): (state: OuterState) => ReturnValue { | ||
return state => secondSelector(selector(state)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { memo, useState, useMemo } from 'react'; | ||
import React from 'react'; | ||
import { EuiDataGrid } from '@elastic/eui'; | ||
import { useSelector } from 'react-redux'; | ||
import * as selectors from '../../store/selectors'; | ||
import { usePageId } from '../use_page_id'; | ||
|
||
export const AlertIndex = memo(() => { | ||
usePageId('alertsPage'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where we call the |
||
|
||
const columns: Array<{ id: string }> = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider wrapping this in |
||
{ id: 'alert_type' }, | ||
{ id: 'event_type' }, | ||
{ id: 'os' }, | ||
{ id: 'ip_address' }, | ||
{ id: 'host_name' }, | ||
{ id: 'timestamp' }, | ||
{ id: 'archived' }, | ||
{ id: 'malware_score' }, | ||
]; | ||
|
||
const [visibleColumns, setVisibleColumns] = useState(() => columns.map(({ id }) => id)); | ||
|
||
const json = useSelector(selectors.alertListData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is pretty neat |
||
|
||
const renderCellValue = useMemo(() => { | ||
return ({ rowIndex, columnId }: { rowIndex: number; columnId: string }) => { | ||
if (json.length === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe instead
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
return null; | ||
} | ||
|
||
if (columnId === 'alert_type') { | ||
return json[rowIndex].value.source.endgame.metadata.key; | ||
} else if (columnId === 'event_type') { | ||
return json[rowIndex].value.source.endgame.data.file_operation; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you confirm that all alerts are guaranteed to have this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will do this properly in future PRs. We just want to get some basic code structure and patterns merged now. |
||
} else if (columnId === 'os') { | ||
return json[rowIndex].value.source.host.os.name; | ||
} else if (columnId === 'ip_address') { | ||
return json[rowIndex].value.source.host.ip; | ||
} else if (columnId === 'host_name') { | ||
return json[rowIndex].value.source.host.hostname; | ||
} else if (columnId === 'timestamp') { | ||
return json[rowIndex].value.source.endgame.timestamp_utc; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs l10n. See https://github.com/elastic/kibana/pull/55590/files#diff-de600f9eb8a3427216e75a1cbf4bae6bR93 for ideas There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. We will add intl in future PRs. We just want to get some basic code structure and patterns merged now. |
||
} else if (columnId === 'archived') { | ||
return null; // TODO change this once its available in backend | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can ya move this to a github issue plz There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed comment. Github issue here https://github.com/elastic/endpoint-app-team/issues/87 |
||
} else if (columnId === 'malware_score') { | ||
return json[rowIndex].value.source.endgame.data.malware_classification.score; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you confirm that all alerts have this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will do this properly in future PRs. We just want to get some basic code structure and patterns merged now. |
||
} | ||
return null; | ||
}; | ||
}, [json]); | ||
|
||
return ( | ||
<EuiDataGrid | ||
aria-label="Alert List" | ||
rowCount={json.length} | ||
// Required. Sets up three columns, the last of which has a custom schema we later define down below. | ||
// The second column B won't allow clicking in to see the content in a popup. | ||
// The first column defines an starting width of 150px and prevents the user from resizing it | ||
columns={columns} | ||
// This allows you to initially hide columns. Users can still turn them on. | ||
columnVisibility={{ | ||
visibleColumns, | ||
setVisibleColumns, | ||
}} | ||
// Often used in combination with useEffect() to dynamically change the render. | ||
renderCellValue={renderCellValue} | ||
/> | ||
); | ||
}); |
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.
please add a comment to all exported identifiers