SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server #82406

pgayvallet · 2020-11-03T09:29:05Z

Summary

Change the default value of savedObjects.maxImportPayloadBytes to 25mb (26214400) (Ability to import saved object files larger than 10MB #36126)
Fix a bug causing the server to crash when importing an invalid file (SavedObjects management: importing an invalid NDJSON files causes the server to crash. #82403)
Display the error message (as returned by the server) when the SO import fails (Saved Object Management: saved objects import errors should be displayed to the user #66387)

Note: this is only displaying the technical error. Ideally, we would have a more user-friendly error, but this is harder to achieve, as there are some informations that we don't have on the client-side (such as the maxBodyPayload). Also, some errors are directly returned from HAPI (413) so it would not be possible to alter their format. I still think this is acceptable, and way better than not displaying any message at all.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Release Note

Fix a bug causing Kibana to crash when importing a file with an invalid format from the saved object management section.

pgayvallet · 2020-11-03T14:37:34Z

src/core/server/saved_objects/routes/utils.ts

+export async function createSavedObjectsStreamFromNdJson(ndJsonStream: Readable) {
+  const savedObjects = await createPromiseFromStreams([
+    ndJsonStream,
+    createSplitStream('\n'),
+    createMapStream((str: string) => {
+      if (str && str.trim() !== '') {
+        return JSON.parse(str);
+      }
+    }),


Soooo, long story short:

Uncatched errors from streams (either unhandled promises or plain errors) are causing the process to crash due to unhandled errors (forget streams were THAT much of a pain since I've been using observables)

The implementation of createPromiseFromStreams that is used here

kibana/src/core/server/saved_objects/import/collect_saved_objects.ts

Lines 49 to 50 in cd86b81

const collectedObjects: Array<SavedObject<{ title?: string }>> = await createPromiseFromStreams([

readStream,

relies on stream.pipeline under the hood, that is supposed to intercept errors occurring during the stream/transform chain, however errors that occurred within a pipe of the argument observable are not.

The direct consequence is that any error thrown from the piping chain that is done in createSavedObjectsStreamFromNdJson was uncatched, therefor killing the server.

The correct solution would probably be to stop using streams and moving to observable instead, but these are significant changes (and collectSavedObjects is also used by the spaces plugin when copying to space), so the only solution I found for now was to collect the initial saved objects inside createSavedObjectsStreamFromNdJson and recreate a stream from that. That way, errors during the createSavedObjectsStreamFromNdJson stream chain are properly thrown via the added await createPromiseFromStreams. A FTR test has been added to assert that.

Note that this doesn't change the performances or memory usage: we were already collecting all emissions of the SO stream here before:

kibana/src/core/server/saved_objects/import/collect_saved_objects.ts

Line 49 in cd86b81

const collectedObjects: Array<SavedObject<{ title?: string }>> = await createPromiseFromStreams([

👍

I suspect if we're adding more saved object types to the export (to allow us to deprecate multi-tenancy) we might want to support real streaming imports/exports again and that will maybe allow us to remove the import payload size limit completely. But that's something to revisit once we know this is a requirement.

elasticmachine · 2020-11-03T14:38:28Z

Pinging @elastic/kibana-platform (Team:Platform)

rudolf

We finally have import error messages in the UI 🙌

rudolf · 2020-11-04T11:07:19Z

src/core/server/saved_objects/routes/utils.ts

+export async function createSavedObjectsStreamFromNdJson(ndJsonStream: Readable) {
+  const savedObjects = await createPromiseFromStreams([
+    ndJsonStream,
+    createSplitStream('\n'),
+    createMapStream((str: string) => {
+      if (str && str.trim() !== '') {
+        return JSON.parse(str);
+      }
+    }),


👍

I suspect if we're adding more saved object types to the export (to allow us to deprecate multi-tenancy) we might want to support real streaming imports/exports again and that will maybe allow us to remove the import payload size limit completely. But that's something to revisit once we know this is a requirement.

pgayvallet · 2020-11-04T11:55:25Z

I suspect if we're adding more saved object types to the export (to allow us to deprecate multi-tenancy) we might want to support real streaming imports/exports

For exports, probably.

For imports, I guess we would still need to collect all SO to perform a single bulkCreate operation, so we would still 'break' the stream at some point during the operation

…-import-error

mshustov · 2020-11-04T13:47:00Z

...ugins/saved_objects_management/public/management_section/objects_table/components/flyout.tsx

          }
        >
          <EuiFilePicker
+            accept=".ndjson, .json"


Thanks 😄

kibanamachine · 2020-11-04T14:42:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 26dd44a

Metrics [docs]

async chunks size

id	before	after	diff
`savedObjectsManagement`	187.3KB	187.6KB	+304.0B

History

💚 Build #85488 succeeded 7dda1b8
💔 Build #85481 failed d769ae4
💔 Build #85459 failed de34060

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

…lure and avoid invalid file to crash the server (elastic#82406) * display error cause in case of import failure * avoid server crash when importing invalid file * remove unused translations * fix unit tests * change savedObjects.maxImportPayloadBytes default to 25mb * fix types and logic

…lure and avoid invalid file to crash the server (elastic#82406) * display error cause in case of import failure * avoid server crash when importing invalid file * remove unused translations * fix unit tests * change savedObjects.maxImportPayloadBytes default to 25mb * fix types and logic # Conflicts: # test/functional/apps/management/_import_objects.ts

…lure and avoid invalid file to crash the server (#82406) (#82613) * display error cause in case of import failure * avoid server crash when importing invalid file * remove unused translations * fix unit tests * change savedObjects.maxImportPayloadBytes default to 25mb * fix types and logic # Conflicts: # test/functional/apps/management/_import_objects.ts

…lure and avoid invalid file to crash the server (#82406) (#82612) * display error cause in case of import failure * avoid server crash when importing invalid file * remove unused translations * fix unit tests * change savedObjects.maxImportPayloadBytes default to 25mb * fix types and logic

pgayvallet added 5 commits November 3, 2020 10:26

display error cause in case of import failure

de34060

avoid server crash when importing invalid file

cd0e3db

remove unused translations

e2e38a6

fix unit tests

7ff6126

change savedObjects.maxImportPayloadBytes default to 25mb

d769ae4

pgayvallet added release_note:fix Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.10.1 v7.11.0 labels Nov 3, 2020

pgayvallet changed the title ~~SavedObjects management: display explicit import error in case of failure~~ SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server Nov 3, 2020

fix types and logic

7dda1b8

pgayvallet commented Nov 3, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review November 3, 2020 14:38

pgayvallet requested a review from a team as a code owner November 3, 2020 14:38

rudolf approved these changes Nov 4, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-66387-display…

26dd44a

…-import-error

mshustov reviewed Nov 4, 2020

View reviewed changes

pgayvallet merged commit e2cbde3 into elastic:master Nov 4, 2020

pgayvallet mentioned this pull request Nov 4, 2020

[7.x] SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server (#82406) #82612

Merged

pgayvallet mentioned this pull request Nov 4, 2020

[7.10] SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server (#82406) #82613

Merged

lucabelluccini mentioned this pull request Dec 2, 2020

[DOC] Clarify compatibility of Saved Objects across versions #63090

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server #82406

SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server #82406

Uh oh!

pgayvallet commented Nov 3, 2020 •

edited

Loading

Uh oh!

pgayvallet Nov 3, 2020

Uh oh!

rudolf Nov 4, 2020

Uh oh!

elasticmachine commented Nov 3, 2020

Uh oh!

rudolf left a comment

Uh oh!

rudolf Nov 4, 2020

Uh oh!

pgayvallet commented Nov 4, 2020

Uh oh!

mshustov Nov 4, 2020

Uh oh!

kibanamachine commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	const collectedObjects: Array<SavedObject<{ title?: string }>> = await createPromiseFromStreams([
	readStream,

SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server #82406

SavedObjects management: display explicit import error in case of failure and avoid invalid file to crash the server #82406

Uh oh!

Conversation

pgayvallet commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Release Note

Uh oh!

pgayvallet Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

rudolf Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 3, 2020

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

rudolf Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Nov 4, 2020

Uh oh!

mshustov Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 4, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pgayvallet commented Nov 3, 2020 •

edited

Loading