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

Introduce HttpApi and HttpResource services #44620

Closed
mshustov opened this issue Sep 2, 2019 · 8 comments
Closed

Introduce HttpApi and HttpResource services #44620

mshustov opened this issue Sep 2, 2019 · 8 comments
Labels
discuss Feature:http Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0

Comments

@mshustov
Copy link
Contributor

mshustov commented Sep 2, 2019

Kibana should have the ability to distinguish between API calls/resource loading, as they have different URL rules and authorization rules.
For this, we are going to provide 2 different services working on top of the HTTP service.

HttpApiService

This would expose a createRouter function that is already prefixed with /api/<plugin-name>. Supports multiple routes registering (more details #44174)
In the future, this contract would evolve to be Open API oriented.

httpApiService.add([{
  method: 'get',
  path: 'resource-name',   // served under `/api/{pluginId}/resource-name`
  handler(){}
}]);

HttpResourceService

There are 2 different cases for static resources:

  • HTML page serving. We need to call a function to collect data and populate HTML page. Kibana configures the page with necessary headers (for example, CSP, CORS, etc.). HTML pages may be served from a "root-level" URL /my-page/.
  • static resources serving, like images/archives/etc. This use-case may use static file handlers in route declaration HTTP Service supports static files serving #41955 under the hood. We don't need a route handler, but a way to declare a file to serve / a directory to serve from.
httpResourceService.add({
  path: 'my-page', // server under '/my-page/'
  fromRoot: true,
  handler(){}
});
httpResourceService.add([{
  path: 'resource-name', // server under '/{pluginId}/resource-name/'
  directory: ...
}]);
Original description

In New platform, we force plugins to defines their routes prefixed with plugin id (`/security/api` for example). When plugin migrates to New Platform, it will introduce breaking changes in minor versions, because plugin api will be moved from `api/{pluginName}/...` to `{pluginName}/...` . Also, there are page routes like `/login` and `/logout` that we can't be moved at least until major version as well.

I originally planned to force prefixed url registration in the new platform and set up a proxy to legacy url for BWC. But on the 7.5 planning meeting, we discussed that we are going to drop server side of the Legacy platform ~v7.7. So now I'm inclining towards temporarily introducing an additional route configuration option that allows creating a route relative to the root url without plugin id prefix.

router.get({
    isRelativeToRoot: true,
    path: 'api/security/...',
    validate:...
  },
  async (context, req, res) => ...
);

This option will be used only for BWC as a part of #40768. We can safely remove it from a public interface just before v8 release.

Any other ideas?

@mshustov mshustov added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Sep 2, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov changed the title Allow plugins registering a global routes Allow plugins registering global routes Sep 2, 2019
@mshustov mshustov changed the title Allow plugins registering global routes Allow plugins registering unprefixed route handlers Sep 2, 2019
@epixa
Copy link
Contributor

epixa commented Sep 3, 2019

What do you think about creating two lightweight services in front of http and exposing them to plugins instead of http directly? Both would use the HttpService behind the scenes.

APIService

This would expose a createRouter function that is already prefixed with /api/<plugin-name>. It is essentially the same as we do with http today but it includes the api prefix. In the future, this contract would evolve to be Open API oriented.

BrowserResourceService

I'm open to naming on this one, but it would effectively be a GET-only router interface over the HTTPService with the ability to optionally define a "root" resource.

@mshustov
Copy link
Contributor Author

mshustov commented Sep 4, 2019

@epixa It makes a lot of sense and aligned with an approach proposed in #21424

APIService
This would expose a createRouter function that is already prefixed with /api/. It is essentially the same as we do with http today but it includes the api prefix. In the future, this contract would evolve to be Open API oriented.

Do we want to distinguish between public and internal API endpoints? as discussed in #21424. I'd assume we want to use OpenAPI for both, but without BWC guarantees for internal API. In this case, we may want to use different prefixes for them.

httpAPIService.add({
  path: 'resource-name',   // served under `/api/{pluginId}/resource-name`
  public: true,
  handler(){}
});

BrowserResourceService
I'm open to naming on this one, but it would effectively be a GET-only router interface over the HTTPService with the ability to optionally define a "root" resource.

There are different cases for static resources:

  • A plugin provides an HTML page. Usually, we need to call a function to collect data and populate HTML page.
  • A plugin provides static resources, like images/archives/etc. This use-case may use static file handlers in route declaration HTTP Service supports static files serving #41955 under the hood. Here we don't need a route handler, but a way to declare a file to serve / a directory to serve from. Are there use-cases to serve something from "root"?
httpResourceService.add({
  path: 'resource-name', // server under '/resource-name/'
  fromRoot: true,
  handler(){}
});
httpResourceService.add({
  path: 'resource-name', // server under '/{pluginId}/resource-name/'
  directory: ...
});

@elastic/kibana-platform

@epixa
Copy link
Contributor

epixa commented Sep 4, 2019

Do we want to distinguish between public and internal API endpoints?

Personally, I'm in favor of this, but from a pragmatic standpoint, I think it's best if we decouple that effort from this. We know in 7.x we need to support a more free-form route definition that isn't tied to Open API, and ease-of-migration seems paramount in the short term. I do think having the ApiService already exist is a step in the right direction though. What do you think?

A plugin provides an HTML page. Usually, we need to call a function to collect data and populate HTML page.

I'm not aware of any plugins providing straight HTML pages at the moment. I'm aware of four places where we use a root resource today: /status, /login, /logout, /goto/*.

  • Status renders a special app bundle. I didn't dig into this logic, but it probably makes sense for status to be in core anyway, so we could just leverage the internal services for this.
  • Goto is the redirect route from the short url service. It's not clear to me whether this should be part of core or not, but it is important that we keep the /goto/* route around otherwise we break all existing short URL links.
  • Logout responds with the result of renderAppWithDefaultConfig, which IIRC is essentially just loading src/core/public without bothering to load custom advanced settings from Elasticsearch.
  • Login has logic for conditionally redirecting or responding with renderAppWithDefaultConfig.

I think I'd be happy with most solutions that keep these things working and makes migration as easy as possible. Either way, having a dedicated service for this seems to make sense, no?

A plugin provides static resources, like images/archives/etc. [...] Are there use-cases to serve something from "root"?

I don't see a concrete reason to allow root-level static resources, or at least I'm not familiar with any such usage today. I think we have traditionally not put constraints on how plugins expose their static resources, but that seems like a sensible thing to do down the line. Again, so long as migration is easy, I don't think we can go wrong here.

@mshustov
Copy link
Contributor Author

mshustov commented Sep 4, 2019

I do think having the ApiService already exist is a step in the right direction though. What do you think?
I think I'd be happy with most solutions that keep these things working and makes migration as easy as possible. Either way, having a dedicated service for this seems to make sense, no?

Yes, maintaining different route declarations for

  • API endpoints
  • dynamic resources (HTML pages)
  • static resources (assets)
    is rather awkward.
    So I'd go on with proposed approach to differentiate them via separate services.

@joshdover
Copy link
Contributor

I'm a proponent of the different services approach as well. Though they are all built on HTTP, the concerns of each area are different and we have plans to evolve at least one of those (OpenAPI) pretty extensively.

In the short term, how do we feel about changing the createRouter function that is exposed by HttpSetup to include the /api/<pluginId> prefix now to unblock #44664?

@mshustov mshustov changed the title Allow plugins registering unprefixed route handlers Introduce HttpApi and HttpResource services Sep 6, 2019
jfsiii pushed a commit to jfsiii/kibana that referenced this issue Sep 6, 2019
The server plugin isn't ready to be full NP yet (i.e. "ManagerPlugin implements Plugin", etc) but we don't need lots of features that would provide anyway (e.g. defining contexts, etc).

If we change the plugin to only accept a router instance, we can get get NP router and pull the elasticsearch & savedobject clients (when SO client merges) off the new `context` value in the handlers.

The big thing I want to wait for is the support for `router.route` that's coming in elastic#44620
@joshdover
Copy link
Contributor

We should leverage this service to enforce the API conventions listed in the styleguide:

  • API routes must start with the /api/ path segment, and should be followed by the plugin id if applicable:
  • Kibana uses snake_case for the entire API, just like Elasticsearch. All urls, paths, query string parameters, values, and bodies should be snake_case formatted.

@crob611
Copy link
Contributor

crob611 commented Jan 28, 2020

Canvas had some issues on Cloud with serving a static resource that we have that is created as part of the build process. Wanted to make sure we don't run into a similar issues when we move to using Resource Service in the future.

See #49538 for what the final solution was and links to the issues we went through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:http Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

No branches or pull requests

6 participants