Move `BasePathProxy` to the new platform #19424

azasypkin · 2018-05-24T21:11:41Z

Moves basepath proxy to the new platform.

Depends on #18562

azasypkin · 2018-05-29T16:19:13Z

package.json

@@ -130,6 +130,7 @@
    "glob-all": "3.0.1",
    "good-squeeze": "2.1.0",
    "h2o2": "5.1.1",
+    "h2o2-latest": "npm:h2o2@8.1.2",


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).

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?

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.

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

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

azasypkin · 2018-05-29T16:23:51Z

src/cli/cluster/base_path_proxy.js

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


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

[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

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.

azasypkin · 2018-05-29T16:27:11Z

src/cli/cluster/base_path_proxy.js

+  const basePathProxy = createBasePathProxy({ server, config });
+
+  await basePathProxy.configure({
+    isKibanaPath: (path) => {


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.

I think it's fine.

azasypkin · 2018-05-29T16:31:44Z

src/cli/cluster/cluster_manager.js

@@ -78,6 +82,10 @@ export default class ClusterManager {
      })
    ];

+    // 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;


note: I hate that piece of code, ideally I'd refactor this constructor and move most of the stuf to a initialization/configuration method instead, but that'd take much longer. Please tell me if you see the better way.

azasypkin · 2018-05-29T16:36:26Z

src/core/server/http/https_redirect_server.ts

+ * under the License.
+ */
+
+import { Request, ResponseToolkit, Server } from 'hapi-latest';


note: it's totally independent type of server (I was surprised when I figured out how many different Hapi instances we create in Kibana, btw), so it deserves its own file.

Is there a way to make this not independent of the main kibana server? Or, no benefit? I'd like to talk about alternatives at some point.

also question: what does this have to do with the basePathProxy server?

Is there a way to make this not independent of the main kibana server? Or, no benefit? I'd like to talk about alternatives at some point.

Not sure I'm following, what do you mean? It should always be an independent http server that sits on its own port, I mean we can't have single hapi server that works with two different ports (at least I don't know about such feature).

also question: what does this have to do with the basePathProxy server?

Good catch, it has nothing to do with basePathProxy, I'm just fulfilling the promise to move redirect server to a separate file (#18562 (comment)) and packed this change into this PR to expedite that a bit (how naive! I know).

Sounds fine!

azasypkin · 2018-05-29T16:39:03Z

src/core/server/legacy_compat/index.ts

@@ -61,3 +68,12 @@ export const injectIntoKbnServer = (rawKbnServer: any) => {
    },
  };
 };
+
+export const createBasePathProxy = (rawKbnServer: any) => {


note: couldn't think of a better approach for now, maybe you have other ideas.

this is what, among other things, I wanted to talk to you about over zoom/chat. :)

Sure, just ping me. Btw, it's the first thing I'm going to get rid of in #19994, so this temp solution shouldn't live for too long.

azasypkin · 2018-05-29T16:50:11Z

src/server/http/index.js

+    state: {
+      strictHeader: false,
+    },
+    routes: {


note: the browser tests revealed the problem I haven't noticed before - if cors is only configured in new platform then all requests that are forwarded to the legacy platform assume that cors is disabled that breaks browser tests. Soooo, it seems we still should preserve some part of server configuration for the legacy platform.

Hmm, I think it will be easy to forget about this duplication if we decide to tweak the server settings in the future... Maybe some comments in both places will be helpful... idk, this is the type of code that we usually don't touch unless there is a decent sized issue like #19808 and only applying the fix to one set of settings or the other might make the problem seemed fix to light testing but be outsized if not fixed correctly...

Yeah, we can probably solve this duplication in a more sophisticated way (like kbnServer.newPlatform.proxyListener.getConnectionOptions()), but I feel like proper comments in both places should work just fine. I'll add these comments, but let me know if you prefer something more than that.

azasypkin · 2018-05-29T16:52:14Z

@elastic/kibana-platform would you mind giving a preliminary feedback on this PR?

Thanks!

…te `ClusterManager` test snapshot.

spalger

Just took a cursory look, noticed a couple things, had a couple questions, and will take a deeper look tomorrow.

spalger · 2018-06-13T00:49:37Z

src/core/server/root/index.ts

  private readonly loggingService: LoggingService;

  constructor(
    rawConfig$: Observable<RawConfig>,
-    private readonly env: Env,
+    protected readonly env: Env,


Is this necessary? Doesn't look like this.env is accessed outside of the Root class.

Right, it's not necessary, thanks! Looks like a left over from my previous PR version.

spalger · 2018-06-13T00:57:32Z

src/server/http/index.js

+    state: {
+      strictHeader: false,
+    },
+    routes: {


Hmm, I think it will be easy to forget about this duplication if we decide to tweak the server settings in the future... Maybe some comments in both places will be helpful... idk, this is the type of code that we usually don't touch unless there is a decent sized issue like #19808 and only applying the fix to one set of settings or the other might make the problem seemed fix to light testing but be outsized if not fixed correctly...

spalger · 2018-06-13T00:58:52Z

src/core/server/http/http_server.ts

+  }
+
+  const server = new Server(options);
+  server.listener.on('clientError', (err, socket) => {


I think we need to make sure that this server.listener gets this treatment ec69a02

Yep, thanks!

spalger · 2018-06-13T01:00:53Z

src/core/server/http/https_redirect_server.ts

+      }]`
+    );
+
+    this.server = new Server({


This server probably needs the fixes from ec69a02 too, no?

Since this request responds in the onRequest phase there isn't actually a need for state, routes.cors or routes.payload settings like we have elsewhere, but I'm slightly concerned about these three servers drifting in other ways down the road...

This server probably needs the fixes from ec69a02 too, no?

Theoretically yes, but not sure for what use cases redirect server it's used in practice. I'll add it here. That also means that we forgot to apply ec69a02 in master...

Since this request responds in the onRequest phase there isn't actually a need for state, routes.cors or routes.payload settings like we have elsewhere, but I'm slightly concerned about these three servers drifting in other ways down the road...

Are you implying that we may want to use configureHttpServer (successor of setup_connection) here as well? Sounds like we can and probably even should do that.

Nice, yeah, this looks good.

…add comments for duplicated server options, use the same server options for main http server, base path http server and http->https redirect server).

azasypkin · 2018-06-13T09:59:52Z

Note to myself: don't merge master directly to this branch, merge master into kbn-core instead and then merge kbn-core into this branch (forgot about that and that's why there are two merge commits 🙈 ) :)

rhoboat · 2018-06-21T01:09:35Z

src/core/server/http/base_path_proxy_server.ts

+    }
+  }
+
+  public async start() {


nit: up to you, of course, but wondering if you think splitting start into smaller pieces would make this easier to read? maybe each server.route(...) can be its own method, maybe something like a setupRoutes for them all.

Yep, I think having setupRoutes is a good idea, thanks!

rhoboat · 2018-06-21T01:20:10Z

src/core/server/http/base_path_proxy_server.ts

+          async (request, responseToolkit) => {
+            await blockUntil();
+            return responseToolkit.continue;
+          },


This is interesting. So if we didn't have blockUntil abstraction, we'd hardcode waiting for the listener to be ready? (Just asking if I got that right) If that's true, then how does it look without this abstraction--would we need to reference ClusterManager here? My main motivation in asking is, wouldn't it always be a good idea to first wait for the listener on that port before proxying to it?

So if we didn't have blockUntil abstraction, we'd hardcode waiting for the listener to be ready? (Just asking if I got that right). If that's true, then how does it look without this abstraction--would we need to reference ClusterManager here?

Yeah, and listener here is the process fork (we call it worker) that initializes another hapi server. So we'd either need a reference to ClusterManager or its worker at least, but I'm not sure I want new platform know about these things yet :)

My main motivation in asking is, wouldn't it always be a good idea to first wait for the listener on that port before proxying to it?

Do you mean we should make blockUntil as required option (not optional as it's now) and hence always register this pre hook?

Do you mean we should make blockUntil as required option (not optional as it's now) and hence always register this pre hook?

Yes, either require blockUntil, or more generally, just go ahead and always run a default blockUntil that waits for a listener to be ready. Is that not possible because we don't have awareness of what listener to wait for?

I'll just require blockUntil for now.

rhoboat · 2018-06-21T01:22:36Z

src/core/server/http/base_path_proxy_server.ts

+        const { oldBasePath, kbnPath = '' } = request.params;
+
+        const isGet = request.method === 'get';
+        const isBasepathLike = oldBasePath.length === 3;


Is this too assumptive? Just checking that the length is 3? Hmm.. Why do we need this check anyway?

Also, why is it important that the request method is GET ?

I've tried to explain that in the comment above, but I just copied that "thing" from master.

rhoboat · 2018-06-21T01:24:53Z

src/core/server/http/base_path_proxy_server.ts

+    // It may happen that basepath has changed, but user still uses the old one,
+    // so we can try to check if that's the case and just redirect user to the
+    // same URL, but with valid basepath.
+    server.route({


I have some questions about this route. How is it that the basePath would change? Is that something we could subscribe to? So, the way this is written, we will always have a parameter oldBasePath in request.params when the basePath has changed? I don't see oldBasePath anywhere else in the code, so I am confused.

Sorry it looks like I'm failed to add more clarity to this weird thing from master, so I'll be really grateful if you can come up with a better wording! :) See

kibana/src/cli/cluster/base_path_proxy.js

Lines 115 to 133 in f0cc432

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

}

});

}

It took me some time to really understand what's going on here, but I think I finally got it: it's a dev-mode only thing, when you run kibana with yarn start we generate random base path (e.g. /foo), then for some reason you modified source code and decided to restart Kibana. After restart we'll have a new base path (e.g. /bar), but if you had already opened some Kibana web page in the browser with /foo base path it would be a pain to manually rewrite base path. So this route will allow you to just refresh web page with the old base path (/foo) and be redirected to the same page with new base path (/bar) automagically. It's brittle (oldBasePath.length === 3) and won't work for any Kibana URL just for some that we know about, so I really classify this as magic :/

Ahhh, very interesting. I appreciate the explanation, thank you! So it's from master. 🆗

rhoboat · 2018-06-21T01:41:29Z

src/cli/cluster/base_path_proxy.js

 import { setupLogging } from '../../server/logging';

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


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?

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

rhoboat · 2018-06-21T02:49:38Z

src/core/server/http/http_tools.ts

+      key: readFileSync(ssl.key!),
+      passphrase: ssl.keyPassphrase,
+      secureOptions: ssl.getSecureOptions(),
+    } as TLSOptions;


question: does saying as TLSOptions basically act as validating that the object can be cast to that type? Or is it not quite as strict as validation?

Hmm, now I'm not sure why I used this construct, it's great that you've noticed! It should be written like this instead, I'll change it:

const tlsOptions: TLSOptions = { ca: config.ssl.certificateAuthorities && config.ssl.certificateAuthorities.map(caFilePath => readFileSync(caFilePath) ), cert: readFileSync(ssl.certificate!), ciphers: config.ssl.cipherSuites.join(':'), // We use the server's cipher order rather than the client's to prevent the BEAST attack. honorCipherOrder: true, key: readFileSync(ssl.key!), passphrase: ssl.keyPassphrase, secureOptions: ssl.getSecureOptions(), }; options.tls = tlsOptions;

rhoboat · 2018-06-21T02:52:27Z

src/core/server/http/http_tools.ts

+ */
+export function getServerOptions(
+  config: HttpConfig,
+  { configureTLS = true } = {}


Interesting. Why default to true?

Well, it's probably arguable, but my reasoning is that config should dictate what server options should be produced by default and if config says ssl:enabled then corresponding options should be included, unless it's specifically overriden for some reason. Does it make sense?

Ah, yes, i see. Makes sense. So even if the config doesn't have ssl:enabled we will try to parse the ssl config?

So even if the config doesn't have ssl:enabled we will try to parse the ssl config?

We parse entire config even if TLS is not enabled (https://github.com/elastic/kibana/blob/kbn-core/src/core/server/http/http_config.ts#L110).

But in this function we won't touch TLS settings if it's not enabled in the config even if configureTLS hint set to true. We just don't have use case for that. We can do that in the future if we need to, but it may be not as easy as it sounds: if TLS isn't enabled in config we may skip some validity checks during config parsing, so here we'll have to check whether it can be enabled by checking that certificate and key are set an so on.

Cool, makes sense. configureTLS is really like "tryToConfigureTLS"

Yeah, exactly

rhoboat · 2018-06-21T02:55:00Z

src/core/server/http/http_tools.ts

+  { configureTLS = true } = {}
+) {
+  // Note that all connection options configured here should be exactly the same
+  // as in the legacy platform server (see `server/http/index`). Any change


can we be super clear please and say src/server/http/index ?

and also, can we be super clear in src/server/http/index.js on line 39, and say src/core/server/http/http_tools?

can we be super clear please and say src/server/http/index ?
and also, can we be super clear in src/server/http/index.js on line 39, and say src/core/server/http/http_tools?

Sure!

rhoboat · 2018-06-21T03:03:00Z

src/core/server/http/https_redirect_server.ts

+    if (!config.ssl.enabled || config.ssl.redirectHttpFromPort === undefined) {
+      throw new Error(
+        'Redirect server cannot be started when [ssl.enabled] is set to `false`' +
+          ' or [ssl.redirectHttpFromPort] is not specified.'


question: does this happen? when do we get into this start function but ssl is disabled or redirectHttpFromPort is undefined?

You're right, currently there is no code that would call start in this case. I was just thinking from position of HttpsRedirectServer that doesn't know who's calling it and receives config with optional ssl.redirectHttpFromPort.

Generic solution for such case would be passing defined redirectHttpFromPort as an argument so that this code can be sure that this is defined, but the problem that this method uses getServerOptions under the hood that requires full HttpConfig... Maybe we can redesign this at some point, but I don't have better ideas right now.

Please tell me what you think would be better to do here and I'll do that.

Hm, now I'm confused about something else. This https redirect server is for routing requests from the http server to a new https server.so why would we ignore the TLS part of options in this server? My understanding is wrong somewhere.

This https redirect server is for routing requests from the http server to a new https server.

Yes.

so why would we ignore the TLS part of options in this server?

Cause this server should only accept HTTP traffic, or it's not what you're asking? Also you can take a look at these issues:

#10142
#10948
#10930

Thanks for links to issues, read through them to refresh my memory on this.

rhoboat · 2018-06-21T03:12:07Z

src/server/http/setup_connection.js

@@ -1,85 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor


Was this setupConnection only ever used in that one place? Hm! (doesn't require a response, rhetorical 😛 )

It's used in two places in master, for base path proxy server and main server. Changes in #18562 made setupConnection to be used only in base path proxy and in this PR we get rid of it completely :)

rhoboat

I left a bunch of comments. I need some clarification on some things. I'd be willing to do a subsequent pass after that.

Two things I request: tests for the src/cli/cluster/base_path_proxy.js and something about this in a README somewhere.

azasypkin · 2018-06-21T09:26:50Z

It would be great if we could update the README in src/core/README.md to reflect the changes from this PR. Maybe a section about how the basepath proxy works. Or maybe another place, maybe that is not the right README (Maybe src/core/server/README.md, which is stubbed see_no_evil )

Definitely, but If you don't mind I'll do that in the scope of #14870 (I intentionally left it out here to save some time since Readme can be easily updated after feature freeze :)).

azasypkin · 2018-06-22T09:01:47Z

src/cli/cluster/configure_base_path_proxy.test.js

+    });
+  });
+
+  describe('configured with the correct `isKibanaPath` and `blockUntil` functions.', async () => {


note: one can say these aren't canonical unit tests as there's too much internal stuff is tested and they will be correct, but I just feel more comfortable with these tests than without them :)

azasypkin · 2018-06-22T09:04:55Z

Thanks for review @archanid! I think it should be ready for the next review pass.

epixa

I could probably approve as-is, but I wanted to get some clarity on something first. Why does basepathproxy get its own path through root rather than just set up alongside the existing http server and the redirect server?

epixa · 2018-06-22T14:49:17Z

src/core/server/dev/dev_config.ts

+  /**
+   * @internal
+   */
+  public static schema = createDevSchema;


Why not just make this private?

It's used outside of this class, see https://github.com/elastic/kibana/blob/kbn-core/src/core/server/config/config_service.ts#L141

rhoboat · 2018-06-22T15:34:20Z

src/core/server/http/__tests__/http_redirect_server.test.ts

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


So uh, should this file be called https_redirect_server.test.ts instead?

haha, yeah, silly me

My LGTM is pending that change. ;)

azasypkin · 2018-06-22T16:12:21Z

Why does basepathproxy get its own path through root rather than just set up alongside the existing http server and the redirect server?

I wanted to keep "startup/cluster model" as close to what we have in master as possible. Even though I may not like it, it's hard to improve without finishing #19994 first. Currently base path proxy lives together with cluster manager in the parent process, where KbnServer is not created (hence new platform "normal" Root is not initialized), so for the new platform it feels like a separate entry point or "different" Root which doesn't need all the services/servers "normal" Root would create.

Does that answer your question @epixa or you were asking about different thing?

epixa · 2018-06-22T16:58:10Z

@azasypkin That does answer my question, thanks. I agree it's not ideal, but we can revisit this after we merge #19994.

epixa

LGTM

rhoboat

I pulled and tested with ssl options, basepath, without basepath, redirect http->https server. Reviewed the code closely.

spalger

Pulled the code, everything works great! I'm not a fan of the amount of instantiate-then-mutate going on when I think with a little reorganization we could be passing all the necessary options for different objects at instantiation time, which I suspect would simplify things a bit and allow us to remove many of the if (!dep) throw ... type situations we are seeing in different places, but that said I think it's possible these are just a symptom of this being a port and if so I'd be fine merging it as is.

spalger · 2018-06-22T17:23:51Z

src/cli/cluster/cluster_manager.js

+    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;


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

spalger · 2018-06-22T17:28:18Z

src/core/server/http/base_path_proxy_server.ts

+  private server?: Server;
+  private httpsAgent?: HttpsAgent;
+
+  get basePath() {


Do these need to be getters?

Not necessarily, I'll turn them into methods

spalger · 2018-06-22T17:33:28Z

src/core/server/http/base_path_proxy_server.ts

+  }
+
+  private setupRoutes() {
+    if (this.server === undefined) {


If this was just inline in start(), or was a helper that received the server and options as an argument, we wouldn't need to worry about weird states like this.

I agree that these === undefined checks aren't nice, but defining routes inside start cluttered this metod a bit, so during review we agreed to moved this to a dedicated method. "Static" helper will need additional httpsAgent parameter (or create it inside and return to BasePathProxyServer so that we can destroy it once server is stopped).

None of the alternatives stands out to be honest, and that's really an ergonomics issue with the approach we use across many services/servers inside new platform: we lazily create stuff (e.g. server) inside of start method, right before we want to use it so that we can asynchronously (via Observable) grab the latest configuration object. And we can't put everything into start in every case.

So I'm going to keep this as is for now if you don't mind but obviously we should discuss this "pattern" at some point and find a trade off so that we can have a ready recommendation for other Kibana folks once they start interacting with new platform.

spalger · 2018-06-22T17:37:30Z

src/cli/cluster/configure_base_path_proxy.js

+  const basePathProxy = createBasePathProxy({ server, config });
+
+  await basePathProxy.configure({
+    isKibanaPath: path => {


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

Yeah, sure, will use that name.

spalger · 2018-06-22T18:30:14Z

src/cli/cluster/configure_base_path_proxy.js

+
+  const basePathProxy = createBasePathProxy({ server, config });
+
+  await basePathProxy.configure({


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?

azasypkin · 2018-06-22T19:25:14Z

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?

You can't even imagine how that irks me @spalger :) I'll read your comments more attentively on Monday to make sure I don't miss anything, but from what I remember the main issue here is that Worker needs to know basePath at the instantiation time, but basePath is dynamically generated by the BasePathProxy so it needs to be created first and at the same time it requires blockUntil that in its turn depends on Worker itself. We could create BasePathProxy that generates basePath in the constructor and get basePath before we call configure (the thing that requires blockUntil that depends on Worker), but the idea of using half-initialized BasePathProxy was killing me. At that time I couldn't think anything better apart from extensive refactoring of the way ClusterManager and Workers are connected and instantiated. But will give it another thought on Monday :)

spalger · 2018-06-22T19:39:21Z

Awesome, yeah, I figured there were good reasons for all of the weird mutation stuff happening. Don't spend too much time on it, best I can tell it works and it's NOT (typo, oops) totally confusing, so feel free to merge if you'd rather move onto other things.

azasypkin · 2018-06-25T10:43:36Z

Awesome, yeah, I figured there were good reasons for all of the weird mutation stuff happening. Don't spend too much time on it, best I can tell it works and it's NOT (typo, oops) totally confusing, so feel free to merge if you'd rather move onto other things.

Thanks, yeah I think I'll merge it to unblock other pending PRs, but won't forget about it and try to handle some of the most confusing parts in #19994 :)

azasypkin mentioned this pull request May 24, 2018

[New Platform] Replace Express with Hapi 17. #18562

Merged

5 tasks

azasypkin force-pushed the new-platform-basepath branch from e466e71 to 3759ed1 Compare May 25, 2018 13:52

azasypkin changed the base branch from new-platform to kbn-core May 29, 2018 15:11

azasypkin force-pushed the new-platform-basepath branch from 3759ed1 to 04bd398 Compare May 29, 2018 15:11

azasypkin commented May 29, 2018

View reviewed changes

azasypkin changed the title ~~[WIP] Move BasePathProxy to the new platform~~ Move BasePathProxy to the new platform May 29, 2018

azasypkin added feedback_needed Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels May 29, 2018

Move BasePathProxy to new platform.

cf5df17

azasypkin force-pushed the new-platform-basepath branch from 04bd398 to cf5df17 Compare May 29, 2018 21:29

azasypkin mentioned this pull request May 30, 2018

Use Joi for the config schema validation #19556

Merged

azasypkin added 2 commits May 31, 2018 19:51

Add tests for HttpsRedirectService. Update HttpService tests.

f4e7cad

Do not set serverWorker if BasePathProxy is not initialized. Upda…

f202383

…te `ClusterManager` test snapshot.

azasypkin requested review from epixa, rhoboat and spalger June 5, 2018 09:38

spalger reviewed Jun 13, 2018

View reviewed changes

azasypkin added 3 commits June 13, 2018 09:25

Merge branch 'master' into kbn-core-basepath

6d8f117

Handle review feedback (port keepAliveTimeout setting from master, …

781dc5b

…add comments for duplicated server options, use the same server options for main http server, base path http server and http->https redirect server).

Merge branch 'kbn-core' into kbn-core-basepath

08491fa

Merge branch 'kbn-core' into kbn-core-basepath

29a7927

rhoboat reviewed Jun 21, 2018

View reviewed changes

Handle review feedback.

b487a1b

azasypkin commented Jun 22, 2018

View reviewed changes

epixa reviewed Jun 22, 2018

View reviewed changes

rhoboat reviewed Jun 22, 2018

View reviewed changes

epixa approved these changes Jun 22, 2018

View reviewed changes

rhoboat approved these changes Jun 22, 2018

View reviewed changes

spalger approved these changes Jun 22, 2018

View reviewed changes

azasypkin added 3 commits June 25, 2018 09:41

Rename http_redirect_server.test.ts to https_redirect_server.test.ts

b3509f3

Rename isKibanaPath to shouldRedirectFromOldBasePath.

b6a09f6

Turn basePath and targetPort getters into methods.

941b733

azasypkin merged commit 9fd5e43 into elastic:kbn-core Jun 25, 2018

azasypkin deleted the new-platform-basepath branch June 25, 2018 10:44

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

		@@ -1,85 +0,0 @@
		/*
		* Licensed to Elasticsearch B.V. under one or more contributor


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

		await basePathProxy.configure({

Move BasePathProxy to the new platform #19424

Move BasePathProxy to the new platform #19424

Conversation

azasypkin commented May 24, 2018 • edited Loading

Choose a reason for hiding this comment

rhoboat Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin Jun 21, 2018 • 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

rhoboat Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin May 29, 2018 • 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

azasypkin Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin commented May 29, 2018

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Jun 13, 2018 • 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

azasypkin commented Jun 13, 2018

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 Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin Jun 21, 2018 • 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

rhoboat Jun 21, 2018 • 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

Move `BasePathProxy` to the new platform #19424

Move `BasePathProxy` to the new platform #19424

azasypkin commented May 24, 2018 •

edited

Loading

rhoboat Jun 21, 2018 •

edited

Loading

azasypkin Jun 21, 2018 •

edited

Loading

rhoboat Jun 21, 2018 •

edited

Loading

azasypkin May 29, 2018 •

edited

Loading

azasypkin May 29, 2018 •

edited

Loading

azasypkin Jun 13, 2018 •

edited

Loading

azasypkin Jun 13, 2018 •

edited

Loading

azasypkin Jun 21, 2018 •

edited

Loading

rhoboat Jun 21, 2018 •

edited

Loading

azasypkin Jun 21, 2018 •

edited

Loading

rhoboat Jun 21, 2018 •

edited

Loading

rhoboat Jun 21, 2018 •

edited

Loading

rhoboat left a comment •

edited

Loading