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

Handle ID changes to Saved Objects installed by Fleet #108959

Closed
3 tasks done
joshdover opened this issue Aug 17, 2021 · 11 comments · Fixed by #119527
Closed
3 tasks done

Handle ID changes to Saved Objects installed by Fleet #108959

joshdover opened this issue Aug 17, 2021 · 11 comments · Fixed by #119527
Assignees
Labels
Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature required-for-8.0 This work is required to be done before 8.0 lands, bc it relates to a breaking change or similar. Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0

Comments

@joshdover
Copy link
Contributor

joshdover commented Aug 17, 2021

As part of the #27004, the IDs for many Saved Object types will change for objects installed by Fleet packages in spaces other than the "default" space.

These ID changes will break a few features in the Fleet UI:

  • Uninstalling an integration and it's assets will not delete the installed assets
  • Links from the "assets" tab for an integration will no longer work and be in a broken empty state:

image

This issue is intended to outline a short-term solution for this problem for the 8.0 release, while the long-term solution is to fully support Spaces within Fleet #99116.

Background

  • Fleet installs Kibana assets into the Space the user is currently using, however we don't currently store the space ID.
    • When Fleet tries to uninstall Kibana assets, it only searches the current space, so the assets won’t be deleted if attempting from the UI of another space from which the assets were installed.
    • The asset list is broken on the integration detail page if viewing from a different Space than the integration was installed in (similar to the image above)
    • This should be handled long-term by support Space selection UX during the installation process, see [Fleet] Package assets should not break when installed within a space #74353 for more information.
  • The Saved Object migration that will regenerate IDs in 8.x does not support space-agnostic types, like the epm-packages type
    • The reason for this is that the references feature does not currently support pointing to objects in other spaces or of different "namespace types”. In other words, the references feature doesn’t support a global object (eg. a epm-package) to a non-global object (eg. dashboard).
    • @jportner has identified that the Kibana Alerting team is facing a similar challenge and has opened an issue that proposes a workaround: [Discuss] Saved object references (global -> non-global) #107575. This workaround would require that Fleet adds a migration for the epm-packages type to handle migrating the IDs in its internal installed_kibana array.
    • The challenge with this workaround is that we need to know the space ID that these assets were installed into in order for us to be able to generate the new ID. As seen in above, we are missing this information.
  • IDs will be regenerated only for objects in spaces that are not in the “default” space

Proposed changes

Decision: We have decided to go with option (2) for the 8.0 release and have the option to pursue option (1) once the saved object types are shareable in a future 8.x release.

Option 1: Updating IDs on epm-packages (higher effort) - not possible until later in 8.x

Once a Saved Object type becomes shareable in 8.x, it's IDs will be re-written for all objects not in the default space. This will break the uninstall flow and asset links as noted at the top of the issue.

Given that we are not currently storing the space ID that an asset was installed into, in order to update the IDs on the epm-packages object, we're going to need to add some logic that runs on Kibana startup to find these objects and their new IDs:

  1. For each epm-packages object:
    1. For each installed_kibana asset in the package object:
      1. Use the Saved Object resolve API against each asset ID and each space ID to find all spaces where this object exists and has been copied to.
        • Q: Should we filter out any assets that have been modified from the original shipped in the package?
        • Q: Can we run this only for assets that are now shareable in the current version?
          • A: Yes, this should be possible. During the start lifecycle, the core.savedObjects.getTypeRegistry() API exposes the details we need for each type. Specifically the convertToMultiNamespaceTypeVersion field should be <= the current version.
      2. Delete all existing copies found above.
      3. Create a new version of the asset, sharing the asset to each space previously identified and using the same ID as before.
      4. Delete any alias saved objects that point this ID to another ID in each space.

This would result in these assets existing in all the same spaces they existed in before and would not break any URL bookmarks pointing to these assets. It would also allow us to have a single Saved Object that represents these assets and would allow for clean uninstalls in the future.

Option 2: Reducing duplicates on package upgrades and installs (lower effort)

Alternatively, if we need to cut scope, we could take a shorter path forward and move from using the bulkCreate Saved Object API to using the import API. We would want to do this because if any Fleet assets are upgraded or re-created for any reason in a non-default Space, the bulkCreate API will create duplicates, whereas the import API will overwrite the existing objects that were installed prior to 8.0.

We'd also need to use the resolve API during the uninstall process and for populating the "assets" tab in the integrations UI for locating the new object IDs for assets installed in a non-default space.

This option still has the drawbacks that exist today for all Fleet packages that are installed into a non-default Space:

  • Trying to uninstall the integration from another Space from which it was installed in results in the assets being left behind (not deleted).
  • Links from the "assets" tab for an integration don't work when viewing from another Space

This option has the additional drawback of leaving around some tech debt and broken ID references in the epm-packages objects for some time that we'll need to consider when adding any other features that reference these IDs or until #74353 is addressed.

Implementation Scope

  • Update Kibana asset install logic to use import instead of bulkCreate here:
    const createResults = await savedObjectsClient.bulkCreate(toBeSavedObjects, {
    overwrite: true,
    });
    • This requires we use the SavedObjectImporter class provided by src/core/server.
    • This class is currently built to accept a Readable ndjson stream of objects, so we'll need to convert our objects into this format.
    • We should use the options { createNewCopies: false, overwrite: true }
  • Update Kibana asset uninstall logic to use a bulkResolve pre-flight check in order to find the updated IDs before calling delete here:
    return savedObjectsClient.delete(type, id);
  • Update Integrations Details asset tab to use bulkResolve to find the updated IDs for links.

Option 3: Do nothing (zero effort)

If we do nothing, we'll have a similarly broken experience to what we have today. However, we'll be increasing the blast radius of the two issues presented above (uninstalls don't remove these objects and the asset links are broken), except it will be broken for all integrations installed in a non-default space whereas before they were only broken when viewing an integration from a different Space from where it was installed from.

As noted by @jportner below, it also creates the potential for alias conflicts for users who reinstall or upgrade a package in a non-default Space:

In addition to these problems, reinstalling assets in Option 3 will create "alias conflict" scenarios that we are desperately trying to avoid. Legacy URL aliases are created for objects that are converted, and recreating an object in a space with its old ID (using the create API) will cause problems for end users that use deep links for objects. We have a mostly-acceptable workflow for when this happens on a single object, but that process breaks down when you have embedded objects and/or transitive references. IMO Option 3 is a non-starter.

cc @jportner

@joshdover joshdover added v8.0.0 Team:Fleet Team label for Observability Data Collection Fleet team required-for-8.0 This work is required to be done before 8.0 lands, bc it relates to a breaking change or similar. labels Aug 17, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jportner
Copy link
Contributor

jportner commented Aug 23, 2021

From the proposed Option 1:

Once a Saved Object type becomes shareable in 8.x, it's IDs will be re-written for all objects not in the default space. This will break the uninstall flow and asset links as noted at the top of the issue.

It's implied but I wanted to restate that assets (such as dashboards, etc.) will first be made share-capable in the 8.0 release, and fully shareable in a future 8.x release. The latter has not yet been planned.

Q: Can we run this only for assets that are now shareable in the current version?

  • A: Yes, this should be possible. During the start lifecycle, the core.savedObjects.getTypeRegistry() API exposes the details we need for each type. Specifically the convertToMultiNamespaceTypeVersion field should be <= the current version.

FWIW, defining convertToMultiNamespaceTypeVersion < 8.0 is prohibited. The best way for consumers to figure out of an asset is shareable is to use SavedObjectsTypeRegistry.isShareable(type).


From the proposed Option 2:

We'd also need to use the resolve API during the uninstall process and for populating the "assets" tab in the integrations UI for locating the new object IDs for assets installed in a non-default space.

I just wanted to mention that resolve will only correctly fetch assets that were migrated/converted. It will not find assets that were first installed into one space and then copied into another space.

Instead, you can use find for that based on originId -- this will fetch assets that were migrated/converted and that were copied.


From the proposed Option 3:

However, we'll be increasing the blast radius of the two issues presented above (uninstalls don't remove these objects and the asset links are broken), except it will be broken for all integrations installed in a non-default space whereas before they were only broken when viewing an integration from a different Space from where it was installed from.

In addition to these problems, reinstalling assets in Option 3 will create "alias conflict" scenarios that we are desperately trying to avoid. Legacy URL aliases are created for objects that are converted, and recreating an object in a space with its old ID (using the create API) will cause problems for end users that use deep links for objects. We have a mostly-acceptable workflow for when this happens on a single object, but that process breaks down when you have embedded objects and/or transitive references. IMO Option 3 is a non-starter.


Thanks for putting this together, Josh. I am happy to consult on Option 1 or 2 if need be.

@joshdover
Copy link
Contributor Author

FWIW, defining convertToMultiNamespaceTypeVersion < 8.0 is prohibited. The best way for consumers to figure out of an asset is shareable is to use SavedObjectsTypeRegistry.isShareable(type).

Great, didn't realize this API existed. This works even better 😄

In addition to these problems, reinstalling assets in Option 3 will create "alias conflict" scenarios that we are desperately trying to avoid. Legacy URL aliases are created for objects that are converted, and recreating an object in a space with its old ID (using the create API) will cause problems for end users that use deep links for objects. We have a mostly-acceptable workflow for when this happens on a single object, but that process breaks down when you have embedded objects and/or transitive references. IMO Option 3 is a non-starter.

Thanks for calling this out, it sounds like a very ugly situation for we could run into for users who upgrade or install a package into a non-default Space for the second time after the ID re-write migration has been run. I'll update the issue description to reflect this.

Currently, we plan on pursuing Option 1 since it's the smoothest path forward for users and likely will more closely match the UX we want to achieve long-term for Fleet-installed assets.

@jportner
Copy link
Contributor

Currently, we plan on pursuing Option 1 since it's the smoothest path forward for users and likely will more closely match the UX we want to achieve long-term for Fleet-installed assets.

Good to know, but that won't be achievable in 8.0, since we don't plan on making index patterns, dashboards, etc. shareable in that release. So perhaps Option 2 could be a stepping stone in the 8.0 release, thoughts?

@joshdover
Copy link
Contributor Author

Good to know, but that won't be achievable in 8.0, since we don't plan on making index patterns, dashboards, etc. shareable in that release. So perhaps Option 2 could be a stepping stone in the 8.0 release, thoughts?

My thinking is that we can implement it in a way that it will 'just work' once these types become shareable by using the isShareable() API. But that won't work if something else is going to break when these types become "share-capable", I must be a missing a detail there on what exactly the difference between these two modes are.

@jportner
Copy link
Contributor

My thinking is that we can implement it in a way that it will 'just work' once these types become shareable by using the isShareable() API. But that won't work if something else is going to break when these types become "share-capable", I must be a missing a detail there on what exactly the difference between these two modes are.

Yes that's doable, the problem is that the objects won't be fully shareable until sometime in 8.x -- again, the work to move from "share-capable" to "fully shareable" has not yet been planned.
The differences are outlined in detail in the new Sharing Saved Objects developer guide.

For the sake of argument, let's assume that dashboards/etc will be fully shareable in 8.2.
So if Option 1 is implemented now, then what happens if a Fleet package gets upgraded (and as a result, assets get reinstalled) in a Kibana instance while it is running 8.0 or 8.1?
If Fleet is using bulkCreate for this, then we can get get "duplicate" assets in custom spaces, which will result in legacy URL conflicts when users try to view those dashboards/etc.

This won't negatively impact Fleet directly, but it will affect the plugin authors for those assets.

@joshdover
Copy link
Contributor Author

Yes that's doable, the problem is that the objects won't be fully shareable until sometime in 8.x -- again, the work to move from "share-capable" to "fully shareable" has not yet been planned.
The differences are outlined in detail in the new Sharing Saved Objects developer guide.

Got it, so the IDs are not changing at the same time the objects actually support multiple spaces. So we won't be able to create a new object that is shared to all spaces that the asset may have been installed into whenever the IDs change in the 8.0 release.

It seems option 2 is going to be the best path forward until we can have a true multi-space experience for Fleet assets, at which point we should also do something similar to option 1. It would be great if we could coordinate to release UX changes in the Fleet & Integrations apps to support multiple Spaces at the same time that Dashboards, Visualizations, index patterns, etc. support sharing, but I don't think that's absolutely necessary.

@jportner
Copy link
Contributor

It seems option 2 is going to be the best path forward until we can have a true multi-space experience for Fleet assets, at which point we should also do something similar to option 1.

👍

It would be great if we could coordinate to release UX changes in the Fleet & Integrations apps to support multiple Spaces at the same time that Dashboards, Visualizations, index patterns, etc. support sharing, but I don't think that's absolutely necessary.

I'm sure I will be at least somewhat involved in the changes for those code owners, I'll do my best to coordinate with y'all when the time comes.
We'll need to work from the inside out, in terms of the potential object reference graph -- meaning that index patterns will need to be shareable first, then visualizations/lenses, then dashboards. Since those are all owned by different teams, I don't expect them to all be shareable in the same release.

@jportner
Copy link
Contributor

I've got a draft PR to change the Sample Data installer to use the import API: #116378

Fleet will need to make the same changes to its asset installer for those assets to continue working as expected post-8.0.

@jportner jportner added the Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature label Oct 27, 2021
hop-dev added a commit that referenced this issue Dec 6, 2021
* Do not add fields to index patterns

* remove redundant tests

* install index patterns before package install

* update remove comment

* use import to create package assets

Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement.

* Import index patterns

* use resolve when deleting index patterns

* fix: asset type validation

* add option to override supported import types

* make ml-module importable

* Revert "add option to override supported import types"

This reverts commit 1f48e6e.

* remove hidden: false from ml-module

* use resolve when deleting assets

* make security-rule SO type importable

* use bulkResolve to get package assets

* fix tests

* fix 'multiple' tests

* remove unused function

* create index patterns at the same time as other assets

* remove unused test

* Fix integration tests
We were checking for an error before the import was complete.

* tidy for PR

* add missing test assets

* do not attempt to delete missing assets

* resolve any reference errors that occur on import

* await installKibanaAssets immediately

* show assets not found when assets installed in a different space

* fix delete asset check on force upgrade

* add comment about reference errors

* remove a couple of appContextService dependencies
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Dec 6, 2021
* Do not add fields to index patterns

* remove redundant tests

* install index patterns before package install

* update remove comment

* use import to create package assets

Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement.

* Import index patterns

* use resolve when deleting index patterns

* fix: asset type validation

* add option to override supported import types

* make ml-module importable

* Revert "add option to override supported import types"

This reverts commit 1f48e6e.

* remove hidden: false from ml-module

* use resolve when deleting assets

* make security-rule SO type importable

* use bulkResolve to get package assets

* fix tests

* fix 'multiple' tests

* remove unused function

* create index patterns at the same time as other assets

* remove unused test

* Fix integration tests
We were checking for an error before the import was complete.

* tidy for PR

* add missing test assets

* do not attempt to delete missing assets

* resolve any reference errors that occur on import

* await installKibanaAssets immediately

* show assets not found when assets installed in a different space

* fix delete asset check on force upgrade

* add comment about reference errors

* remove a couple of appContextService dependencies
kibanamachine added a commit that referenced this issue Dec 6, 2021
* Do not add fields to index patterns

* remove redundant tests

* install index patterns before package install

* update remove comment

* use import to create package assets

Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement.

* Import index patterns

* use resolve when deleting index patterns

* fix: asset type validation

