[NP] add http resources sub-service #61797

mshustov · 2020-03-30T12:59:25Z

Summary

Parent issue #50654
This PR introduces HttpResources services built on top of HTTP + rendering services. It provides API allowing plug-ins to respond on an incoming request with:

a pre-configured HTML page bootstrapping Kibana client app
custom HTML page
custom JS script file

With these changes rendering is becoming an internal service and should not be directly used by the plugins.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dev docs

Shall your server-side plugin needs to respond to an incoming request with HTML page bootstrapping Kibana client app, a custom HTML page or a custom JS script, you can use HttpResources service to achieve that.

httpResources.register({ path: 'my_app', validate: false }, (context, req, res) =>
  res.renderCoreApp()
);

httpResources.register({ path: 'my_app/foo', validate: false }, (context, req, res) =>
  res.renderHtml({ body: '<html><p>Hi</p></html>' })
);

httpResources.register({ path: 'my_app/bar', validate: false }, (context, req, res) =>
  res.renderJs({ body: 'alert(...);'})
);

elasticmachine · 2020-03-30T12:59:27Z

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

src/core/server/server.ts

src/legacy/server/kbn_server.d.ts

x-pack/plugins/security/server/routes/views/logout.ts

src/core/server/http/router/headers.ts

src/core/server/http_resources/http_resources_service.ts

pgayvallet · 2020-04-03T16:24:56Z

src/core/server/legacy/legacy_service.ts

-        createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
+        createRouter: () => router,


This is not really a create anymore, but I don't think using a single instance have any negative impact so it looks fine to me.

Agree. I believe we can refactor it to a router static property. We can rename to getRouter if we prefer a mockable function. WDYT? That could be done in a separate PR.

This change was done only in the CoreSetup instance we are using in the legacy service right? renaming it or exposing the router property would make it diverge from the plugins CoreSetup interface, so I think it's alright to keep it that way.

Spoke too fast, the same change is done in src/core/server/plugins/plugin_context.ts. Changing to router/getRouter makes sense then I guess. I would go to getRouter personally, even if we are using both getters and direct property access on some of our APIs. Can be done at a later time

pgayvallet · 2020-04-03T16:59:01Z

src/core/server/server.ts

+    const renderingSetup = await this.rendering.setup({
+      http: httpSetup,
+      legacyPlugins,
+      uiPlugins,
+    });
+
+    const httpResourcesSetup = this.httpResources.setup({
+      http: httpSetup,
+      rendering: renderingSetup,
+    });


This httpResource-> rendering-> http dependency blocking httpResource to be a sub-service of http is a shame.

(just thinking) Maybe rendering should be (at least internally) at sub-service of http, as it's actually only used to render http-related things, but that's not a priority, and current implem is fine.

rendering service in the current implementation depends on plugin & uiSettings services. I don't think low-level services should have tight coupling. Rendering service is an application-specific service, and it's one step up in architecture.

x-pack/plugins/security/server/routes/authentication/oidc.ts

x-pack/plugins/security/server/routes/views/logged_out.ts

mshustov · 2020-04-08T14:09:09Z

src/core/server/http_resources/integration_tests/http_resources_service.test.ts

+        const { http, httpResources } = await root.setup();
+
+        const router = http.createRouter('');
+        const resources = httpResources.createRegistrar(router);


I'm not happy that we have to duplicate the logic from plugin_context file, but it's simpler than adding a separate plugin for tests.

mshustov · 2020-04-08T14:11:17Z

src/core/server/http_resources/integration_tests/http_resources_service.test.ts

