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

Move BasePathProxy to the new platform #19424

Merged
merged 11 commits into from
Jun 25, 2018
Merged
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
"glob-all": "3.0.1",
"good-squeeze": "2.1.0",
"h2o2": "5.1.1",
"h2o2-latest": "npm:[email protected]",
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we were discussing this previously so I know you may worry about using these yarn aliases and two separate hapi and h202 libs, but I'll get rid of it closer to the merge if we eventually decide that hapi 17 is no-go for now (still hope we can go with the latest and greatest for server/core, but it's not blocker just easier to work with: latest docs, @types/... typings, and subjectively better response/request management API ergonomics).

Copy link

@rhoboat rhoboat Jun 21, 2018

Choose a reason for hiding this comment

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

So the current status is that we are still going with hapi 17 (#18562 merged), yes? Can we resolve this to a single h202 now, or still no?

Copy link
Member Author

@azasypkin azasypkin Jun 21, 2018

Choose a reason for hiding this comment

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

My plan was to use Hapi 17 in kbn-core as long as possible and discuss this when #18951 is ready to be merged. If we decide to move forward with the Hapi 14 then I'll create a dedicated PR just for Hapi 17 -> Hapi 14 migration. But I'd really want to keep using Hapi 17 and start battle-testing it through new platform and eventually push #13802 forward.

Can we resolve this to a single h202 now, or still no?

Hmm, what do you mean? Currently h202 plugin is used by both old (Hapi 14) and new (Hapi 17) platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I'm going to see how that affects the build size to make more informed decision.

Copy link

Choose a reason for hiding this comment

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

Oh, I see, we still need both because we are still running this plugin in both hapi 14 and 17. 👍

"handlebars": "4.0.5",
"hapi": "14.2.0",
"hapi-latest": "npm:[email protected]",
Expand Down
146 changes: 35 additions & 111 deletions src/cli/cluster/base_path_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,123 +18,47 @@
*/

import { Server } from 'hapi';
import { notFound } from 'boom';
import { map, sample } from 'lodash';
import { map as promiseMap, fromNode } from 'bluebird';
import { Agent as HttpsAgent } from 'https';
import { readFileSync } from 'fs';

import { setupConnection } from '../../server/http/setup_connection';
import { registerHapiPlugins } from '../../server/http/register_hapi_plugins';
import { createBasePathProxy } from '../../core';
import { setupLogging } from '../../server/logging';

const alphabet = 'abcdefghijklmnopqrztuvwxyz'.split('');
export async function configureBasePathProxy(config) {
Copy link

@rhoboat rhoboat Jun 21, 2018

Choose a reason for hiding this comment

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

So, this is the function that takes in a config, creates a basePathProxy, and then runs .configure on it using the input config. It retuns a configured basePathProxy.

What do you think of naming this configure_base_path_proxy or configured_base_path_proxy? I think base_path_proxy as a name should probably refer to the server, which is in src/core/server/http/base_path_proxy_server.js (which we appended 'server' to, but could work even without that).

Also, what about a simple test file in this directory for this function?

Copy link
Member Author

@azasypkin azasypkin Jun 21, 2018

Choose a reason for hiding this comment

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

Yeah, +1 to both of your ideas, will handle them.

// New platform forwards all logs to the legacy platform so we need HapiJS server
// here just for logging purposes and nothing else.
const server = new Server();
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we're hostages of Hapi 14 and even-better until we roll out new logging system or do some other magic :)

Copy link

@rhoboat rhoboat Jun 21, 2018

Choose a reason for hiding this comment

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

[redacted]

I had asked why, but I think it makes sense. Will be happy to create a new logging system not dependent on even-better

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it will be handled in the scope of #13241 probably. And I don't want to use even-better in the new platform even the latest version.

setupLogging(server, config);

const basePathProxy = createBasePathProxy({ server, config });

await basePathProxy.configure({
isKibanaPath: (path) => {
Copy link
Member Author

@azasypkin azasypkin May 29, 2018

Choose a reason for hiding this comment

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

note: Tell me if isKibanaPath and/or blockUntil look like premature abstractions to you. The motivation was to:

  • not pass ClusterManager or ClusterManager.workers to new platform directly
  • not have /app, /login, /logout and /status knowledge in new platform for now.

Copy link

Choose a reason for hiding this comment

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

I think it's fine.

const isApp = path.startsWith('app/');
const isKnownShortPath = ['login', 'logout', 'status'].includes(path);

return isApp || isKnownShortPath;
},

blockUntil: () => {
// Wait until `serverWorker either crashes or starts to listen.
// The `serverWorker` property should be set by the ClusterManager
// once it creates the worker.
const serverWorker = basePathProxy.serverWorker;
if (serverWorker.listening || serverWorker.crashed) {
return Promise.resolve();
}

export default class BasePathProxy {
constructor(clusterManager, config) {
this.clusterManager = clusterManager;
this.server = new Server();
return new Promise((resolve) => {
const done = () => {
serverWorker.removeListener('listening', done);
serverWorker.removeListener('crashed', done);

this.targetPort = config.get('dev.basePathProxyTarget');
this.basePath = config.get('server.basePath');
resolve();
};

const sslEnabled = config.get('server.ssl.enabled');
if (sslEnabled) {
this.proxyAgent = new HttpsAgent({
key: readFileSync(config.get('server.ssl.key')),
passphrase: config.get('server.ssl.keyPassphrase'),
cert: readFileSync(config.get('server.ssl.certificate')),
ca: map(config.get('server.ssl.certificateAuthorities'), readFileSync),
rejectUnauthorized: false
serverWorker.on('listening', done);
serverWorker.on('crashed', done);
});
}
});

if (!this.basePath) {
this.basePath = `/${sample(alphabet, 3).join('')}`;
config.set('server.basePath', this.basePath);
}

const ONE_GIGABYTE = 1024 * 1024 * 1024;
config.set('server.maxPayloadBytes', ONE_GIGABYTE);

setupLogging(this.server, config);
setupConnection(this.server, config);
registerHapiPlugins(this.server, config);

this.setupRoutes();
}

setupRoutes() {
const { clusterManager, server, basePath, targetPort } = this;

server.route({
method: 'GET',
path: '/',
handler(req, reply) {
return reply.redirect(basePath);
}
});

server.route({
method: '*',
path: `${basePath}/{kbnPath*}`,
config: {
pre: [
(req, reply) => {
promiseMap(clusterManager.workers, worker => {
if (worker.type === 'server' && !worker.listening && !worker.crashed) {
return fromNode(cb => {
const done = () => {
worker.removeListener('listening', done);
worker.removeListener('crashed', done);
cb();
};

worker.on('listening', done);
worker.on('crashed', done);
});
}
})
.return(undefined)
.nodeify(reply);
}
],
},
handler: {
proxy: {
passThrough: true,
xforward: true,
agent: this.proxyAgent,
protocol: server.info.protocol,
host: server.info.host,
port: targetPort,
}
}
});

server.route({
method: '*',
path: `/{oldBasePath}/{kbnPath*}`,
handler(req, reply) {
const { oldBasePath, kbnPath = '' } = req.params;

const isGet = req.method === 'get';
const isBasePath = oldBasePath.length === 3;
const isApp = kbnPath.startsWith('app/');
const isKnownShortPath = ['login', 'logout', 'status'].includes(kbnPath);

if (isGet && isBasePath && (isApp || isKnownShortPath)) {
return reply.redirect(`${basePath}/${kbnPath}`);
}

return reply(notFound());
}
});
}

async listen() {
await fromNode(cb => this.server.start(cb));
this.server.log(['listening', 'info'], `basePath Proxy running at ${this.server.info.uri}${this.basePath}`);
}

return basePathProxy;
}
22 changes: 16 additions & 6 deletions src/cli/cluster/cluster_manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import { debounce, invoke, bindAll, once, uniq } from 'lodash';

import Log from '../log';
import Worker from './worker';
import BasePathProxy from './base_path_proxy';
import { Config } from '../../server/config/config';
import { transformDeprecations } from '../../server/config/transform_deprecations';
import { configureBasePathProxy } from './base_path_proxy';

process.env.kbnWorkerType = 'managr';

Expand All @@ -33,10 +33,14 @@ export default class ClusterManager {
const transformedSettings = transformDeprecations(settings);
const config = await Config.withDefaultSchema(transformedSettings);

return new ClusterManager(opts, config);
const basePathProxy = opts.basePath
? await configureBasePathProxy(config)
: undefined;

return new ClusterManager(opts, config, basePathProxy);
}

constructor(opts, config) {
constructor(opts, config, basePathProxy) {
this.log = new Log(opts.quiet, opts.silent);
this.addedCount = 0;
this.inReplMode = !!opts.repl;
Expand All @@ -47,8 +51,8 @@ export default class ClusterManager {
'--server.autoListen=false',
];

if (opts.basePath) {
this.basePathProxy = new BasePathProxy(this, config);
if (basePathProxy) {
this.basePathProxy = basePathProxy;

optimizerArgv.push(
`--server.basePath=${this.basePathProxy.basePath}`,
Expand Down Expand Up @@ -78,6 +82,12 @@ export default class ClusterManager {
})
];

if (basePathProxy) {
// Pass server worker to the basepath proxy so that it can hold off the
// proxying until server worker is ready.
this.basePathProxy.serverWorker = this.server;
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea that constructing the ClusterManager mutates the basePathProxy irks me, what if we did most of the stuff happening in this constructor up in ClusterManager.create() and then just passed the workers, log, basePathProxy, etc. into the constructor? Then we could have access to the serverWorker and just pass it to the configureBasePathProxy()

}

// broker messages between workers
this.workers.forEach((worker) => {
worker.on('broadcast', (msg) => {
Expand Down Expand Up @@ -120,7 +130,7 @@ export default class ClusterManager {
this.setupManualRestart();
invoke(this.workers, 'start');
if (this.basePathProxy) {
this.basePathProxy.listen();
this.basePathProxy.start();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,6 @@ Object {
],
"type": "log",
},
Object {
"@timestamp": "## @timestamp ##",
"message": "starting the server",
"pid": "## PID ##",
"tags": Array [
"info",
"root",
],
"type": "log",
},
Object {
"@timestamp": "## @timestamp ##",
"message": "starting server :tada:",
Expand Down
5 changes: 4 additions & 1 deletion src/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,7 @@
* under the License.
*/

export { injectIntoKbnServer } from './server/legacy_compat';
export {
injectIntoKbnServer,
createBasePathProxy,
} from './server/legacy_compat';
44 changes: 44 additions & 0 deletions src/core/server/dev/dev_config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { schema, TypeOf } from '../config/schema';

const createDevSchema = schema.object({
basePathProxyTarget: schema.number({
defaultValue: 5603,
}),
});

type DevConfigType = TypeOf<typeof createDevSchema>;

export class DevConfig {
/**
* @internal
*/
public static schema = createDevSchema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just make this private?

Copy link
Member Author

Choose a reason for hiding this comment

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


public basePathProxyTargetPort: number;

/**
* @internal
*/
constructor(config: DevConfigType) {
this.basePathProxyTargetPort = config.basePathProxyTarget;
}
}
20 changes: 20 additions & 0 deletions src/core/server/dev/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { DevConfig } from './dev_config';
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`throws if [redirectHttpFromPort] is in use 1`] = `[Error: Redirect server cannot be started when [ssl.enabled] is set to \`false\` or [ssl.redirectHttpFromPort] is not specified.]`;

exports[`throws if [redirectHttpFromPort] is not specified 1`] = `[Error: Redirect server cannot be started when [ssl.enabled] is set to \`false\` or [ssl.redirectHttpFromPort] is not specified.]`;

exports[`throws if SSL is not enabled 1`] = `[Error: Redirect server cannot be started when [ssl.enabled] is set to \`false\` or [ssl.redirectHttpFromPort] is not specified.]`;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`logs error is already started 1`] = `
exports[`logs error if already started 1`] = `
Object {
"debug": Array [],
"error": Array [],
Expand Down
Loading