Allow routes to specify the idle socket timeout in addition to the payload timeout #73730

kobelb · 2020-07-29T19:20:11Z

Summary

An attempt to make #72362 work with #73103.

Route definitions can now specify the idleSocket timeout and the payload timeout. ~~There are still a few "hazards" to this interface until hapijs/hapi#4123 merges, which is exercised by the unit tests.~~ This PR works around hapijs/hapi#4123 by specifying a socket timeout in the route definition when the payload timeout is specified, and then uses the onPreAuth lifecycle event of the route to set the real timeouts.

Dev Docs

Route definitions can now specify the idleSocket timeout in addition to the payload timeout.

Resolves #73557

idle socket timeout, not the payload timeout

mshustov · 2020-08-04T12:06:36Z

src/core/server/http/http_server.ts

+        if (timeout?.idleSocket) {
+          socketTimeout = timeout.idleSocket;
+        } else if (payloadTimeout) {
+          // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond


It's rather confusing that socketTimeout is completely ignored. a couple of options:

use a higher value Math.max(socketTimeout, payloadTimeout + 1)

do not rely on Hapi and set socket.setTimeout(...) in a request interceptor manually

It's rather confusing that socketTimeout is completely ignored

If the route specifies the timeout.idleSocket it will always be used. Even if it leads to an error being thrown if it's smaller than the payload timeout...

use a higher value Math.max(socketTimeout, payloadTimeout + 1)

If the route definition tries to explicitly set the idle socket timeout to be smaller than the payload timeout, it feels less surprising to consumers to just throw an explicit error.

do not rely on Hapi and set socket.setTimeout(...) in a request interceptor manually

I'm hopeful that this will be a temporary requirement, and hapijs/hapi#4123 will merge shortly. The only route that I'm aware of that will need to set the idle socket timeout is Fleet, which is adjusting the timeout to be super long to support long-polling, so they won't hit us leaking HapiJS constraints.

Do you know if hapijs/hapi#4123 is going to be backported to Hapi v17?

Do you know if hapijs/hapi#4123 is going to be backported to Hapi v17?

I don't... I just asked for clarification in hapijs/hapi#4123 (comment).

Per hapijs/hapi#4123 (comment)

If this lands, it will only be for v20. If you need support for an older version, you'll have to check on a commercial license - #4114.

3d8901a works around the HapiJS constraints by setting a socket timeout on the route definition when a payload timeout is specified, and then uses the onPreAuth lifecycle event to set the actual socket timeout... It's not possible to do so in the onRequest lifecycle event because the route hasn't been looked up yet per https://hapi.dev/api/?v=19.2.0#request-lifecycle

mshustov · 2020-08-04T12:09:00Z

src/core/server/http/http_server.ts

@@ -164,7 +164,16 @@ export class HttpServer {
        const { authRequired, tags, body = {}, timeout } = route.options;
        const { accepts: allow, maxBytes, output, parse } = body;
        // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests
-        const payloadTimeout = isSafeMethod(route.method) || timeout == null ? undefined : timeout;
+        const payloadTimeout = isSafeMethod(route.method) ? undefined : timeout?.payload;


Let's rename payload --> payloadParsing maybe?

…timeout-2

src/core/server/http/integration_tests/router.test.ts

kobelb · 2020-08-05T22:43:23Z

src/core/server/http/router/route.ts

+    /**
+     * Milliseconds to receive the payload
+     */
+    payload?: Method extends 'get' | 'options' ? undefined : number;


Per #73730 (comment), were you suggesting that we rename payload to payloadParsing? I don't think just calling this the payload timeout is super expressive, but I'm afraid of reusing "parsing" here as that might make users think it relates to

kibana/src/core/server/http/router/route.ts

Line 110 in 2c939ea

parse?: boolean | 'gunzip';

+1 to payloadParsing not being correct and maybe confusing since this timeout does not have to do with parsing. I actually prefer the current naming, though if I had my way, we'd stop using the word payload and always use body since that is what the HTTP spec calls it.

kobelb · 2020-08-10T14:00:37Z

@elasticmachine merge upstream

joshdover

Since there is some differing behavior in the socket behavior when using 'fake requests', I think it would be good to have at least 1-2 functional tests that use real HTTP connections.

joshdover · 2020-08-10T19:21:40Z

src/core/server/http/router/route.ts

+    /**
+     * Milliseconds to receive the payload
+     */
+    payload?: Method extends 'get' | 'options' ? undefined : number;


+1 to payloadParsing not being correct and maybe confusing since this timeout does not have to do with parsing. I actually prefer the current naming, though if I had my way, we'd stop using the word payload and always use body since that is what the HTTP spec calls it.

joshdover · 2020-08-10T19:29:30Z

src/core/server/http/http_server.ts

+            payload: [allow, maxBytes, output, parse, timeout?.payload].some(
              (v) => typeof v !== 'undefined'
            )


(slightly out of scope) Do we really need this .some check to return undefined instead of an object of undefined values? I would expect Hapi is using Joi under the hood to populate the defaults here anyways.

It is... per #50783 (comment) HapiJS doesn't let us set the payload option for a GET or HEAD route definition...

src/core/server/http/http_server.ts

Co-authored-by: Josh Dover <[email protected]>

FrankHassanabad

Looked over the changes to security solutions and this looks good to me 👍

…timeout-2

kobelb · 2020-08-11T13:41:33Z

@elasticmachine merge upstream

kobelb · 2020-08-17T19:44:35Z

@elasticmachine merge upstream

kibanamachine · 2020-08-17T21:33:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 44ab6dc

Build metrics

✅ unchanged

History

💚 Build #67499 succeeded 2bd6bb9
💚 Build #67413 succeeded 0b0c488
💔 Build #67406 failed d4e7d2a
💔 Build #67405 failed 31dd497
💚 Build #67364 succeeded 20ff93f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

roncohen · 2020-08-19T11:21:37Z

thank you for working on this!

* master: (112 commits) [Ingest Manager] Fix agent config rollout rate limit to use constants (elastic#75364) Update Node.js to version 10.22.0 (elastic#75254) [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (elastic#74953) [Discover] Fix histogram cloud tests (elastic#75268) Uiactions to navigate to visualize or maps (elastic#74121) Use prefix search invis editor field/agg combo box (elastic#75290) Fix docs in trigger alerting UI (elastic#75363) [SIEM] Fixes search bar Cypress test (elastic#74833) Add libnss3.so to Dockerfile template (reporting) (elastic#75370) [Discover] Create field_button and add popovers to sidebar (elastic#73226) [Reporting] Network Policy: Do not throw from the intercept handler (elastic#75105) [Reporting] Increase capture.timeouts.openUrl to 1 minute (elastic#75207) Allow routes to specify the idle socket timeout in addition to the payload timeout (elastic#73730) [src/dev/build] remove node-version from snapshots (elastic#75303) [ENDPOINT] Reintroduced tabs to endpoint management and migrated pages to use common security components (elastic#74886) [Canvas] Remove dependency on legacy expressions APIs (elastic#74885) Skip failing test in CI (elastic#75266) [Task Manager] time out work when it overruns in poller (elastic#74980) [Drilldowns] misc improvements & fixes (elastic#75276) Small README note on bumping memory for builds (elastic#75247) ...

…yload timeout (#73730) (#75350) * Route options timeout -> timeout.payload * timeout.idleSocket can now be specified per route * Removing nested ternary * Fixing integration tests * Trying to actually fix the integration tests. Existing tests are hitting idle socket timeout, not the payload timeout * Fixing payload post timeout integration test * Fixing PUT and DELETE payload sending too long tests * Fixing type-script errors * GET routes can't specify the payload timeout, they can't accept payloads * Removing some redundancy in the tests * Adding 'Transfer-Encoding: chunked' to the POST test * Fixing POST/GET/PUT quick tests * Adding idleSocket timeout test * Removing unnecessary `isSafeMethod` call * Updating documentation * Removing PUT/DELETE integration tests * Working around the HapiJS bug * Deleting unused type import * The socket can be undefined... This occurs when using @hapi/shot directly or indirectly via Server.inject. In these scenarios, there isn't a socket. This can also occur when a "fake request" is used by the hacky background jobs: Reporting and Alerting... * Update src/core/server/http/http_server.ts Co-authored-by: Josh Dover <[email protected]> * Adding payload timeout functional tests * Adding idle socket timeout functional tests * Adding better comments, using ?? instead of || * Fixing the plugin fixture TS * Fixing some typescript errors * Fixing plugin fixture tsconfig.json Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Josh Dover <[email protected]>

kobelb added 5 commits July 29, 2020 10:12

Route options timeout -> timeout.payload

6ce7f1e

timeout.idleSocket can now be specified per route

4e3b566

Removing nested ternary

26541ba

Fixing integration tests

4048150

Trying to actually fix the integration tests. Existing tests are hitting

8267a2c

idle socket timeout, not the payload timeout

mshustov reviewed Aug 4, 2020

View reviewed changes

kobelb added 10 commits August 5, 2020 07:43

Merge remote-tracking branch 'upstream/master' into core/idle-socket-…

80e6259

…timeout-2

Fixing payload post timeout integration test

66cf715

Fixing PUT and DELETE payload sending too long tests

1a26b10

Fixing type-script errors

c03a0ce

GET routes can't specify the payload timeout, they can't accept payloads

b5b7026

Removing some redundancy in the tests

7429fea

Adding 'Transfer-Encoding: chunked' to the POST test

4f7e9e1

Fixing POST/GET/PUT quick tests

828eab8

Adding idleSocket timeout test

c092cbe

Removing unnecessary isSafeMethod call

2c939ea

kobelb commented Aug 5, 2020

View reviewed changes

src/core/server/http/integration_tests/router.test.ts Outdated Show resolved Hide resolved

kobelb commented Aug 5, 2020

View reviewed changes

kobelb added 4 commits August 5, 2020 16:04

Updating documentation

d300b9f

Removing PUT/DELETE integration tests

84e06fa

Working around the HapiJS bug

3d8901a

Deleting unused type import

afbb7e8

kobelb marked this pull request as ready for review August 6, 2020 17:14

kobelb requested review from a team as code owners August 6, 2020 17:14

kobelb added v8.0.0 v7.9.0 and removed v7.9.0 labels Aug 6, 2020

Merge branch 'master' into core/idle-socket-timeout-2

87634ff

kobelb mentioned this pull request Aug 10, 2020

Allowing routes to specify the idle socket timeout #72362

Closed

joshdover reviewed Aug 10, 2020

View reviewed changes

Update src/core/server/http/http_server.ts

20ff93f

Co-authored-by: Josh Dover <[email protected]>

FrankHassanabad approved these changes Aug 10, 2020

View reviewed changes

kobelb added 8 commits August 10, 2020 15:36

Merge remote-tracking branch 'upstream/master' into core/idle-socket-…

0e8fd23

…timeout-2

Adding payload timeout functional tests

dc430b9

Adding idle socket timeout functional tests

31dd497

Adding better comments, using ?? instead of ||

25b6db0

Fixing the plugin fixture TS

d4e7d2a

Fixing some typescript errors

2862387

Merge remote-tracking branch 'upstream/master' into core/idle-socket-…

4c6a15d

…timeout-2

Fixing plugin fixture tsconfig.json

0b0c488

Merge branch 'master' into core/idle-socket-timeout-2

2bd6bb9

joshdover approved these changes Aug 17, 2020

View reviewed changes

Merge branch 'master' into core/idle-socket-timeout-2

44ab6dc

kobelb merged commit 83bc500 into elastic:master Aug 18, 2020

kobelb deleted the core/idle-socket-timeout-2 branch August 18, 2020 17:46

kobelb mentioned this pull request Aug 18, 2020

[7.x] Allow routes to specify the idle socket timeout in addition to the payload timeout (#73730) #75350

Merged

spalger mentioned this pull request Aug 19, 2020

Failing test: Plugin Functional Tests.test/plugin_functional/test_suites/core/route·ts - core route timeouts idle socket should timeout if payload sending has too long of an idle period #75440

Closed

nchaulet mentioned this pull request Aug 20, 2020

[Ingest Manager] Better default for agent checkin long polling #75552

Closed

pgayvallet mentioned this pull request Nov 4, 2020

Failing test: Jest Integration Tests.src/core/server/http/integration_tests - Options timeout idleSocket should timeout if payload sending has too long of an idle period #82002

Closed

mshustov mentioned this pull request Dec 15, 2020

Upgrade to hapi version 20 #85406

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow routes to specify the idle socket timeout in addition to the payload timeout #73730

Allow routes to specify the idle socket timeout in addition to the payload timeout #73730

kobelb commented Jul 29, 2020 •

edited

Loading

mshustov Aug 4, 2020

kobelb Aug 5, 2020

mshustov Aug 6, 2020

kobelb Aug 6, 2020

kobelb Aug 6, 2020

kobelb Aug 6, 2020

mshustov Aug 4, 2020

kobelb Aug 5, 2020 •

edited

Loading

joshdover Aug 10, 2020

kobelb commented Aug 10, 2020

joshdover left a comment

joshdover Aug 10, 2020

joshdover Aug 10, 2020

kobelb Aug 10, 2020

FrankHassanabad left a comment

kobelb commented Aug 11, 2020

kobelb commented Aug 17, 2020

kibanamachine commented Aug 17, 2020

roncohen commented Aug 19, 2020

Allow routes to specify the idle socket timeout in addition to the payload timeout #73730

Allow routes to specify the idle socket timeout in addition to the payload timeout #73730

Conversation

kobelb commented Jul 29, 2020 • edited Loading

Summary

Dev Docs

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

kobelb Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb commented Aug 10, 2020

joshdover 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

FrankHassanabad left a comment

Choose a reason for hiding this comment

kobelb commented Aug 11, 2020

kobelb commented Aug 17, 2020

kibanamachine commented Aug 17, 2020

💚 Build Succeeded

Build metrics

History

roncohen commented Aug 19, 2020

kobelb commented Jul 29, 2020 •

edited

Loading

kobelb Aug 5, 2020 •

edited

Loading