-
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] Unifying the test index name for resolver and alerts #59073
[Endpoint] Unifying the test index name for resolver and alerts #59073
Conversation
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
static ENDPOINT_INDEX_NAME = 'endpoint-agent*'; | ||
static EVENT_INDEX_NAME = 'endpoint-events-*'; | ||
static ALERT_INDEX_NAME = 'events-endpoint-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.
so alerts always go to '-1' and events can go to anywhere starting with 'events-endpoint-'?
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 the same question... where do the other events go? Why "-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.
No, ultimately the alerts will go to events-endpoint-<some rollover number>
same as other events. I had to hard code -1
because I ran into an issue getting this working with the alert details API.
@madirey @peluja1012 @marshallmain
The alert details API use the elasticsearch's get API which requires a specific index: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-get.html the problem with that is I don't think the alert list api returns the specific index that a document is in with the response and the event index will roll over based on the ILM policy. So it'll change from events-endpoint-000001
to events-endpoint-000002
etc. I tried having the alert details request take a pattern like events-endpoint-*
but that causes a 500 from elasticsearch because the get API needs a single index.
I think we need to encode the index of the document in the _id
or something so when the frontend passes it back, the backend can determine the specific index since we won't know that ahead of time.
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 index is returned by ES for each hit when searching... we can return that in the response to the FE and/or cache it... will need some logic to look it up if it's not available, or we could just change it to use the search 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.
Is this a case where we should use aliases for the alert indexes to unify them into a single index endpoint?
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.
@achuguy hmm I don't think we could use an alias to abstract the search/get api. The endpoint will use an alias for all events in general so it doesn't know the specific rollover index either. We could use a pipeline to break them out but we already made the decision to write the alerts and events to a single index.
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 Yeah, that sounds like a good approach...
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 If the endpoint is using an alias to write to, can you use a get with the same alias or are you specifically trying to isolate you get to a single index?
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.
@achuguy so I believe you can have an alias that points to multiple indices but it doesn't look like you can do a GET
on an alias with multiple indices:
You can execute searches I believe though but I think we should avoid doing a search since we have all the info we need to just run a GET
.
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, thanks for clearing that up for me
@@ -7,7 +7,7 @@ | |||
import { ResolverEvent, LegacyEndpointEvent } from '../../../../common/types'; | |||
|
|||
function isLegacyData(data: ResolverEvent): data is LegacyEndpointEvent { | |||
return data.agent.type === 'endgame'; | |||
return data.agent?.type === '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.
The endpoint isn't sending the agent.type
field yet so guard against a crash here
I recall a test file for alerts had my-index, does this need updating: |
Thanks @EricDavisX yeah it was updated |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack OpenID Connect API Integration Tests.x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth·js.apis OpenID Connect authentication finishing handshake should succeed if both the OpenID Connect response and the cookie are providedStandard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
5 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…tic#59073) * Unifying the test index name for resolver and alerts * Endpoint isn't sending the agent field so check for it Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Friendly reminder: Looks like this PR hasn’t been backported yet. |
5 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…tic#59073) * Unifying the test index name for resolver and alerts * Endpoint isn't sending the agent field so check for it Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
7.x/7.7: dfea6de |
This PR unifies the alerts and events index name. These names need to be the same for retrieving events and alerts since they will be stored in the same index. The name I used is more inline with what the package will have: elastic/package-registry#223
After this change goes in, you'll want to use
events-endpoint-1
to store the alerts and events for testing.