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

Basic server side import API for saved objects #32158

Merged
merged 48 commits into from
Mar 8, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Feb 27, 2019

PR implements the first steps of the saved objects import API for #4759.

Two basic endpoint are added under POST /api/saved_objects/_import and POST /api/saved_objects/_resolve_import_conflicts that throws an error after 10,000 objects. The removal of the limit will come in a future PR when using streams to optimize scalability and performance.

Also a future PR will change the management UI.

Dev-Docs

Added server side API for importing saved objects

New endpoints /api/saved_objects/_import and /api/saved_objects/_resolve_import_errors now exists to import saved objects.

@mikecote mikecote added release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 v7.2.0 labels Feb 27, 2019
@mikecote mikecote self-assigned this Feb 27, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine

This comment has been minimized.

@mikecote mikecote requested a review from a team March 6, 2019 21:20
@mikecote mikecote added the review label Mar 6, 2019
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.

Left the security api suites for @kobelb to review, but looking great so far!

objectMode: true,
async transform(obj, enc, done) {
if (counter >= limit) {
return done(new Error(`Limit of ${limit} objects reached`));
Copy link
Contributor

@spalger spalger Mar 6, 2019

Choose a reason for hiding this comment

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

We should probably verify that the import API behaves appropriately when more than 10000 objects are imported, since we're using createConcatStream() we don't have to worry about partial imports happening, but I'm pretty sure it will respond with a 500 and no meaningful error in this case, when ideally we should be responding with a 4xx response that instructs the user to send less objects. Maybe we want to be able to customize the behavior of createLimitStream() when the limit is encountered? Or maybe we should just define this Transform stream inline so we have full control of the behavior in import.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spalger Interesting, I did the test and yeah it does throw a 500. Because of this, I moved createLimitStream inline to throw a 400 error. I've added functional tests for this as well.

As I changed this, I discovered the API doesn't accept files larger than 1MB. I had to add a config for what size we want to allow on import. I set the default to 50MB? The 10k object es archive is about 20MB when exported from the API.

Let me know your thoughts about these changes.

Copy link
Contributor

@spalger spalger Mar 7, 2019

Choose a reason for hiding this comment

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

Sounds good to me, but I wonder what Kibana performance is like when a 20 MB import is loaded into memory... maybe we should consider lowering the default limit to 5000 or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed and after doing some research, the import for 10k records takes about 3s and blocks the event loop up to ~800ms due to garbage collection. The fix will come when we use streams to not have all the objects in memory. We will leave the limit as is (limit is also configurable by users).

@mikecote mikecote requested a review from spalger March 7, 2019 16:04
@elasticmachine

This comment has been minimized.

@mikecote mikecote requested a review from spalger March 7, 2019 20:51
@elasticmachine

This comment has been minimized.

const { user = {}, spaceId = DEFAULT_SPACE_ID, tests } = definition;

describeFn(description, () => {
before(() => esArchiver.load('saved_objects/spaces'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test case for when we try to import a file which we're only authorized to import some of the saved object types? We should be able to accomplish this by fabricating a saved object type, similar to what we do with the find APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added tests to cover this scenario, we can iterate on the approach when you get a chance.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

@mikecote mikecote merged commit 7cf9131 into elastic:master Mar 8, 2019
mikecote added a commit to mikecote/kibana that referenced this pull request Mar 8, 2019
* Initial work

* Add overwrite and skip support

* Cleanup and add tests

* Move code into separate files

* Remove reduce

* New API parameters

* Add support to replace references

* Add better error handling

* Add spaces tests

* Fix return type in collectSavedObjects

* Apply PR feedback

* Update jest tests due to jest version upgrade

* Add docs

* WIP

* Split import routes pt1

* Add tests

* Fix broken tests

* Update docs and fix broken test

* Add successCount to _import endpoint

* Make skip by default in resolution API

* Update tests for removal of skips

* Add back support for skips

* Add success count

* Add back resolve import conflicts x-pack tests

* Remove writev from filter stream

* Delete _mock_server.d.ts file

* Rename lib/import_saved_objects to lib/import

* Filter records at stream level for conflict resolution

* Update docs

* Add tests to validate documentation

* Return 200 instead of other code for errors, include errors array

* Change [] to {}

* Apply PR feedback

* Fix import object limit to not return 500

* Change some wording in the docs

* Fix status code

* Apply PR feedback pt2

* Lower maxImportPayloadBytes to 10MB

* Add unknown type tests for import

* Add unknown type tests for resolve_import_conflicts

* Fix tslint issues
mikecote added a commit that referenced this pull request Mar 8, 2019
* Initial work

* Add overwrite and skip support

* Cleanup and add tests

* Move code into separate files

* Remove reduce

* New API parameters

* Add support to replace references

* Add better error handling

* Add spaces tests

* Fix return type in collectSavedObjects

* Apply PR feedback

* Update jest tests due to jest version upgrade

* Add docs

* WIP

* Split import routes pt1

* Add tests

* Fix broken tests

* Update docs and fix broken test

* Add successCount to _import endpoint

* Make skip by default in resolution API

* Update tests for removal of skips

* Add back support for skips

* Add success count

* Add back resolve import conflicts x-pack tests

* Remove writev from filter stream

* Delete _mock_server.d.ts file

* Rename lib/import_saved_objects to lib/import

* Filter records at stream level for conflict resolution

* Update docs

* Add tests to validate documentation

* Return 200 instead of other code for errors, include errors array

* Change [] to {}

* Apply PR feedback

* Fix import object limit to not return 500

* Change some wording in the docs

* Fix status code

* Apply PR feedback pt2

* Lower maxImportPayloadBytes to 10MB

* Add unknown type tests for import

* Add unknown type tests for resolve_import_conflicts

* Fix tslint issues
@spalger
Copy link
Contributor

spalger commented Mar 8, 2019

💃🎉💃🎉💃🎉💃🎉💃🎉💃🎉💃

@mikecote mikecote added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Mar 27, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants