Proxy ExpressJS requests to HapiJS. #13844

azasypkin · 2017-09-05T14:16:09Z

Really early-draft of ExpressJS (new platform) and HapiJS (existing platform) integration.

This is supposed to handle #12520

azasypkin · 2017-09-07T13:29:34Z

platform/cli/cli.ts

+      root.logger.get('proxy'),
+      argv.kbnServer
+    ));
+    proxy.on('platform-start', async () => await root.start());


I'm just exploring the idea, but maybe it's not that bad if we start root only when old platform starts Hapi with our proxy as a listener. This way we reuse port that old platform supplies us via listener so we don't have to read kbnServer.settings.server.port...

azasypkin · 2017-09-07T13:30:15Z

platform/config/Env.ts

 interface WithConfig {
  config?: string;
+  kbnServer?: any;


Don't really know where to plug this kbnServer in - env was the easiest one :)

It works for now. I might want to move it later, but for the PoC it's 👍

azasypkin · 2017-09-07T13:33:58Z

platform/server/http/HttpServer.ts

+    const proxy = this.env.getProxy();
+    if (proxy) {
+      proxy.bind(this.server);
+      port = proxy.getPort();


The real port is passed by old Kibana in setup_connection -> proxy.listen(...) so we don't have to think which port to use here, just grab what old platform gave us.

azasypkin · 2017-09-07T13:34:56Z

platform/server/http/HttpServer.ts

-    this.app.use([
+
+    // We will register body parser only for routes defined in the new platform.
+    /*this.app.use([


It looks like we don't have to register body parsers "globally" and we can do that on route basis (maybe even router level). I'll check.

++ I like the idea of adding them on routers instead

azasypkin · 2017-09-07T13:36:24Z

platform/server/http/Proxy/Proxy.ts

+
+import { Logger } from '../../../logging';
+
+export class Proxy extends EventEmitter {


It's not really a "proxy" - it's more like a "something-else", so just semi-randomly picked "proxy" :)

ProxyToLegacyPlatform, just to be clearer?

azasypkin · 2017-09-07T13:39:36Z

platform/server/http/Proxy/Proxy.ts

+    this.port = port;
+
+    this.emit('platform-start');
+    callback && callback();


Ideally we should call callback only when platform is ready, but I should think how to do that. And I haven't noticed any issues with this approach yet.

If we removed the events and rather injected start and stop functions when creating the Proxy, we could call the callback when they finish instead.

Good idea ✔️

azasypkin · 2017-09-07T13:41:18Z

src/server/http/setup_connection.js

@@ -3,6 +3,8 @@ import { map } from 'lodash';
 import secureOptions from './secure_options';

 export default function (kbnServer, server, config) {
+  const proxy = kbnServer && kbnServer.proxy;


I definitely don't want to expose proxy property on the kbnServer, but I haven't figured out the better way yet.... Do you have any ideas?

We've already got tons of things on it, so I think it's okey. We could probably rename this to be clearer. newPlatformProxyListener or something like that?

azasypkin · 2017-09-07T13:47:28Z

UPDATE:

I've dug into Hapi's server.inject to understand if there is a way disable buffering of request/response and it seems impossible. Hapi uses shot under the hood and shot always reads entire request and response body into a buffer if it receives stream :/.
So I started to play with custom listener instead and although it looks quite hacky, it seems to work. I've pushed updated code if you want to take a look. Now proxy stuff works correctly, response and request body is neither parsed nor buffered. We can apply body parsers just to our new-platform routes and we should be fine (I'll verifying this assumption). Also our custom listener (Proxy in my PR) will need a bit of tuning if we decide to go with something like this (like what server events to forward to old platform, do we want to pass real connection instance to old kibana etc.)

Example:

http://localhost:5601/api/saved_objects will be forwarded to the old platform
http://localhost:5601/api/saved_objects/type will stay in the new platform

/cc @kjbekkelund @spalger

kimjoar

This is a great start. I think we should get rid of code that lets the new platform start on its own, and always have it start through old Kibana (aka we always receive kbnServer etc).

kimjoar · 2017-09-12T11:17:07Z

platform/cli/cli.ts

@@ -20,7 +21,7 @@ export const parseArgv = (argv: Array<string>) =>
    .epilogue(args.docs)
    .check(args.check(args.options)).argv;

-const run = (argv: { [key: string]: any }) => {
+export const run = (argv: { [key: string]: any }) => {


It feels like we should create a separate runWithLegacyKbnServer. We probably need to explore not listening for SIGHUP, SIGINT etc here, and rather receive that through the old platform somehow. I've got a feeling we might want to receive the config from the old platform too, and have a transformLegacyConfig or something like that which transforms it into whatever we need/want in the new platform.

(Or just rewrite this code to be explicit about kbnServer always being there, and getting rid of node scripts/platform entirely. Then, when we get to K7 we can add whatever we need to have the new platform start cleanly on its own)

Don't have strong opinion on that, having runWithLegacyKbnServer sounds reasonable to me. I'd get rid of node scritps/platform only once we have dev setup ready, I still use ts:start with scripts/platform to quickly test changes.

I've got a feeling we might want to receive the config from the old platform too, and have a transformLegacyConfig or something like that which transforms it into whatever we need/want in the new platform.

I'll explore that.

kimjoar · 2017-09-12T11:26:31Z

platform/server/http/Proxy/Proxy.ts

+    this.port = port;
+
+    this.emit('platform-start');
+    callback && callback();


If we removed the events and rather injected start and stop functions when creating the Proxy, we could call the callback when they finish instead.

kimjoar · 2017-09-12T11:28:15Z

platform/server/http/Proxy/Proxy.ts

+export class Proxy extends EventEmitter {
+  private server: Server;
+  private readonly eventHandlers: any;
+  private port: number;


Isn't specified in the constructor, so should be:

private port?: number;

(Also: I'm hoping they implement microsoft/TypeScript#8476 soon)

kimjoar · 2017-09-12T11:33:56Z

platform/server/http/Proxy/Proxy.ts

+      listening: forwardEvent.bind(this, 'listening'),
+      error: forwardEvent.bind(this, 'error'),
+      clientError: forwardEvent.bind(this, 'clientError'),
+      connection: forwardEvent.bind(this, 'connection')


Why are these needed? (Might be obvious, just add a comment about it in the code)

Why are these needed?

Hapi relies on these events, some are critical, some aren't:

listening: https://github.com/hapijs/hapi/blob/v14.2.0/lib/connection.js#L120

error: https://github.com/hapijs/hapi/blob/v14.2.0/lib/connection.js#L166

clientError: https://github.com/hapijs/hapi/blob/v14.2.0/lib/connection.js#L92

connection: https://github.com/hapijs/hapi/blob/v14.2.0/lib/connection.js#L142

Might be obvious, just add a comment about it in the code

Sure ✔️

kimjoar · 2017-09-12T11:34:58Z

platform/server/http/Proxy/Proxy.ts

+
+  getConnections(callback: (error?: Error, count?: number) => void) {
+    // This method is used by `even-better` (before we start platform).
+    // It seems that the latest version of parent `good` doesn't use this anymore.


Hopefully we'll get rid of it soon: #13800

Yeah, that would be nice

kimjoar · 2017-09-12T11:37:02Z

platform/server/http/Proxy/Proxy.ts

+
+import { Logger } from '../../../logging';
+
+export class Proxy extends EventEmitter {


ProxyToLegacyPlatform, just to be clearer?

kimjoar · 2017-09-12T11:44:39Z

platform/server/http/__tests__/HttpServer.test.ts

@@ -12,8 +14,17 @@ let app: any;
 let port: number;

 beforeEach(() => {
+  const log: Logger = {


Not something we need to fix in this PR: There is a logger mock in ../../../logging/__mocks__, not sure if that works out-of-the-box here. We need some way of sharing this across tests, though, just so we don't have to keep mocks like this up-to-date. Jest has some automatic mocking support, but I haven't looked at it yet.

kimjoar · 2017-09-12T11:47:02Z

src/server/http/setup_connection.js

@@ -3,6 +3,8 @@ import { map } from 'lodash';
 import secureOptions from './secure_options';

 export default function (kbnServer, server, config) {
+  const proxy = kbnServer && kbnServer.proxy;


We've already got tons of things on it, so I think it's okey. We could probably rename this to be clearer. newPlatformProxyListener or something like that?

azasypkin · 2017-09-22T12:05:58Z

platform/cli/cli.ts

@@ -52,4 +53,24 @@ const run = (argv: { [key: string]: any }) => {
  }
 };

+export const runWithLegacyKbnServer = (kbnServer: any) => {
+  const root = new Root(
+    getLegacyConfig$(kbnServer),


@kjbekkelund I've added some initial support for transforming legacy config to a new format, could you please take a look?

It also required some changes to Duration and ByteSize since in the old Kibana we use numbers for these type of values (either number of ms or bytes). I prefer the new platform approach though (strings like 1mb etc.) and would get rid of number support once we finish migration. Alternatively LegacyPlatformConfig can transform numbers to format new platform understands like newconfig.timeout: "${oldconfig.timeout}"ms, but I found it a bit clumsy :)

Awesome, I'll take a look now.

kimjoar

I just scanned quickly through the code and noted a couple things. Will try to run it etc now, just wanted to submit the comments so far.

kimjoar · 2017-09-22T13:02:20Z

platform/lib/ByteSizeValue/__tests__/index.test.ts

@@ -31,6 +31,38 @@ describe('parsing units', () => {
  });
 });

+describe('#constructor', () => {
+  test('throws if number of bytes is negative', () => {
+    expect(() => new ByteSizeValue(-1024)).toThrowError(


I'd like to see the full error message here, maybe use toThrowErrorMatchingSnapshot instead?

kimjoar · 2017-09-22T13:24:26Z

platform/legacy/LegacyPlatformConfig.ts

+
+export const getLegacyConfig$ = (kbnServer: any) => {
+  return Observable.from([
+    new Proxy({ [configProperty]: kbnServer.config }, configProxyHandler)


Proxy isn't supported in IE11, I think(?) Or do we maybe do some Babel magic which enables it?

Very gooood point! Will check, totally forgot about that

Ugh, likely will have to get rid of Proxy and update ConfigService to work a bit differently :/

But first will check https://www.npmjs.com/package/proxy-polyfill

No way here, got rid of Proxy in favor of an additional abstraction (RawConfig) around config plain object.

kimjoar · 2017-09-22T13:29:50Z

platform/cli/cli.ts

@@ -52,4 +53,24 @@ const run = (argv: { [key: string]: any }) => {
  }
 };

+export const runWithLegacyKbnServer = (kbnServer: any) => {


Not necessary yet, but before we merge we should create a interface LegacyKbnServer { ... } with the fields we use in ts files, just so we don't type it as any.

kimjoar · 2017-09-22T13:42:50Z

platform/cli/cli.ts

+    () => root.shutdown()
+  );
+
+  // TODO: Do something to reload config.


We could for example register a callback on kbnServer in this file, and then call that whenever

kibana/src/cli/serve/serve.js

Line 173 in c79e223

process.on('SIGHUP', function reloadConfig() {

triggers. Hm, maybe that would be weird as we would "go around" the old platform.

Another idea could be to create a BehaviorSubject in legacy Kibana that we expose as an Observable. We'd next on whenever the sighup triggers in the above code I linked. So here we'd only need to map over it and do the transform from legacy config stuff. Then we could remove this process.on('SIGHUP' entirely from this function.

No matter what we choose I feel it's important that the new platform can do "full config reload" from day 1.

Honestly, both options look the same to me, just that in the second option code will be rxjs-fied (unless I'm missing something). I'll experiment with various options and see what we can get.

👍 Both approaches are fine with me too

kimjoar · 2017-09-22T13:52:19Z

platform/legacy/LegacyPlatformProxifier.ts

+    private readonly log: Logger,
+    readonly kbnServer: any,
+    private readonly startPlatform: () => Promise<any>,
+    private readonly stopPlatform: () => Promise<any>


Promise<void> for both of these

kimjoar · 2017-09-22T15:01:35Z

platform/legacy/LegacyPlatformProxifier.ts

+    super();
+
+    const forwardEvent = (name: string, ...args: any[]) => {
+      this.log.info(`Event is being forwarded: ${name}`);


info -> debug

kimjoar · 2017-09-22T15:01:43Z

platform/legacy/LegacyPlatformProxifier.ts

+  }
+
+  proxy(request: IncomingMessage, response: OutgoingMessage) {
+    this.log.info(


info -> debug

kimjoar · 2017-09-22T15:12:32Z

platform/legacy/LegacyPlatformProxifier.ts

+    callback && callback();
+  }
+
+  async close(callback: Function) {


Maybe:

async close(callback: () => {}) {

you mean () => void right? If so, then ✔️

kimjoar · 2017-09-22T15:12:50Z

platform/legacy/LegacyPlatformProxifier.ts

+    return this.server && this.server.address();
+  }
+
+  async listen(port: number, host: string, callback: Function) {


callback: Function

to

callback: () => void

kimjoar · 2017-09-22T15:44:03Z

platform/legacy/LegacyPlatformProxifier.ts

+  ) {
+    super();
+
+    const forwardEvent = (name: string, ...args: any[]) => {


niiiitpick:

const forwardEvent = (name: string) => (...args: any[]) => { this.log.info(`Event is being forwarded: ${name}`); this.emit(name, ...args); };

then there no need to .bind below

kimjoar · 2017-09-22T15:47:31Z

platform/server/http/HttpServer.ts

+        cert: readFileSync(config.ssl.certificate!),
+        ca:
+          config.ssl.certificateAuthorities &&
+          config.ssl.certificateAuthorities!.map(caFilePath =>


Don't need the ! here, I think

Right, ✔️

kimjoar · 2017-09-22T16:11:16Z

platform/config/Env.ts

@@ -42,6 +45,10 @@ export class Env {
    return resolve(this.pluginsDir, pluginName, 'target', 'dist');
  }

+  getProxy(): LegacyPlatformProxifier | undefined {


getNewPlatformProxyListener?

kimjoar · 2017-09-22T16:15:51Z

platform/cli/cli.ts

@@ -52,4 +53,24 @@ const run = (argv: { [key: string]: any }) => {
  }
 };

+export const runWithLegacyKbnServer = (kbnServer: any) => {


Btw, if we remove the process.on stuff below, I think we should extract this into a separate file (and a separate folder) as there's no cli related stuff left in this function.

I agree if we don't have process.on stuff it will reside in legacy sub-folder.

@kjbekkelund regarding SIGINT and SIGTERM: it seems old platform doesn't handle these events (it only cleans pid file on SIGINT). I'm not sure if we should handle these events in the new platform if it's run within old one and basically acts as an advanced HTTP server, what do you think?

I don't think we should listen for them specifically, but we probably want to listen for something that signals that we should shut down (just to make sure we run all the shutdown stuff correctly for later when we remove the old platform). However, I'm not sure how we indicate that in current platform.

kimjoar · 2017-09-25T14:40:07Z

platform/legacy/LegacyPlatformProxifier.ts

+  ) {
+    super();
+
+    const forwardEvent = (name: string) => (name: string, ...args: any[]) => {


Remove name: string, from second function here

Oh, yeah, good catch. I'm really surprised that it still works :) Will figure that out

Ah, main 'request' event is still being forwarded correctly from another method

azasypkin · 2017-10-02T19:01:55Z

platform/legacy/LegacyPlatformConfig.ts

+    configPath = LegacyConfigToRawConfigAdapter.flattenConfigPath(configPath);
+
+    // Explicitly disable plugins until they are supported by the legacy config.
+    if (!this.legacyConfig.has(configPath) && configPath.endsWith('.enabled')) {


Not sure what would be the best solution here. New platform checks enabled status for every plugin (eg. pid.enabled), but old platform doesn't know these keys and throws error. So special casing and disabling such plugins was the simplest solution.

We can remove this (as #14288 is merged)

azasypkin · 2017-10-02T19:04:05Z

platform/legacy/__tests__/LegacyPlatformProxifier.test.ts

+import { EventEmitter } from 'events';
+import { IncomingMessage, ServerResponse } from 'http';
+
+const mockConfigService = jest.fn(() => ({


A lot of stuff here is just to mock Root, maybe we just need RootMock or something, but I didn't think about it too much.

What about something like this:

diff --git a/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts b/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts index 57ce148ea4..f25cfad479 100644 --- a/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts +++ b/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts @@ -1,34 +1,6 @@ import { EventEmitter } from 'events'; import { IncomingMessage, ServerResponse } from 'http'; -const mockConfigService = jest.fn(() => ({ - atPath: jest.fn() -})); -jest.mock('../../config/ConfigService', () => ({ - ConfigService: mockConfigService -})); - -const mockServer = jest.fn(() => ({ - start: jest.fn(), - stop: jest.fn() -})); -jest.mock('../../server', () => ({ Server: mockServer })); - -const mockLoggingService = jest.fn(() => ({ - upgrade: jest.fn(), - stop: jest.fn() -})); -jest.mock('../../logging/LoggingService', () => ({ - LoggingService: mockLoggingService -})); - -const mockMutableLoggerFactory = jest.fn(() => ({ - get: jest.fn(() => ({ info: jest.fn(), debug: jest.fn() })) -})); -jest.mock('../../logging/LoggerFactory', () => ({ - MutableLoggerFactory: mockMutableLoggerFactory -})); - class mockNetServer extends EventEmitter { address() { return { port: 1234, family: 'test-family', address: 'test-address' }; @@ -43,7 +15,6 @@ jest.mock('net', () => ({ })); import { BehaviorSubject } from 'rxjs'; -import { Root } from '../../root'; import { Env, ObjectToRawConfigAdapter } from '../../config'; import { LegacyPlatformProxifier } from '..'; import { Server } from 'net'; @@ -54,11 +25,17 @@ beforeEach(() => { const env = new Env('.', {}); const config$ = new BehaviorSubject(new ObjectToRawConfigAdapter({})); - root = new Root(config$, env, () => {}); + root = { + start: jest.fn(), + shutdown: jest.fn(), + logger: { + get: jest.fn(() => ({ + info: jest.fn(), + debug: jest.fn() + })) + } + } as any; proxifier = new LegacyPlatformProxifier(root); - - jest.spyOn(root, 'start'); - jest.spyOn(root, 'shutdown'); }); test('correctly binds to the server.', () => {

Yep, it's much better, thanks 👍 any to the rescue :)

azasypkin · 2017-10-02T19:08:52Z

src/cli/serve/serve.js

@@ -171,10 +173,17 @@ export default function (program) {
    }

    process.on('SIGHUP', function reloadConfig() {
-      const settings = getCurrentSettings();
+      const settings = transformDeprecations(getCurrentSettings());
+      const config = new Config(kbnServer.config.getSchema(), settings);


I use existing schema since I need schemas registered by plugins (e.g. elasticsearch), default schema doesn't contain those.

azasypkin · 2017-10-02T19:14:32Z

src/cli/serve/serve.js

      kbnServer.server.log(['info', 'config'], 'Reloaded logging configuration due to SIGHUP.');
+
+      // If new platform config subscription is active, let's notify it with the updated config.
+      if (kbnServer.newPlatformConfig) {


There are only two legacy kibana server tests that are failing because of this PR. I haven't fixed them in this PR, but plan to do so in the follow-ups, just need more time to figure out the best way to do so.

src/ui/__tests__/ui_exports_replace_injected_vars.js - test creates KibanaServer with custom plugins path that doesn't include elasticsearch and config doesn't include schema for it as well, but it's a requirement for the new platform, fix is easy.

src/cli/serve/__tests__/reload_logging_config.js - this test is full of magic and fails just because of absence of tags property in new platform LogRecord, fix is also trivial, but I've deferred that to PR that will introduce legacy-json and legacy-pattern layouts.

👍 We don't have to do them in this PR.

azasypkin · 2017-10-02T19:15:21Z

@kjbekkelund PR is now ready for your review :)

…aks.

…g.dest,logging.json` values into new format.

… anyway.

…plugins) for new config on SIGHUP.

kimjoar

This is a first pass. It's looking good, and I think we're almost there. I just wanted to send this feedback before taking another round through this to make sure I understand all of it.

kimjoar · 2017-10-04T08:39:03Z

platform/config/Env.ts

@@ -1,8 +1,11 @@
 import * as process from 'process';
 import { resolve } from 'path';

+import { LegacyPlatformProxifier } from '../legacy';
+
 interface WithConfig {


Maybe rename to EnvOptions, and rename argv to options in the Env class?

kimjoar · 2017-10-04T10:58:51Z

platform/legacy/LegacyPlatformConfig.ts

+    configPath = LegacyConfigToRawConfigAdapter.flattenConfigPath(configPath);
+
+    // Explicitly disable plugins until they are supported by the legacy config.
+    if (!this.legacyConfig.has(configPath) && configPath.endsWith('.enabled')) {


We can remove this (as #14288 is merged)

kimjoar · 2017-10-04T11:03:31Z

platform/server/http/HttpServer.ts

-    this.httpServer.close();
+    this.log.info('stopping http server');
+
+    if (!this.server) {


nitpick: if (this.server == null)

✔️ (== undefined since we changed type to Server | undefined above).

kimjoar · 2017-10-04T11:06:06Z

platform/server/http/HttpServer.ts

 import { Router } from './Router';

 export class HttpServer {
  private readonly app: express.Application;
-  private readonly httpServer: http.Server;
+  private server: Server | null = null;


nitpick: Any specific reason for null? Otherwise, maybe use undefined instead? I'm okey with either, but undefined is a bit more consistent with the rest of the code base.

Nope, there is no reason, just used null for some reason :) By the way, should we use server: Server | undefined or just server?: Server?

✔️ (chose private server?: Server;, let me know if you disagree)

kimjoar · 2017-10-04T11:07:54Z

platform/server/http/HttpServer.ts

+    this.server = this.initializeServer(config);
+
+    const proxyListener = this.env.getNewPlatformProxyListener();
+    if (proxyListener) {


nitpick: proxyListener !== undefined

kimjoar · 2017-10-04T11:31:16Z

platform/legacy/index.ts

+  );
+
+  kbnServer.newPlatformProxyListener = new LegacyPlatformProxifier(
+    new Root(config$, Env.createDefault({ kbnServer }), () => {})


nitpick: Maybe make the last param just default to noop in Root?

✔️ (I'm trying to avoid using lodash where possible, so used () => {})

kimjoar · 2017-10-04T12:09:21Z

platform/legacy/__tests__/LegacyPlatformProxifier.test.ts

+import { EventEmitter } from 'events';
+import { IncomingMessage, ServerResponse } from 'http';
+
+const mockConfigService = jest.fn(() => ({


What about something like this:

diff --git a/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts b/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts index 57ce148ea4..f25cfad479 100644 --- a/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts +++ b/platform/legacy/__tests__/LegacyPlatformProxifier.test.ts @@ -1,34 +1,6 @@ import { EventEmitter } from 'events'; import { IncomingMessage, ServerResponse } from 'http'; -const mockConfigService = jest.fn(() => ({ - atPath: jest.fn() -})); -jest.mock('../../config/ConfigService', () => ({ - ConfigService: mockConfigService -})); - -const mockServer = jest.fn(() => ({ - start: jest.fn(), - stop: jest.fn() -})); -jest.mock('../../server', () => ({ Server: mockServer })); - -const mockLoggingService = jest.fn(() => ({ - upgrade: jest.fn(), - stop: jest.fn() -})); -jest.mock('../../logging/LoggingService', () => ({ - LoggingService: mockLoggingService -})); - -const mockMutableLoggerFactory = jest.fn(() => ({ - get: jest.fn(() => ({ info: jest.fn(), debug: jest.fn() })) -})); -jest.mock('../../logging/LoggerFactory', () => ({ - MutableLoggerFactory: mockMutableLoggerFactory -})); - class mockNetServer extends EventEmitter { address() { return { port: 1234, family: 'test-family', address: 'test-address' }; @@ -43,7 +15,6 @@ jest.mock('net', () => ({ })); import { BehaviorSubject } from 'rxjs'; -import { Root } from '../../root'; import { Env, ObjectToRawConfigAdapter } from '../../config'; import { LegacyPlatformProxifier } from '..'; import { Server } from 'net'; @@ -54,11 +25,17 @@ beforeEach(() => { const env = new Env('.', {}); const config$ = new BehaviorSubject(new ObjectToRawConfigAdapter({})); - root = new Root(config$, env, () => {}); + root = { + start: jest.fn(), + shutdown: jest.fn(), + logger: { + get: jest.fn(() => ({ + info: jest.fn(), + debug: jest.fn() + })) + } + } as any; proxifier = new LegacyPlatformProxifier(root); - - jest.spyOn(root, 'start'); - jest.spyOn(root, 'shutdown'); }); test('correctly binds to the server.', () => {

kimjoar · 2017-10-04T12:22:43Z

platform/legacy/__mocks__/LegacyConfigMock.ts

@@ -0,0 +1,27 @@
+export class LegacyConfigMock {


Maybe add a note above this with something like:

This is a partial mock of ~/src/server/config/config.js

kimjoar · 2017-10-04T12:23:02Z

platform/legacy/__mocks__/LegacyConfigMock.ts

+  readonly has: jest.Mock<boolean>;
+
+  constructor(public __rawData: Map<string, any> = new Map()) {
+    this.get = jest.fn(key => {


Not sure if better, but could be:

diff --git a/platform/legacy/__mocks__/LegacyConfigMock.ts b/platform/legacy/__mocks__/LegacyConfigMock.ts index 05d5614c1a..dc91c8e84a 100644 --- a/platform/legacy/__mocks__/LegacyConfigMock.ts +++ b/platform/legacy/__mocks__/LegacyConfigMock.ts @@ -1,10 +1,7 @@ export class LegacyConfigMock { - readonly get: jest.Mock<any>; - readonly set: jest.Mock<void>; - readonly has: jest.Mock<boolean>; + constructor(public __rawData: Map<string, any> = new Map()) {} - constructor(public __rawData: Map<string, any> = new Map()) { - this.get = jest.fn(key => { + get = jest.fn(key => { // Real legacy config throws error if key is not presented in the schema. if (!this.__rawData.has(key)) { throw new TypeError(`Unknown schema key: ${key}`); @@ -13,7 +10,7 @@ export class LegacyConfigMock { return this.__rawData.get(key); }); - this.set = jest.fn((key, value) => { + set = jest.fn((key, value) => { // Real legacy config throws error if key is not presented in the schema. if (!this.__rawData.has(key)) { throw new TypeError(`Unknown schema key: ${key}`); @@ -22,6 +19,5 @@ export class LegacyConfigMock { this.__rawData.set(key, value); }); - this.has = jest.fn(key => this.__rawData.has(key)); - } + has = jest.fn(key => this.__rawData.has(key)); }

It's better, thanks :) We should always offload constructor code if there is a way to do so.

✔️

kimjoar · 2017-10-04T13:08:57Z

src/cli/serve/serve.js

      kbnServer.server.log(['info', 'config'], 'Reloaded logging configuration due to SIGHUP.');
+
+      // If new platform config subscription is active, let's notify it with the updated config.
+      if (kbnServer.newPlatformConfig) {


👍 We don't have to do them in this PR.

kimjoar · 2017-10-04T15:49:57Z

Went through it again, and I just have the mostly nitpicks above, otherwise I'm good with merging this.

…names, do not expose RxJS Subject to legacy platform).

azasypkin · 2017-10-05T12:09:21Z

Pushed commit that handles your comments @kjbekkelund, should be ready for the next round.

kimjoar

Looks like we’re good to go! 🎉🎉 LGTM

azasypkin · 2017-10-05T12:58:35Z

Thanks! 🎉 Let's see if it sticks.

azasypkin added the Feature:New Platform label Sep 5, 2017

azasypkin force-pushed the issue-12520-expressjs-hapi branch from 8391303 to 213f7ee Compare September 6, 2017 10:20

azasypkin commented Sep 7, 2017

View reviewed changes

kimjoar reviewed Sep 12, 2017

View reviewed changes

azasypkin force-pushed the issue-12520-expressjs-hapi branch 4 times, most recently from 8bd9bb2 to 7565b4e Compare September 22, 2017 11:58

azasypkin commented Sep 22, 2017

View reviewed changes

kimjoar reviewed Sep 22, 2017

View reviewed changes

kimjoar reviewed Sep 25, 2017

View reviewed changes

azasypkin changed the title ~~[WIP] Proxy ExpressJS requests to HapiJS.~~ Proxy ExpressJS requests to HapiJS. Oct 2, 2017

azasypkin commented Oct 2, 2017

View reviewed changes

azasypkin added 3 commits October 4, 2017 13:39

Proxy ExpressJS requests to HapiJS.

c75d2f4

Add support for application/nd-json.

ec9eb2f

Simplify Kibana proxy middleware.

64ef4cb

azasypkin added 19 commits October 4, 2017 13:40

Introduce LegacyKbnServer interface.

9a65ada

Env.proxy --> Env.getNewPlatformProxyListener and other minor twe…

1a14061

…aks.

Fix event forwarding signature.

b90527e

Transform legacy `logging.verbose,logging.quiet,logging.silent,loggin…

a87a832

…g.dest,logging.json` values into new format.

Do not transform PID and XPack configs since plugins will be disabled…

1d0af47

… anyway.

Propagate legacy config updates to the new platform.

b909f07

Simplify injectIntoKbnServer.

6482bbf

Update ConfigService tests.

39a9eaf

Update applyArgv tests.

fdb878f

Add Env tests.

6a780d8

Update RawConfigService tests.

7893ab0

Add LegacyConfig tests.

cf9cc7d

Add LegacyPlatformProxifier tests.

2f86060

Update HttpService tests.

1baefac

Include body parser for new platform routes only.

b4f6f71

Update HttpServer tests.

3d9b397

Use schema of existing config (eg. including schemas provided by the …

52f7091

…plugins) for new config on SIGHUP.

Add tests for request forwarding.

83944c6

Get rid of special case for plugin.enabled in the legacy config class.

6385ef4

azasypkin force-pushed the issue-12520-expressjs-hapi branch from 7166d81 to 6385ef4 Compare October 4, 2017 11:47

kimjoar reviewed Oct 4, 2017

View reviewed changes

Handle review feedback (improved tests, undefined checks, variable …

629b3f6

…names, do not expose RxJS Subject to legacy platform).

kimjoar approved these changes Oct 5, 2017

View reviewed changes

azasypkin merged commit 50231b1 into elastic:new-platform Oct 5, 2017

azasypkin deleted the issue-12520-expressjs-hapi branch October 5, 2017 13:06

kimjoar mentioned this pull request Oct 10, 2017

Running old and new platform at the same time #12520

Closed

kimjoar added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Nov 21, 2017


		import { Logger } from '../../../logging';

		export class Proxy extends EventEmitter {

Proxy ExpressJS requests to HapiJS. #13844

Proxy ExpressJS requests to HapiJS. #13844

Conversation

azasypkin commented Sep 5, 2017

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

azasypkin Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Sep 7, 2017 • 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

azasypkin commented Sep 7, 2017 • edited Loading

kimjoar 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

azasypkin Sep 18, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Sep 25, 2017 • 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

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

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

azasypkin Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Sep 7, 2017 •

edited

Loading

azasypkin Sep 7, 2017 •

edited

Loading

azasypkin commented Sep 7, 2017 •

edited

Loading

azasypkin Sep 18, 2017 •

edited

Loading

azasypkin Sep 25, 2017 •

edited

Loading

azasypkin Sep 25, 2017 •

edited

Loading