Sharing saved objects, phase 2 #80945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jportner
force-pushed
the
issue-54837-ssobjects-phase-2
branch
3 times, most recently
from
a6c68ac
to
6a41023
Compare
jportner
force-pushed
the
issue-54837-ssobjects-phase-2
branch
2 times, most recently
from
2256c32
to
82fa05a
Compare
1 task
Refactor some code and unit tests to prepare for following commits. No functionality changes.
Test cases were missing for multi-namespace types. Added those tests. No functionality changes.
1. The migration loop no longer resets if the object does not have a [type] field. Objects never have this field after being deserialized from their raw document format, this functionality appears to be unnecessary and can cause unexpected behavior (though migrations still worked because the loop would get restarted). 2. Changed one unit test case that depended upon the aforementioned condition. This appeared to be an invalid test case which would not actually be encountered in a real world scenario, since it relied on an object containing a [type] field. 3. The migration loop now resets if an object's migrationVersion has explicitly been increased by a transform function. This is expected behavior and a unit test exercises it, but it was a side effect of the aforementioned condition.
These can only be applied using the `convertToMultiNamespaceTypeVersion` field when defining the object type. Doing so will cause the document migrator to apply a special transform function to convert the document from a single-namespace type to a multi-namespace type in the specified version. Note that this transform is applied before any consumer-defined migrations in the same version.
These are applied to all types using the `convertToMultiNamespaceTypeVersion` field when defining any object type. Any time the Kibana version is changed, all reference transforms for applicable versions are applied, and all documents' `referencesMigrationVersion` fields are updated to match the Kibana version.
jportner
force-pushed
the
issue-54837-ssobjects-phase-2
branch
from
82fa05a
to
358dd27
Compare
New integration tests (except for Jest integration tests) are intentionally excluded. Will be added in a separate commit after conversion transforms include aliases.
jportner
force-pushed
the
issue-54837-ssobjects-phase-2
branch
from
358dd27
to
aeb2ea6
Compare
jportner
force-pushed
the
issue-54837-ssobjects-phase-2
branch
from
aeb2ea6
to
dd9ecd6
Compare
jportner
added
release_note:skip
pgayvallet
reviewed
Dec 4, 2020
src/core/server/saved_objects/migrations/core/document_migrator.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/document_migrator.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrations/core/document_migrator.ts
Outdated
Show resolved
Hide resolved
Comment on lines
1686
to
1766
| return omit(savedObject, 'namespace') as SavedObject<T>; | ||
| return omit(savedObject, ['namespace', 'referencesMigrationVersion']) as SavedObject<T>; |
There was a problem hiding this comment.
Why are we omitting referencesMigrationVersion if we are keeping migrationVersion. (But TIL, I wasn't aware migrationVersion was exposed...)
There was a problem hiding this comment.
I just figured there's no reason to expose it to consumers (and doing so would mean I have to change a ton of integration tests).
There was a problem hiding this comment.
We expose the migrationVersion so that we know which migrations to run (if any) on incoming data. That way if an integration read a document from v7.9.0 and then at a later point writes it back to v7.10.0 we will migrate the v7.9.0 document before writing it into ES.
We want the same behaviour for referencesMigrationVersion, otherwise an integration which does an POST /api/saved_objects/<type>/<id>?overwrite=true will create a document which has invalid references.
There was a problem hiding this comment.
Core could inject it's own migrations into buildActiveMigrations https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrations/core/document_migrator.ts#L194-L222
So that if a dashboard defined a migration for v7.9.0, but core wants to migrate it in v7.11.0 (to perform the referencesMigration) then the latestVersion for the dashboard type becomes 7.11.0. The advantage is that the API only has a single migrationVersion field. Consumers really shouldn't have to know all the versions of all the kinds of migrations we applied. (And based on #70815 we'll move from migrationVersion: {dashboard: '7.11.0'} to migrationVersion: '7.11.0').
The disadvantage is it's less explicit where a migration comes from. If a developer inspects a dashboard they might be surprised that it was migrated in 7.11.0 because they never defined such a migration for their type.
I'm a little bit torn between the developer experience and keeping the API simpler and cleaner. It would be good to hear others thoughts on this?
If we do keep an additional field I would vote for it to be renamed to coreMigrationVersion so that it could be used anytime core wants to apply a migration like for instance to implement #70815.
There was a problem hiding this comment.
@rudolf I'm going to reply to your comments out of order 😄
If we do keep an additional field I would vote for it to be renamed to
coreMigrationVersionso that it could be used anytime core wants to apply a migration like for instance to implement #70815.
Agreed, that sounds better and I'll refer to it as that from now on.
We want the same behaviour for referencesMigrationVersion, otherwise an integration which does an
POST /api/saved_objects/<type>/<id>?overwrite=truewill create a document which has invalid references.
I respectfully disagree. I feel strongly that coreMigrationVersion should be an implementation detail that is not exposed to consumers. I specifically designed this so "convert" and "reference" transforms would not be applied when documents are created, but only during the startup migration process.
We don't enforce referential integrity when objects are created, and we don't know if the outbound reference is valid or not. We do enforce referential integrity (to some extent) when importing objects; in that case, potential ID collisions are detected, IDs are regenerated (in the case of createNewCopies mode or "unresolvable conflicts"), and references are updated if need be before any objects are created.
If we silently change reference IDs when objects are created, I think it'll be quite confusing to consumers. Not to mention that to avoid this behavior, consumers would have to specify a coreMigrationVersion equal to Kibana's current version when they create objects -- and that input would need to change every time Kibana is upgraded.
The disadvantage is it's less explicit where a migration comes from. If a developer inspects a dashboard they might be surprised that it was migrated in 7.11.0 because they never defined such a migration for their type.
I don't think we should attempt to expose coreMigrationVersion externally or try to tie "reference" transforms to the regular migrationVersion instead. These transforms are only applied because of other objects which may potentially be due to an external plugin that does not come bundled with Kibana.
I think it's safe and reasonable to say:
- When you upgrade Kibana, we guarantee your saved object references will stay intact when object types are converted
- After you upgrade Kibana, your old exports can still be imported, and we guarantee your saved object references will stay intact
- After you upgrade Kibana, if you're manually recreating/updating existing documents using the experimental saved objects APIs, you're on your own to make sure your object IDs are still correct and your references are intact.
There was a problem hiding this comment.
Renamed referencesMigrationVersion to coreMigrationVersion in abd7d2b!
Added validation to ensure that documents with a newer coreMigrationVersion will result in an error. Also renamed some existing tests to be more accurate.
Consumers can now see this field when retrieving existing objects, and they can set it when creating new objects.
mattkime
approved these changes
Jan 19, 2021
rudolf
approved these changes
Jan 20, 2021
Fixed merge conflicts from elastic#88720.
Fix merge conflicts from elastic#87996.
💛 Build succeeded, but was flaky
Test FailuresX-Pack Saved Object Tagging Functional Tests.x-pack/test/saved_object_tagging/functional/tests/maps_integration·ts.saved objects tagging - functional tests maps integration creating "before each" hook for "allows to select tags for a new map"Standard OutStack TraceMetrics [docs]Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
9 tasks
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
kibanamachine
added
the
backport missing
kibanamachine
removed
the
backport missing
jportner
added a commit
that referenced
this pull request
Jan 22, 2021
9 tasks
This was referenced Dec 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #54837
Blocked by: #86247
Several commits are just refactoring existing code with no functionality changes.
This PR allows existing single-namespace saved object types to be migrated (converted) into multi-namespace types. The details are outlined in the linked issue, but the primary changes are listed below.
Migrations
The Saved Objects Migration process previously only applied one type of transform function -- what I now refer to as a "migration transform". These are completely defined by consumers, and can alter objects in many ways, up to and including changing the object type itself.
This PR adds two additional transform functions:
These additional transforms are still applied in an idempotent manner: for any given type, reference transforms are applied first, then conversion transforms, and then migration transforms.
Consumers can apply these conversions by specifying the
convertToMultiNamespaceTypeVersionfield during object registration (see docs).Aliases
LegacyUrlAlias objects are created for any type that is converted while in a non-default namespace. This allows consumers to access the object ("alias target") using its old ID.
Consumers who expose object IDs to end-users externally (e.g., in URLs) should eschew
SavedObjectsClient.get()in favor of the newSavedObjectsClient.resolve()method once they convert these objects to multi-namespace types. The introduction of this method is not a breaking change in and of itself; however, if consumers implement it they will need to take the new response type into account for their UI flows, and that could be a breaking change for any consumers who rely on 3rd-party integrations with these URLs.UI changes
As discussed in the linked issue, there is potential for "alias conflict" situations. This primarily may occur by sharing an object which had previously been copied to the target space. We will add a check and additional screen to attempt to prevent this from happening in the Share to Space Flyout. However, it is not required for this PR, so I opted to split that into a separate follow-on PR.
Docs preview: https://kibana_80945.docs-preview.app.elstc.co/diff