-
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][EPM] Retrieve Index Pattern from Ingest Manager #63016
[Endpoint][EPM] Retrieve Index Pattern from Ingest Manager #63016
Conversation
…enabling ingest in the base tests
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
Pinging @elastic/ingest-management (Feature:EPM) |
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.
LGTM 👍
|
||
async getIndexPattern(ctx: RequestHandlerContext, datasetPath: string) { | ||
try { | ||
const pattern = await this.service.getESIndexPattern( |
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.
Should (can?) any of these call be cached?
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, I haven't created an issue for it yet but we'll create a cache so we don't have to do so many calls to the saved objects.
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.
@@ -0,0 +1,11 @@ | |||
/* |
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 curious why you had to create this typescript definition 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.
Yeah it wasn't necessary, it seemed like most test directories had their own but I think it's only useful if we define our own services and page_objects.
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.
Changes for tests look good. 👍
@jonathan-buttner Can you explain this a little? The Endpoint client code is responsible for calling the ingest server side setup function? I know you mentioned it to me, but I can't remember what the reasoning was. |
return await this.getIndexPattern(ctx, EndpointAppConstants.EVENT_DATASET); | ||
} | ||
|
||
async getMetadataIndexPattern(ctx: RequestHandlerContext) { |
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.
would you mind adding a quick doc comment to public methods? maybe
/**
* fetch and return the index pattern used to store Endpoint host metadata.
*/
I think these types of comments will help a lot, esp. considering the size of the org
x-pack/plugins/endpoint/server/routes/alerts/details/handlers.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/endpoint/server/routes/alerts/details/handlers.ts
Outdated
Show resolved
Hide resolved
}; | ||
} | ||
|
||
export function registerIndexPatternRoute(router: IRouter, endpointAppContext: EndpointAppContext) { |
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 this for the alert list feature? if so, thoughts on moving it to the alerting routes module?
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 it's currently only used for the alert list. The alert list uses an index pattern when doing time based filtering I think. I think having an index pattern is required when doing that.
I could foresee other pages (Host maybe?) needing an index pattern if they want to do any time based filtering too. If you'd prefer it in the alert list for now until it's needed in the future, that's fine too though.
@@ -34,7 +39,7 @@ describe('query builder', () => { | |||
aggs: { | |||
total: { | |||
cardinality: { | |||
field: 'host.id.keyword', | |||
field: 'host.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.
im a bit of a noob about this. can you explain this change? is this just keeping us in sync w/ latest mappings?
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 it's because the mapping that was being used for metadata was based off a dynamically generated mapping created by default by elasticsearch when no mapping exists. Now that we have solidified and generated a mapping using ECS we want to use that. With the new mapping the field keyword
no longer exists.
https://www.elastic.co/blog/strings-are-dead-long-live-strings
"successful" : 1, | ||
"skipped" : 0, | ||
"failed" : 0 | ||
"took": 343, |
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 you explain the change here? also, anyone know why this is large and not gzipped?
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.
@jonathan-buttner This will be on me to 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.
@oatkiller most of the changes are just spacing and renaming the index being used:
my vs code default formatter is removing the spaces like these, I can revert them if you prefer:
I believe this is not gzipped because it's directly imported in typescript code here:
https://github.com/elastic/kibana/pull/63016/files/2829689ae9674970d57644212863734d34d2cd5b#diff-b1e4abedd6d2153cf169796ebae56a93R27
@nnamdifrankie yeah if you could fix it that'd be great!
): Promise<string | undefined>; | ||
} | ||
|
||
export class ESIndexPatternSavedObjectService implements ESIndexPatternService { |
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.
side note, classes in ts are also interfaces, so ESIndexPatternSavedObjectService
can be used exactly the same as ESIndexPatternService
Using a class as an interface
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.
Ah cool! Didn't realize that.
export default function({ getService }: FtrProviderContext) { | ||
const supertest = getService('supertest'); | ||
|
||
describe('Endpoint index pattern API', () => { |
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 mentioned this earlier, but what is the purpose of exposing these APIs?
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 it doesn't have to be, it's only used by the alert list but could be useful if other UIs need to do time based filtering of a list.
@oatkiller yeah, the Ingest Manager plugin will handle calling the That service is then used before our endpoint api_integration tests run here: https://github.com/jonathan-buttner/kibana/blob/endpoint-ingest-index-pattern/x-pack/test/api_integration/apis/endpoint/index.ts#L17 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (40 commits) [APM]Upgrade apm-rum agent to latest version to fix full page reload (elastic#63723) add deprecation warning for legacy 3rd party plugins (elastic#62401) Migrate timelion vis (elastic#62819) Replacebad scope link with actual values (elastic#63444) Index pattern management UI -> TypeScript and New Platform Ready (create_index_pattern_wizard) (elastic#63111) [SIEM] Threat hunting enhancements: Filter for/out value, Show top field, Copy to Clipboard, Draggable chart legends (elastic#61207) [Maps] fix term join agg key collision (elastic#63324) [Ingest] Fix agent config key sorting (elastic#63488) [Monitoring] Fixed server response errors (elastic#63181) update elastic charts to 18.3.0 (elastic#63732) Start services (elastic#63720) [APM] Encode spaces when creating ML job (elastic#63683) Uptime 7.7 docs (elastic#62228) [DOCS] Updates remote cluster and ccr docs (elastic#63517) [Maps] Add 3rd party vector tile support (elastic#62084) [Endpoint][EPM] Retrieve Index Pattern from Ingest Manager (elastic#63016) [Endpoint] Host Details Policy Response Panel (elastic#63518) [Uptime] Certificate expiration threshold settings (elastic#63682) Refactor saved object types to use `namespaceType` (elastic#63217) [SIEM][CASE] Create comments sequentially (elastic#63692) ...
…63767) * Endpoint successfully depending on ingest manager to initialize * Moving the endpoint functional tests to their own directory to avoid enabling ingest in the base tests * Removing page objects and other endpoint fields from base functional * Updating code owners with new functional location * Adding index pattern functionality * Missed a file * Pointing resolver tests at endpoint functional tests * Pointing space tests at the endpoint functional directory * Adding ingest service to do setup and tests for 500s * Correcting services path * Adding jest test names * Updating es archives with the correct mapping and index names * Fixing import error * Adding resolver tests to code owners * enabling epm flag for functional tests * adding correct tag to test * Removing the version information and unneeded xsrf * Addressing endpoint index pattern feedback * Removing unused import * Renaming index pattern to es index pattern * Fixing missed index pattern calls * Removing unused import * Fixing type error * Moving es_index_pattern outside of installed and fixing function name * Keeping the event index the same for now * Wrapping index pattern await in try catch * Address PR feedback, adding comments Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Important Change
When having the endpoint send metadata use the index
metrics-endpoint-default-1
.Summary
I'm going to leave this PR in draft until the the endpoint-ingest dependency PR is merged here: #62871Once that PR is merged this one's number of changed files will go down.This PR exposes functionality in the Ingest Manager to construct an index pattern based on the contents of a package. The endpoint app will rely on the ingest manager to retrieve index patterns for querying elasticsearch. The advantage of using the package to construct the index pattern is it isolates the logic for defining the index patterns to only the endpoint package so it only needs to be maintained there. The downside is a request must be made to access a saved object when the index pattern is retrieved for each time that index pattern is needed. This is done once per request when needed. If a route handler makes multiple queries, it only retrieves the pattern once. I spoke with the Kibana team about the extra overhead of using saved objects and they recommended not worrying about it for now. If it becomes an issue we can implement a cache within the endpoint app.
Notable Changes
Ingest Manager
patterns
field which is a map ofdataset
to constructed index pattern (e.g.events
->events-endpoint-*
)api_integration
test directory to handle setting up the ingest manager before the endpoint api_integration tests. This is needed because even though the endpoint app depends on the ingest manager, since the UIs are not used it's setup function will not be calledEndpoint
id
field when returning them to the UI. I'll make that change in a follow up PR once this merged.Testing
If you'd like to test this PR you'll need to add a couple flags to your
kibana.dev.yml
file:The ingest manager will pull the packages from the default package registry URL. If you did want to run your own package registry you can point ingest manager to it using the
xpack.ingestManager.epm.registryUrl
(more info here: https://github.com/elastic/package-registry and https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/README.md)For the endpoint app you can try populating elasticsearch with events from the alerts es_archive and make sure you can still see them in the alert list view: