Saved-object export custom logic

[kobelb]

When alerts and actions are exported their secrets and API keys are intentionally excluded from the export. As such, when these saved-objects are imported, it will require additional effort from the end-user to make these alerts and actions active. The end-user will be warned of this additional requirement per #84977, but we'd like to perform additional logic on export so that these alerts and actions are explicitly marked as disabled. Performing this custom logic on export is safer than doing so on import, because if it fails on import the user will potentially only have some of their saved-objects persisted.

This will also allow us to work-around some of the limitations incurred from using saved-object references to model a composition relationship as originally discussed in #82064. For example, when a case is export we'd like to use the custom logic hook to automatically include the related comments, etc.

[elasticmachine]

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

[pgayvallet]

Thinking out loud here:

First, I guess hooks requiring user interaction are out of the question here right? we are only speaking of 'standalone' hooks without prompt to the user or anything?

For example, when a case is export we'd like to use the custom logic hook to automatically include the related comments, etc.

Then I guess we need to support asynchronous hooks, right? As the hook would need to use the SO api to fetch the additional objects to include in the export.

I'm also thinking of the format of such API:

I was thinking of adding something like (don't mind the naming)

interface EnhancedObjectExport {
  obj: SavedObject, // allow to modify the object before export
  additionalObjects?: SavedObject[], // allow to include other objects for export
}

SavedObjectsType.onExport(obj: SavedObject): EnhancedObjectExport | Promise<EnhancedObjectExport>

However

• the type registration is performed during setup, so the SO client is not accessible when registering the hook
• also we probably want a scoped client / context for the hook, so we need to provide the request or at least something similar to core's RequestHandlerContext

We could maybe just use the same approach and add an additional context parameter to the hook?

interface ExportContext {
    savedObjects: {
        client: SavedObjectsClientContract;
    }
   // and more
}

SavedObjectsType.onExport(obj: SavedObject, context: ExportContext): EnhancedObjectExport | Promise<EnhancedObjectExport>

But maybe some hooks logic would require to access the owner plugin's API or service, in which case this would not be good enough.

We could just propagate the 'raw' RequestHandlerContext to the hook, but that's bad isolation imho.

Also work noting that once we find the correct pattern for an export hook, adding the equivalent API for import should be rather trivial.

[kobelb]

First, I guess hooks requiring user interaction are out of the question here right? we are only speaking of 'standalone' hooks without prompt to the user or anything?

Correct.

Then I guess we need to support asynchronous hooks, right? As the hook would need to use the SO api to fetch the additional objects to include in the export.

👍

With regard to the API design, I defer to your alls expert opinions :)

[rudolf]

But maybe some hooks logic would require to access the owner plugin's API or service, in which case this would not be good enough.

The types of plugins that require custom export hooks are most likely the ones with slightly more complex domains. So I think this should be the preferred way plugins implement their export hook, by calling a service which contains their business logic. But this means they will probably be calling start lifecycle services.

Plugins could "hack" around the lifecycles, but maybe this should be a start lifecycle API. It's not so nice that you have to make several calls to the core savedObjects service, but it feels like this would still be a better developer experience.

This does mean there's theoretically a time where Kibana could accept an incoming request to export saved objects before the custom export logic has been registered. But with sync lifecycles this should only be one tick and we could in the future only start the server one tick after start.

[pgayvallet]

Plugins could "hack" around the lifecycles, but maybe this should be a start lifecycle API. It's not so nice that you have to make several calls to the core savedObjects service, but it feels like this would still be a better developer experience.

