-
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
EMT-179: implement metadata query versioning based on ingest manager installed ES assets #77252
EMT-179: implement metadata query versioning based on ingest manager installed ES assets #77252
Conversation
# Conflicts: # x-pack/plugins/security_solution/server/endpoint/endpoint_app_context_services.ts # x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts # x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.test.ts # x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.ts # x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts
@elasticmachine merge upstream |
…:nnamdifrankie/kibana into EMT-179-version-metadata-api-and-queries
Pinging @elastic/ingest-management (Team:Ingest Management) |
@@ -8,6 +8,7 @@ export const eventsIndexPattern = 'logs-endpoint.events.*'; | |||
export const alertsIndexPattern = 'logs-endpoint.alerts-*'; | |||
export const metadataIndexPattern = 'metrics-endpoint.metadata-*'; | |||
export const metadataCurrentIndexPattern = 'metrics-endpoint.metadata_current-*'; | |||
export const metadataTransformPrefix = 'metrics-endpoint.metadata-current-default'; |
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'm stumbling over 3 -
in here as dataset / namespace should not contain -
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 is actually the dataset namespace metrics-endpoint.metadata
. The current-default
is the name of the transform file as current-default.json
. I will be moving the transform as per your request to the metrics-endpoint.metadata_current
and use the filename default.json
, which means the assets name will be prefixed metrics-endpoint.metadata_current-default
. I am working a PR for that, but this is not dependent on that as I will have to test everything end to end before the PR is opened.
*/ | ||
|
||
const IGNORED_ELASTIC_AGENT_IDS = [ | ||
'00000000-0000-0000-0000-000000000000', |
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 looks a bit hacky to be honest. I wonder if we could find a better way to do 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.
this is done because the Endpoint will send up documents before getting the first Integration Policy from the Agent. As a result, we'll have IDs that we can't compare against any Agents to determine if an Agent or Endpoint is unenrolled.
An alternative solution, I think, would require some form of messaging from the Endpoint itself as we will not be able to figure our which Agent the Endpoint comes from. @ferullo had said in the past the we should continue to send this initial document, which I agree with since it can signify an important step in the lifecycle and help with debugging later. It's just very difficult to tell when an Endpoint is unenrolled or stopped, etc.
Another long term solution would be to form our views to look for Agents first. Then enrich the list of Agents with Endpoint data. If we cannot find Endpoint data for that Agent, we show some message (Endpoint pending, etc...). I think this could be a vision for more Integration specific views in Ingest. Per Integration, we could see a list of Agents with some Integration specific (i.e. Endpoint) data enrichments.
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.
Even if endpoint did not receive a policy yet, I assume it already knows about the Agent id under which it is running? Or is this only sent down with the policy?
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.
Even if endpoint did not receive a policy yet, I assume it already knows about the Agent id under which it is running? Or is this only sent down with the policy?
This is only sent down with the Policy. This "initial" policy is hardcoded and ships with the Endpoint. We don't get updated information until the Endpoint gets the first Policy from Agent. These initial documents are sent after the ES connection is established. Is this correct @ferullo ? Just want to make sure I'm not misrepresenting anything
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.
@blakerouse @michalpristas It would be nice if we could ship down the agent id already as part of the initial "negotiation" between Agent and 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.
@ruflin The initial configuration that we pass to Endpoint already contains the fleet.agent.id
. At no point do we send Endpoint a configuration without fleet.agent.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.
@blakerouse If I understood the above correct, endpoint already starts to send events before they receive the first policy. If that is the case, is there a way we could have the agent.id as part of the "initiation" phase?
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.
@ruflin I question how that is possible, being that Endpoint would not even know the elasticsearch output information until Agent sends them the configuration that already includes fleet.agent.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.
I question how that is possible, being that Endpoint would not even know the elasticsearch output information until Agent sends them the configuration that already includes
fleet.agent.id
.
@ferullo my understanding is that the Endpoint has the initial documents ready to send and this is essentially a flush?
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.
Yes. Endpoint caches data internally to send later when it does not have an active connection to ES.
packageService: { | ||
getInstalledEsAssetReferences: async ( | ||
savedObjectsClient: SavedObjectsClientContract, | ||
pkgName: string | ||
): Promise<EsAssetReference[]> => { | ||
const installation = await getInstallation({ savedObjectsClient, pkgName }); | ||
return installation?.installed_es || []; | ||
}, | ||
}, |
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.
@jfsiii please let me if you want this moved to another 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.
Thanks for asking. I think it'd ideally it'd be defined elsewhere, but I'm fine shipping as-is and leaving that for later. I'll find/create a ticket and reference this there.
): Promise<MetadataQueryStrategy>; | ||
} | ||
|
||
export const createMetadataService = (packageService: PackageService): MetadataService => { |
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.
does this get called on package upgrade/downgrade? Does the Kibana server need to be restarted?
Pulled it down and tried it with the UI, so far everything is working. I'll also test it with an old package to make sure the UI still works without the upgrade - @nnamdifrankie you said that we can force install an older package, how does that work? |
I have not been able to get a smooth transition to work for upgrade and downgrade. Right now the steps that have worked for me is the following:
|
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.
👍 for Ingest Manger changes (adding/exposing PackageService
)
packageService: { | ||
getInstalledEsAssetReferences: async ( | ||
savedObjectsClient: SavedObjectsClientContract, | ||
pkgName: string | ||
): Promise<EsAssetReference[]> => { | ||
const installation = await getInstallation({ savedObjectsClient, pkgName }); | ||
return installation?.installed_es || []; | ||
}, | ||
}, |
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.
Thanks for asking. I think it'd ideally it'd be defined elsewhere, but I'm fine shipping as-is and leaving that for later. I'll find/create a ticket and reference this there.
@elasticmachine merge upstream |
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 👍
Had a few minor suggestion/questions, but all optional.
) | ||
), | ||
total: totalNumberOfHosts, | ||
query_strategy_version: hostListQueryResult.queryStrategyVersion, |
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.
🕺
Awesome. So the UI will use this new response property to conditionally show the KQL bar if version is v2
👍
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.
Yes
return endpointAppContext.logFactory.get('metadata'); | ||
}; | ||
export const BASE_ENDPOINT_ROUTE = '/api/endpoint'; | ||
export const METADATA_REQUEST_V1_ROUTE = `${BASE_ENDPOINT_ROUTE}/v1/metadata`; |
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 routes are all for non-UI usage, correct? From the UI side, we will continue to use the METADATA_REQUEST_ROUTE
defined below, right?
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.
Yes, it is transparent to the UI
export const METADATA_REQUEST_ROUTE = `${BASE_ENDPOINT_ROUTE}/metadata`; | ||
export const GET_METADATA_REQUEST_ROUTE = `${METADATA_REQUEST_ROUTE}/{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.
Side note: it would be great if these two const
could be elevated to x-pack/plugins/security_solution/common/endpoint/constants.ts
so that the UI can also use it. See the entries there now for Trusted apps for a reference
{ | ||
path: `${GET_METADATA_REQUEST_ROUTE}`, | ||
validate: GetMetadataRequestSchema, | ||
options: { authRequired: true, tags: ['access:securitySolution'] }, |
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.
Q. What does the tags
do?
Because I may have missed that in the Trusted Apps 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.
it is for space access control
@@ -0,0 +1,103 @@ | |||
/* |
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.
suggestion: name this file mocks
. Seems to be common across kibana. Also, I often see the function names have the word mock
in it - ex. createV1SearchResponseMock()
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsasync chunks size
distributable file count
History
To update your PR or re-run it, just comment with: |
…installed ES assets (elastic#77252) * EMT-179: initial refactor for versioning * EMT-179: move things before pulling from master * EMT-179: fix build * EMT-179: clean up * EMT-179: add ingest hook, and improve all tests * EMT-179: fix build * EMT-179: clean up * EMT-179: fix build * EMT-179: fix build * EMT-179: clean up * EMT-179: more clean up * EMT-179: clean up * EMT-179: fix build Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
https://github.com/elastic/security-team/issues/179
#76545