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

Fix broken usages of bulkCreate #75597

Merged

Conversation

jportner
Copy link
Contributor

Summary

A recent change to the saved object bulkCreate operation allowed consumers to specify an object's version to take advantage of Elasticsearch's optimistic concurrency control (when overwriting existing objects). However, some consumer usages of bulkCreate were not updated and can accidentally pass in an invalid version. In these cases, the version needs to be filtered from the source data before executing bulkCreate.

Resolves #75596.

@jportner jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.1 labels Aug 20, 2020
@jportner jportner requested review from gmmorris and LeeDr August 20, 2020 19:28
@jportner
Copy link
Contributor Author

jportner commented Aug 20, 2020

@elastic/siem @elastic/endpoint-app-team I found another potentially problematic usage of bulkCreate in your code:

postUserActions: async ({ client, actions }: PostCaseUserActionArgs) => {
try {
this.log.debug(`Attempting to POST a new case user action`);
return await client.bulkCreate(
actions.map((action) => ({ type: CASE_USER_ACTION_SAVED_OBJECT, ...action }))
);
} catch (error) {
this.log.debug(`Error on POST a new case user action: ${error}`);
throw error;
}
},

I'm not sure where actions comes from; is it possible for any objects in the actions array to inadvertently specify a version field? If so, we'll need to change this code to filter out the version. Let me know if that's the case and I can add it to this PR.


Edit: I dug into this one on my own. The postUserActions API only allows action objects with atttributes and references fields to be passed in, not whole raw saved objects. It appears this usage is safe / not broken.

@jportner
Copy link
Contributor Author

jportner commented Aug 20, 2020

@elastic/ml-ui I found another potentially problematic usage of bulkCreate in your code:

async saveKibanaObjects(objectExistResults: ObjectExistResponse[]) {
let results = { saved_objects: [] as any[] };
const filteredSavedObjects = objectExistResults
.filter((o) => o.exists === false)
.map((o) => o.savedObject);
if (filteredSavedObjects.length) {
results = await this.savedObjectsClient.bulkCreate(
// Add an empty migrationVersion attribute to each saved object to ensure
// it is automatically migrated to the 7.0+ format with a references attribute.
filteredSavedObjects.map((doc) => ({
...doc,
migrationVersion: doc.migrationVersion || {},
}))
);
}
return results.saved_objects;
}

I'm not sure where objectExistResults comes from; is it possible for any objects in the objectExistResults array to inadvertently specify a version field? If so, we'll need to change this code to filter out the version. Let me know if that's the case and I can add it to this PR.


Edit: I dug into this one on my own. The saveKibanaObjects method is only currently used with output from the createSavedObjectsToSave method. This looks to be safe (does not include a root-level version field), but since it relies on usage of the any type, any changes would not be caught by the type checker.

@jportner
Copy link
Contributor Author

jportner commented Aug 20, 2020

@elastic/kibana-core-ui I found another potentially problematic usage of bulkCreate in your code:

