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

[NP] add http resources sub-service #61797

Merged
merged 36 commits into from
Apr 16, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Mar 30, 2020

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

For maintainers

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(...);'})
);

@mshustov 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
@elasticmachine
Copy link
Contributor

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

@mshustov mshustov mentioned this pull request Mar 30, 2020
4 tasks
src/core/server/server.ts Outdated Show resolved Hide resolved
src/legacy/server/kbn_server.d.ts Show resolved Hide resolved
x-pack/plugins/security/server/routes/views/logout.ts Outdated Show resolved Hide resolved
src/core/server/http/router/headers.ts Show resolved Hide resolved
src/core/server/http_resources/http_resources_service.ts Outdated Show resolved Hide resolved
Comment on lines -286 to +287
createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
createRouter: () => router,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +152 to +161
const renderingSetup = await this.rendering.setup({
http: httpSetup,
legacyPlugins,
uiPlugins,
});

const httpResourcesSetup = this.httpResources.setup({
http: httpSetup,
rendering: renderingSetup,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mshustov mshustov requested review from a team as code owners April 8, 2020 14:02
const { http, httpResources } = await root.setup();

const router = http.createRouter('');
const resources = httpResources.createRegistrar(router);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(response.header['x-kibana']).toBe('42');
});

it('can adjust route config', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pgayvallet
Copy link
Contributor

Ack: will review today

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/core/server/http/router/router.ts Outdated Show resolved Hide resolved
});
});

it('can attach headers, except the CSP & "content-type" headers', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines -286 to +287
createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
createRouter: () => router,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 20 to 28
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 []`
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this too

Copy link
Contributor Author

@mshustov mshustov Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mshustov mshustov Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legrego removed

@legrego legrego self-requested a review April 9, 2020 15:45
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/core/server/http_resources/types.ts Show resolved Hide resolved
src/core/server/http_resources/types.ts Show resolved Hide resolved
src/core/server/http_resources/types.ts Outdated Show resolved Hide resolved
@mshustov mshustov requested a review from joshdover April 14, 2020 16:36
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppArch change LGTM.

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for consolidating the API surface. Changes LGTM!

@mshustov mshustov merged commit af09fed into elastic:master Apr 16, 2020
@mshustov mshustov deleted the issue-50654-http-resources branch April 16, 2020 14:09
mshustov added a commit to mshustov/kibana that referenced this pull request Apr 16, 2020
* 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
Copy link
Contributor

💚 Build Succeeded

History

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

mshustov added a commit that referenced this pull request Apr 16, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants