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

Migrate Kibana plugin "System API" to New Platform #43970

Closed
azasypkin opened this issue Aug 26, 2019 · 6 comments · Fixed by #53734
Closed

Migrate Kibana plugin "System API" to New Platform #43970

azasypkin opened this issue Aug 26, 2019 · 6 comments · Fixed by #53734
Assignees
Labels
blocker enhancement New value added to drive a business result Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@azasypkin
Copy link
Member

Both Security and Reporting plugins use addSystemApiHeader and isSystemApiRequest methods exposed by the Kibana legacy plugin. And we have to use various workarounds to access these methods from the new platform plugins.

It feels like something that belongs to platform, but not sure how exactly it should be represented there.

cc @restrry @kobelb

@azasypkin azasypkin added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform enhancement New value added to drive a business result labels Aug 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@kobelb
Copy link
Contributor

kobelb commented Aug 26, 2019

I believe that isSystemAPIRequest is only consumed to allow us to determine when we should be extending the user's session. We don't want non-user initiated background XHR's to extend their session timeout. Now that sessions are part of the OSS NP, perhaps we should at least consider renaming these headers and the method names?

@mshustov
Copy link
Contributor

depend on #39330
although we can start working on it separately.

Now that sessions are part of the OSS NP, perhaps we should at least consider renaming these headers and the method names?

Shall we? kbn-system-api is already a part of OSS, although only x-pack plugins set it.
https://github.com/restrry/kibana/blob/2e279aa2bdb194897b7187c91de20b36d95708f6/src/legacy/ui/public/system_api/system_api.js#L43
https://github.com/restrry/kibana/blob/2e279aa2bdb194897b7187c91de20b36d95708f6/src/legacy/core_plugins/kibana/server/lib/system_api.js#L29

It feels like something that belongs to platform, but not sure how exactly it should be represented there.

same here. it looks like Kibana specific information, not directly related to the HTTP layer, so it could be a part of the HTTP Service contract. http.isSystemApi(request).
OTOH we use it only to identify where an incoming request belongs to and probably most developers expect KibanaRequest to provide this information. In this case, we have to unify interfaces for both server and client-side Request object, because isSystemApiRequest is used on both sides.

Shouldn't we get rid of addSystemApiHeader method on the client and provide a separate HttpFetchOptions option to distinguish between calls? @eliperelman

@joshdover
Copy link
Contributor

@restrry I agree on a lot of points here.

It seems that the mechanics of how system api is communicated between the client and server should be concealed from plugins. I think this can easily be solved with:

  • systemRequest: boolean property on HttpFetchOptions on the client.
  • isSystemRequest(): boolean property on both the KibanaRequest on the server and the equivalent on the client.

@joshdover
Copy link
Contributor

I've hit one major snag here. The client-side HttpService's fetch method returns a Promise<any> which is the body of the response. This makes adding any additional metadata very difficult since we cannot easily add a property. While changing the HttpService's interface itself to add a object layer in-between is not difficult, it's used in many many places already.

Though this will be a lot of work, I think we will have other needs to add properties and methods to the response object returned here. Given that, I'm going to attempt this refactor, but it is risky.

@mshustov
Copy link
Contributor

mshustov commented Dec 6, 2019

@joshdover I tripped me up in the licensing plugin as with the current fetch implementation is impossible to access response headers. So 👍to extend this functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker enhancement New value added to drive a business result Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants