Skip to content
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] Fix endpoint tests with data streams #68794

Merged
merged 8 commits into from
Jun 11, 2020

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Jun 10, 2020

This PR addresses the endpoint test issue: #68584

Background

The endpoint code relies on the Ingest manager. The Ingest manager leverages the new data streams feature from ES. Data streams enforce a couple of things that cause issues with usage of es archiver.

Data streams

Data streams aren't normal indices. They're more of a group of indices. The backing index for a data stream is .ds-<index name>-<roll over number>. So when es archiver puts data in an index say events-endpoint the actual backing index will be .ds-events-endpoint-00001.

The Issues

Inserting documents using ES Archiver

When inserting documents into a data stream you have to use create otherwise an error will be returned:

"name": "ResponseError",
  "meta": {
    "body": {
      "error": {
        "root_cause": [
          {
            "type": "illegal_argument_exception",
            "reason": "The provided expression [events-endpoint-1] matches a data stream, specify the corresponding concrete indices instead."
          }
        ],
        "type": "illegal_argument_exception",
        "reason": "The provided expression [events-endpoint-1] matches a data stream, specify the corresponding concrete indices instead."
      },
      "status": 400
    },
    "statusCode": 400,

To get around this I've updated es archiver to always use create instead of index.

I tried to solve this by having our archives put data directly into the backing index of a data stream .ds-.... The problem with this is the data stream does not get created until we send data to an index that matches the data stream template. So if we create/modify the backing index before the data stream is created it will not be part of the data stream and our queries will fail.

Another solution would be to post up a document that causes the stream to be created, then have es archiver modify the backing index with the documents we actually want in the tests. I don't love the idea of having to hard code the backing indices everywhere because there's a chance the names could change.

ES Archiver creating indices with a specific mapping

Data streams do not allow updating the mapping of the stream as a whole, instead you have to update the mapping of the specific backing index. The normal flow for es archiver is to create the index with a specific mapping before indexing the documents. If ES archiver tries to post up a mapping for events-endpoint it will fail because this is not allowed for data streams.

Our app leverages the endpoint package to install the templates for data streams. The ingest manager handles constructing the templates and installing them for us. When our tests run these templates are already installed (they're installed either explicitly before the tests run, or for functional tests they're done on startup of Kibana).

Since the templates are there we don't need es archiver to send a mapping for us which is why I have deleted them. I believe we could get this to work by pointing the mapping file to the backing index and having the data.json.gz indices point to the data stream name. This adds more maintenance burden in my opinion though.

Deleting data streams

To delete a data stream you have to issue a DELETE _data_stream/<stream name or pattern>. Es archiver is not able to do this. Es archiver uses the index api to delete indices which will fail because it is not an index. To get around this I've created a helper function for delete data streams for each event type. I don't love this solution either because we will have to update the index patterns whenever they change in the package. We can iterate on that later though.

@jonathan-buttner jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver Feature:Endpoint Elastic Endpoint feature v7.9.0 labels Jun 10, 2020
@jonathan-buttner jonathan-buttner requested review from a team as code owners June 10, 2020 17:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@@ -30,7 +30,7 @@ export function createIndexDocRecordsStream(client: Client, stats: Stats, progre
stats.indexedDoc(doc.index);
body.push(
{
index: {
create: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data streams will fail if using index

@@ -66,26 +67,27 @@ export default function ({ getService }: FtrProviderContext) {
const nextPrevPrefixOrder = 'order=desc';
const nextPrevPrefixPageSize = 'page_size=10';
const nextPrevPrefix = `${nextPrevPrefixQuery}&${nextPrevPrefixDateRange}&${nextPrevPrefixSort}&${nextPrevPrefixOrder}&${nextPrevPrefixPageSize}`;
const alertIndex = 'events-endpoint-1';
const alertIndex = '.ds-events-endpoint-1-000001';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alert tests need the exact backing index for a couple of the tests. I don't love this, another option would be to just remove those tests I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the event is quite different from the other indices, I am guessing this is the best right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's going to change soon. It depends on the conclusion of this discussion: https://github.com/elastic/endpoint-app-team/issues/102

@spalger
Copy link
Contributor

spalger commented Jun 10, 2020

It sounds like when esArchiver is asked to archive a datastream it should write the docs a little differently to our archives and maybe drop a special record into mappings.json that will clear the data stream rather than try to delete the index. Basically, we should probably add full support to esArchiver (maybe in a follow up PR since this gets tests working again which were failing) rather than leave the test code so coupled to the internal indexing procedure that the esArchiver uses and rely on people manually deleting the mappings.json files.

await esArchiver.unload('endpoint/alerts/host_api_feature');
// the endpoint uses data streams and es archiver does not support deleting them at the moment so we need
// to do it manually
await deleteEventsStream(getService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help it run a bit faster, you can wrap these two calls in a promise.all

@jonathan-buttner
Copy link
Contributor Author

@spalger

It sounds like when esArchiver is asked to archive a datastream it should write the docs a little differently to our archives and maybe drop a special record into mappings.json that will clear the data stream rather than try to delete the index. Basically, we should probably add full support to esArchiver (maybe in a follow up PR since this gets tests working again which were failing) rather than leave the test code so coupled to the internal indexing procedure that the esArchiver uses and rely on people manually deleting the mappings.json files.

Yep that sounds good to me. Although, because we rely on the Ingest manager to create the templates for us, we actually don't need the mapping files. It might actually be better to not have them for our tests because every time we update the Endpoint package we'd have to update the mappings as well. In the tests we should probably specifically rely on the template/mapping that the endpoint package and ingest manager install rather than creating our own. This would allow us to find bugs in the endpoint package's mapping definition.

It might be nice to have an option in es archiver to not save the mappings.

@jonathan-buttner
Copy link
Contributor Author

@elasticmachine merge upstream

@jonathan-buttner
Copy link
Contributor Author

@elasticmachine retest this

@spalger
Copy link
Contributor

spalger commented Jun 10, 2020

because we rely on the Ingest manager to create the templates for us, we actually don't need the mapping files. It might actually be better to not have them for our tests because every time we update the Endpoint package we'd have to update the mappings as well. In the tests we should probably specifically rely on the template/mapping that the endpoint package and ingest manager install rather than creating our own. This would allow us to find bugs in the endpoint package's mapping definition.

It might be nice to have an option in es archiver to not save the mappings.

Sorry for the dumb question, but is the ingest manager a part of Kibana? If the ingest manager changes the way the mapping is setup will that require a PR to Kibana where we would see the breaks? They wouldn't just start breaking master right?

@jonathan-buttner
Copy link
Contributor Author

because we rely on the Ingest manager to create the templates for us, we actually don't need the mapping files. It might actually be better to not have them for our tests because every time we update the Endpoint package we'd have to update the mappings as well. In the tests we should probably specifically rely on the template/mapping that the endpoint package and ingest manager install rather than creating our own. This would allow us to find bugs in the endpoint package's mapping definition.
It might be nice to have an option in es archiver to not save the mappings.

Sorry for the dumb question, but is the ingest manager a part of Kibana? If the ingest manager changes the way the mapping is setup will that require a PR to Kibana where we would see the breaks? They wouldn't just start breaking master right?

Not a dumb question. Yeah it's an app in Kibana: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/README.md

Yes and no to your question. The ingest manager builds the mapping here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/template/template.ts#L61

So that could would require a PR that would fail if it broke our tests. But the other part is the endpoint package which the ingest manager uses to build the template. The endpoint package is not stored in Kibana. If the package changed it would start breaking our tests. That's actually related to the work you're doing here with the docker service: #68173
and
#68172

Once that's in we can pin our endpoint package for the tests and avoid that issue.

Although it would be useful to test kibana with the package that is deployed in the cloud in some automated way. We still need to work that out though.

@spalger
Copy link
Contributor

spalger commented Jun 10, 2020

In that case I think it makes sense for the esArchiver to not store the mappings and instead store a record that we're dealing with a data stream so that it can still take care of emptying the underlying indexes before writing the docs in data.json

@jonathan-buttner
Copy link
Contributor Author

jonathan-buttner commented Jun 10, 2020

In that case I think it makes sense for the esArchiver to not store the mappings and instead store a record that we're dealing with a data stream so that it can still take care of emptying the underlying indexes before writing the docs in data.json

Yeah sounds good. That's mainly our use case. I think in the scenario where someone wants to use data streams and wants esArchiver to create the data stream for them, we'd want to save the mapping and some indication that it should be a data stream. So on a load it would use the mapping with this api: https://www.elastic.co/guide/en/elasticsearch/reference/7.x/indices-create-data-stream.html to create the data stream and then write the data.json.

@jonathan-buttner
Copy link
Contributor Author

@spalger do these changes look good to you? Any concerns with merging this?

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jonathan-buttner jonathan-buttner merged commit 60f4a80 into elastic:master Jun 11, 2020
@jonathan-buttner jonathan-buttner deleted the es-archiver-ds branch June 11, 2020 16:35
jonathan-buttner added a commit that referenced this pull request Jun 12, 2020
* Temporary fix to get tests working again with data streams

* Removing mappings and renabling tests

* optionally using create for our tests only has a stop gap

* Adding default for internal function

* Removing tests that could fail if backing index changes

* Removing unused import

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@charlie-pichette
Copy link
Contributor

This also addresses #68638.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants