elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.6k stars 8.21k forks source link

Saved objects import/export should include spaces #91615

Open rudolf opened 3 years ago

rudolf commented 3 years ago

When users use the saved objects import/export UI or API they are able to export all objects from a specific space or import objects into a specific space, but the saved objects in the NDJSON file itself are stripped of all namespace information.

This causes several problems:

  1. When making a backup export, users loose all space information and have to manually keep track of the space they exported from to correctly restore their saved objects.
  2. Some organisations have hundreds or thousands of spaces making it impractical to backup/restore each space's saved objects separately.
  3. Once saved objects can be shared between multiple spaces in 8.0:
    1. We want to remove the ability to "psuedo-copy" saved objects between spaces. That is, it will no longer be possible to export an object from space a and importing it into space b without changing the id of the document. One way to remove "psuedo-copies" is to always generate new id's on import (https://github.com/elastic/kibana/issues/81904). However, this will make it impossible for users to perform a backup restore with the import API. For instance if a user accidentally broke a dashboard they won't be able to re-import that dashboard to restore it to it's working state, instead the import will create a new dashboard.
    2. It won't be possible for users to restore their saved objects in a way that preserves which spaces a saved object was shared to.
rudolf commented 3 years ago

Since importing saved objects will add and potentially change existing saved objects in other spaces than the user's current space, users will have to have access to all spaces which the import will affect. However, the user experience can be quite frustrating when imports stop working because an existing saved object has been saved to a different space to which the current user doesn't have access. So it feels like import/export needs to be limited to "administrator" users who have access to all spaces.

elasticmachine commented 3 years ago

Pinging @elastic/kibana-core (Team:Core)

elasticmachine commented 3 years ago

Pinging @elastic/kibana-security (Team:Security)

jportner commented 3 years ago

However, the user experience can be quite frustrating when imports stop working because an existing saved object has been saved to a different space to which the current user doesn't have access.

FWIW, today (as of Sharing Saved Objects phase 1.5), when we run into this situation ("unresolvable conflict") we simply regenerate the imported object's ID. However, once import/export is space-aware, we can instead show an appropriate conflict warning to the end user, then overwrite these objects by setting the "initialNamespaces" upon create. I feel like this would be a better user experience.

So it feels like import/export needs to be limited to "administrator" users who have access to all spaces.

I worry that "non-global admins" (users who don't have all-spaces-admin-access) will chafe at the idea that they cannot export or import saved objects. Sometimes Kibana is used in "multi-tenant" scenarios where each tenant gets their own Kibana space. I think we could still support a space-aware import/export for non-global admins, even if it means they must skip importing an object when an unresolvable conflict occurs.


Edit: forgot to mention, I think this is blocked until we can support bulk copy-to-space (#52098 and #90025). In the multi-tenant scenario we ostensibly have users that want to "stage" new spaces using import/export, but they will need an alternative when this is no longer possible.

pgayvallet commented 3 years ago

I worry that "non-global admins" (users who don't have all-spaces-admin-access) will chafe at the idea that they cannot export or import saved objects.

We already discussed about it a while ago with @kobelb (and probably others from the team), but I think there are two distinct scenarios here:

The (new) admin backup/restore, where we do want to preserve all spaces information, and that should probably only be performed by admins

And the 'classic' (current) import/export, that can, for example, be used to copy SOs from one environment to another one, and not necessarily to the same space, where stripping space information during export and relying on the current user's (or API call's) space make sense.

Also note that

Some organisations have hundreds or thousands of spaces making it impractical to backup/restore each space's saved objects separately.

Even if we do enhance the API to preserve namespace information during export and use it during import, there is current no way (from the UI or the API) to export objects from multiple (distinct) spaces at the same time, which seems like pretty mandatory for the feature.

That makes me think that it could even be two totally distinct features, from different administration sections. It would also means that this would be an enhancement, and not a breaking change, as the current SO management import/export would remain unchanged.

For the 'backup/restore' feature, we may also want to only allow types and/or spaces selection, instead of allowing to filter and search for specific objects by searchableFields as the SOM is currently allowing via the searchBar. User would basically choose

For https://github.com/elastic/kibana/issues/82020, we may also want to allow administrators to remap the spaces during import, aka 'objects in space X in the backup file would be reimported in space Y'

@kobelb @elastic/kibana-security wdyt?

joshdover commented 3 years ago

@elastic/kibana-security Friendly bump on this. We'd like to align on an approach in the next few weeks so that we can prioritize any necessary work on our end for the 8.0 release.

jportner commented 3 years ago

Friendly bump on this. We'd like to align on an approach in the next few weeks so that we can prioritize any necessary work on our end for the 8.0 release.

:+1:

That makes me think that it could even be two totally distinct features, from different administration sections. It would also means that this would be an enhancement, and not a breaking change, as the current SO management import/export would remain unchanged.

For the 'backup/restore' feature, we may also want to only allow types and/or spaces selection, instead of allowing to filter and search for specific objects by searchableFields as the SOM is currently allowing via the searchBar. User would basically choose

  • which spaces to backup
  • which types to backup in selected spaces
  • we probably just include all references without asking him, backup without nested objects doesn't make any sense (let's talk about tags later)
  • that's it.

I like this proposal. I think we should limit this new feature to users who are "global admins", those who have privileges to access the global resource (e.g., "All Spaces"). If a user is not a global admin, each exported object's namespaces array would be redacted by the security plugin.

Just to be clear: the idea here is that the kibana-wide backup would include the actual spaces themselves, yes?

Thoughts/questions that come to mind:

Overall I agree with the approach at a high level, I think we need to work out some of the details and what the UX would look like.

For #82020, we may also want to allow administrators to remap the spaces during import, aka 'objects in space X in the backup file would be reimported in space Y'

I think we need to tease out the details of this a bit more.

kobelb commented 3 years ago

@pgayvallet is my understanding correct that you're proposing leaving the import/export API how it behaves today, where it only works for a single space, always excludes the namespaces on export and uses the current space on import. And to support the additional use-cases, we'd add a backup/restore API that would work across all Spaces and include the namespaces on export and respect them on import?

If my understanding is correct, this leaves me with a couple questions:

I ask these questions, because I've been wondering whether we should just deprecate the existing import/export API and make a new backup/restore API that works for all situations.

legrego commented 3 years ago

I ask these questions, because I've been wondering whether we should just deprecate the existing import/export API and make a new backup/restore API that works for all situations.

I'd love to explore this further. I'm not sure how much pushback we would get from the community (or support), but it's an exciting proposal.

Would both the import/export and backup/restore use the same NDJSON data-structure?

IMO the new feature is an opportunity to use a new data structure, with first-class support for metadata.

pgayvallet commented 3 years ago

@jportner

I think we should limit this new feature to users who are "global admins"

Yea, this is meant to be an administrator-only feature, to restricting to the highest permissions / role seems fine.

the idea here is that the kibana-wide backup would include the actual spaces themselves, yes?

Good question. I think we want to, but retaining the space-specific information such as the permission matrix may be a pain to reimport, e.g to another instance where the defined roles are not the same. I'm afraid exporting the spaces themselves would also require to manage / include the associated roles, which is all but trivial, wdyt?

if a multi-namespace object exists in space X, and we want to create an object with the same ID in space Y, the SOR pre-flight checks will prohibit that without allowing an overwrite. We could change the overwrite behavior, or just delete any affected objects before re-creating them.

If we focus on backup/restore, I'd say erasing existing conflicting objects should be ok. The way I see it, we don't want to support object-level granularity as we do with the current import/export feature.

Are we going to make any attempt to enforce referential integrity of existing objects that might be affected

Same, in the scope of backup/restore, we should probably make the assumption that a backup respects the ref integrity, as any backup-ed object also did include its references, so this seems like an edge case.

Would we pair this feature with a new Saved Object Management page that displays a table of all saved objects from all spaces

The way I see it, we will have another page for this feature, however we will not do down to the object granularity: We will list the spaces, the number of object per type once the user selects a list of spaces, but we will never list the objects as we do in the current SOM page.

Overall I agree with the approach at a high level, I think we need to work out some of the details and what the UX would look like.

Definitely, but I just want to be sure we're all ok with the high-level idea before going down on the details.

Would we enforce a 1:1 mapping when importing a Kibana-wide backup, or would we want to allow 1:n mapping (objects in space X and space Y both get imported into space Z)

YTBD. Even if I'm not sure to see a need to import multiple input spaces into the same output one, I'm sure some customers will.

Is re-mapping needed for the MVP of the new feature? I'd guess probably not

Probably not. I think we do want this for 8.0 though. Seems like a strong requirement for https://github.com/elastic/kibana/issues/82020

@kobelb

is my understanding correct that you're proposing leaving the import/export API how it behaves today, where it only works for a single space, always excludes the namespaces on export and uses the current space on import. And to support the additional use-cases, we'd add a backup/restore API that would work across all Spaces and include the namespaces on export and respect them on import

That's it yea.

Would we end up supporting both the import/export and backup/restore long-term?

My thinking is:

My conclusion is:

Would both the import/export and backup/restore use the same NDJSON data-structure

ATM, I don't see any technical blocker to do so. Now, functionally, use one feature to generate an 'export' file and the other one to import it may lead to unexpected behavior and confusion for the end-user, so I wonder if that's really a good solution.

Also (as Larry said already), TBH, the current NDJSON format is not that great. E.g one year ago we had to add an additional metadata entry at the end, that is not following the same structure, to store meta such as number of non found objects and so. Allowing us to use distinct formats would also allow us to challenge the existing one.

I ask these questions, because I've been wondering whether we should just deprecate the existing import/export API and make a new backup/restore API that works for all situations

All, please read - this summarize all the previous points

The question and its implications are complex, but to summarize (this also kinda summarize all the other discussions):

In term of UX/UI, I strongly think that the current feature and backup/export would require distinct UIs to best address them. As already explained, selecting spaces and types during a backup is not the same workflow as picking objects as done in the current SOM app. Also, the required permissions for both features are not the same.

In term of API, I also do think we should have distinct HTTP endpoint / associated service methods. the current /_export endpoint is already a patchwork mixing the 'per id' and 'per type' exports, so adding an additional 'type' of export doesn't seems like the way to go.

In term of technical implementation, I feel like the current code was conceived (and patched multiple times) to only address the current feature. I'm afraid adapting it to support this additional multi-spaces export would be a pain.

In term of exported data format (ndjson), the main (only?) reason for this format was the possibility to stream the results to the client 'in real time' during the export. Functionality that was never effectively used, and that is now impossible due to the addition of the additional metadata entry at the end of the stream. Given that, imho, the cons of the format are quite impacting (plain JSON is just way easier to manipulate), I do think that this would be a good time to challenge and/or revisit the format.

However, I did assume that we wanted to preserve the current import/export API, it never came to my mind that we could develop a new API supporting both the current features and the new ones.

I do think this is an option (and probably the best one), if (and only if):

wdyt all?

kobelb commented 3 years ago

Distinct management sections

@pgayvallet, I'm interested in why you're pushing for distinct management sections and screens. My opinion isn't the only one that matters, but I don't hate the saved-object management screen. There are imperfections to it, like being able to edit the raw JSON of saved-objects, but I still see the utility in allowing a user to have a unified view of all of their saved-objects and allowing them to perform common functions across them.

Use-cases

I think we need to take a step-back and think through the ideal UX and API to enable all of the valid use-cases. The following are the use-cases that I've thought of, segmented to within a cluster and between different clusters. I've offered a bit of commentary on these use-cases. We don't have to support all of them initially, but we should make sure that the choices we make will work for all of the valid use-cases long-term.

Within a single cluster

Pseudo-copy

I don't think we should support this in 8.0+. It's a surprising legacy behavior that we should just get rid of ASAP.

True copy between Spaces

A large number of users will likely want to do this. We shouldn't restrict this functionality to users who have the ability to manage saved-objects in all spaces. We don't need to rely on the import/export or backup/restore APIs to perform this action, and we can add a dedicated copy endpoint(s).

Backup/Restore one-to-many saved-objects within a Space

A large number of users will likely want to do this as well, including users who only have access to a single Space. As long as the operation is restricted to a single Space, we can drop all space information on export, ignore space information on import, or include the space information and ensure we're importing the saved-object to the space it was exported from.

Backup/Restore one-to-many saved-objects across Spaces

Fewer users will potentially want to do this, but I don't think we can safely assume that only users who can manage saved-objects in all spaces will want this functionality. If I happened to be the admin of 2 spaces out of 1000, I'd want to be able to see all of the saved-objects in those two spaces and selectively backup/restore them.

Backup/Restore everything across all Spaces

Only allowing users who can manage saved-objects across all Spaces is the logical option here

Between clusters

Copy between clusters, ignoring the source Space

Users may have different spaces in different clusters, but still want to copy saved-objects between them. A long as the user has access to manage saved-objects in the source/target cluster and Spaces, they should be able to perform this operation.

Copy between clusters, including the source Spaces

Even if users only have access to a subset of spaces in the source space, they should be able to export all of these saved-objects into the target cluster and spaces, while maintaining the source space information.

Changing NDJSON data-format

Users can currently export saved-objects from the previous major version of Kibana: https://www.elastic.co/guide/en/kibana/current/managing-saved-objects.html#_compatibility_across_versions. It'd be beneficial to our users to continue supporting exports from 7.x in 8.x.

pgayvallet commented 3 years ago

@kobelb

I'm interested in why you're pushing for distinct management sections and screens

Because (the way I see it) these are totally distinct user workflows (see https://github.com/elastic/kibana/issues/91615#issuecomment-828313314), and the backup features is not based on a UI displaying a table listing all SOs, as we don't want this granularity for the feature.

But once again, this was my gut feeling based on my assumptions, please challenge me on that. If we think that it makes more sense to just add a space selector in the SOM table header and a checkbox during the export (and import) to preserve space information, I would be pleased to go in that direction.

Note that adding the feature to SOM and allowing all users to export/import from/to all spaces they're allowed seems like a huge pain in term of security checks during the import, to ensure that all objects are imported to spaces where the user has correct permissions. This is another reason why only allowing administrator to use multi-space import/export feels very pragmatic to me.

Use cases

True copy between Spaces We don't need to rely on the import/export or backup/restore APIs to perform this action, and we can add a dedicated copy endpoint(s).

Is that true though? What about objects with export/import hooks?

Users can currently export saved-objects from the previous major version of Kibana. It'd be beneficial to our users to continue supporting exports from 7.x in 8.x.

It's even mandatory for https://github.com/elastic/kibana/issues/82020, as this is how users will migrate from legacy multitenancy in 7.x to spaces in 8.x.

However, just look back to the 'legacy' export format. We don't have any metadata attached to either the legacy or the current export format. Do you know how we are currently making the distinction between these two formats during import?

Please have a look look at this beauty:

https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/plugins/saved_objects_management/public/management_section/objects_table/components/flyout.tsx#L161

That's right, we're looking at the file extension. What will be the plan when we'll be forced to introduce breaking changes to the format (trust me, it will happen, sooner or later). We will export in .ndjson2? then ndjson3?

I agree that we do need to preserve BWC, but we also DO need a better export format. As the legacy format will go away in 8.0, my pragmatism would say to preserve support to the current .ndjson format, but to also introduce a new, extensible format to futur-proof ourselves. It could be a jsonformat, it could be an ndjson with a top metadata element, or anything, but I do think we need it.

However I agree that this is not needed by this specific feature. I'm just afraid that by adding functionalities one by one without looking at the big picture, we'll end up doing way more work than necessary and being forced to rewrite more things more times. E.g the recent issue about the need to exclude connectors from export based on a predicate.

kobelb commented 3 years ago

If we think that it makes more sense to just add a space selector in the SOM table header and a checkbox during the export (and import) to preserve space information, I would be pleased to go in that direction.

Me too :) I think we need to get design involved to help us figure out whether or not this is reasonable.

Note that adding the feature to SOM and allowing all users to export/import from/to all spaces they're allowed seems like a huge pain in term of security checks during the import, to ensure that all objects are imported to spaces where the user has correct permissions. This is another reason why only allowing administrator to use multi-space import/export feels very pragmatic to me.

I think it'd be fine to only allow administrators to use multi-space import/export in the short-term. However, I think we should make sure that our UI and API design can evolve over time to facilitate users who don't have access to every single space. As users lean into spaces for solving their multi-tenancy concerns, fewer and fewer users will be global administrators.

True copy between Spaces We don't need to rely on the import/export or backup/restore APIs to perform this action, and we can add a dedicated copy endpoint(s).

Is that true though? What about objects with export/import hooks?

You're right that we do need to make sure that the existing logic in export/import hooks applies to copy as well. It was my intent to say that we could have a dedicated HTTP API for doing so, it doesn't have to rely on the import/export or backup/restore HTTP APIs.

I agree that we do need to preserve BWC, but we also DO need a better export format. As the legacy format will go away in 8.0, my pragmatism would say to preserve support to the current .ndjson format, but to also introduce a new, extensible format to futur-proof ourselves. It could be a jsonformat, it could be an ndjson with a top metadata element, or anything, but I do think we need it.

However I agree that this is not needed by this specific feature. I'm just afraid that by adding functionalities one by one without looking at the big picture, we'll end up doing way more work than necessary and being forced to rewrite more things more times. E.g the recent issue about the need to exclude connectors from export based on a predicate.

100%.

pgayvallet commented 3 years ago

If we think that it makes more sense to just add a space selector in the SOM table header and a checkbox during the export

Ok, I'm going to KISS and assume we'll go that way for now. So, let's focus a bit on the technicals now:

From what I see:

we're currently hard-removing the namespaces from the objects during the export process

https://github.com/elastic/kibana/blob/49078c82bc10379f0f8a9691b33f366751e4831a/src/core/server/saved_objects/export/saved_objects_exporter.ts#L142-L144

This could easily be changed with an export-scoped option.

However it seems to be only for multi-namespace types:

https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/core/types/saved_objects.ts#L87-L88

We apparently aren't surfacing the namespace property in any of the SO read apis, and the exporter is leveraging them (bulkGet and find specifically). This is going to be a blocker for the feature, as I don't think we have any other option than to surface the namespace of the objects for the SO exporter to add it to the exported data.

Also, I tend to forget that you can't specify the initial namespace for single-namespace types when using create or bulkCreate (or any other operation) as the spaces wrapper ensure so:

https://github.com/elastic/kibana/blob/106afd41b689fa0c3fc680d14b838d29650473e3/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts#L128-L132

@elastic/kibana-security Do you have any idea of the estimated cost to allow these API to allow the consumer/caller to specify the namespace as it's done for initialNamespaces (and just ensure the consumer is allowed to create the given type(s) in given namespace instead of throwing if the option is specified)? This seems to be mandatory to develop this feature. We would still need to ensure the consumer got the correct permissions to perform the operation, but I think this would be handled by security anyway (right?)

Note: we would also need to change the SavedObjectsBulkCreateObject type to allow to specify namespace per-object, as the global second parameter (SavedObjectsCreateOptions) would not be sufficient to bulk create objects in different namespace (especially as we'll be potentially importing a mix of single and multi NS objects)

In short:

@elastic/kibana-security what's you take on that?

jportner commented 3 years ago

In short:

  • we need the read APIs of the SO repository to now returns the namespace property of the object. A potential non-breaking option might be to leverage the already existing namespaces field for that, and populate also for single-NS types (even if I feel like there is a lot of implications doing so)

I'd prefer to leverage the existing namespaces field for this. We already populate it for single-NS types (introduced in #67644 so we can differentiate between two different objects with the same ID when searching across spaces).

  • we need the spaces SO wrapper to allow consumers to specify the namespace option for write operations
  • we need a way to specify single or multiple namespace at the object granularity during a bulkCreate operation

It wouldn't be a lot of work to allow this. We would need to normalize the namespaces in the SOR (namespaces: ['default'] should be changed to namespace: undefined). We would also need to ensure that consumers don't try to set initialNamespaces: ['*'] for a single-namespace object type, probably just throw a 400 error in this case as well.

Using namespaces and initialNamespaces for single-namespace types isn't pretty, but we only have one first-party consumer for this (import API) and it will also simplify our implementation in the import code.

pgayvallet commented 3 years ago

I'd prefer to leverage the existing namespaces field for this. We already populate it for single-NS types

I actually missed that, but you're right, getSavedObjectFromSource populates the namespaces field for all non-agnostic types, so I think it's just perfectly fine to leverage it during exports.

create: consumers can specify this already

Can they? Reading the code of the spaces wrapper, I assumed otherwise, at least for single-ns types, as we're throwing if options.namespace is specified.

https://github.com/elastic/kibana/blob/b2d36b863bc945479a12022a7a2cc4902ad2bd45/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts#L109-L114

Or did you mean they can already use initialNamespaces for multi-ns types? In which case we still need to find a solution for single-ns types? Or did I miss something?

bulkCreate: perhaps we could just allow the consumer to use the existing initialNamespaces field for this, and throw a 400 error if the consumer specifies more than one space ID for a single-namespace type

Using SavedObjectsBulkCreateObject.initialNamespaces would be perfectly fine with me, if you're alright with it.

Using namespaces and initialNamespaces for single-namespace types isn't pretty, but we only have one first-party consumer for this (import API)

Yea, well, if you ask me, this approach is perfectly fine imho. I even kinda like the fact that the API is the same for single and multi-namespace types, and that we just perform validation under the hood.

jportner commented 3 years ago

create: consumers can specify this already

Can they? Reading the code of the spaces wrapper, I assumed otherwise, at least for single-ns types, as we're throwing if options.namespace is specified.

Sorry for the confusion, that slipped my mind. You're right, consumers can't specify the namespace option directly. There are workarounds for this -- the Reporting plugin generates a fake request with a space prefix so they can get a scoped SOC for a desired space: https://github.com/elastic/kibana/blob/f97aad30f4be6a6f8a3b541621c2229db225c2ab/x-pack/plugins/reporting/server/core.ts#L282-L300

SavedObjectsClient.create() also supports the initialNamespaces option though, so we shouldn't need to add anything else 😄

Sounds like we are in agreement on the technicals, then.

pgayvallet commented 3 years ago

@MichaelMarcialis @thesmallestduck just a quick ping, in case you want to get involved.

ATM, the plan is to just use the existing SO management section, and

Does that sound alright with you, on a functional/design POV?

MichaelMarcialis commented 3 years ago

Does that sound alright with you, on a functional/design POV?

Hey, @pgayvallet! I'm coming into this conversation sideways, so I apologize in advance if I'm misinterpreting anything. Am I correct in assuming the spaces filter you mention (and possibly related table column) will be referencing saved objects that are shared across spaces?

I assume the request for my involvement is in reference to my past design suggestions as part of the saved object tagging feature, as some of those suggestions included improvements to saved object management. If that's the case, based on what I'm reading here, I'm OK with the notion of updating the existing saved object management page as you've described. In fact, that approach is largely what my design suggestions for the saved object management page entailed (along with a handful of other UX quality-of-life improvements). Assuming these designs are still relevant to the conversation, please feel free to have a look and let me know if you'd also like me to put some time on the calendar to discuss further, if need be. Thanks.

pgayvallet commented 3 years ago

We should probably split this issue in two parts:

  1. allow the usage of initialNamespaces for the SOR/SOC's create and bulkCreate operations for all SO types regardless of their namespaceType configuration (and update the security/consistency checks accordingly, e.g cannot specify namespaces without correct permissions, cannot specify multiple namespaces when the object is SavedObjectsNamespaceType = 'single'...)

  2. update the export/import logic to leverage this new feature

@legrego @jportner do you think part 1. should be handled by security or by core? I suspect the implications in the spaces (and security?) plugin(s) are quite significant?

jportner commented 3 years ago

@legrego @jportner do you think part 1. should be handled by security or by core? I suspect the implications in the spaces (and security?) plugin(s) are quite significant?

I can handle it. We are already doing authZ checks for initialNamespaces so there shouldn't be anything extra to do there.

pgayvallet commented 3 years ago

@jportner Just for our roadmap / prioritization, do you know when you're planning to work on https://github.com/elastic/kibana/issues/101904?

jportner commented 3 years ago

@jportner Just for our roadmap / prioritization, do you know when you're planning to work on #101904?

I plan to work on it as soon as my current PR is ready for review. Will probably be tomorrow!

pgayvallet commented 3 years ago

I started working on it, and I have a few remarks:

Adding multi-space support to SO import/export effectively have the requirement to also support multiple spaces in the whole SOM ui (which we totally overlooked).

E.g:

On a more global level, the whole Kibana app was conceived in a space-isolated manner. We're introducing a precedent here, by allowing to visualize and interact with objects from other spaces.

I don't thing anything would be impossible here, but the whole 'make the SOM page multi-NS compatible' is something we just totally forgot to take into consideration, and that is going to require some work. I just want to be sure we're all on the same page and that we all agree that this must be done (the other option would be to just adapt the HTTP import/export APIs to support multi-NS without doing any change to the UI, but that feel wrong, doesn't it?)

cc @kobelb (told ya) @lukeelmers

jportner commented 3 years ago
  • we need a new spaces column to the SO table to dissociate which space(s) each object is in. Atm if I create 3 spaces and go to the SOM listing and select all of them, I got 3 Advanced Settings [8.0.0] objects and I have no idea which one is in which space

We have this right now (provided by the Spaces plugin) but it 1. it is commented out until shareable objects are available in the SOM page, and 2. it only shows other spaces (not the current space). We could easily change it so it shows the current space, too.

  • we need to manage actions for objects that are not in the current space. For example I'm in space A, I choose to list objects from space B. Should I be able to inspect these objects? Should I be able to navigate to these objects? This would be far from trivial. I guess the most pragmatic approach would be to simply disable the actions.

Good point. I was under the impression that we wanted to remove "inspect objects" from the SOM page anyway? That still leaves navigation though. And we would have to take into account objects that exist in multiple spaces. So, yeah, new UX needed for that / non-trivial.

  • there are bulk actions too. Should the user be allowed to select objects from other spaces than the active one and then perform a bulk delete?

So, today the only other example of a page that has "cross-space-aware" saved objects in Kibana is ML. There is some reasonable expectation/precedent that doing stuff in Kibana in one space won't affect other spaces. Having the isolation is kind of a nice feature.

That said, we are talking about a management page here and I think the current behavior doesn't really make a whole lot of sense. Other management pages (such as role management and user management) also affect the rest of Kibana. Not to mention that if you delete an object that is shared, it gets deleted from all spaces that it exists in.

A couple more questions come to mind:

  1. Should we restrict "cross-space-aware SOM" to users who are full Kibana admins? (e.g., they have All access to the global resource *, all current and future spaces)
  2. Just spitballing, we could consider adding a switch to the top of the page to optionally show objects from other spaces. That way if you don't want to impact objects from other spaces, you have the isolation.
pgayvallet commented 3 years ago

Good point. I was under the impression that we wanted to remove "inspect objects" from the SOM page anyway? That still leaves navigation though. And we would have to take into account objects that exist in multiple spaces

Yea. I think just disabling the actions if the object is not present in the active space is acceptable until we take a closer look at what we may exactly want.

Should we restrict "cross-space-aware SOM" to users who are full Kibana admins? (e.g., they have All access to the global resource *, all current and future spaces)

I thought a lot about this, and I'm not sure of the correct solution. Restricting to full admins would definitely be an option, and (probably) the easiest one.

Another option would be to filter the spaces listed in the UI depending on the user's permissions in each of those spaces (probably via a new export value inside the space's authorizedPurposes?)

wdyt?

Just spitballing, we could consider adding a switch to the top of the page to optionally show objects from other spaces.

I was planning to keep the current behavior (only current space) by default anyway, to avoid introducing unnecessary disruption to the UI.

I was thinking about just adding another filter defaulting to the current space:

Screenshot 2021-08-17 at 16 12 06 Screenshot 2021-08-17 at 16 12 11 Screenshot 2021-08-17 at 16 12 16

But maybe the only modes we want are 'active space' and 'all spaces', in which case, a toggle switch would probably make more sense, I'm unsure. What do you (and @kobelb) think? (Note that if we go with the select, we would need to add a * option at the top to allow targeting all spaces without selecting them all)

pgayvallet commented 3 years ago

I love how it's impossible to identify all the things that are going to be on your way when touching the SO import/export code before starting to try to implementing it.

A few blockers:

Export

SOM UI

pgayvallet commented 3 years ago

Effectively, blocked by:

jportner commented 3 years ago

I thought a lot about this, and I'm not sure of the correct solution. Restricting to full admins would definitely be an option, and (probably) the easiest one.

Another option would be to filter the spaces listed in the UI depending on the user's permissions in each of those spaces (probably via a new export value inside the space's authorizedPurposes?)

wdyt?

I think restricting to full admins is the best thing to do, at least for now. Otherwise you may have a situation where you have permission to import to only a subset of the specified spaces, for example. How would we handle that? Just fail the whole import, I guess?

But maybe the only modes we want are 'active space' and 'all spaces', in which case, a toggle switch would probably make more sense, I'm unsure. What do you (and @kobelb) think? (Note that if we go with the select, we would need to add a * option at the top to allow targeting all spaces without selecting them all)

Well, we still want users to be able to export/import without namespace/s metadata, right? (the way it functions today) So how will we differentiate whether to include that metadata in the export? We could just include it if multiple spaces are selected, but what about the case where there is only the Default space -- maybe I still want to include the space metadata.

So I think a switch may be a better solution. We can always come up with a more complex control later on if need be.

pgayvallet commented 3 years ago

I think restricting to full admins is the best thing to do, at least for now. Otherwise you may have a situation where you have permission to import to only a subset of the specified spaces, for example

That would be fine with me. In term of API, how can I leverage the security plugin to know the user is a 'full admin' ? Is this a specific role?

Side question: should we only show the 'namespace' column in such situation? Technically that's a bit complex, and as we're going to need it for everyone once the shareable type are there, I'd say it's fine to have this new column even when not allowed to perform multi-namespace exports.

Well, we still want users to be able to export/import without namespace/s metadata, right? (the way it functions today)

We do. I added a new include namespace information in addition to what was the only option we have when exporting: include related objects. When unchecked, the namespaces info are not included (as it's done currently)

I'm unsure what the default value should be though. I guess that, for (user-)BWC, it should be disabled by default?

So I think a switch may be a better solution. We can always come up with a more complex control later on if need be

The POC is currently using a select (and to be honest, I'm not sure how a toggle would play in an EUI's searchbar). Let's try with that once the technical prerequisites are complete, and we'll iterate from here.

jportner commented 3 years ago

That would be fine with me. In term of API, how can I leverage the security plugin to know the user is a 'full admin' ? Is this a specific role?

Good question. No, it's not a specific role. A "full admin" has all access in * (all spaces). We have a few other situations where we use this (for example, you can only manage spaces if you have "full admin" access).

I am assuming that we want a user who has read-only access in all spaces to be able to export all objects, too. I don't think we have a special term for these users, but let's call them "read-only admins" 😄

These privileges are defined here: https://github.com/elastic/kibana/blob/ea062391bb741b87a36a8d8dbedf96454b4e8c84/x-pack/plugins/security/server/authorization/privileges/privileges.ts#L103-L122

So you'll probably want to do a few things:

  1. Add API privileges and UI capabilities here:
    diff --git a/x-pack/plugins/security/server/authorization/privileges/privileges.ts b/x-pack/plugins/security/server/authorization/privileges/privileges.ts
    index c38a5c9a44f..a9ee584de23 100644
    --- a/x-pack/plugins/security/server/authorization/privileges/privileges.ts
    +++ b/x-pack/plugins/security/server/authorization/privileges/privileges.ts
    @@ -111,12 +111,18 @@ export function privilegesFactory(
             actions.ui.get('management', 'kibana', 'spaces'),
             actions.ui.get('catalogue', 'spaces'),
             actions.ui.get('enterpriseSearch', 'all'),
    +            actions.api.get('importAllSavedObjects'),
    +            actions.ui.get('savedObjectsManagement', 'importAcrossSpaces'),
    +            actions.api.get('exportAllSavedObjects'),
    +            actions.ui.get('savedObjectsManagement', 'exportAcrossSpaces'),
             ...allActions,
           ],
           read: [
             actions.login,
             actions.version,
             actions.api.get('decryptedTelemetry'),
    +            actions.api.get('exportAllSavedObjects'),
    +            actions.ui.get('savedObjectsManagement', 'exportAcrossSpaces'),
             ...readActions,
           ],
         },
  2. Define separate HTTP routes for "import saved objects across spaces" and "export saved objects across spaces", controlled with the access:importAllSavedObjects and access:exportAllSavedObjects route tags, respectively
  3. Use the UI capabilities for savedObjectsManagement.importAcrossSpaces and savedObjectsManagement.exportAcrossSpaces to modify the UI accordingly

We do. I added a new include namespace information in addition to what was the only option we have when exporting: include related objects. When unchecked, the namespaces info are not included (as it's done currently)

Gotcha, that makes sense. So if the user has already changed the top select filter to display multiple spaces, that option is selected already in the Export flyout / cannot be disabled?

I'm unsure what the default value should be though. I guess that, for (user-)BWC, it should be disabled by default?

Yes I think so.

The POC is currently using a select (and to be honest, I'm not sure how a toggle would play in an EUI's searchbar). Let's try with that once the technical prerequisites are complete, and we'll iterate from here.

Sounds good.

pgayvallet commented 3 years ago

So you'll probably want to do a few things:

Thanks a lot for the pointers

So if the user has already changed the top select filter to display multiple spaces, that option is selected already in the Export flyout / cannot be disabled?

That's the plan yes, even if not implemented yet in the PR.

Define separate HTTP routes for "import saved objects across spaces" and "export saved objects across spaces", controlled with the access:importAllSavedObjects and access:exportAllSavedObjects route tags, respectively

We talked about this sync, and we agreed that distinct endpoints, owned by core, seems like the best compromise.

pgayvallet commented 3 years ago

So, just when I thought https://github.com/elastic/kibana/pull/109196 was getting close to be ready-ish, I discovered that the whole conflict resolving logic is currently only supporting a single namespace as the target, which is not sufficient for the cross-space import where you can import shareable types into multiple spaces, and need to check for conflicts accordingly.

After discussing with @jportner, enhancing the behavior to support such scenarios is not an easy task, and, more importantly, it's functionally unclear how we should handle conflicts in such cases, and if we want to take shortcuts because the API is currently only limited to 'super users', or if we shouldn't, which would allow us to more easily open the feature to other users with proper space restriction.

Given the sync discussion we had today with @kobelb @rudolf @legrego and @lukeelmers about how we should handle pseudo-copy in the 'new' import, it seems like we'll probably want to do the same for the cross-space import (which could actually, once we address the PR's current super-user restriction, be the start of this new, BWC, import).

This is why the team decided to put https://github.com/elastic/kibana/pull/109196 on hold until we have a more precise vision of how we want to handle conflict resolution and pseudo-copy.

rudolf commented 2 years ago

Notes from our last meeting (1 Sep 2021):

The problem with psuedo-copies

Accounting for all the conflict scenarios introduces significant complexity and tech debt to the saved objects codebase. But more than that, it introduces significant complexity to our users. Even if we have a UI that allows users to resolve conflicts it’s really hard for a user to understand why this conflict is occurring, and in many cases users won’t have the necessary context to resolve a conflict. E.g. faced with JSON of two siem-detection-engine-rule-status documents it’s not obvious which one should be overridden and what the impact of doing that might be.

To provide a better experience for our users we should prevent them from encountering these conflicts in the first place, e.g. remove the ability to create pseudo-copies of documents.

Avoiding pseudo-copies on import

To avoid pseudo-copies on import we would need to introduce a new API which takes not only the documents, but also all the namespaces a document is shared to. Importing with this API can only do two types of operations, either we override existing documents or we make a true copy by regenerating their id (completely dissociating them from their source). These operations are simple enough, the difficult part is in how we handle the namespaces of the imported documents.

  1. Namespaces when overriding existing documents Assuming a user has the necessary permissions, importing a document with _id: 1234 will override the existing document with the same id. Depending on the reason for the import, we believe users can benefit from three different strategies for how the namespaces of the document are treated:
    1. Override the destination namespaces with the source document’s namespaces, effectively removing from existing or sharing to new namespaces.
      1. Perfect backup/restore scenario, after the import these objects will be 100% the same as when they were exported including which namespaces it is shared with.
      2. Requires the most privileges:
        1. at least one existing space
        2. all new spaces
        3. all spaces being removed
      3. Probably best suited to administrators
    2. Take the union of the namespaces, potentially sharing the document to new namespaces but never removing it from namespaces it’s currently shared to.
      1. Restores your backup to at least the same spaces it was shared with before.
      2. Requires less privileges:
        1. at least one existing space
        2. all new spaces
    3. Ignore the source namespaces completely, the document’s contents will be overwritten, but it remains shared to exactly the same namespaces as it was before the import
      1. If there’s no existing document, import it into the default namespace, or a specified namespace
      2. Requires the least privileges:
        1. at least one existing space
      3. Essentially what import is today, except some additional behaviour meant to keep backwards compatibility with how import worked before multi namespace saved objects
        1. if there’s a existing object with the same originId as the source saved object, we override that object
  2. When namespaces aren’t available (e.g. importing from 7.x)
    1. the document’s contents will be overwritten, but it remains shared to exactly the same namespaces as before
thomheymann commented 2 years ago

Linking back to my comment regarding a recent PR that fixed 3 import bugs and surfaced a new bug since it's related to the discussion here: https://github.com/elastic/kibana/pull/121046#pullrequestreview-850164754

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.

pgayvallet commented 2 years ago

removing my assignment as I'm no longer actively working on this one