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
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
140 changes: 0 additions & 140 deletions src/cli/cluster/base_path_proxy.js

This file was deleted.

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 './configure_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
64 changes: 64 additions & 0 deletions src/cli/cluster/configure_base_path_proxy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* 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 { Server } from 'hapi';
import { createBasePathProxy } from '../../core';
import { setupLogging } from '../../server/logging';

export async function configureBasePathProxy(config) {
// 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();
setupLogging(server, config);

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

await basePathProxy.configure({
Copy link
Contributor

Choose a reason for hiding this comment

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

design question: Why are the args in configure() separate from the options passed to createBasePathProxy()? Doesn't it just add complexity to the BasePathProxyRoot which might not be fully configured when it's methods are called?

isKibanaPath: path => {
Copy link
Contributor

@spalger spalger Jun 22, 2018

Choose a reason for hiding this comment

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

I think the naming here is a little misleading, the idea originally was to define a subset of the urls that would support redirects, with the primary intention being to support urls that would be in browsers url bars, and not urls that are likely to be hard coded into code (apis, assets, etc.). Maybe we should rename this to shouldRedirectFromOldBasePath(path: string) => boolean

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, sure, will use that name.

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

return new Promise(resolve => {
const done = () => {
serverWorker.removeListener('listening', done);
serverWorker.removeListener('crashed', done);

resolve();
};

serverWorker.on('listening', done);
serverWorker.on('crashed', done);
});
},
});

return basePathProxy;
}
Loading