Do not limit payload size for requests that are proxied to the legacy Kibana. #21750

azasypkin · 2018-08-07T18:59:25Z

In this PR we set a very large max payload size for the core route that proxies requests to the "legacy" Kibana that will do proper size checks based on legacy route max size overrides if any.

PR includes test that fails on master and should pass with this change. I'd like to incorporate changes that I introduced in max_payload_size.test.js to kbnTestServer itself so that other tests that rely on kbnTestServer inject requests through the core to be as close as possible to real setup. I haven't done that yet, hence WIP label for this PR.

Fixes #21737

azasypkin · 2018-08-07T19:00:16Z

src/server/http/__snapshots__/max_payload_size.test.js.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`fails with 400 if payload size is larger than default and route config allows 1`] = `"{\\"statusCode\\":400,\\"error\\":\\"Bad Request\\",\\"message\\":\\"Payload content length greater than maximum allowed: 200\\"}"`;


note: Hapi 17 finally returns 413 🎉 hapijs/hapi@044f8b3

azasypkin · 2018-08-07T19:01:03Z

src/server/http/max_payload_size.test.js

+function makeRequest(opts) {
+  // Inject request through new platform http server that we can access through `proxyListener`
+  // provided by the new platform.
+  return kbnServer.newPlatform.proxyListener.root.server.http.service.httpServer.server.inject(opts);


note: sooooooooooooo awful, I know 🙈

epixa · 2018-08-07T19:01:45Z

Does this mean that endpoints registered through the new platform will by default have a 1GB max payload size?

azasypkin · 2018-08-07T19:04:49Z

Does this mean that endpoints registered through the new platform will by default have a 1GB max payload size?

Nope, it's route-specific override, the rest of the routes registered in the new platform will use default set on the server level.

elasticmachine · 2018-08-07T20:25:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

… Kibana.

alexfrancoeur · 2018-08-09T11:57:30Z

@azasypkin @epixa @rashidkpc tested this PR with Canvas master and it worked like a charm. I uploaded an 11mb workpad without any errors or issues.

rashidkpc · 2018-08-09T12:26:35Z

@azasypkin thank you so much jumping on this, it really means a lot when something gets fixed this fast.

…ugh core top level Hapi server.

azasypkin · 2018-08-09T13:16:28Z

src/server/http/xsrf.test.js

@@ -186,7 +181,7 @@ describe('xsrf request filter', function () {
        });

        expect(resp.statusCode).toBe(400);
-        expect(resp.result.message).toBe('Request must contain a kbn-xsrf header.');
+        expect(resp.result).toMatchSnapshot(`${method} reject response`);


note: I switched to snapshot here since resp.result is "raw" string now: request is proxied (and "abandoned") from core to the legacy Kibana and hence response isn't deserialized by the Hapi we use in core.

azasypkin · 2018-08-09T13:19:19Z

src/server/kbn_server.js

    await this.ready();
+
+    const { server } = this;


note: not sure why it was done this way before (server property may not be created before ready is called), so I'm changing that to not require consumers (tests in this case) to always call ready before they call listen. So please let me know what you think.

azasypkin · 2018-08-09T13:27:56Z

src/test_utils/kbn_server.js

@@ -25,7 +25,10 @@ import KbnServer from '../../src/server/kbn_server';

 const DEFAULTS_SETTINGS = {
  server: {
-    autoListen: false,
+    autoListen: true,


note: I have mixed feelings on that, but can't think of any other better way :/ We should start core's root to have access to the underlying server and it's only started when legacy hapi servers calls listen.

Even after #19994, we won't register routes and create Hapi server if autoListen is false, and we don't want to expose Hapi and hence inject either. So we need to think of a better alternative to testKbnServer at some point.

Hmm, is there value in splitting up the logic in the new platform like it's split in the legacy platform, so the server is setup (routes registered, etc.) without listening, and then when calling listen() on the legacy platform would somehow tell the new platform to start listening too?

The primary reason I ask is that given the new platforms dependency on listen for setup, it seems that there is little point in having a non-listening KbnServer anymore... If we can't or don't want to support a non-listening by "ready" version of the new platform maybe we should just move the listen() logic into ready() and get rid of the autoListen config option...

Hmm, is there value in splitting up the logic in the new platform like it's split in the legacy platform, so the server is setup (routes registered, etc.) without listening, and then when calling listen() on the legacy platform would somehow tell the new platform to start listening too?

Maybe, we can discuss that when we come closer to the plugin support when we start thinking where in the server lifecycle we do route registration. IIRC one of the reasons of putting this kind of staff in start/stop was based on the idea that someday we may want to stop service/server, reload config (not only logging, but the one that can affect routes, http config etc.) and start service/server again.

The primary reason I ask is that given the new platforms dependency on listen for setup, it seems that there is little point in having a non-listening KbnServer anymore...

I think it's still needed in dev mode when kbnServer is created by optimizer worker process or what you mean?

I think it's still needed in dev mode when kbnServer is created by optimizer worker process or what you mean?

But the optimizer starts a listening version of the Kibana server, which is proxied to by the actual server, so it can serve the optimizer output.

What I mean is that even if the KbnServer consumer doesn't need HTTP to be bound for its purposes, the KbnServer must bind to a port to function properly, so I feel like we should get rid of the listen() method and just make it a part of KbnServer initialization, part of the promise that is returned by ready(), and remove the server.autoListen config option.

Whoops, I'm totally wrong... see

kibana/src/cli/cluster/cluster_manager.js

Line 51 in 4ebd3d3

'--server.autoListen=false',

Please ignore my request to remove server.autoListen :)

We pass --server.autoListen=false to the optimizer, which writes the bundle assets to the bundle directory and the actual server just pauses requests while the optimizer is running, then serves the requests from the bundle directory... This means that the server is semi-functional, it just can't handle routing via the kbnServer.inject() method, which won't be a problem as we move further toward the new platform.

azasypkin · 2018-08-09T13:30:30Z

@azasypkin @epixa @rashidkpc tested this PR with Canvas master and it worked like a charm. I uploaded an 11mb workpad without any errors or issues.

Awesome, thanks for verifying this!

@azasypkin thank you so much jumping on this, it really means a lot when something gets fixed this fast.

No problem! The plan is to eventually cover such core functionality with various tests so that we're not hit by something like this again :/

elasticmachine · 2018-08-09T13:57:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-09T13:58:05Z

packages/kbn-plugin-helpers tests failed, it doesn't look like something that may be related. Retrying.

azasypkin · 2018-08-09T13:58:17Z

retest

elasticmachine · 2018-08-09T15:36:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-11T16:55:29Z

src/core/server/http/http_server.ts

@@ -77,6 +77,9 @@ export class HttpServer {
            output: 'stream',
            parse: false,
            timeout: false,
+            // Having such a large value (1 GB) here will allow legacy routes to override
+            // maximum allowed payload size set in the core http server if needed.
+            maxBytes: 1024 * 1024 * 1024,


Is Inifinty out of the question here? Just not sure I see the point in putting a hard 1GB limit on something that should be customizable by routes and even users at some level.

Infinity won't work (including positive Infinity) as it's validated by Joi.number().integer().positive().default(1024 * 1024), I can probably use Number.MAX_SAFE_INTEGER if you prefer, I just used what we use for base path proxy.

I guess that'd be fine too. Another alternative might be to use the max limit from any route (determined at runtime somehow so plugins can be included), but I'm guessing we don't have a good way to determine that.

Another alternative might be to use the max limit from any route (determined at runtime somehow so plugins can be included), but I'm guessing we don't have a good way to determine that.

Yeah, I can't think of any easy way to determine from core route what route will be used by the "legacy" Kibana. Let's try Number.MAX_SAFE_INTEGER and see how it goes.

spalger · 2018-08-11T16:57:14Z

src/server/http/version_check.test.js

@@ -28,11 +27,7 @@ const version = require(src('../package.json')).version;

 describe('version_check request filter', function () {
  function makeRequest(kbnServer, opts) {


Mind making this an async function and awaiting the return value of makeRequest() (same for other instances)? I find things less ambiguous when promise returning functions are clearly indicated as such by using async/await, even when it's not technically necessary.

Sure, don't have strong opinion on that, I'll make this async ~~with throw err; in catch~~.

spalger · 2018-08-11T17:02:25Z

src/test_utils/kbn_server.js

+  // Since all requests to Kibana hit core http server first and only after that
+  // are proxied to the "legacy" Kibana we should inject requests through the top
+  // level Hapi server used by the core.
+  return kbnServer.newPlatform.proxyListener.root.server.http.service.httpServer.server.inject(


😨 Maybe kbnServer.newPlatform.proxyListener could expose a method that is a little less... hacky?

I can, but then we'll have hacky stuff in non-test code since most of these nested properties are private in TS as they should be. Aaaand I'll get rid of kbnServer.newPlatform entirely for 6.5 and master (still need a couple of PRs). Or you have some specific suggestion in mind already?

okay, it sounds like you have a plan, I'll avert my eyes and let you do what needs to be done

Yep, I have some plan, still in flux though, but I'll see what I can come up with in the scope of #19994.

spalger · 2018-08-11T17:03:37Z

src/ui/field_formats/__tests__/field_formats_mixin.js

  beforeEach(async () => {
-    const kbnServer = createServer();
+    kbnServer = createServer();


Is this the only place we were using createServer() that wasn't calling kbnServer.close() which I assume is only really necessary because of the change to autoListen: true by default.

Yeah, that's the only place I found.

… async/await for test functions returning promises.

azasypkin · 2018-08-13T07:26:09Z

@spalger I've pushed changes we discussed, should be ready for another pass.

elasticmachine · 2018-08-13T09:01:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM

elasticmachine · 2018-08-13T23:05:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

… Kibana. (elastic#21750)

…legacy Kibana. (#21952) * Do not limit payload size for requests that are proxied to the legacy Kibana. (#21750) * Adapt basepath tests to the 6.x behaviour.

…legacy Kibana. (#21951) * Do not limit payload size for requests that are proxied to the legacy Kibana. (#21750) * Adapt basepath tests to the 6.x behaviour.

azasypkin · 2018-08-14T08:38:28Z

6.4: c50e7bd
6.x/6.5: f7462fc

… Kibana. (elastic#21750)

azasypkin added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v6.4.0 WIP Work in progress labels Aug 7, 2018

azasypkin commented Aug 7, 2018

View reviewed changes

Do not limit payload size for requests that are proxied to the legacy…

9074ae9

… Kibana.

Make sure all tests that rely on kbnTestServer inject requests thro…

2cefabf

…ugh core top level Hapi server.

azasypkin force-pushed the issue-21737-max-payload branch from b99fbc9 to 2cefabf Compare August 9, 2018 13:11

azasypkin commented Aug 9, 2018

View reviewed changes

azasypkin requested review from spalger and epixa August 9, 2018 15:37

azasypkin added review v6.5.0 and removed WIP Work in progress labels Aug 9, 2018

spalger reviewed Aug 11, 2018

View reviewed changes

Use Number.MAX_SAFE_INTEGER as max payload size in proxy route. Use…

f63d36b

… async/await for test functions returning promises.

spalger approved these changes Aug 13, 2018

View reviewed changes

Merge branch 'master' into issue-21737-max-payload

17a8670

azasypkin merged commit e26cfc6 into elastic:master Aug 14, 2018

azasypkin deleted the issue-21737-max-payload branch August 14, 2018 05:37

This was referenced Aug 14, 2018

[6.x] Do not limit payload size for requests that are proxied to the legacy Kibana. (#21750) #21951

Merged

[6.4] Do not limit payload size for requests that are proxied to the legacy Kibana. (#21750) #21952

Merged

azasypkin added a commit to azasypkin/kibana that referenced this pull request Aug 14, 2018

Do not limit payload size for requests that are proxied to the legacy…

753b989

… Kibana. (elastic#21750)

azasypkin added the backport pending label Aug 14, 2018

azasypkin added backported and removed backport pending labels Aug 14, 2018

cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 21, 2018

Do not limit payload size for requests that are proxied to the legacy…

e85d7ac

… Kibana. (elastic#21750)

cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 23, 2018

Do not limit payload size for requests that are proxied to the legacy…

2253b4a

… Kibana. (elastic#21750)

		@@ -0,0 +1,3 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`fails with 400 if payload size is larger than default and route config allows 1`] = `"{\\"statusCode\\":400,\\"error\\":\\"Bad Request\\",\\"message\\":\\"Payload content length greater than maximum allowed: 200\\"}"`;

		@@ -28,11 +27,7 @@ const version = require(src('../package.json')).version;

		describe('version_check request filter', function () {
		function makeRequest(kbnServer, opts) {

Do not limit payload size for requests that are proxied to the legacy Kibana. #21750

Do not limit payload size for requests that are proxied to the legacy Kibana. #21750

Conversation

azasypkin commented Aug 7, 2018 • edited Loading

azasypkin Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented Aug 7, 2018

azasypkin commented Aug 7, 2018

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

alexfrancoeur commented Aug 9, 2018

rashidkpc commented Aug 9, 2018

Choose a reason for hiding this comment

azasypkin Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin commented Aug 9, 2018

elasticmachine commented Aug 9, 2018

💔 Build Failed

azasypkin commented Aug 9, 2018 • edited Loading

azasypkin commented Aug 9, 2018

elasticmachine commented Aug 9, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Aug 11, 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 Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Aug 11, 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

azasypkin Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

azasypkin commented Aug 13, 2018

elasticmachine commented Aug 13, 2018

💚 Build Succeeded

spalger left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 13, 2018

💚 Build Succeeded

azasypkin commented Aug 14, 2018

azasypkin commented Aug 7, 2018 •

edited

Loading

azasypkin Aug 7, 2018 •

edited

Loading

azasypkin Aug 9, 2018 •

edited

Loading

spalger Aug 11, 2018 •

edited

Loading

spalger Aug 13, 2018 •

edited

Loading

azasypkin commented Aug 9, 2018 •

edited

Loading

spalger Aug 11, 2018 •

edited

Loading

azasypkin Aug 11, 2018 •

edited

Loading

azasypkin Aug 11, 2018 •

edited

Loading

azasypkin Aug 13, 2018 •

edited

Loading