Fix import bugs #121046

jportner · 2021-12-11T06:49:19Z

Resolves #121038 and #122149 and #120312

I combined these three bugfixes into the same PR because they are all touching the same part of the codebase, and I thought it might be harder on everyone to manage three separate PRs.

⚠️ This is best reviewed on a per-commit basis! ⚠️

The first three commits are simple refactoring:

ce3b8ed - Refactor unit tests
Now our mocks are more well-defined and easier to work with. Also this removes one of the annoying jest warnings regarding multiple "mocks/index.ts" files.
76f17de - Refactor getImportIdMapForRetries
This function should have been in its own module and it was aggravating me, so I went ahead and moved it.
78f2428 - Refactor types
This renames importIdMap to importStateMap, since I'm changing it to do more than just track what IDs are getting changed (more on that below). It also adds TSdocs so this part of the codebase is less obtuse!

The last three commits are the actual bugfixes. I won't go into too much detail here, the linked issues are quite thorough:

bb26d6c - Fix import exact match bug (issue Import saved object does not prefer exact match when using overwrite=true #121038)
This is pretty simple; if we found a regular conflict for an object (exact match), we need to avoid subsequently searching for origin conflicts (inexact match conflicts).
9184efb - Fix copy inexact match bug (issue Copying saved objects misses some "inexact match" origin conflicts #122149)
This is also pretty simple; we just needed to remove the namespace: prefix from the query in checkOriginConflicts.
96b1583 - Fix broken references for share-capable objects (issue Importing objects can result in broken references for share-capable objects #120312)
This one is a bit more involved... 😅 I had to add a completely new checkReferenceOrigins module (that is based on the existing checkOriginConflicts module), and I had to change what we are tracking in the importStateMap. Before, this map had an entry for each object that is being imported. Now, this map has an entry for each object that is being imported and each outbound reference that is not included in the import file. We rely on the existing ID-remapping functionality in the createSavedObjects module to facilitate changing reference IDs based on a shared origin.

All three of these include updates to unit tests and integration tests as well.

Change some mocks for better readability.

Split this function into its own module.

Renamed `importIdMap` to `importStateMap`, changed `id` value to `destinationId`, and added TSdocs.

jportner

Author's notes for reviewers.

jportner · 2022-01-06T16:53:00Z

src/core/server/saved_objects/import/lib/check_reference_origins.ts

+    const [object] = savedObjects;
+    if (id !== object.id) {
+      return {
+        key: getObjectKey({ type, id }),


I'm using getObjectKey here instead of a string template ${type}:${id}, and I'm using parseObjectKey further below.

I'm doing this here because of feedback during recent changes that I made to the SOR, where we had a lot of these string templates and we decided we'd prefer to use a common function instead.

There are tons of other ${type}:${id} string templates in the import module, but I opted not to change all of them because this PR already turned out to be bigger than expected 😅 we'll leave that tech debt for another day!

jportner · 2022-01-06T16:53:59Z

src/core/server/saved_objects/import/lib/types.ts

+export interface ImportStateValue {
+  /**
+   * This attribute indicates that the object for this entry is *only* a reference, it does not exist in the import file.
+   */
+  isOnlyReference?: boolean;


This isOnlyReference attribute is new. I don't love the name, but I couldn't think of anything better. I'm totally open to suggestions here.

I don't really have anything that much better. isOutboundReference? referencedObjects?

🤷 yeah I think the only thing that would make it clearer would be to have a separate state map just for references, but that would add a lot of boilerplate and complexity so I wanted to avoid that.

jportner · 2022-01-06T17:40:11Z

x-pack/test/saved_object_api_integration/common/suites/import.ts

@@ -8,23 +8,34 @@
 import expect from '@kbn/expect';
 import { SuperTest } from 'supertest';
 import type { Client } from '@elastic/elasticsearch';
+import type { SavedObjectReference } from 'src/core/server';
 import { SAVED_OBJECT_TEST_CASES as CASES } from '../lib/saved_object_test_cases';
 import { SPACES } from '../lib/spaces';
 import { expectResponses, getUrlPrefix, getTestTitle } from '../lib/saved_object_test_utils';
 import { ExpectResponseBody, TestCase, TestDefinition, TestSuite } from '../lib/types';

 export interface ImportTestDefinition extends TestDefinition {


The import and resolveImportErrors integration tests were designed to import a mixture of objects that don't exist yet, with objects that already exist. This would generate conflict errors, and the destination objects would need to be overwritten. Each suite exercises three different scenarios:

createNewCopies: false, overwrite: false

createNewCopies: false, overwrite: true

createNewCopies: true, overwrite: false`

The problem is there is another type of error that can occur, a "missing references" error, that originates from the validateReferences module. We never had tests for this before, and I thought it would be prudent to add tests for this now, since we need to make assertions that Kibana is correctly matching origins for references anyway.

Unfortunately, these test suites needed some significant refactoring to make this happen. There are a lot of ways to skin this cat, but I tried to make the most minimal changes possible. I think there is a lot of room for improvement, but I didn't want this PR to be any bigger than it already is.

jportner · 2022-01-06T17:40:57Z

x-pack/test/saved_object_api_integration/common/suites/import.ts

+export const SPECIAL_TEST_CASES: Record<string, ImportTestCase> = Object.freeze({
+  HIDDEN,


We already treated the "HIDDEN" test case as a special case before, I'm just making that clearer now.

pgayvallet

Last commit was sure fun to review, but I think it's LGTM overall.

We really need to cleanup and refactor our FTR tests related to import/export though, it's becoming VERY hard to review this part of the code.

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

src/core/server/saved_objects/import/lib/validate_references.ts

thomheymann

Verified #121038, #122149, and #120312 locally and working as defined.

As discussed though I worry about the complexity of the import and copy functionality.

There are a lot of edge cases which did not behave the way I would have expect in order to preserve the (accidental) legacy behaviour:

There are situations now where depending on the type of saved objects or the number of references those saved objects have, the "overwrite existing saved objects" setting gets ignored for some of the imported objects but not for others
Similarly the "resolve conflicts manually" setting gets ignored where users are asked to resolve conflicts themselves for some saved objects even though they selected to automatically resolve conflicts. If that's the case we probably shouldn't provide this option at all.
There are also situations where conflicts are ignored when index patterns are missing causing duplicate saved objects despite "overwrite existing saved objects" being selected.

I appreciate that we want to have as few breaking changes as possible but I find all of the above quite problematic from a user and maintenance perspective since it's almost impossible to predict what is actually going to happen on import.

The sheer number of scenarios and user flows also means we end up with a lot of code complexity and ultimately these types of bugs and edge cases.

I think it would be good to try and simplify this functionality in the future.

rudolf · 2022-01-12T12:13:22Z

@thomheymann I completely agree. We had some discussions in September for how we could solve this and we were mostly aligned on the approach but unfortunately we haven't made any further progress on this see #91615 (comment) I think it would be useful for you to add your comment to that issue.

jportner · 2022-01-12T17:52:26Z

@elasticmachine merge upstream

jportner · 2022-01-12T17:53:45Z

Thanks all, I also agree this is way too complex and there are too many edge cases. More motivation for us to actually solve this and do away with this import API once and for all.

kibana-ci · 2022-01-12T19:12:35Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #16506 succeeded c81a43a
💔 Build #16497 failed 662b306
💚 Build #16180 succeeded 96b1583
💚 Build #15999 succeeded 9e2ca5a15b326e60bd601a54a561e45efa1e6344
💚 Build #15461 succeeded ed9c4e94addbeb0129ce2dd59633f444de1d3554
💔 Build #15459 failed 68885a11ebe95e09cfae013719a373dbda286a94

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

(cherry picked from commit 116d74a)

kibanamachine · 2022-01-12T19:15:33Z

💚 All backports created successfully

Status	Branch	Result
✅	8.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit 116d74a) Co-authored-by: Joe Portner <[email protected]>

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Dec 11, 2021

jportner force-pushed the issue-120312-fix-import-broken-references branch from ce1137f to a152353 Compare December 13, 2021 19:21

jportner linked an issue Dec 14, 2021 that may be closed by this pull request

Importing objects can result in broken references for share-capable objects #120312

Closed

jportner linked an issue Dec 30, 2021 that may be closed by this pull request

Copying saved objects misses some "inexact match" origin conflicts #122149

Closed

jportner force-pushed the issue-120312-fix-import-broken-references branch from a152353 to f69e2d4 Compare December 30, 2021 16:04

jportner mentioned this pull request Dec 30, 2021

[Security Solution] [Platform] Migrate pre-8.0 action connector ids in rule params on import to post-8.0 _id #120975

Merged

2 tasks

jportner force-pushed the issue-120312-fix-import-broken-references branch 3 times, most recently from ed9c4e9 to 9e2ca5a Compare January 5, 2022 20:02

Joe Portner added 5 commits January 6, 2022 11:32

Refactor unit tests

ce3b8ed

Change some mocks for better readability.

Refactor getImportIdMapForRetries

76f17de

Split this function into its own module.

Refactor types

78f2428

Renamed `importIdMap` to `importStateMap`, changed `id` value to `destinationId`, and added TSdocs.

Fix import exact match bug (issue 121038)

bb26d6c

Fix copy inexact match bug (issue 122149)

9184efb

jportner force-pushed the issue-120312-fix-import-broken-references branch from 9e2ca5a to ed3cd39 Compare January 6, 2022 16:34

Fix broken references for share-capable objects (issue 120312)

96b1583

jportner force-pushed the issue-120312-fix-import-broken-references branch from ed3cd39 to 96b1583 Compare January 6, 2022 17:27

jportner commented Jan 6, 2022

View reviewed changes

jportner marked this pull request as ready for review January 6, 2022 17:41

jportner requested review from a team as code owners January 6, 2022 17:41

pgayvallet approved these changes Jan 9, 2022

View reviewed changes

Joe Portner added 3 commits January 10, 2022 12:17

Merge branch 'main' into pr/jportner/121046

ccb52b2

PR review feedback

662b306

Remove unneeded export

c81a43a

thomheymann approved these changes Jan 12, 2022

View reviewed changes

rudolf approved these changes Jan 12, 2022

View reviewed changes

thomheymann mentioned this pull request Jan 12, 2022

Saved objects import/export should include spaces #91615

Open

Merge branch 'main' into issue-120312-fix-import-broken-references

4c97ce8

jportner enabled auto-merge (squash) January 12, 2022 17:54

jportner merged commit 116d74a into elastic:main Jan 12, 2022

kibanamachine mentioned this pull request Jan 12, 2022

[8.0] Fix import bugs (#121046) #122877

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 12, 2022

Fix import bugs (elastic#121046)

f07d037

(cherry picked from commit 116d74a)

kibanamachine added a commit that referenced this pull request Jan 12, 2022

Fix import bugs (#121046) (#122877)

4a711e2

(cherry picked from commit 116d74a) Co-authored-by: Joe Portner <[email protected]>

jportner deleted the issue-120312-fix-import-broken-references branch January 19, 2022 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix import bugs #121046

Fix import bugs #121046

jportner commented Dec 11, 2021 •

edited by thomheymann

Loading

jportner left a comment

jportner Jan 6, 2022

jportner Jan 6, 2022

pgayvallet Jan 9, 2022

jportner Jan 10, 2022

jportner Jan 6, 2022

jportner Jan 6, 2022

pgayvallet left a comment

thomheymann left a comment •

edited

Loading

rudolf commented Jan 12, 2022 •

edited

Loading

jportner commented Jan 12, 2022

jportner commented Jan 12, 2022

kibana-ci commented Jan 12, 2022

kibanamachine commented Jan 12, 2022

		export const SPECIAL_TEST_CASES: Record<string, ImportTestCase> = Object.freeze({
		HIDDEN,

Fix import bugs #121046

Fix import bugs #121046

Conversation

jportner commented Dec 11, 2021 • edited by thomheymann Loading

jportner left a comment

Choose a reason for hiding this comment

jportner Jan 6, 2022

Choose a reason for hiding this comment

jportner Jan 6, 2022

Choose a reason for hiding this comment

pgayvallet Jan 9, 2022

Choose a reason for hiding this comment

jportner Jan 10, 2022

Choose a reason for hiding this comment

jportner Jan 6, 2022

Choose a reason for hiding this comment

jportner Jan 6, 2022

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

thomheymann left a comment • edited Loading

Choose a reason for hiding this comment

rudolf commented Jan 12, 2022 • edited Loading

jportner commented Jan 12, 2022

jportner commented Jan 12, 2022

kibana-ci commented Jan 12, 2022

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Jan 12, 2022

💚 All backports created successfully

Questions ?

jportner commented Dec 11, 2021 •

edited by thomheymann

Loading

thomheymann left a comment •

edited

Loading

rudolf commented Jan 12, 2022 •

edited

Loading