From 261c02925261d9216502c971ce3a2afd6d57d7d3 Mon Sep 17 00:00:00 2001 From: Brandon Kobel Date: Tue, 18 Aug 2020 10:46:08 -0700 Subject: [PATCH] Allow routes to specify the idle socket timeout in addition to the payload timeout (#73730) * 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 * 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 Co-authored-by: Josh Dover --- ...a-plugin-core-server.routeconfigoptions.md | 2 +- ...-core-server.routeconfigoptions.timeout.md | 7 +- src/core/server/http/http_server.test.ts | 314 ++++++++++++------ src/core/server/http/http_server.ts | 37 ++- .../http/integration_tests/router.test.ts | 280 ++++++++++------ src/core/server/http/router/request.ts | 12 +- src/core/server/http/router/route.ts | 15 +- src/core/server/server.api.md | 5 +- test/plugin_functional/config.ts | 1 + .../core_plugin_route_timeouts/kibana.json | 8 + .../core_plugin_route_timeouts/package.json | 17 + .../server/index.ts | 23 ++ .../server/plugin.ts | 124 +++++++ .../core_plugin_route_timeouts/tsconfig.json | 12 + .../test_suites/core/index.ts | 25 ++ .../test_suites/core/route.ts | 174 ++++++++++ .../server/routes/import_list_item_route.ts | 4 +- 17 files changed, 843 insertions(+), 217 deletions(-) create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/package.json create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json create mode 100644 test/plugin_functional/test_suites/core/index.ts create mode 100644 test/plugin_functional/test_suites/core/route.ts diff --git a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md index fee6124f8d866..17fafe2af0de1 100644 --- a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md +++ b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md @@ -19,6 +19,6 @@ export interface RouteConfigOptions | [authRequired](./kibana-plugin-core-server.routeconfigoptions.authrequired.md) | boolean | 'optional' | Defines authentication mode for a route: - true. A user has to have valid credentials to access a resource - false. A user can access a resource without any credentials. - 'optional'. A user can access a resource if has valid credentials or no credentials at all. Can be useful when we grant access to a resource but want to identify a user if possible.Defaults to true if an auth mechanism is registered. | | [body](./kibana-plugin-core-server.routeconfigoptions.body.md) | Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody | Additional body options [RouteConfigOptionsBody](./kibana-plugin-core-server.routeconfigoptionsbody.md). | | [tags](./kibana-plugin-core-server.routeconfigoptions.tags.md) | readonly string[] | Additional metadata tag strings to attach to the route. | -| [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | number | Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes | +| [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | {
payload?: Method extends 'get' | 'options' ? undefined : number;
idleSocket?: number;
} | Defines per-route timeouts. | | [xsrfRequired](./kibana-plugin-core-server.routeconfigoptions.xsrfrequired.md) | Method extends 'get' ? never : boolean | Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain kbn-xsrf header. - false. Disables xsrf protection.Set to true by default | diff --git a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md index 479fcf883ec4d..f602a8913964f 100644 --- a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md +++ b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md @@ -4,10 +4,13 @@ ## RouteConfigOptions.timeout property -Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes +Defines per-route timeouts. Signature: ```typescript -timeout?: number; +timeout?: { + payload?: Method extends 'get' | 'options' ? undefined : number; + idleSocket?: number; + }; ``` diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 53c7fdf9f8037..28913a73ca4bc 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -799,6 +799,7 @@ test('exposes route details of incoming request to a route handler', async () => authRequired: true, xsrfRequired: false, tags: [], + timeout: {}, }, }); }); @@ -906,6 +907,9 @@ test('exposes route details of incoming request to a route handler (POST + paylo authRequired: true, xsrfRequired: true, tags: [], + timeout: { + payload: 10000, + }, body: { parse: true, // hapi populates the default maxBytes: 1024, // hapi populates the default @@ -993,129 +997,249 @@ describe('body options', () => { }); describe('timeout options', () => { - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a POST', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + describe('payload timeout', () => { + test('POST routes set the payload timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.post( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } + } + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .post('/') + .send({ test: 1 }) + .expect(200, { + timeout: { + payload: 300000, + }, + }); + }); - const router = new Router('', logger, enhanceWithContext); - router.post( - { - path: '/', - validate: false, - options: { timeout: 300000 }, - }, - (context, req, res) => { - try { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } catch (err) { - return res.internalError({ body: err.message }); + test('DELETE routes set the payload timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.delete( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, { - timeout: 300000, + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .delete('/') + .expect(200, { + timeout: { + payload: 300000, + }, + }); }); - }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a GET', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + test('PUT routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.put( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } + } + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .put('/') + .expect(200, { + timeout: { + payload: 300000, + }, + }); + }); - const router = new Router('', logger, enhanceWithContext); - router.get( - { - path: '/', - validate: false, - options: { timeout: 300000 }, - }, - (context, req, res) => { - try { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } catch (err) { - return res.internalError({ body: err.message }); + test('PATCH routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.patch( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).get('/').expect(200, { - timeout: 300000, + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .patch('/') + .expect(200, { + timeout: { + payload: 300000, + }, + }); }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a DELETE', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + describe('idleSocket timeout', () => { + test('uses server socket timeout when not specified in the route', async () => { + const { registerRouter, server: innerServer } = await server.setup({ + ...config, + socketTimeout: 11000, + }); - const router = new Router('', logger, enhanceWithContext); - router.delete( - { - path: '/', - validate: false, - options: { timeout: 300000 }, - }, - (context, req, res) => { - try { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } catch (err) { - return res.internalError({ body: err.message }); + const router = new Router('', logger, enhanceWithContext); + router.get( + { + path: '/', + validate: { body: schema.any() }, + }, + (context, req, res) => { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).delete('/').expect(200, { - timeout: 300000, + ); + registerRouter(router); + + await server.start(); + await supertest(innerServer.listener) + .get('/') + .send() + .expect(200, { + timeout: { + idleSocket: 11000, + }, + }); }); - }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PUT', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + test('sets the socket timeout when specified in the route', async () => { + const { registerRouter, server: innerServer } = await server.setup({ + ...config, + socketTimeout: 11000, + }); - const router = new Router('', logger, enhanceWithContext); - router.put( - { - path: '/', - validate: false, - options: { timeout: 300000 }, - }, - (context, req, res) => { - try { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } catch (err) { - return res.internalError({ body: err.message }); + const router = new Router('', logger, enhanceWithContext); + router.get( + { + path: '/', + validate: { body: schema.any() }, + options: { timeout: { idleSocket: 12000 } }, + }, + (context, req, res) => { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).put('/').expect(200, { - timeout: 300000, + ); + registerRouter(router); + + await server.start(); + await supertest(innerServer.listener) + .get('/') + .send() + .expect(200, { + timeout: { + idleSocket: 12000, + }, + }); }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PATCH', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + test(`idleSocket timeout can be smaller than the payload timeout`, async () => { + const { registerRouter } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); - router.patch( + router.post( { path: '/', - validate: false, - options: { timeout: 300000 }, + validate: { body: schema.any() }, + options: { + timeout: { + payload: 1000, + idleSocket: 10, + }, + }, }, (context, req, res) => { - try { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } catch (err) { - return res.internalError({ body: err.message }); - } + return res.ok({ body: { timeout: req.route.options.timeout } }); } ); + registerRouter(router); + await server.start(); - await supertest(innerServer.listener).patch('/').expect(200, { - timeout: 300000, - }); }); }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 4b70f58deba99..99ab0ef16c2f9 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -163,13 +163,17 @@ export class HttpServer { const validate = isSafeMethod(route.method) ? undefined : { payload: true }; 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 kibanaRouteState: KibanaRouteState = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), }; + // To work around https://github.com/hapijs/hapi/issues/4122 until v20, set the socket + // timeout on the route to a fake timeout only when the payload timeout is specified. + // Within the onPreAuth lifecycle of the route itself, we'll override the timeout with the + // real socket timeout. + const fakeSocketTimeout = timeout?.payload ? timeout.payload + 1 : undefined; + this.server.route({ handler: route.handler, method: route.method, @@ -177,13 +181,29 @@ export class HttpServer { options: { auth: this.getAuthOption(authRequired), app: kibanaRouteState, + ext: { + onPreAuth: { + method: (request, h) => { + // At this point, the socket timeout has only been set to work-around the HapiJS bug. + // We need to either set the real per-route timeout or use the default idle socket timeout + if (timeout?.idleSocket) { + request.raw.req.socket.setTimeout(timeout.idleSocket); + } else if (fakeSocketTimeout) { + // NodeJS uses a socket timeout of `0` to denote "no timeout" + request.raw.req.socket.setTimeout(this.config!.socketTimeout ?? 0); + } + + return h.continue; + }, + }, + }, tags: tags ? Array.from(tags) : undefined, // TODO: This 'validate' section can be removed once the legacy platform is completely removed. // We are telling Hapi that NP routes can accept any payload, so that it can bypass the default // validation applied in ./http_tools#getServerOptions // (All NP routes are already required to specify their own validation in order to access the payload) validate, - payload: [allow, maxBytes, output, parse, payloadTimeout].some( + payload: [allow, maxBytes, output, parse, timeout?.payload].some( (v) => typeof v !== 'undefined' ) ? { @@ -191,15 +211,12 @@ export class HttpServer { maxBytes, output, parse, - timeout: payloadTimeout, + timeout: timeout?.payload, } : undefined, - timeout: - timeout != null - ? { - socket: timeout + 1, // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond - } - : undefined, + timeout: { + socket: fakeSocketTimeout, + }, }, }); } diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 434e22e3cf6f5..e19c348511f1a 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -304,126 +304,204 @@ describe('Options', () => { }); describe('timeout', () => { - it('should timeout if configured with a small timeout value for a POST', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + const writeBodyCharAtATime = (request: supertest.Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + resolve(res); + }); + } + }, interval); + request.on('error', (err) => { + clearInterval(intervalId); + reject(err); + }); + }); + }; - router.post( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); - expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).post('/b').expect(200, {}); - }); + describe('payload', () => { + it('should timeout if POST payload sending is too slow', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - it('should timeout if configured with a small timeout value for a PUT', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 100 }, + }, + path: '/a', + validate: false, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + await server.start(); - router.put( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.put({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); - expect(supertest(innerServer.listener).put('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).put('/b').expect(200, {}); - }); + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); - it('should timeout if configured with a small timeout value for a DELETE', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + await expect(result).rejects.toMatchInlineSnapshot(`[Error: Request Timeout]`); + }); - router.delete( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.delete({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); - expect(supertest(innerServer.listener).delete('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).delete('/b').expect(200, {}); - }); + it('should not timeout if POST payload sending is quick', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - it('should timeout if configured with a small timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + router.post( + { + path: '/a', + validate: false, + options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, + }, + async (context, req, res) => res.ok({}) + ); + await server.start(); - router.get( - // Note: There is a bug within Hapi Server where it cannot set the payload timeout for a GET call but it also cannot configure a timeout less than the payload body - // so the least amount of possible time to configure the timeout is 10 seconds. - { path: '/a', validate: false, options: { timeout: 100000 } }, - async (context, req, res) => { - // Cause a wait of 20 seconds to cause the socket hangup - await new Promise((resolve) => setTimeout(resolve, 200000)); - return res.ok({}); - } - ); - router.get({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); - expect(supertest(innerServer.listener).get('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).get('/b').expect(200, {}); + await expect(result).resolves.toHaveProperty('status', 200); + }); }); - it('should not timeout if configured with a 5 minute timeout value for a POST', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + describe('idleSocket', () => { + it('should timeout if payload sending has too long of an idle period', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.post( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).post('/a').expect(200, {}); - }); + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 10 }, + }, + }, + async (context, req, res) => { + return res.ok({}); + } + ); - it('should not timeout if configured with a 5 minute timeout value for a PUT', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + await server.start(); - router.put( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); - await supertest(innerServer.listener).put('/a').expect(200, {}); - }); + const result = writeBodyCharAtATime(request, '{}', 20); - it('should not timeout if configured with a 5 minute timeout value for a DELETE', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + await expect(result).rejects.toThrow('socket hang up'); + }); - router.delete( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).delete('/a').expect(200, {}); - }); + it(`should not timeout if payload sending doesn't have too long of an idle period`, async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - it('should not timeout if configured with a 5 minute timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 1000 }, + }, + }, + async (context, req, res) => { + return res.ok({}); + } + ); - router.get( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).get('/a').expect(200, {}); + await server.start(); + + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); + + await expect(result).resolves.toHaveProperty('status', 200); + }); + + it('should timeout if servers response is too slow', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 1000, payload: 100 }, + }, + }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + + await server.start(); + await expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); + }); + + it('should not timeout if servers response is quick', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 2000, payload: 100 }, + }, + }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 10)); + return res.ok({}); + } + ); + + await server.start(); + await expect(supertest(innerServer.listener).post('/a')).resolves.toHaveProperty( + 'status', + 200 + ); + }); }); }); }); diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 93ffb5aa48259..278bc222b754b 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -211,15 +211,21 @@ export class KibanaRequest< private getRouteInfo(request: Request): KibanaRequestRoute { const method = request.method as Method; - const { parse, maxBytes, allow, output } = request.route.settings.payload || {}; - const timeout = request.route.settings.timeout?.socket; + const { parse, maxBytes, allow, output, timeout: payloadTimeout } = + request.route.settings.payload || {}; + // net.Socket#timeout isn't documented, yet, and isn't part of the types... https://github.com/nodejs/node/pull/34543 + // the socket is also undefined when using @hapi/shot, or when a "fake request" is used + const socketTimeout = (request.raw.req.socket as any)?.timeout; const options = ({ authRequired: this.getAuthRequired(request), // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 xsrfRequired: (request.route.settings.app as KibanaRouteState)?.xsrfRequired ?? true, tags: request.route.settings.tags || [], - timeout: typeof timeout === 'number' ? timeout - 1 : undefined, // We are forced to have the timeout be 1 millisecond greater than the server and payload so we subtract one here to give the user consist settings + timeout: { + payload: payloadTimeout, + idleSocket: socketTimeout === 0 ? undefined : socketTimeout, + }, body: isSafeMethod(method) ? undefined : { diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index 676c494bec522..ce898a34e6b2c 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -146,10 +146,19 @@ export interface RouteConfigOptions { body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; /** - * Timeouts for processing durations. Response timeout is in milliseconds. - * Default value: 2 minutes + * Defines per-route timeouts. */ - timeout?: number; + timeout?: { + /** + * Milliseconds to receive the payload + */ + payload?: Method extends 'get' | 'options' ? undefined : number; + + /** + * Milliseconds the socket can be idle before it's closed + */ + idleSocket?: number; + }; } /** diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 60b01aa06d07f..03545284e14fb 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1886,7 +1886,10 @@ export interface RouteConfigOptions { authRequired?: boolean | 'optional'; body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; tags?: readonly string[]; - timeout?: number; + timeout?: { + payload?: Method extends 'get' | 'options' ? undefined : number; + idleSocket?: number; + }; xsrfRequired?: Method extends 'get' ? never : boolean; } diff --git a/test/plugin_functional/config.ts b/test/plugin_functional/config.ts index c611300eade10..9ec82e291e537 100644 --- a/test/plugin_functional/config.ts +++ b/test/plugin_functional/config.ts @@ -31,6 +31,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) { return { testFiles: [ + require.resolve('./test_suites/core'), require.resolve('./test_suites/custom_visualizations'), require.resolve('./test_suites/panel_actions'), require.resolve('./test_suites/core_plugins'), diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json new file mode 100644 index 0000000000000..6fbddad22b764 --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json @@ -0,0 +1,8 @@ +{ + "id": "core_plugin_route_timeouts", + "version": "0.0.1", + "kibanaVersion": "kibana", + "configPath": ["core_plugin_route_timeouts"], + "server": true, + "ui": false +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json new file mode 100644 index 0000000000000..a9c520338457b --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json @@ -0,0 +1,17 @@ +{ + "name": "core_plugin_route_timeouts", + "version": "1.0.0", + "main": "target/test/plugin_functional/plugins/core_plugin_route_timeouts", + "kibana": { + "version": "kibana", + "templateVersion": "1.0.0" + }, + "license": "Apache-2.0", + "scripts": { + "kbn": "node ../../../../scripts/kbn.js", + "build": "rm -rf './target' && tsc" + }, + "devDependencies": { + "typescript": "3.9.5" + } +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts new file mode 100644 index 0000000000000..4fdd6ef0ab04a --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts @@ -0,0 +1,23 @@ +/* + * 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 { CorePluginRouteTimeoutsPlugin } from './plugin'; +export { PluginARequestContext } from './plugin'; + +export const plugin = () => new CorePluginRouteTimeoutsPlugin(); diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts new file mode 100644 index 0000000000000..c5bdf6cce084c --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts @@ -0,0 +1,124 @@ +/* + * 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 { Plugin, CoreSetup } from 'kibana/server'; +import { schema } from '@kbn/config-schema'; + +export interface PluginARequestContext { + ping: () => Promise; +} + +declare module 'kibana/server' { + interface RequestHandlerContext { + pluginA?: PluginARequestContext; + } +} + +export class CorePluginRouteTimeoutsPlugin implements Plugin { + public setup(core: CoreSetup, deps: {}) { + const { http } = core; + + const router = http.createRouter(); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 100 }, + }, + path: '/short_payload_timeout', + validate: false, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 10000 }, + }, + path: '/longer_payload_timeout', + validate: false, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 10 }, + }, + path: '/short_idle_socket_timeout', + validate: { + body: schema.maybe( + schema.object({ + responseDelay: schema.maybe(schema.number()), + }) + ), + }, + }, + async (context, req, res) => { + if (req.body?.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body!.responseDelay)); + } + return res.ok({}); + } + ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 5000 }, + }, + path: '/longer_idle_socket_timeout', + validate: { + body: schema.maybe( + schema.object({ + responseDelay: schema.maybe(schema.number()), + }) + ), + }, + }, + async (context, req, res) => { + if (req.body?.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body!.responseDelay)); + } + return res.ok({}); + } + ); + } + + public start() {} + public stop() {} +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json new file mode 100644 index 0000000000000..d0751f31ecc5e --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json @@ -0,0 +1,12 @@ +{ + "extends": "../../../../tsconfig.json", + "compilerOptions": { + "outDir": "./target", + "skipLibCheck": true + }, + "include": [ + "server/**/*.ts", + "../../../../typings/**/*" + ], + "exclude": [] +} diff --git a/test/plugin_functional/test_suites/core/index.ts b/test/plugin_functional/test_suites/core/index.ts new file mode 100644 index 0000000000000..5852e21fc3b39 --- /dev/null +++ b/test/plugin_functional/test_suites/core/index.ts @@ -0,0 +1,25 @@ +/* + * 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 { PluginFunctionalProviderContext } from '../../services'; + +export default function ({ loadTestFile }: PluginFunctionalProviderContext) { + describe('core', function () { + loadTestFile(require.resolve('./route')); + }); +} diff --git a/test/plugin_functional/test_suites/core/route.ts b/test/plugin_functional/test_suites/core/route.ts new file mode 100644 index 0000000000000..becde49a69714 --- /dev/null +++ b/test/plugin_functional/test_suites/core/route.ts @@ -0,0 +1,174 @@ +/* + * 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 expect from '@kbn/expect'; +import { Test } from 'supertest'; +import { PluginFunctionalProviderContext } from '../../services'; + +export default function ({ getService }: PluginFunctionalProviderContext) { + const supertest = getService('supertest'); + + describe('route', function () { + describe('timeouts', function () { + const writeBodyCharAtATime = (request: Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + resolve(res); + }); + } + }, interval); + request.on('error', (err) => { + clearInterval(intervalId); + reject(err); + }); + }); + }; + + describe('payload', function () { + it(`should timeout if POST payload sending is too slow`, async () => { + // start the request + const request = supertest + .post('/short_payload_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('Request Timeout'); + } + ); + }); + + it(`should not timeout if POST payload sending is quick`, async () => { + // start the request + const request = supertest + .post('/longer_payload_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + }); + + describe('idle socket', function () { + it('should timeout if payload sending has too long of an idle period', async function () { + // start the request + const request = supertest + .post('/short_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 20); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('socket hang up'); + } + ); + }); + + it('should not timeout if payload sending does not have too long of an idle period', async function () { + // start the request + const request = supertest + .post('/longer_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":0}', 10); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + + it('should timeout if servers response is too slow', async function () { + // start the request + const request = supertest + .post('/short_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 0); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('socket hang up'); + } + ); + }); + + it('should not timeout if servers response is fast enough', async function () { + // start the request + const request = supertest + .post('/longer_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 0); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + }); + }); + }); +} diff --git a/x-pack/plugins/lists/server/routes/import_list_item_route.ts b/x-pack/plugins/lists/server/routes/import_list_item_route.ts index d46c943d95fe9..f7ecc7ac1ac83 100644 --- a/x-pack/plugins/lists/server/routes/import_list_item_route.ts +++ b/x-pack/plugins/lists/server/routes/import_list_item_route.ts @@ -27,7 +27,9 @@ export const importListItemRoute = (router: IRouter, config: ConfigType): void = parse: false, }, tags: ['access:lists-all'], - timeout: config.importTimeout.asMilliseconds(), + timeout: { + payload: config.importTimeout.asMilliseconds(), + }, }, path: `${LIST_ITEM_URL}/_import`, validate: {