Introduce HttpResource service #50654

mshustov · 2019-11-14T14:32:36Z

Discussion in #44620

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', // served under '/my-page/'
  fromRoot: true,
  handler(){}
});
httpResourceService.add([{
  path: 'resource-name', // served under '/{pluginId}/resource-name/'
  directory: ...
}]);

Plan

move static assets serving to the New platform Serve static assets from NP #60490
add API to respond with a dynamic asset
add API to respond with the CoreApp HTML page [NP] add http resources sub-service #61797
add API to respond with a custom HTML page [NP] add http resources sub-service #61797

The text was updated successfully, but these errors were encountered:

elasticmachine · 2019-11-14T14:32:38Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-01-23T14:33:12Z

Should this be a sub-service of http instead of a whole new service ?

http.resources.add({
  path: 'my-page', // server under '/my-page/'
  fromRoot: true,
  handler(){}
});

mshustov · 2020-03-16T16:47:01Z

Should this be a sub-service of http instead of a whole new service ?

Let's look at HttpServiceSetup . If we want to have a common set of interceptors, auth/base path utils, etc. it sounds reasonable. However, we need to remove access to a plain router and provide HttpResource & HttpApi services instead.

HttpResource service use-cases:

To serve a static HTML page to bootstrap the Kibana app on the client.

via url
Example 1
Proposed API:

HttpResourceSetup.registerEntryPoint: ({ path: string }) => void

after performing some logic within a route handler
Example:1 2
Proposed API: extend ResponseFactory interface to render the core app:

http.resources.add({path: '/', validate:..., options: {auth:...}}, (context, req, res) => {
  return res.app.render()
})

to serve a dynamic HTML page.

Examples: 1 2
Proposed API: extend ResponseFactory interface to render a custom app:

http.resources.add({path: '/', validate:..., options: {auth:...}}, (context, req, res) => {
  return res.app.render({
    // will add CSP & 'cache-control': 'private, no-cache, no-store' headers
    body: `<!DOCTYPE html> ...`
  })
})

to serve static assets from a directory.

icons, images, fonts, etc.
Examples: 1 2 3
Proposed API:

HttpResourceSetup.registerStaticDir: ({ path: string, dirPath: string }) => void

It can be an internal API. We can enforce a convention for plugins to provide assets via the public/assets folder.

to serve dynamic assets

Examples:

js scripts 1 2 . Ignore, that they are prefixed with api. Fix is in flight https://github.com/elastic/kibana/pull/53010/files#diff-c806328f0fd88063a770cab12b512135L18
js scripts. canvas case
Proposed API: extend ResponseFactory interface to respond with an asset:

http.resources.add({path: '/', validate:..., options: {auth:...}}, (context, req, res) => {
  // will add etag header
  return res.assets.custom({
    body: '..',
    headers: { 'content-type': 'text/javascript' }
  })
})

joshdover · 2020-03-16T17:42:58Z

A sub-service / API makes sense to me. I think it makes finding these features much more ergonomic.

One thing that is a little inconsistent with these proposed APIs is that registering HTTP routes happens on a IRouter instance, rather than directly on the core.http object. Should we introduce these APIs on the IRouter interface for consistency?

Alternatively, what if we changed the http.createRouter().get(...) API to simply be http.router.get(...) (a router is created for the plugin automatically). This could be done later or as a follow up probably.

On the specific proposals above, I think they all make sense to me except the res.app.render() method:

http.resources.add({path: '/', validate:..., options: {auth:...}}, (context, req, res) => {
  return res.app.render()
})

http.resources.add({path: '/', validate:..., options: {auth:...}}, (context, req, res) => {
  return res.app.render({
    // will add CSP & 'cache-control': 'private, no-cache, no-store' headers
    body: `<!DOCTYPE html> ...`
  })
})

These two usages actually are different enough that I think they should be separate functions. The first renders the Core bootstrapping HTML page, while the second renders a custom HTML document. The only thing they both do is include the standard headers (CSP, cache-control). I also don't think the function for rendering an HTML document should be in the app namespace since it's not really an application in the way we use that term elsewhere.

Two APIs would make more sense to me: res.renderCoreApp() and res.renderHtml({ body: "<!DOCTYPE html> ..." }).

mshustov · 2020-03-31T17:35:44Z

