Initial server-side support for sharing saved-objects phase 1.5 by jportner · Pull Request #66089 · elastic/kibana

jportner · 2020-05-11T18:19:27Z

Server-side component to #58139.

Introduces support for importing, exporting, and copying multi-namespace saved objects on the Kibana server.

⚠️ This is targeting a feature branch.

Code owners except for kibana-security and kibana-platform may be interested in waiting until a final PR is opened against master.

Commit Descriptions

I tried to break this PR down the smallest possible portions with individual commits. Each commit message is quite descriptive. I recommend any reviewers to check this PR commit-by-commit.

Notable Changes

Added optional originId attribute for saved objects, which is used during import
Implemented two pre-flight search requests when importing multi-namespace saved objects to check for various conflict scenarios within the namespace ("exact match conflict", "inexact match conflict", and "ambiguous conflict")
Modified import conflict resolutions:
- Added ability to specify ~~idToOverwrite~~destinationId, which is used to resolve inexact match conflicts and ambiguous conflicts
- ~~Added ability to Duplicate the object; this is mutually-exclusive with Overwrite, and instead it will create a new object with a random ID, unsetting any originId in the process~~
- Now, no import objects are created until all resolvable errors are addressed. Resolvable errors are conflict, ambiguous_conflict, and missing_references. Unresolvable errors are unsupported_type and unknown; these will not block object creation. E.g., if you import an NDJSON file with 10 objects that results in 1 unsupported_type error, that object will be skipped but the other 9 will be created.

Miscellaneous Notes

The majority of the code changes/additions are in the src/core/server/saved_objects/import directory. Wherever it made sense, I abstracted logic out into separate sub-modules. I also wound up rewriting the unit tests in several files, partly because the existing tests did not separate concerns from sub-modules properly. I used module mocks liberally to reduce the testing concerns of each individual sub-module.

Concerning integration tests: the existing X-Pack functional/integration tests largely focused on the combination of Spaces and Security plugins, but basically ignored testing import objects with references. The Core Jest integration tests, however, included a few cases to test different reference scenarios (including missing references and resolution). I followed this pattern, and did not attempt to write any new reference-focused X-Pack integration test cases.

jportner

Edit: ignore this comment.

SavedObject.error type did not specify the error attribute and did not reflect what is actually returned by SavedObjectsRepository. Updated this for consistency, and updated relevant saved object unit tests to utilize the SavedObjectsErrorHelper functions instead of manually mocking error objects.

`importSavedObjectsFromStream` and `resolveSavedObjectImportErrors` previously expected an array of supported saved object types. Updated these functions to use the SavedObjectTypeRegistry directly to obtain the list of importable/exportable types. This simplifies logic for other consumers (e.g., Spaces), and it lays the groundwork for regenerating IDs of multi-namespace saved objects upon import.

This is only intended to be set during migration or create operations. It is in place to support future changes for migrating and importing multi-namespace saved objects, so IDs can be regenerated deterministically. This will allow saved objects to be overwritten consistently if they are imported multiple times into a given space.

Single-namespace saved objects do not include the `namespace` field after deserialization, hence it was never exported. Multi-namespace saved objects do include the `namespaces` field, but this should be omitted when the object is exported from Kibana.

bulkCreate now attaches error metadata to indicate when an unresolvable conflict occurred.

When attempting to search with a simple_query_string term, the `searchFields` option only allows for selecting type-specific attributes (e.g., the fields in the `attributes` object). Every element in `searchFields` is prefixed by the object type before being added to the simple_query_string term. This commit adds a `rawSearchFields` option which is not prefixed by the object type -- this way, searches can use fields that are not specific to the object type (such as `_id` or `originId`).

Kibana does not officially support importing multiple objects with the same type/ID simultaneously. While this will not fail, it will result in a confusing outcome, because these objects would conflict with each other (and overwrite each other if that flag is used). This commit simply adds validation to throw a descriptive error if this situation is encountered. It should never happen in normal usage of Kibana, only if someone is tampering with an exported NDJSON file or otherwise misusing the API.

Imports for single-namespace saved objects ensured that, at most, one "copy" of an object was made in any given space. Re-importing an object into a space would result in a conflict that could be overwritten. This commit ensures that multi-namespace saved objects can be imported, and that they behave in the same way. Primary changes: 1. Pre-flight searches to ensure that conflicts are made with the correct ID, and that ambiguous conflicts are detected. 2. Abstract out behavior of bulkCreate to handle the case of unresolvable conflicts, which shouldn't be exposed to consumers. 3. Support resolving ambiguous_conflict import errors by allowing consumers to select which conflicting object should be overwritten.

Saved object import that resulted in a Conflict error previously had one resolution, which is "overwrite". This commit adds another resolution, "duplicate", which will create a new object with a different ID instead.

These test suites do not currently include test cases for multi- namespace saved object types. However, the test suites broke due to the changes to the import APIs. I fixed the few problems that were present, and took this opportunity to refactor some of the test suites to make them more readable and maintainable.

Also fixed existing Jest integration tests, but did not add new test cases for those since they are currently disabled in CI.

The implementation already included this field in some cases when using this as a return type, for some reason the type checker did not prohibit that though.

elasticmachine · 2020-05-26T16:25:53Z

Pinging @elastic/ingest-management (Team:Ingest Management)

jportner

Notes for reviewers.

jportner · 2020-05-26T16:32:10Z

src/core/public/saved_objects/saved_objects_client.ts

-type SavedObjectsFindOptions = Omit<SavedObjectFindOptionsServer, 'namespace' | 'sortOrder'>;
+type SavedObjectsFindOptions = Omit<
+  SavedObjectFindOptionsServer,
+  'namespace' | 'sortOrder' | 'rawSearchFields'


I omitted rawSearchFields here because we only need to be able to use it on the server side.

Despite this, it's still showing up in the public API docs, I suspect due to some quirk of the docs generation process.

Oversight on the last commit...

pgayvallet

I think this one was even bigger than the phase 1 review.

A few nits and comments, but did not found anything major. My biggest concern is that every phase of this SO sharing issue makes the (already way too complex) SO code yet more complex and hard to follow, but I don't really see how that can be avoided to be honest if we do want to implement this thing.

I will let @rudolf have a second opinion, but this looks good to me.

src/core/server/saved_objects/import/regenerate_ids.ts

src/core/server/saved_objects/import/regenerate_ids.test.ts

src/core/server/saved_objects/import/regenerate_ids.ts

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts

x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.ts

src/core/server/saved_objects/import/check_conflicts.ts

src/core/server/saved_objects/import/create_saved_objects.ts

src/core/server/saved_objects/service/lib/repository.ts

Updated docs to reflect latest changes.

Now uses consistent 'params' objects.

rudolf

First-pass, mostly left some documentation nits.

rudolf · 2020-07-16T07:30:01Z

docs/api/saved-objects/import.asciidoc

    Indicates a successful call.

+[[saved-objects-api-import-example]]
 ==== Examples


Thanks for raising this, I do think this is something we should address at some point. I personally find it very frustrating when documentation examples don't work.

docs/api/saved-objects/import.asciidoc

docs/api/spaces-management/copy_saved_objects.asciidoc

src/core/public/public.api.md

rudolf · 2020-07-16T09:41:52Z

src/core/server/saved_objects/service/lib/search_dsl/query_params.ts