installSavedObjects = async () => {
this.setState({
isInstalling: true,
});
let resp;
try {
resp = await this.props.bulkCreate(this.props.savedObjects, {
overwrite: this.state.overwrite,
});
} catch (error) {

I'm not sure where this.props.savedObjects comes from; is it possible for any objects in the savedObjects array to inadvertently specify a version field? If so, we'll need to change this code to filter out the version. Let me know if that's the case and I can add it to this PR.


Edit: I dug into this one on my own -- the /api/kibana/home/tutorials/ route returns an array of tutorial objects, which may optionally include saved object data that needs to be created. One of these (APM) does include a saved object with an invalid version field, which causes a 400 error. This can be observed during the APM tutorial at http://localhost:5601/app/home#/tutorial/apm by clicking the "Load Kibana objects" button, which errors out. I'll need to push a separate PR to update this, waiting to hear back from the SIEM and ML teams on the other two questions before doing that.

@LeeDr
Copy link

LeeDr commented Aug 20, 2020

This appeared to work from the beats side;

$ ./metricbeat.exe setup --dashboards
Loading dashboards (Kibana must be running and reachable)
Loaded dashboards

But it doesn't look like everything worked.

  1. I see a metricbeat-* index pattern but there's no fields. The browser console shows;
    GET http://localhost:5620/api/index_patterns/_fields_for_wildcard?pattern=metricbeat-*&meta_fields=_source&meta_fields=_id&meta_fields=_type&meta_fields=_index&meta_fields=_score 404 (Not Found)

And the UI shows an error No matching indices found: No indices match pattern "metricbeat-*";
image

  1. The Saved Object UI shows 780 saved objects (some are built-in, but most are from this metricbeat setup). So it looks like it mostly worked.

  2. Once I started metricbeat and it loaded data, at least the [Metricbeat System] Host overview ECS dashboard did work OK.

@LeeDr
Copy link

LeeDr commented Aug 20, 2020

I manually created an index pattern for the .kibana alias and it shows all the fields for it. So it's not a general problem with the index patterns page.

@LeeDr
Copy link

LeeDr commented Aug 20, 2020

When I look at the metricbeat index pattern saved object it looks very strange with a parsedUrl pointing to my local Kibana instance;

   "index-pattern": {
      "fieldFormatMap": "{\"activemq.broker.memory.broker.pct\":
{\"id\":\"percent\",\"params\":{\"parsedUrl\":{\"origin\":\"http://localhost:5620\",\"pathname\":\"/app/management/kibana/indexPatterns/patterns/metricbeat-*\",\"basePath\":\"\"}}},\"activemq.broker.memory.store.pct\":
{\"id\":\"percent\",\"params\":{\"parsedUrl\":{\"origin\":\"http://localhost:5620\",\"pathname\":\"/app/management/kibana/indexPatterns/patterns/metricbeat-*\",\"basePath\":\"\"}}},\"activemq.broker.memory.temp.pct\":
{\"id\":\"percent\",\"params\":{\"parsedUrl\":{\"origin\":\"http://localhost:5620\",\"pathname\":\"/app/management/kibana/indexPatterns/patterns/metricbeat-*\",\"basePath\":\"\"}}},\"activemq.queue.memory.broker.pct\":
{\"id\":\"percent\",\"params\":{\"parsedUrl\":{\"origin\":\"http://localhost:5620\",\"pathname\":\"/app/management/kibana/indexPatterns/patterns/metricbeat-*\",\"basePath\":\"\"}}},\"activemq.topic.memory.broker.pct\":
{\"id\":\"percent\",\"params\":{\"parsedUrl

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

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

@LeeDr
Copy link

LeeDr commented Aug 21, 2020

@jportner I retested your PR but took a slightly different path this time. I started your PR branch from kibana, not from x-pack and I downloaded the OSS version of filebeat (not default dist of metricbeat) from this morning's unified master build; https://snapshots.elastic.co/8.0.0-edd1f0a6/downloads/beats/filebeat/filebeat-oss-8.0.0-SNAPSHOT-linux-x86.tar.gz

I ran setup instead of setup --dashboards and again it appeared to work;

leedr@leedr-G501JW:~/Downloads/filebeat-8.0.0-SNAPSHOT-linux-x86$ ./filebeat setup
Overwriting ILM policy is disabled. Set `setup.ilm.overwrite: true` for enabling.

Index setup finished.
Loading dashboards (Kibana must be running and reachable)
Loaded dashboards
Loaded Ingest pipelines

But just like my previous test with metricbeat, Kibana Index Patterns shows 0 fields.

And going to discover logged errors;

Error: Could not locate that index-pattern-field (id: @timestamp)
    at FieldParamType.deserialize (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:49932:15)
    at http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38266:26
    at Array.forEach (<anonymous>)
    at AggConfig.setParams (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38242:25)
    at AggConfig.set type [as type] (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38559:10)
    at AggConfig.setType (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38566:15)
    at new AggConfig (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38219:10)
    at AggConfigs.createAggConfig (http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38648:21)
    at http://localhost:5620/9007199254740991/bundles/plugin/data/data.plugin.js:38664:41
    at Array.forEach (<anonymous>)

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - Regarding my last test with OSS Kibana and OSS filebeat. After I start filebeat and have docs in the index then the Index Page does show the fields and Discover works fine.

It might be a slight regression that the Index Patterns page now doesn't work properly until after there are docs in the index? I'll have to try to reproduce that on other builds to check. I'll file a new issue if that's the case.

@jportner jportner merged commit 68dc4e8 into elastic:master Aug 21, 2020
@jportner jportner deleted the issue-75596-fix-beats-dashboard-import branch August 21, 2020 18:30
@LeeDr
Copy link

LeeDr commented Aug 24, 2020

All my tests passed on master. I think this is good to backport.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 24, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 24, 2020
mikecote added a commit that referenced this pull request Aug 31, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Aug 31, 2020
mikecote added a commit that referenced this pull request Aug 31, 2020
…writing a document (#76280)

* Revert "Fix more broken usages of `bulkCreate` (#76005) (#76131)"

This reverts commit 44a017b.

* Revert "Filter saved object `version` during legacy import (#75597) (#75793)"

This reverts commit 6e82885.

* Revert "[7.9] [Saved objects] Add support for version on create & bulkCreate when overwriting a document (#75172) (#75498)"

This reverts commit 00836e5.
@mikecote mikecote removed the v7.9.1 label Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing Beats dashboards results in an error
5 participants