New Platform and Legacy platform servers integration by mshustov · Pull Request #39047 · elastic/kibana

mshustov · 2019-06-16T09:42:41Z

Summary

Security integration into New platform has several pain points related to running authc/authz and sharing their results to both New & Legacy platform servers. This PR removes a concept of 2 different servers, instead:

The New platform creates hapi server
The New platform passes created server to the Legacy platform, which extends it accordingly
The New platform runs listening on a port.

Advantages

simple architecture. we can get rid of LegacyPlatformProxy
simple API integration, because we use the same server, the request objects, etc. can get rid of IncomingMessage
Security plugin migration is not a blocker for other plugin migration anymore. All New Platform routes are protected by implementation in the Legacy platform.
Security handles only the one server, no needs to think how to proxy results to another HTTP server instance, how to share route declarations (auth: false/true}
we can postpone refactoring several places that use Security directly dashboard_mode, setup x-pack headers

Disadvantages

Legacy platform mutates and extends server instance in the wild. that could affect New platform in an unpredictable way. The new platform doesn't expose low-level details like hapi to the plugins, perhaps my fears are exaggerated.
It's hard to reason about what pieces already migrated to New platform

Both points can be solved via decorating hapi extension points(server.ext, server.expose) to track its usage in the Legacy platform and log deprecation warnings.

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-06-16T10:37:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Required to use a common security interceptor for incoming http requests

elasticmachine · 2019-06-16T15:09:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

src/core/server/legacy/legacy_service.ts

elasticmachine · 2019-06-17T07:48:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-17T10:07:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-17T11:13:16Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-06-17T11:13:18Z

Pinging @elastic/kibana-security

epixa · 2019-06-17T13:55:35Z

One problem this introduces is in upgrading hapi. The existing setup allowed us to upgrade hapi in the new platform independently of the old platform, which is helpful because the new platform has much better test coverage around http, so we can rely more heavily on our tests to flag issues. We took advantage of this when doing our hapi 14 -> 17 upgrade.

I'm not necessarily against this change if it's a pragmatic way forward and unblocks migration, but I think moving all of the authc/authz stuff over to the new platform is still crucial in the short term even with this change in place. We have to dramatically scale back the scope of http related features that are being used in the legacy platform otherwise we're looking at another multi-month long effort to upgrade hapi, which just distracts us from new platform work anyway.

epixa

The http proxy stuff is non-intuitive, so at the very least I'm happy that will go.

One general piece of feedback for future PRs: try to limit variable renaming and such when it isn't a necessary byproduct of your change. There were a lot of variable renames to add the "mock" prefix to things in tests in this PR that made it harder to review.

src/core/server/http/http_service.test.ts

src/core/server/http/integration_tests/http_service.test.ts

src/core/server/http/http_service.test.ts

src/core/server/http/integration_tests/http_service.test.ts

src/core/server/index.ts

src/core/server/legacy/legacy_service.ts

elasticmachine · 2019-06-17T16:17:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

LGTM, glad we managed to get rid of "proxifier"! Just one concern that I think we should address before we merge this.

src/core/server/legacy/legacy_service.ts

azasypkin · 2019-06-17T16:39:05Z

src/core/server/http/http_service.ts

@@ -102,14 +99,26 @@ export class HttpService implements CoreService<HttpServiceSetup, HttpServiceSta
    }

    return {


question: do we consume this return value anywhere?

the core doesn't use it anymore. @toddself are you going to use isListening somewhere?

packages/kbn-test/src/functional_tests/lib/run_kibana_server.js

mshustov · 2019-06-18T14:33:10Z

One problem this introduces is in upgrading hapi. The existing setup allowed us to upgrade hapi in the new platform independently of the old platform, which is helpful because the new platform has much better test coverage around http, so we can rely more heavily on our tests to flag issues. We took advantage of this when doing our hapi 14 -> 17 upgrade.

That's true. OTOH both servers are a bit coupled in the current implementation. All our internals interacting with hapi entities, have to support both versions. For example, KibanaRequest.from(request: Hapi.Request). Probably it will be easier:

to update both servers at the same time
to postpone updating until the migration effort is finished.

elasticmachine · 2019-06-18T14:42:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover · 2019-06-18T15:20:32Z

I don't have any immediate concerns. I took a look at the mutating code we do have in the legacy world and haven't found anything too egregious. It helps that none of these mutations will be exposed to the NP plugins directly.

In terms of Hapi upgrade-ability, this will make it harder. Though, taking a look at Hapi release notes for the most recent major (v18), I don't see anything new or improved that would necessarily be something we need. The new features introduced would probably be something we implement in the NP's abstractions anyways (if we even wanted them in the first place). The breaking changes in v18 also don't appear as nearly as bad as v17 was.

@restrry's point above that the implementations are coupled already is a good argument for needing to do them both at the same time anyways (at least with the current design).

If we move forward with this, the Security plugin still plans to move to the NP in the near-term, correct? I think it's important that gets completed and fleshed out sooner than later. We need to be sure there's not any glaring holes in our HTTP service design now rather than late in the 7.x lifecycle.

Haven't taken a deep dive into the code just yet (will do today though).

kobelb · 2019-06-18T15:22:04Z

If we move forward with this, the Security plugin still plans to move to the NP in the near-term, correct?

Regardless of the decision here, we'll be moving forward with the NP migration for authc/authz.

eliperelman · 2019-06-18T15:53:58Z

src/core/server/http/http_server.ts

    await this.server.start();
    const serverPath = this.config!.rewriteBasePath || this.config!.basePath || '';
-    this.log.debug(`http server running at ${this.server.info.uri}${serverPath}`);
+    this.log.info(`http server running`);


Unnecessary template string. Also, can you elaborate why this log needs to change?

I need to log with info level something similar to Server running from the Legacy platform. I decided to use http server running, also re-phrase this one to highlight the difference between them.

src/core/server/http/http_service.test.ts

src/core/server/http/http_service.ts

src/core/server/legacy/integration_tests/legacy_service.test.ts

eliperelman · 2019-06-18T16:05:21Z

src/core/server/legacy/integration_tests/legacy_service.test.ts

+      root = kbnTestServer.createRoot();
+    }, 30000);
+
+    afterEach(async () => await root.shutdown());


Can be shortened to:

afterEach(() => root.shutdown());

src/core/server/legacy/integration_tests/legacy_service.test.ts

elasticmachine · 2019-06-18T17:57:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover

It should be noted that requests to NP endpoints will now be logged by the request logger (@elastic/good plugin) in the legacy system (when --logging.verbose=true). This is technically a behavior change, but we don't have any NP endpoints yet, so 🤷‍♂ .

NP requests are not currently logged by the NP logger, so there aren't duplicates, but we should migrate request logging to NP.

src/core/server/http/http_server.ts

src/core/server/http/http_service.ts

mshustov · 2019-06-19T03:53:41Z

NP requests are not currently logged by the NP logger, so there aren't duplicates, but we should migrate request logging to NP.

yes, I will create an issue. there are other small pieces that we can start migrating to NP - logging, all hapi lifecycle events

updated #39330

elasticmachine · 2019-06-19T05:04:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

@eli

* New and Legacy platforms share http server instance. Required to use a common security interceptor for incoming http requests * generate docs * remove excessive contract method * add test for New platform compatibility * address comments part #1 * log server running only for http server * fix test. mutate hapi request headers for BWC with legacy * return 503 on start * address @eli comments * address @joshdover comments

@eli

* New and Legacy platforms share http server instance. Required to use a common security interceptor for incoming http requests * generate docs * remove excessive contract method * add test for New platform compatibility * address comments part #1 * log server running only for http server * fix test. mutate hapi request headers for BWC with legacy * return 503 on start * address @eli comments * address @joshdover comments

mshustov added 2 commits June 16, 2019 16:05

New and Legacy platforms share http server instance.

4788eb9

Required to use a common security interceptor for incoming http requests

generate docs

52bfa2d

mshustov force-pushed the new-legacy-server-integration branch from fa343d4 to 52bfa2d Compare June 16, 2019 14:09

remove excessive contract method

280be38

mshustov commented Jun 17, 2019

View reviewed changes

src/core/server/legacy/legacy_service.ts Show resolved Hide resolved

add test for New platform compatibility

2a75541

mshustov marked this pull request as ready for review June 17, 2019 11:14

mshustov requested a review from a team as a code owner June 17, 2019 11:14

mshustov requested a review from epixa June 17, 2019 11:14

mshustov changed the title ~~[WIP] New Platform and Legacy platform servers integration~~ New Platform and Legacy platform servers integration Jun 17, 2019

mshustov requested a review from azasypkin June 17, 2019 11:41

epixa reviewed Jun 17, 2019

View reviewed changes

mshustov added 2 commits June 17, 2019 17:11

address comments part #1

4e5a846

Merge branch 'master' into new-legacy-server-integration

a033248

mshustov force-pushed the new-legacy-server-integration branch from e089a18 to a033248 Compare June 17, 2019 15:12

azasypkin approved these changes Jun 17, 2019

View reviewed changes

mshustov requested a review from epixa June 18, 2019 13:31

return 503 on start

9ddd9c4

mshustov force-pushed the new-legacy-server-integration branch from 235ce49 to 9ddd9c4 Compare June 18, 2019 13:42

eliperelman suggested changes Jun 18, 2019

View reviewed changes

address @eli comments

79915e5

mshustov requested review from eliperelman and joshdover June 18, 2019 17:02

joshdover approved these changes Jun 18, 2019

View reviewed changes

src/core/server/http/http_server.ts Outdated Show resolved Hide resolved

src/core/server/http/http_service.ts Show resolved Hide resolved

src/core/server/http/http_service.ts Show resolved Hide resolved

src/core/server/http/http_service.ts Show resolved Hide resolved

address @joshdover comments

aaabf88

mshustov force-pushed the new-legacy-server-integration branch from a62ed69 to aaabf88 Compare June 19, 2019 04:03

eliperelman approved these changes Jun 19, 2019

View reviewed changes

mshustov merged commit a75d777 into elastic:master Jun 19, 2019

mshustov deleted the new-legacy-server-integration branch June 19, 2019 14:32

mshustov mentioned this pull request Jun 19, 2019

[7.x] New Platform and Legacy platform servers integration (#39047) #39263

Merged

mshustov mentioned this pull request Jun 19, 2019

LP plugins can leverage new http server during migration #36120

Closed

3 tasks

mshustov mentioned this pull request Jun 20, 2019

Move HttpService functionality from Legacy to New platform #39330

Closed

jen-huang added the v8.0.0 label Jun 26, 2019

jen-huang mentioned this pull request Jun 26, 2019

Add base path to rollup index pattern endpoint's request to internal endpoint #39745

Merged

mshustov removed the review label Jun 30, 2019

chrisronline mentioned this pull request Aug 30, 2019

All requests to Kibana server have Connection: keep-alive #44537

Closed

		@@ -102,14 +99,26 @@ export class HttpService implements CoreService<HttpServiceSetup, HttpServiceSta
		}

		return {

Conversation

mshustov commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Advantages

Disadvantages

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 16, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 16, 2019

💚 Build Succeeded

Uh oh!

Uh oh!

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 17, 2019

Uh oh!

elasticmachine commented Jun 17, 2019

Uh oh!

epixa commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epixa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azasypkin Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

mshustov Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshustov commented Jun 18, 2019

Uh oh!

elasticmachine commented Jun 18, 2019

💔 Build Failed

Uh oh!

joshdover commented Jun 18, 2019

Uh oh!

kobelb commented Jun 18, 2019

Uh oh!

eliperelman Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

mshustov Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliperelman Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jun 18, 2019

💔 Build Failed

mshustov commented Jun 16, 2019 •

edited

Loading

epixa commented Jun 17, 2019 •

edited

Loading

mshustov commented Jun 19, 2019 •

edited

Loading