+        expect(response.header['x-kibana']).toBe('42');
+      });
+
+      it('can adjust route config', async () => {


This test used to test types. Should be removed as soon as we've got dedicated toolset for this #53762

pgayvallet · 2020-04-09T09:22:09Z

Ack: will review today

pgayvallet

Some minor NITs and a few question, overall LGTM in current state.

src/core/server/http/router/router.ts

pgayvallet · 2020-04-09T12:46:28Z

src/core/server/http_resources/http_resources_service.test.ts

+          });
+        });
+
+        it('can attach headers, except the CSP & "content-type" headers', async () => {


(for all registrar methods) is ignoring the forbidden headers silently what we want or should we throw an error if they are provided?

We shouldn't throw an exception as it's not a critical problem. Is it worth logging a warning message instead, maybe?

pgayvallet · 2020-04-09T12:48:58Z

src/core/server/http_resources/http_resources_service.ts

+export class HttpResourcesService implements CoreService<InternalHttpResourcesSetup> {
+  private readonly logger: Logger;
+  constructor(core: CoreContext) {
+    this.logger = core.logger.get('http-resources');


Internally it's a distinct service, but publicly a sub service of http -> http-resources or http.resources prefix?

The service shouldn't know about its place in the hierarchy, that's why it's nothttp.resources. Probably, the platform is interested in the internal service logs. Thus we are fine to keep it as http-resources or httpResources?

src/core/server/http_resources/integration_tests/http_resources_service.test.ts

pgayvallet · 2020-04-09T13:09:03Z

src/core/server/legacy/legacy_service.ts

-        createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
+        createRouter: () => router,


Spoke too fast, the same change is done in src/core/server/plugins/plugin_context.ts. Changing to router/getRouter makes sense then I guess. I would go to getRouter personally, even if we are using both getters and direct property access on some of our APIs. Can be done at a later time

pgayvallet · 2020-04-09T13:28:15Z

x-pack/plugins/security/server/routes/views/index.test.ts

+    expect(routeParamsMock.httpResources.register.mock.calls.map(([{ path }]) => path))
+      .toMatchInlineSnapshot(`
      Array [
-        "/security/account",
        "/security/logged_out",
-        "/logout",
-        "/security/overwritten_session",
      ]
    `);
+    expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot(
+      `Array []`
+    );


Not sure to understand why some routes are no longer in either assertions?

I'm confused by this too

Because /security/account & /security/overwritten_session use httpResources.registerCoreApp shorthand helper.
While /logout uses registerAnonymousCoreApp helper.
I didn't add them since the test case name states only /login route check.
To note: routeParamsMock is called for Login public API endpoints.

That shows that we violate one of our development principles: Prefer one way to do things . Lets probably remove registerCoreApp & registerAnonymousCoreApp shorthands?

That shows that we violate one of our development principles: Prefer one way to do things . Lets probably remove registerCoreApp & registerAnonymousCoreApp shorthands?

I think that makes sense, but I'll defer to your team on that

@legrego removed

joshdover

High-level design and test coverage looks great, few nits 👍

src/core/server/http_resources/http_resources_service.ts

src/core/server/http_resources/types.ts

test/plugin_functional/plugins/rendering_plugin/server/plugin.ts

streamich

AppArch change LGTM.

legrego

Thanks for consolidating the API surface. Changes LGTM!

* add HttpResources basic implementation * expose http resources to plugins * add mocks * move http resources to a separate service * hide rendering service * adopt internal types * expose HttpResources service to plugins * update platform mocks * plugins start using HttpResources API * remove RenderingServiceSetup export * RenderingServiceSetup --> InternalRenderingServiceSetup * improve types * remove httpRespources leftovers from http service * remove rendering types from RequestHanlderContext * fix security plugin tests * add unit tests for httpResources service * add unit tests * remove outdated cache-control header * restructure http resources service * merge getUiPlugins and discover * static route declaration shouldnt require auth & validate * update docs * use HttpResources service instad of rendering * address comments * update docs * roll back unnecessary changes * use getVars for rendering * dont pass app. it is not public API * remove static registers * update migration guide

kibanamachine · 2020-04-16T16:07:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ce0b74e

History

💚 Build #41165 succeeded ce0b74e
💚 Build #40717 succeeded 54d3a70
💔 Build #40602 failed 7f52fb3
💔 Build #40584 failed 6d0de20
💚 Build #39565 succeeded 02d8315

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* add HttpResources basic implementation * expose http resources to plugins * add mocks * move http resources to a separate service * hide rendering service * adopt internal types * expose HttpResources service to plugins * update platform mocks * plugins start using HttpResources API * remove RenderingServiceSetup export * RenderingServiceSetup --> InternalRenderingServiceSetup * improve types * remove httpRespources leftovers from http service * remove rendering types from RequestHanlderContext * fix security plugin tests * add unit tests for httpResources service * add unit tests * remove outdated cache-control header * restructure http resources service * merge getUiPlugins and discover * static route declaration shouldnt require auth & validate * update docs * use HttpResources service instad of rendering * address comments * update docs * roll back unnecessary changes * use getVars for rendering * dont pass app. it is not public API * remove static registers * update migration guide

mshustov added 3 commits March 30, 2020 14:55

add HttpResources basic implementation

601ee0f

expose http resources to plugins

543ef00

add mocks

03f4219

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 v7.8.0 labels Mar 30, 2020

mshustov mentioned this pull request Mar 30, 2020

Introduce HttpResource service #50654

Closed

4 tasks

mshustov added 14 commits April 1, 2020 12:33

move http resources to a separate service

b514e7b

hide rendering service

9e34b0e

adopt internal types

4c276bf

expose HttpResources service to plugins

8a1e10f

update platform mocks

1b4ef8e

plugins start using HttpResources API

18309fc

remove RenderingServiceSetup export

5e3d085

RenderingServiceSetup --> InternalRenderingServiceSetup

5fc5beb

Merge branch 'master' into issue-50654-http-resources

6ca04a0

improve types

b96f741

remove httpRespources leftovers from http service

900d40e

remove rendering types from RequestHanlderContext

4700564

fix security plugin tests

4daa02c

add unit tests for httpResources service

b9f3099

pgayvallet reviewed Apr 4, 2020

View reviewed changes

mshustov added 5 commits April 6, 2020 15:34

Merge branch 'master' into issue-50654-http-resources

cebf397

add unit tests

d988c76

Merge branch 'master' into issue-50654-http-resources

8150a9e

remove outdated cache-control header

7326679

restructure http resources service

6e8ce80

mshustov requested review from a team as code owners April 8, 2020 14:02

mshustov commented Apr 8, 2020

View reviewed changes

pgayvallet approved these changes Apr 9, 2020

View reviewed changes

legrego self-requested a review April 9, 2020 15:45

joshdover reviewed Apr 9, 2020

View reviewed changes

mshustov added 7 commits April 14, 2020 09:40

Merge branch 'master' into issue-50654-http-resources

6d0de20

address comments

40a9b7f

update docs

7f52fb3

roll back unnecessary changes

02776eb

use getVars for rendering

c7712ac

dont pass app. it is not public API

e01d8ff

Merge branch 'master' into issue-50654-http-resources

54d3a70

mshustov requested a review from joshdover April 14, 2020 16:36

joshdover approved these changes Apr 14, 2020

View reviewed changes

mshustov added 3 commits April 16, 2020 09:56

Merge branch 'master' into issue-50654-http-resources

be8380e

remove static registers

9e58fe0

update migration guide

ce0b74e

streamich approved these changes Apr 16, 2020

View reviewed changes

legrego approved these changes Apr 16, 2020

View reviewed changes

mshustov merged commit af09fed into elastic:master Apr 16, 2020

mshustov deleted the issue-50654-http-resources branch April 16, 2020 14:09

mshustov mentioned this pull request Apr 16, 2020

[7.x] [NP] add http resources sub-service (#61797) #63708

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NP] add http resources sub-service #61797

[NP] add http resources sub-service #61797

mshustov commented Mar 30, 2020 •

edited

Loading

elasticmachine commented Mar 30, 2020

pgayvallet Apr 3, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

pgayvallet Apr 9, 2020

pgayvallet Apr 3, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

pgayvallet commented Apr 9, 2020

pgayvallet left a comment

pgayvallet Apr 9, 2020

mshustov Apr 14, 2020

pgayvallet Apr 9, 2020

mshustov Apr 14, 2020

pgayvallet Apr 9, 2020

pgayvallet Apr 9, 2020

legrego Apr 10, 2020

mshustov Apr 14, 2020 •

edited

Loading

mshustov Apr 14, 2020

legrego Apr 15, 2020

mshustov Apr 16, 2020 •

edited

Loading

joshdover left a comment

streamich left a comment

legrego left a comment

kibanamachine commented Apr 16, 2020

		createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
		createRouter: () => router,

[NP] add http resources sub-service #61797

[NP] add http resources sub-service #61797

Conversation

mshustov commented Mar 30, 2020 • edited Loading

Summary

Checklist

For maintainers

Dev docs

elasticmachine commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Apr 9, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 16, 2020

💚 Build Succeeded

History

mshustov commented Mar 30, 2020 •

edited

Loading

mshustov Apr 14, 2020 •

edited

Loading

mshustov Apr 16, 2020 •

edited

Loading