+function getFieldsForTypes(
+  types: string[],
+  searchFields: string[] = [],
+  rootSearchFields: string[] = []


In other places the repository uses a pattern of type.attributes.field vs type.field to distinguish between root fields/type fields. It would be more consistent if we could do it here too (even though it's a breaking change).

This inconsistency wasn't introduced by this PR, but it highlights the current shortcomings. So just leaving this as a note to the Platform team that this is something we should take a look at at some point.

jportner · 2020-07-16T14:07:19Z

First-pass, mostly left some documentation nits.

Thanks, I believe I addressed them all in 9d7b755.

jportner · 2020-07-21T14:12:30Z

@rudolf did you have any other concerns for now? Or would you like to take another pass on the final PR?

kibanamachine · 2020-08-17T16:03:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7e9736c

Build metrics

async chunks size

id	value	diff	baseline
enterpriseSearch	359.0KB	+17.0B	359.0KB

page load bundle size

id	value	diff	baseline
core	1.2MB	+2.4KB	1.2MB
savedObjectsManagement	216.5KB	+109.0B	216.4KB
upgradeAssistant	64.6KB	+17.0B	64.6KB
total		+2.5KB

Saved Objects .kibana field count

id	value	diff	baseline
originId	1	+1	-

History

💚 Build #67223 succeeded 3133deb
💚 Build #65769 succeeded 077d52b
💔 Build #65647 failed 8cbdc7c
💛 Build #63956 was flaky 842b0df
💛 Build #62258 was flaky 9d7b755

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

pgayvallet

All points from previous reviews have been addressed, did not see anything else in the second pass. Congratulations, you are a real SO ninja now!

LGTM

(Naively) Hope the phase 2 will be smaller though 😅

jportner force-pushed the security/sharing-saved-objects-1.5-server branch 6 times, most recently from 24b44ed to 4a65de5 Compare May 12, 2020 21:01

jportner commented May 12, 2020

View reviewed changes

legrego self-requested a review May 13, 2020 11:04

botelastic bot added the Feature:Embedding Embedding content via iFrame label May 13, 2020

jportner force-pushed the security/sharing-saved-objects-1.5-server branch from de5a3cb to 44fc7c9 Compare May 13, 2020 15:52

jportner removed the Feature:Embedding Embedding content via iFrame label May 13, 2020

jportner added 14 commits May 26, 2020 11:19

Modify bulkCreate to differentiate between conflict types

c1735a6

bulkCreate now attaches error metadata to indicate when an unresolvable conflict occurred.

Server-side support for "duplicate" resolution of import conflicts

924ef96

Saved object import that resulted in a Conflict error previously had one resolution, which is "overwrite". This commit adds another resolution, "duplicate", which will create a new object with a different ID instead.

Add and update API integration tests for Saved Objects and Spaces

a49f114

Also fixed existing Jest integration tests, but did not add new test cases for those since they are currently disabled in CI.

API docs changes

8ab0a11

Add originId field to default saved object mappings

2d84ae2

Add destinationId? to SavedObjectsImportConflictError type

7954483

The implementation already included this field in some cases when using this as a return type, for some reason the type checker did not prohibit that though.

jportner force-pushed the security/sharing-saved-objects-1.5-server branch from dfa3b77 to 7954483 Compare May 26, 2020 16:25

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label May 26, 2020

jportner removed the Team:Fleet Team label for Observability Data Collection Fleet team label May 26, 2020

jportner commented May 26, 2020

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 13, 2020

jportner added 5 commits July 13, 2020 18:31

Fix jest test that failed due to recent merge

ba47780

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

5245b66

Fix functional test failures after merge

6ebf12a

Fix type check

48feb3e

Oversight on the last commit...

Fix unit test... (╯°□°）╯︵ ┻━┻

8ec0e37

elastic deleted a comment from kibanamachine Jul 14, 2020

pgayvallet reviewed Jul 15, 2020

View reviewed changes

jportner added 6 commits July 15, 2020 13:07

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

6136bb0

Updated docs to reflect latest changes.

Rename rawSearchFields to rootSearchFields

3303559

Address most nits from fourth review

90bb5a9

Remove deprecations

ced0415

Refactor parameters of import modules

3570482

Now uses consistent 'params' objects.

@ts-ignore -> @ts-expect-error

6a67e37

rudolf reviewed Jul 16, 2020

View reviewed changes

jportner added 2 commits July 16, 2020 09:06

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

ee8a6aa

Address fifth round of feedback

9d7b755

jportner removed the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 21, 2020

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

842b0df

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 23, 2020

jportner added 4 commits July 30, 2020 15:57

Merge branch 'master' into pr/jportner/66089

8cbdc7c

Fix type check

077d52b

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

3133deb

Merge branch 'security/sharing-saved-objects-1.5' into pr/jportner/66089

7e9736c

pgayvallet approved these changes Aug 18, 2020

View reviewed changes

jportner merged commit d17201f into elastic:security/sharing-saved-objects-1.5 Aug 19, 2020

jportner deleted the security/sharing-saved-objects-1.5-server branch August 19, 2020 14:52

Conversation

jportner commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ This is targeting a feature branch.

Commit Descriptions

Notable Changes

Miscellaneous Notes

Uh oh!

jportner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 26, 2020

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

jportner May 26, 2020

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

rudolf Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rudolf Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

jportner commented Jul 16, 2020

Uh oh!

jportner commented Jul 21, 2020

Uh oh!

kibanamachine commented Aug 17, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

Saved Objects .kibana field count

History

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jportner commented May 11, 2020 •

edited

Loading

jportner left a comment •

edited

Loading