* add option to override supported import types

* make ml-module importable

* Revert "add option to override supported import types"

This reverts commit 1f48e6e.

* remove hidden: false from ml-module

* use resolve when deleting assets

* make security-rule SO type importable

* use bulkResolve to get package assets

* fix tests

* fix 'multiple' tests

* remove unused function

* create index patterns at the same time as other assets

* remove unused test

* Fix integration tests
We were checking for an error before the import was complete.

* tidy for PR

* add missing test assets

* do not attempt to delete missing assets

* resolve any reference errors that occur on import

* await installKibanaAssets immediately

* show assets not found when assets installed in a different space

* fix delete asset check on force upgrade

* add comment about reference errors

* remove a couple of appContextService dependencies

Co-authored-by: Mark Hopkin <mark.hopkin@elastic.co>
@ghost
Copy link

ghost commented Dec 16, 2021

Hi @joshdover
We have created the below proposed test-cases for this PR. We will be validating these scenarios whenever the merges will be available and will update accordingly to the testrail once behaviour is confirmed.

  • Validate that the user can successfully uninstall the integration from different space.
  • Validate any duplicate options are not present on assets tab when user upgrades the package
    a. Within the same space.
    b. From different space.
  • Validate that on the dashboard tab there should not be any duplicate options present when the user upgrades any package.
  • Validate that on the dashboard tab there should not be any duplicate options present when Kibana is upgraded having few installed packages.

For assets tab we have created more scenarios at #74353
Do let us know if we are missing anything.

Thanks!!

@ghost
Copy link

ghost commented Dec 22, 2021

Hi @joshdover, could you please checkout the boxes if the scenarios are correct.

TinLe pushed a commit to TinLe/kibana that referenced this issue Dec 22, 2021
* Do not add fields to index patterns

* remove redundant tests

* install index patterns before package install

* update remove comment

* use import to create package assets

Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement.

* Import index patterns

* use resolve when deleting index patterns

* fix: asset type validation

* add option to override supported import types

* make ml-module importable

* Revert "add option to override supported import types"

This reverts commit 1f48e6e.

* remove hidden: false from ml-module

* use resolve when deleting assets

* make security-rule SO type importable

* use bulkResolve to get package assets

* fix tests

* fix 'multiple' tests

* remove unused function

* create index patterns at the same time as other assets

* remove unused test

* Fix integration tests
We were checking for an error before the import was complete.

* tidy for PR

* add missing test assets

* do not attempt to delete missing assets

* resolve any reference errors that occur on import

* await installKibanaAssets immediately

* show assets not found when assets installed in a different space

* fix delete asset check on force upgrade

* add comment about reference errors

* remove a couple of appContextService dependencies
gbamparop pushed a commit to gbamparop/kibana that referenced this issue Jan 12, 2022
* Do not add fields to index patterns

* remove redundant tests

* install index patterns before package install

* update remove comment

* use import to create package assets

Here I have also moved to importing all assets at once. This is essential when importing to ensure that all saved objects references are imported at once. There is also an efficiencey improvement.

* Import index patterns

* use resolve when deleting index patterns

* fix: asset type validation

* add option to override supported import types

* make ml-module importable

* Revert "add option to override supported import types"

This reverts commit 1f48e6e.

* remove hidden: false from ml-module

* use resolve when deleting assets

* make security-rule SO type importable

* use bulkResolve to get package assets

* fix tests

* fix 'multiple' tests

* remove unused function

* create index patterns at the same time as other assets

* remove unused test

* Fix integration tests
We were checking for an error before the import was complete.

* tidy for PR

* add missing test assets

* do not attempt to delete missing assets

* resolve any reference errors that occur on import

* await installKibanaAssets immediately

* show assets not found when assets installed in a different space

* fix delete asset check on force upgrade

* add comment about reference errors

* remove a couple of appContextService dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature required-for-8.0 This work is required to be done before 8.0 lands, bc it relates to a breaking change or similar. Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants