[Code] Migrate routers/elasticsearch clients/savedobjects clients to NP #47643

mw-ding · 2019-10-08T22:52:59Z

Summary

https://github.com/elastic/code/issues/1596

Finish the migrations of:

Routers
Elasticsearch clients
SavedObjectClient

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-10-09T00:00:20Z

💔 Build Failed

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

elasticmachine · 2019-10-09T01:02:42Z

Pinging @elastic/code (Team:Code)

elasticmachine · 2019-10-09T02:20:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5716ee6944e08091348bb9795025ef6a88e6c602

mw-ding · 2019-10-09T22:05:41Z

x-pack/legacy/plugins/code/server/routes/redirect.ts

@@ -1,39 +0,0 @@
-/*


@spacedragon this file does not seem to be used anywhere, so remove now.

Sure, thanks.

mw-ding · 2019-10-09T22:06:30Z

x-pack/legacy/plugins/code/server/distributed/code_services.test.ts

@@ -17,12 +17,12 @@ import { Logger } from '../log';
 import { ConsoleLoggerFactory } from '../utils/console_logger_factory';

 const log: Logger = new ConsoleLoggerFactory().getLogger(['test']);
-let hapiServer: Server = createTestHapiServer();
+let hapiServer = createTestHapiServer();


@restrry any util functions to create the router for unit tests?

https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md#mock-new-platform-services-in-tests
you can create a mocked instance of the router

import { httpServiceMock } from 'src/core/server/mocks'; const routerMock = httpServiceMock.createRouter();

elasticmachine · 2019-10-09T22:28:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4d84d0fc5ad42cef778b8618e0e8ee274ffc2e5e

elasticmachine · 2019-10-09T23:09:57Z

💔 Build Failed

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

elasticmachine · 2019-10-10T00:43:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 40ef4b76d82806b1b72724843d1b1ba7e1dde484

spacedragon · 2019-10-10T01:37:39Z

x-pack/legacy/plugins/code/server/distributed/multinode/non_code_node_adapter.ts

-      const body: Boom.Payload = await Wreck.read(res, { json: true });
-      throw new Boom(body.message, { statusCode: res.statusCode || 500, data: body.error });
+      const body = await Wreck.read(res, { json: true });
+      throw new Error(body.message);


I'm not sure removing statusCode is right here, some status code is important, such as 404

is it important to proxy this error code to the client? we already logged statusCode in https://github.com/elastic/kibana/pull/47643/files#diff-d170e5fce495c0bd5b8d0e8e28a0a33dR111

you still can use Boom error in the plugin, just add a top level wrapper that handles those errors. example from Security team https://github.com/restrry/kibana/blob/40ef4b76d82806b1b72724843d1b1ba7e1dde484/x-pack/plugins/security/server/authentication/index.ts#L126-L130

I agree simply switching from Boom to Error will change some status code, but I anyway do it for the following reasons:

we nested the Boom HTTP status code deeply into our internal service is not a good idea. This will make the internal service coupled with the http server layer very tightly. This is will make some future changes (like this PR) quite difficult to proceed.

Ideally, we should only append status code in the http server layer, i.e. the route definitions, while define our own Error types (e.g. LangServerNotInitializedError() so that the route definition could assemble the right status code appropriately.

I think our frontend does not rely on the status code too much that a 404 handling will be the same as 500 (@spacedragon need your confirmation). So I anyway make this change.

Thoughts?

At least, 404 is used for that File not found page.

@spacedragon revert back the changes of removing Boom. let's figure this out later. Also, skippe the 2 unit tests here: https://github.com/elastic/kibana/blob/5015a368c3fd581e115248cf85b32941c0f87047/x-pack/legacy/plugins/code/server/distributed/code_services.test.ts#L65. Let's chat offline and fix it later.

x-pack/legacy/plugins/code/server/security.ts

mshustov · 2019-10-10T08:04:11Z

x-pack/legacy/plugins/code/server/distributed/cluster/cluster_node_adapter.ts

+            req: KibanaRequest,
+            res: KibanaResponseFactory
+          ) => {
+            const { context, params } = req.body as RequestPayload;


I'd recommend starting declaring validation from the very beginning. This validation field was made required for safety reasons. If I'm not mistaken, we can declare schemas almost for all the request parameters.

// security.ts case 'post': { this.router.post({ path: route.path, validate: { body: route.validate.body || schema.object({}, { allowUnknowns: true }), ... } // in this file const requestContextSchema = schema.object({ path: schema.string(), resource: schema.string(), }); const validate = { body: schema.object({ context: requestContextSchema, params: schema.object({}, { allowUnknowns: true }), }), }; this.router.route({ validate, ...

As I mentioned before in the channel, I will assign this to each stakeholders which have better context of the schema in the comment.

mshustov · 2019-10-10T08:14:58Z

x-pack/legacy/plugins/code/server/distributed/multinode/non_code_node_adapter.ts

-      const body: Boom.Payload = await Wreck.read(res, { json: true });
-      throw new Boom(body.message, { statusCode: res.statusCode || 500, data: body.error });
+      const body = await Wreck.read(res, { json: true });
+      throw new Error(body.message);


is it important to proxy this error code to the client? we already logged statusCode in https://github.com/elastic/kibana/pull/47643/files#diff-d170e5fce495c0bd5b8d0e8e28a0a33dR111

you still can use Boom error in the plugin, just add a top level wrapper that handles those errors. example from Security team https://github.com/restrry/kibana/blob/40ef4b76d82806b1b72724843d1b1ba7e1dde484/x-pack/plugins/security/server/authentication/index.ts#L126-L130

x-pack/legacy/plugins/code/server/git_operations.ts

mshustov · 2019-10-10T08:36:00Z

x-pack/legacy/plugins/code/server/plugin.ts

  constructor(private readonly initContext: PluginSetupContract) {
    this.log = {} as Logger;
    this.serverOptions = {} as ServerOptions;
  }

-  public setup(core: CoreSetup) {
+  public async setup(core: CoreSetup, npHttp: any) {


Isn't it better to pass a real CoreSetup as the first argument and legacy HTTP server separately? https://github.com/restrry/kibana/blob/40ef4b76d82806b1b72724843d1b1ba7e1dde484/x-pack/legacy/plugins/code/index.ts#L66-L72

We are losing all TypeScript benefits right now. And it will complicate refactoring in the future.

the problem is the server instance is still depended, and we need to pass it in here.

yes, but then you have a real typed CoreSetup as the first argument, not a hack
https://github.com/restrry/kibana/blob/ce9ea0bbb34869356030ffd2eaebe42d97b5fc82/x-pack/legacy/plugins/code/server/plugin.ts#L85
you can switch to setup(core: CoreSetup, _hapiHttp: Server)

x-pack/legacy/plugins/code/server/distributed/code_services.test.ts

mshustov · 2019-10-10T09:24:36Z

x-pack/legacy/plugins/code/server/distributed/code_services.test.ts

  ) => {
    expect(baseUrl).toBe(codeNodeUrl);
    const response = await hapiServer.inject({
      method: 'POST',
      url: path,
-      headers: originRequest.headers,
+      headers: originRequest.headers as any,


Do you need all the headers from KibanaRequest object? We're going to restrict access to authorization header for all plugins except for Security.

Please do let us know when the change happens. There is much more context here of why we need to forward the headers. Let me add a comment here just in case, but this is not something we should handle in this PR.

Btw, this is for the test only.

mshustov · 2019-10-10T09:26:53Z

x-pack/legacy/plugins/code/server/init_es.ts

 import { RepositoryIndexInitializerFactory } from './indexer';
 import { RepositoryConfigController } from './repository_config_controller';
 import { EsClientWithInternalRequest } from './utils/esclient_with_internal_request';
 import { EsClient } from './lib/esqueue';
 import { Logger } from './log';

-export async function initEs(server: Server, log: Logger) {
-  // wait until elasticsearch is ready
-  await server.plugins.elasticsearch.waitUntilReady();


we don't need this logic in the legacy platform anymore?

This will be a question for you. After we use the context.elasticsearch.xxx, I think this is not required. Can you confirm?

No, NP doesn't support it yet #43456

x-pack/legacy/plugins/code/server/plugin.ts

x-pack/legacy/plugins/code/server/routes/check.ts

mw-ding

Given the significant changes in this PR, I won't merge this one until the 7.5 branch has been cut.

mw-ding · 2019-10-10T20:21:42Z

x-pack/legacy/plugins/code/server/distributed/code_services.test.ts

@@ -80,13 +82,13 @@ test('non-code-node could send request to code-node', async () => {
    baseUrl: string,
    path: string,
    payload: RequestPayload,
-    originRequest: Request
+    originRequest: KibanaRequest
  ) => {
    expect(baseUrl).toBe(codeNodeUrl);
    const response = await hapiServer.inject({


@restrry we will need the mock for this one to replace the hapiServer here.

You already mocked router in the test. If you want to add an integration test instead, you can use kbnTestServer. There is an example how to do it https://github.com/elastic/kibana/blob/master/src/core/server/legacy/integration_tests/legacy_service.test.ts

elasticmachine · 2019-10-10T21:42:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 6787c46c25eb3bcc306bc4beae569033a8a32640

elasticmachine · 2019-10-15T19:28:36Z

💔 Build Failed

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

elasticmachine · 2019-10-15T20:18:00Z

💔 Build Failed

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

elasticmachine · 2019-10-15T22:35:58Z

💔 Build Failed

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

elasticmachine · 2019-10-15T23:36:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 59d71c7f86a7bae90792969a828269a203c836e1

elasticmachine · 2019-10-16T20:32:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 441f4fb627e48c574c84ab49ff420785f0d886ec

This reverts commit 7eda19ee6e5b001f46ce86e93bd9694d6a146976.

mw-ding · 2019-10-17T18:46:41Z

x-pack/legacy/plugins/code/server/utils/with_request.ts

+      securityPlugin &&
+      securityPlugin.authorization &&
+      // @ts-ignore
+      securityPlugin.authorization.mode.useRbacForRequest(req);


@azasypkin still use the old one but apply the new KibanaRequest here.

elasticmachine · 2019-10-17T20:05:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 184a6ca

…NP (#47643) (#48598) * Migrate the elasticsearch admin client * initial router migration * update the code service request * more routes migration * remove Boom * remove redirect route * test * more route * addressing comments * remove the notFound * Revert "remove Boom" This reverts commit 7eda19ee6e5b001f46ce86e93bd9694d6a146976. * handle isBoom * more fixes * minor fix * shim security useRbacForRequest

mw-ding added the Team:Code label Oct 9, 2019

mw-ding changed the title ~~Migrate the elasticsearch admin client~~ [Code] Migrate routers and elasticsearch clients to NP Oct 9, 2019

mw-ding requested review from spacedragon, fantapsody, zfy0701, mshustov and joshdover October 9, 2019 22:03

mw-ding added v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 9, 2019

mw-ding commented Oct 9, 2019

View reviewed changes

mw-ding marked this pull request as ready for review October 9, 2019 22:32

mw-ding requested a review from a team as a code owner October 9, 2019 22:32

spacedragon reviewed Oct 10, 2019

View reviewed changes

mshustov reviewed Oct 10, 2019

View reviewed changes

mw-ding requested review from mshustov and spacedragon October 10, 2019 19:52

mw-ding commented Oct 10, 2019

View reviewed changes

mshustov mentioned this pull request Oct 14, 2019

Improve Migration guide with examples and links #46661

Merged

7 tasks

mshustov approved these changes Oct 14, 2019

View reviewed changes

joshdover removed their request for review October 14, 2019 19:51

mw-ding added 15 commits October 17, 2019 09:16

Migrate the elasticsearch admin client

698f8fc

initial router migration

5156887

update the code service request

490f92a

more routes migration

1b40bf0

remove Boom

dfe3c6a

remove redirect route

bc59f6a

test

4340dbe

more route

60c5f7e

addressing comments

c724e10

remove the notFound

171036f

Revert "remove Boom"

48c09e7

This reverts commit 7eda19ee6e5b001f46ce86e93bd9694d6a146976.

handle isBoom

9f81516

more fixes

677dc2e

minor fix

d371a35

shim security useRbacForRequest

184a6ca

mw-ding commented Oct 17, 2019

View reviewed changes

mw-ding changed the title ~~[Code] Migrate routers and elasticsearch clients to NP~~ [Code] Migrate routers/elasticsearch clients/savedobjects clients to NP Oct 17, 2019

mw-ding merged commit 55349d7 into elastic:master Oct 18, 2019

mw-ding mentioned this pull request Oct 18, 2019

[7.x] [Code] Migrate routers/elasticsearch clients/savedobjects clients to NP (#47643) #48598

Merged

[Code] Migrate routers/elasticsearch clients/savedobjects clients to NP #47643

[Code] Migrate routers/elasticsearch clients/savedobjects clients to NP #47643

Conversation

mw-ding commented Oct 8, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 9, 2019

💔 Build Failed

elasticmachine commented Oct 9, 2019

elasticmachine commented Oct 9, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Oct 9, 2019

💔 Build Failed

elasticmachine commented Oct 9, 2019

💔 Build Failed

elasticmachine commented Oct 10, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mw-ding Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

spacedragon Oct 11, 2019 • edited Loading

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 Oct 14, 2019 • edited Loading

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

mw-ding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 10, 2019

💔 Build Failed

elasticmachine commented Oct 15, 2019

💔 Build Failed

elasticmachine commented Oct 15, 2019

💔 Build Failed

elasticmachine commented Oct 15, 2019

💔 Build Failed

elasticmachine commented Oct 15, 2019

💔 Build Failed

elasticmachine commented Oct 16, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Oct 17, 2019

💚 Build Succeeded

mw-ding commented Oct 8, 2019 •

edited

Loading

mshustov Oct 10, 2019 •

edited

Loading

mw-ding Oct 10, 2019 •

edited

Loading

spacedragon Oct 11, 2019 •

edited

Loading

mshustov Oct 14, 2019 •

edited

Loading