Exposing this new hook API on start contract would avoid the getStartServices trick, however there is still the problem that the custom logic may (will) need to access a scoped ES or SO client to fetch the additional objects they need to add to the export (I'm thinking about #82064 which is one of the two usecases we need to address here).

So basically, even if we do provide the hook API on the start contract, we will still need to pass down the request to the hook to allow them to properly scope their clients. Given that, I wonder if exposing the RequestHandlerContext associated with the performing request wouldn't be the easiest solution.

That's the SavedObjectsType.onExport(obj: SavedObject, context: ExportContext): EnhancedObjectExport | Promise<EnhancedObjectExport> proposal from #84980 (comment)

That way, the hook would still belongs to the type definition, and consumers could just use their registered context providers to retrieve their services.

The other option, that is closer to your proposal, would be to have this new hook API exposed on the start contract, and have the ExportContext be its own type, containing the request, and (eventually) preconfigured ES and SO clients.

@rudolf wdyt about that?

[rudolf]

From our recent sync discussion...

Although a request scoped ES/SO client might be useful, plugins will primarily be fetching hidden saved objects which isn't possible from a scoped SO client (unless we change that in #82027)

[pgayvallet]

After thinking a bit more about it, I have a few remarks:

1. We probably want a single hook call with all the objects of a given type instead of per-object hook call

I'm think about the case + case-comment use case here. If we go with something like

onExport(obj: SavedObject): EnhancedObjectExport

the hook registrant will have to perform one ES request for each individual case object during the export to fetch the objects associated with each case, where if we go with

onExport(objs: SavedObject[]): EnhancedObjectExport

the registrant would be able to perform a single request for all the case objects to retrieve their associated comments, which seems way better in term of performances.

2. Regarding exposing the hook API on the setup contract vs the start one

I hear the fact that exposing the API on start would probably be a slightly better DX, as the services that will be required by the hook's logic may only be available once the start lifecycle is performed. However, this is not something really specific to this feature. We already have similar problem for other code snippets that needs to be registered during setup but will not be executed until the plugins start begins (or even ends). I think the getStartService 'issue' is very valid, but should probably be left apart from the discussion here, and handled / discussed separately, as it impacts multiple (well, even a lot) of core APIs. Which is why I think we should stick with exposing this API during setup exclusively.

3. Regarding hook registration via the type registration vs distinct API

If in previous point, I exposed that imho, we should have this hook API exposed on the setup contract. However, we may want to still have a distinct API (dissociated from the type registration) for hook registration. The main reason for that being that the recommended pattern to register SO types looks like

import { myObjectType } from './saved_objects';
// ... in setup
core.savedObjects.registerType(myObjectType)

Having the hook registered via SavedObjectType would force some refactoring of such registration to something like

import { createMyObjectType } from './saved_objects';
core.savedObjects.registerType(createMyObjectType(core.getStartContract, () => this.myService))

instead of

import { myObjectType } from './saved_objects';
// ... in setup
core.savedObjects.registerType(myObjectType)
core.savedObjects.registerExportHook(myObjectType.type, createMyObjectExportHook(core.getStartContract, () => this.myService))

Note: I'm not 100% sure on that one. Having the hook registered via the type definition would restrict hook registration to the type owner, which makes a lot of sense, and would also restrict one hook (max) per type, which is also something we probably want (multiple hooks transforming the exported objects seems unneeded and unnecessary complex to handle). Does anyone have a stronger opinion on that one?

4. Regarding the actual hook API / return value

The proposed API was

interface EnhancedObjectExport {
  obj: SavedObject, // allow to modify the object before export
  additionalObjects?: SavedObject[], // allow to include other objects for export
}

// or SoServiceSetup.registerExportHook
SavedObjectsType.onExport(obj: SavedObject, ctx: ExportContext): EnhancedObjectExport | Promise<EnhancedObjectExport>

however, if we follow my first point, and want a hook signature taking all the objects of a given type instead, I'm unsure what we should have. Maybe we just want the hook to return a replacement list that will effectively be used instead of the initial objects for the type?

SavedObjectsType.onExport(objs: SavedObject[], ctx: ExportContext): SavedObject[] | Promise<SavedObject[]>

with a naive implementation of the exporter code looking like:

let actuallyExported: SavedObject[];
const types = splitByType(exportedObject)
for ([type, objects] of types.entries()) {
  actuallyExported = [
     ...actuallyExported,
     ...(hasHook(type) ? await typeHook(objects) : objects)
  ]
}

does anyone have a better idea?

5. Regarding the export context

As already discussed, we need to provide a context to the hook, at minima the current request, to let the consumers eventually create their scoped clients / service.

However as @rudolf mentioned here, it is likely that plugins will be fetching hidden types, making preconfigured SO clients not that useful. Which is why I think only exposing the request (at least initially) should be good enough, and would avoid adding overcomplicated routehandler-like registrable context, at least for the initial implementation.

@rudolf @kobelb wdyt?

[pgayvallet]

I just created #87807 as a POC of the proposed implementation. Feedback would be greatly appreciated.

[rudolf]

@XavierM based on the way cases is implemented, do you have any feedback on the proposed API?

[rudolf]

1. We probably want a single hook call with all the objects of a given type instead of per-object hook call

+1

2. Regarding exposing the hook API on the setup contract vs the start one

Yeah I'm fine with going with setup for now and working on the developer experience when we move to sync lifecycles and try to reduce the amount of getStartServices calls.

3. Regarding hook registration via the type registration vs distinct API

Don't have a strong opinion either, but I lean towards a single API call because of the benefits you mentioned. Ultimately plugins can easily work around the need to refactor with something like:

core.savedObjects.registerType({...mySavedObjectType, onExport: this.onExport})

4. Regarding the export context

+1 We can schedule some follow-up work once we have two or more plugins using this API to make improvements if necessary and expanding the export context later won't be a breaking change.

[pgayvallet]

Don't have a strong opinion either, but I lean towards a single API call because of the benefits you mentioned.

Yea, after doing the POC for both the import and export hooks, I also think this is slightly better. Will need to update both PRs I guess, as I went the other way in both 😅

Ultimately plugins can easily work around the need to refactor with something like core.savedObjects.registerType({...mySavedObjectType, onExport: this.onExport})

That's a good remark. The fact that these are optional allows to exclude it from the 'static' type definition and add it on the fly during the registration from the plugin code if owners need to.