-
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
EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events #69708
EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events #69708
Conversation
@elasticmachine merge upstream |
…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering
@elasticmachine merge upstream |
…ub.com:nnamdifrankie/kibana into EMT-451_add_endpoint_enrolled_status_filtering
Looks good to me, I wanna make sure @ferullo is aware of the adjustment to the schema https://github.com/elastic/endpoint-package/blob/master/package/endpoint/dataset/metadata/fields/fields.yml#L52 would also like a look from @jonathan-buttner |
/** | ||
* Agent is enrolled with Fleet | ||
*/ | ||
ENROLLED = 'enrolled', |
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.
can we have the enum keys in lower case and match the value?
When (if) try to use this on the front-end it makes it easier to work with when we are working with the value.
@@ -113,6 +122,12 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp | |||
return res.notFound({ body: 'Endpoint Not Found' }); |
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 have not been picking up on this, but I think all returned static error messages should be i18n'ed
(maybe we have discussed this already?)
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 have not been picking up on this, but I think all returned static error messages should be i18n'ed
(maybe we have discussed this already?)
I do not think this applies to server side code, usually the approach is error code. This is the pattern system wide. But we will certainly review that approach.
id | ||
); | ||
if (unenrolledHostId) { | ||
throw Boom.badRequest(`the requested endpoint is unerolled`); |
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.
Same here - should the message be i18n?
@@ -7,5 +7,6 @@ | |||
export const eventsIndexPattern = 'logs-endpoint.events.*'; | |||
export const alertsIndexPattern = 'logs-endpoint.alerts-*'; | |||
export const metadataIndexPattern = 'metrics-endpoint.metadata-*'; | |||
export const metadataMirrorIndexPattern = 'metrics-endpoint.metadata_mirror-*'; |
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.
Sorry - I have not followed the discussions as close as I probably should.
What is the new index used for? just to keep track of unregistered endpoint and used by the "callback" from ingest to get updated when agent unenrolls or endpoint integration is removed from an agent config?
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 will be used to store events that is generated from Security Solution application. We do not want to pollute the index used by the endpoint, and it also give us the ability to reverse course without messing up the state of the application.
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.
There's a couple changes I think we should make:
- Address the alert schema difference between the type defined here and the schema
- Leverage
search_after
instead ofscroll
- Leverage the
filter
context instead ofquery
to improve performance.
@@ -276,6 +276,7 @@ export interface AlertEvent { | |||
type: string; | |||
}; | |||
Endpoint: { | |||
status: EndpointStatus; |
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 we need the endpoint status in the alert event? This doesn't match with what we have in the schema:
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.
Not really
id | ||
); | ||
if (unenrolledHostId) { | ||
throw Boom.badRequest(`the requested endpoint is unerolled`); |
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.
nit: unerolled
-> unenrolled
Would we ever want to show information about an unenrolled host?
hits: { | ||
hits: [ | ||
{ | ||
_index: 'metrics-endpoint.metadata_mirror-default-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.
nit: remove the trailing -1
from the index name
x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.test.ts
Show resolved
Hide resolved
|
||
if (newHits.length > 0) { | ||
const hostIds = newHits | ||
.flatMap((data) => data as HitSource) |
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 I'm missing why we need to flatten the map and then iterate over it again? I don't think we could just do a single map()
call right? to retrieve the _source
.
index: metadataMirrorIndexPattern, | ||
scroll: KEEPALIVE, | ||
body: { | ||
size: 100, |
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 limit it to 100? I think we'll want to reduce the number of API requests to ES and balance that with the amount of data we get back in a single request. I wonder if we should bump this up to over 1k?
.map((hitSource: HitSource) => hitSource._source); | ||
hits.push(...hostIds); | ||
|
||
const innerResponse = await client('scroll', { |
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.
Hmm, I don't think we want to use scroll
here. The docs say:
Scrolling is not intended for real time user requests, but rather for processing large amounts of data, e.g. in order to reindex the contents of one index into a new index with a different configuration.
I think we want to use search_after
instead: https://www.elastic.co/guide/en/elasticsearch/reference/7.8/search-request-body.html#request-body-search-search-after
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 discussed this offline. The function here is to retrieve all the results (only the ids) using a cursor based approach and does not involve any user interaction or pagination. This approach was taken in the event that the number of unerolled endpoints are greater than 10000, which is the default max size of a page. We are also returning only the host ids, so we are very minimal. Only returning the IDs means faster transmission and processing time, and small space requirement . We also keep the previous search context for 10s to 30s. I increased to 30s with the increase in page size, to allow for processing.
|
||
if (newHits.length > 0) { | ||
const hostIds: HostId[] = newHits | ||
.flatMap((data) => data as HitSource) |
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 we can remove this call.
x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.ts
Show resolved
Hide resolved
Here's the Endpoint PR to set this in it's metadata documents https://github.com/elastic/endpoint-dev/pull/7236 |
>; | ||
|
||
// eslint-disable-next-line no-return-await | ||
return await fetchAllUnenrolledHostIdsWithScroll(response, client.callAsCurrentUser); |
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.
nit: I think you can remove the await
since the caller of this function is doing an await
right? Unless we're doing this for call stack debugging?
}); | ||
|
||
// eslint-disable-next-line no-return-await | ||
return await fetchAllUnenrolledHostIdsWithScroll(innerResponse, client, hits); |
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.
nit: I think you can remove the await
since the caller of this function is doing an await
right? Unless we're doing this for call stack debugging?
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
…resence of unenrolled events (elastic#69708) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events
* master: (90 commits) [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513) [SIEM] Replace WithSource with useWithSource hook (elastic#68722) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708) rename old siem kibana config to securitySolution (elastic#69874) Remove unused Resolver code (elastic#69914) [Observability] Fixing dynamic return type based on the appName (elastic#69894) [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419) Fixes special clicks and 3rd party icon sizes in nav (elastic#69767) [APM] Catch annotations index permission error and log warning (elastic#69881) [Endpoint][Ingest Manager] minor code cleanup (elastic#69844) [Logs UI] Logs ui context menu (elastic#69915) Index pattern serialize and de-serialize (elastic#68844) [QA] Unskip functional tests (elastic#69760) [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715) Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863) PR: Provide limit warnings to user when API limits are reached. (elastic#69590) [Maps] Remove broken button (elastic#69853) Makes usage collection methods available on start (elastic#69836) [SIEM][CASE] Improve Jira's labelling (elastic#69892) [Logs UI] Access ML via the programmatic plugin API (elastic#68905) ...
* master: [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513) [SIEM] Replace WithSource with useWithSource hook (elastic#68722) [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708) rename old siem kibana config to securitySolution (elastic#69874) Remove unused Resolver code (elastic#69914) [Observability] Fixing dynamic return type based on the appName (elastic#69894) [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419) Fixes special clicks and 3rd party icon sizes in nav (elastic#69767) [APM] Catch annotations index permission error and log warning (elastic#69881) [Endpoint][Ingest Manager] minor code cleanup (elastic#69844) [Logs UI] Logs ui context menu (elastic#69915) Index pattern serialize and de-serialize (elastic#68844)
Summary
Issue: https://github.com/elastic/endpoint-app-team/issues/451
Checklist
Delete any items that are not applicable to this PR.