[RFC][Search] Background sessions RFC

[lizozom]

Summary

RFC for the Background Sessions feature in Kibana

Replaces #69117

[kobelb]

Just the username isn't sufficient for these to be "per user"... @elastic/kibana-security if they were to store the username AND the realm, would that be sufficient?

[azasypkin]

@elastic/kibana-security if they were to store the username AND the realm, would that be sufficient?

The most unique thing we can get right now is username:realm-name:realm-type (or username:auth-provider-name:auth-provider-type). There is a risk that realm with the same name and type will be reconfigured to work with another IdP/user-store, but we consider this risk as low.

[lukasolson]

I know there is an API to get the current username, but is there a client-side API to get the realm and realm type?

[timroes]

From your chart in the architecture review I understood, that we have a cache (before sending something to background) mapping the request hash and the session id to the actual ES async id. And this cache entry will be stored in a saved object when we're sending it to background. When will an entry be removed from that cache? When it finished loading?

If we remove that when finished loading (or also in general) I'd be curious what happens in this scenario:

We load a dashboard and send it to background while some of the visualizations are already loaded and others are still loading. Will those finished requests of the visualization that have loaded already also been "send to background"? If not, won't all those visualizations that were already loaded when we send to background, fail as soon as we later watch the "all background queries loaded" dashboard, since they will create a cache miss (which fails the request as you explained yesterday). How are those visualizations adressed that are already loaded when we send to background, so they'll still work after we send to background and watch the dashboard later when we get the notification that it's ready?

[ppisljar]

provided some initial comments/questions

[kobelb]

Are there no code-paths in OSS that will call the methods which throw errors in OSS? Or is there some way for consumers of the ISessionService to know whether or not it's appropriate to expire, extend or list the search sessions?

[lukasolson]

We will need to make sure any places that call these methods have an appropriate catch block to handle when these methods are unavailable. This might be useful for showing some sort of "upgrade to a free license" pop-up or some other call to action.

[kobelb]

Will there be a way to distinguish between a not-implemented in OSS error versus other error conditions? Personally, I dislike APIs which throw errors that should always be caught, especially in languages like Javascript/Typescript which are lacking equivalent features to Java's checked exceptions.

[lizozom]

On the other hand, if an API don't return any response (like sessionService.restore(sessionId) for example), we won't know it failed.
What would be an alternative?

[kobelb]

Is my understanding correct that the url is the URL that the user should go to so that they may view the "thing" that created the background session? So if a specific Dashboard created the background session, this would be a URL to view this Dashboard? Does the //TODO comment signify something?

[lukasolson]

That's correct. I added a little more to this section, but @lizozom, did you have anything specifically you wanted to explore more here?

[kobelb]

The ISessionService co-mingles the methods that are associated with the "current session" and explicit sessions as identified by a sessionId. Perhaps we should create two interfaces instead of one?

Somewhat related, what's the expected lifetime of the ISessionService? My fear is that we end up re-using this service, which tracks a "current session", and create bugs by having two sessions overlap.

[lukasolson]

Hmm, totally agreed. @lizozom We might want to sync about this. It might make sense to have any method that is on the service itself require a sessionId, and then to have an actual class that represents the background session, and have the other methods (that don't require the sessionId on the object itself.

[ppisljar]

wasn't the idea to limit the scope to a single active session ?

[kobelb]

I don't really understand the section about the search interceptor. There are quite a few TODOs that really don't help. Is my understanding correct that the search_interceptor is currently an "implementation detail" of the Search Service, so developers will only be consuming the data.search service, which will internally use the search_interceptor transparently?

[lukasolson]

Yes, in fact @lizozom and I have talked about removing the SearchInterceptor entirely since it's just an implementation detail of the search service. We may want to just move these methods directly into the search service.

[ppisljar]

++

[ppisljar]

shouldn't the server and client new session service have same interface ?

[lukeelmers]

++ IMO we should keep these the same unless there's some reason not to.

[lukeelmers]

I don't see trackId here, but I'm guessing it would be in ISearchOptions?

[lukeelmers]

++ IMO we should keep these the same unless there's some reason not to.

[lukeelmers]

Is this supposed to be async?

Suggested change

	list: (options: SavedObjectsFindOptions) => BackgroundSessionAttributes[]
	list: (options: SavedObjectsFindOptions) => Promise<BackgroundSessionAttributes[]>

[kobelb]

This is looking great!

[kobelb]

Adding Background sessions as a top-level feature is rather awkward. Top-level features are generally applications that show up in the side-bar or Kibana specific management sections. As such, after #78152 merges, these will be grouped on the Spaces management screen and the Role management screen:

In my opinion, Background Sessions aren't really a Kibana, Observability, Security or Management feature.

We could potentially model background sessions as a sub-feature of the existing features. For example, this is how this would look for the Dashboard feature:

This avoids the awkwardness of the following:

when set to "read" or "none", the feature will be unavailable

It's entirely possible that I'm mis-understanding what we're trying to achieve here. Do we want to have background sessions available to some users but not others? Or are we just trying to provide a mechanism for users to turn off background sessions, if they don't want to incur the overhead?

[lizozom]

We do want BGS to be available to specific users.
However, I don't think that enabling them per application would be a logical way to enable them.
It's a cross application feature, like telemetry or notifications for example.

[kobelb]

Do we want existing users who only have all/read access to features like Dashboards to be able to create background sessions?

[kobelb]

If we're only concerned with these saved-objects showing up in the management listing, you don't have to make them hidden: true, you can just not declare the management key.

However, I do think that we'll want these to be hidden saved-objects for another reason. Is my understanding correct that we only want the user that created a background session to be able to CRUD their own background sessions? As far as I'm aware, this is only possible currently by declaring the saved-object as being hidden and writing a custom "Client" which performs its own authorization and filtering of these saved-objects.

[legrego]

However, I do think that we'll want these to be hidden saved-objects for another reason. Is my understanding correct that we only want the user that created a background session to be able to CRUD their own background sessions? As far as I'm aware, this is only possible currently by declaring the saved-object as being hidden and writing a custom "Client" which performs its own authorization and filtering of these saved-objects.

++ that's absolutely right. We discussed this here during an earlier iteration of this RFC

[lukasolson]

Yes, that's correct. We don't want these saved objects showing up in the general saved object management listing. We will have a separate, background-session-specific management view. The list will be filtered based on the userId described below, and will be managed through the expire/extend/etc. APIs described below.

[lizozom]

Thanks everyone for your reviews and suggestions!
The RFC has just been moved into its final comment period and it will be merged on Monday, unless no critical input is added.
🙇‍♀️

[kobelb]

After chatting with @lukasolson, the feature controls section has been removed from the RFC. It's still a discussion we need to have, and a decision should be made on how to integrate this with feature controls, sooner rather than later especially if it requires changes to our authorization model. However, RFCs won't ever specify 100% of the details of the final implementation, so we'll deal with this complication out-of-band of the RFC.

[ppisljar]

LGTM

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 73281 or prevent reminders by adding the backport:skip label.