HttpResource & HttpApi service are supposed to be built on top of HttpService. However, adding additional helpers to KibanaResponseFactory only for HttpResources makes it incompatible with RequestHandler and HttpServiceInterceptors. Which in turn makes it impossible to share helpers for http-related API (for example, router.handleLegacyErrors https://github.com/elastic/kibana/blob/3bdbcd0d1a1a2a44bef3e006c6b596a282ea11c0/src/core/server/http/router/error_wrapper.ts, license checker #56926)
There are 3 possible solutions:

to duplicate helpers for different RequestHandler within HttpResource / HttpApi services. We aren't going to add HttpApi service soon, so we will have to have different helpers for HttpResources service and Router.
to introduce HttpResource service helpers (renderCoreApp, renderHtml, etc) in KibanaResponseFactory. That would require significant refactoring, as HttpService is a low-level service and doesn't depend on the other services (uiSettings, rendering, etc).
to use existing HttpRequestContext pattern to expose HttpResources API. The discoverability of the API is the downside of this approach.

I'd say that I'm fine to duplicate helpers for RequestHandler, but it means more work for plugins during migration. From a practical point of view, exposing HttpResources API via HttpRequestContext sounds like the most straightforward approach.

joshdover · 2020-03-31T17:58:40Z

I'd say that I'm fine to duplicate helpers for RequestHandler, but it means more work for plugins during migration.

What extra work would there be for plugins? Is this the problem with route wrappers with different router types that may have different arguments?

Seems like it may be possible to create a wrapper that works for routers with different response factories:

type GenericRequestHandler<P, Q, B, Method, RouterArgs extends any[]> = (
  req: KibanaRequest<P, Q, B, Method>,
  ...args: RouterArgs
) => KibanaResponse | Promise<KibanaResponse>;

const wrapRoute = <P, Q, B, Method, RouterArgs extends any[]> = (
  handler: GenericRequestHandler<P, Q, B, Method, RouterArgs>
) => GenericRequestHandler<P, Q, B, Method, RouterArgs> {
  return (req, ...args: RouterArgs) => {
    // do pre-route things here
    // then run wrapped handler
    return handler(req, ...args);
  }
}

This pattern could also be extended for wrapping the entire IRouter interface(s). Though of course, changing the existing wrappers to work with this is still more work.

Is there something else about option (1) that creates more work for plugins?

Even still, I'm ok with leveraging HttpRequestContext for the time being and changing this later if it saves us significant amounts of time. Just not sure if changing this in the future is going to be a headache.

pgayvallet · 2020-04-01T06:29:08Z

However, adding additional helpers to KibanaResponseFactory only for HttpResources makes it incompatible with RequestHandler and HttpServiceInterceptors. Which in turn makes it impossible to share helpers for http-related API

Just looked at https://github.com/elastic/kibana/pull/61797/files, and I was wondering if (yet another) layer of generics couldn't solve this issue

atm

export type RequestHandler<
  P = unknown,
  Q = unknown,
  B = unknown,
  Method extends RouteMethod = any
> = (
  context: RequestHandlerContext,
  request: KibanaRequest<P, Q, B, Method>,
  response: KibanaResponseFactory
) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;

addition in PR

export type HttpResourcesRequestHandler<P = unknown, Q = unknown, B = unknown> = (
  context: RequestHandlerContext,
  request: KibanaRequest<P, Q, B, 'get'>,
  response: KibanaResponseFactory & HttpResourcesServiceToolkit
) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;

what about

export type RequestHandler<
  P = unknown,
  Q = unknown,
  B = unknown,
  Method extends RouteMethod = any,
  ResponseFactory extends KibanaResponseFactory = KibanaResponseFactory,
> = (
  context: RequestHandlerContext,
  request: KibanaRequest<P, Q, B, Method>,
  response: ResponseFactory
) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;

and

export interface HttpResources {
  registerCoreApp: [...],
  register: <P, Q, B>(
    route: RouteConfig<P, Q, B, 'get'>,
    handler: RequestHandler<P, Q, B, 'get', HttpResourcesResponseFactory>
  ) => void;
}

or even

type HttpResourcesRequestHandler<P = unknown, Q = unknown, B = unknown> = RequestHandler<P, Q, B, 'get', HttpResourcesResponseFactory>;

export interface HttpResources {
  registerCoreApp: [...],
  register: <P, Q, B>(
    route: RouteConfig<P, Q, B, 'get'>,
    handler: HttpResourcesRequestHandler<P, Q, B) => void;
}

with HttpResourcesResponseFactory being a concrete implementation of KibanaResponseFactory & HttpResourcesServiceToolkit

I think that would solve the incompatibility with the request handler wrappers or interceptor, can you confirm?
If it does, I'm still unsure adding additional RequestHandler generic types every time we have to enhance/inherit/adapt/reuse it for yet another usage is going to be very maintainable long term. wdyt?

mshustov · 2020-04-01T16:37:31Z

I think that would solve the incompatibility with the request handler wrappers or interceptor, can you confirm?

yeah, I don't want to overcomplicate the generics. However, it's not such a big problem if we provide wrapper type for a route handler from the core. Thus, plugins don't have to implement it manually
https://github.com/elastic/kibana/pull/61797/files#diff-6b2d2938215b66e48ab680c4f3515499R338

mshustov · 2020-04-17T10:52:00Z

The last piece of the task is to serve a dynamic asset from a route handler. that's used in the canvas plugin

kibana/x-pack/plugins/canvas/server/routes/shareables/download.ts

Line 25 in 8d54fa8

// const file = handler.file(SHAREABLE_RUNTIME_FILE, { confine: false });

The canvas plugin has a workaround in place. So we can switch to more urgent migration tasks meantime. In the future, we need to figure out how to solve the problem without using confine: false that looks like an error-prone approach.

lukeelmers · 2022-08-03T21:46:42Z

This has been sitting without activity for quite some time now, and all tasks have been addressed other than dynamic assets. As mentioned in #50654 (comment), canvas is working around this, so I'm going to close this for now. We can open a new issue in the future if more use cases for dynamic assets come up.

mshustov added blocker Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Nov 14, 2019

mshustov mentioned this issue Nov 14, 2019

Move HttpService functionality from Legacy to New platform #39330

Closed

mshustov self-assigned this Nov 19, 2019

flash1293 mentioned this issue Jan 23, 2020

Migrate tutorial resources #55710

Closed

mshustov mentioned this issue Mar 16, 2020

HTML page rendering service #41964

Closed

joshdover mentioned this issue Mar 16, 2020

Migrate CSP configuration #50644

Closed

3 tasks

mshustov mentioned this issue Mar 18, 2020

Serve static assets from NP #60490

Merged

2 tasks

mshustov mentioned this issue Apr 8, 2020

[NP] add http resources sub-service #61797

Merged

2 tasks

mshustov removed the blocker label Nov 5, 2020

mshustov removed their assignment Aug 25, 2021

lukeelmers closed this as completed Aug 3, 2022

lukeelmers added the Feature:http label Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce HttpResource service #50654

Introduce HttpResource service #50654

mshustov commented Nov 14, 2019 •

edited

Loading

elasticmachine commented Nov 14, 2019

pgayvallet commented Jan 23, 2020

mshustov commented Mar 16, 2020 •

edited

Loading

joshdover commented Mar 16, 2020

mshustov commented Mar 31, 2020

joshdover commented Mar 31, 2020

pgayvallet commented Apr 1, 2020 •

edited

Loading

mshustov commented Apr 1, 2020

mshustov commented Apr 17, 2020

lukeelmers commented Aug 3, 2022

Introduce HttpResource service #50654

Introduce HttpResource service #50654

Comments

mshustov commented Nov 14, 2019 • edited Loading

HttpResourceService

Plan

elasticmachine commented Nov 14, 2019

pgayvallet commented Jan 23, 2020

mshustov commented Mar 16, 2020 • edited Loading

HttpResource service use-cases:

To serve a static HTML page to bootstrap the Kibana app on the client.

to serve a dynamic HTML page.

to serve static assets from a directory.

to serve dynamic assets

joshdover commented Mar 16, 2020

mshustov commented Mar 31, 2020

joshdover commented Mar 31, 2020

pgayvallet commented Apr 1, 2020 • edited Loading

mshustov commented Apr 1, 2020

mshustov commented Apr 17, 2020

lukeelmers commented Aug 3, 2022

mshustov commented Nov 14, 2019 •

edited

Loading

mshustov commented Mar 16, 2020 •

edited

Loading

pgayvallet commented Apr 1, 2020 •

edited

Loading