Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIEM][Detection Engine][Lists] Adds the ability to change the timeout limits from 10 seconds for loads for imports #73103

Merged
merged 13 commits into from
Jul 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ export interface RouteConfigOptions<Method extends RouteMethod>
| [authRequired](./kibana-plugin-core-server.routeconfigoptions.authrequired.md) | <code>boolean &#124; 'optional'</code> | 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 <code>true</code> if an auth mechanism is registered. |
| [body](./kibana-plugin-core-server.routeconfigoptions.body.md) | <code>Method extends 'get' &#124; 'options' ? undefined : RouteConfigOptionsBody</code> | Additional body options [RouteConfigOptionsBody](./kibana-plugin-core-server.routeconfigoptionsbody.md)<!-- -->. |
| [tags](./kibana-plugin-core-server.routeconfigoptions.tags.md) | <code>readonly string[]</code> | Additional metadata tag strings to attach to the route. |
| [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | <code>number</code> | Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes |
| [xsrfRequired](./kibana-plugin-core-server.routeconfigoptions.xsrfrequired.md) | <code>Method extends 'get' ? never : boolean</code> | Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain <code>kbn-xsrf</code> header. - false. Disables xsrf protection.<!-- -->Set to true by default |

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [RouteConfigOptions](./kibana-plugin-core-server.routeconfigoptions.md) &gt; [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md)

## RouteConfigOptions.timeout property

Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes

<b>Signature:</b>

```typescript
timeout?: number;
```
127 changes: 127 additions & 0 deletions src/core/server/http/http_server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,133 @@ 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);

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 });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
timeout: 300000,
});
});

test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a GET', async () => {
mshustov marked this conversation as resolved.
Show resolved Hide resolved
const { registerRouter, server: innerServer } = await server.setup(config);

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 });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).get('/').expect(200, {
timeout: 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);

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 });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).delete('/').expect(200, {
timeout: 300000,
});
});

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

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 });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).put('/').expect(200, {
timeout: 300000,
});
});

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

const router = new Router('', logger, enhanceWithContext);
router.patch(
{
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 });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).patch('/').expect(200, {
timeout: 300000,
});
});
});

test('should return a stream in the body', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

Expand Down
22 changes: 19 additions & 3 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,10 @@ export class HttpServer {
this.log.debug(`registering route handler for [${route.path}]`);
// Hapi does not allow payload validation to be specified for 'head' or 'get' requests
const validate = isSafeMethod(route.method) ? undefined : { payload: true };
const { authRequired, tags, body = {} } = route.options;
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),
Expand All @@ -181,9 +183,23 @@ export class HttpServer {
// 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].some((v) => typeof v !== 'undefined')
? { allow, maxBytes, output, parse }
payload: [allow, maxBytes, output, parse, payloadTimeout].some(
(v) => typeof v !== 'undefined'
)
? {
allow,
maxBytes,
output,
parse,
timeout: payloadTimeout,
}
: undefined,
timeout:
timeout != null
? {
socket: timeout + 1, // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond
}
: undefined,
},
});
}
Expand Down
124 changes: 124 additions & 0 deletions src/core/server/http/integration_tests/router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,130 @@ 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('/');

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, {});
});

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

expect(supertest(innerServer.listener).put('/a')).rejects.toThrow('socket hang up');
await supertest(innerServer.listener).put('/b').expect(200, {});
});

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

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a test for 408 Request timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 408 Request timeout only happens when it is a PUT/POST with a form/payload:
https://hapi.dev/api/?v=19.2.0#-routeoptionspayloadtimeout

And then you have to simulate a connection that starts but then stalls or does it so slowly that the backend decides to then disconnect you because you're taking too long. I looked around at supertest yesterday for a quite a while hoping it had an easy way to explicitly test the condition but it does not look like there is a way for me to be able to start an upload and then stall it.

For the other endpoints such as GET, DELETE which do not take a payload when the socket does a disconnect Hapi server will do a hangup on them on a timeout and not give back a 408 Request timeout. It doesn't advertise that is its behavior but that is the behavior as the tests are showing.

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Jul 28, 2020

Choose a reason for hiding this comment

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

I can set a different switch which is the timeout.server here:
https://hapi.dev/api/?v=19.2.0#-routeoptionstimeoutserver

And then increase the socket timeout to be slightly higher than that and instead of a socket hangup we would get a:

Service Unavailable (503) error response.

For GET, DELETE endpoints and a 408 for when its payload based with PUT/POST, but it seemed like the socket hangup was more preferred from the example you sent me in the tests. Just let me know if you want me to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay if you merge it as is and create an issue for the platform team to add such a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket I created for this as requested:
#73557

await supertest(innerServer.listener).delete('/b').expect(200, {});
});

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

expect(supertest(innerServer.listener).get('/a')).rejects.toThrow('socket hang up');
await supertest(innerServer.listener).get('/b').expect(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('/');

router.post(
{ path: '/a', validate: false, options: { timeout: 300000 } },
Copy link
Contributor

Choose a reason for hiding this comment

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

what do we test here?

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Jul 28, 2020

Choose a reason for hiding this comment

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

This tests and the others around it test that if the timeout is set it will work (sanity test). I didn't think adding an incredibly long lived test to ensure they can exceed the 2 minutes would be ok so I instead opted for a simple sanity check.

But really exceeding the full 2 minutes but not exceeding 5 minutes would be a typical positive unit test that it is possible to exceed the default 2 minutes and operates as we would want it. However, during integration tests I thought I probably shouldn't exceed 2 minutes for the test so instead I just opted for a test that shows when it is set on a regular endpoint call it will operate normally and not cause any issues such as unexpected timeouts.

async (context, req, res) => res.ok({})
);
await server.start();
await supertest(innerServer.listener).post('/a').expect(200, {});
});

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

router.put(
{ path: '/a', validate: false, options: { timeout: 300000 } },
async (context, req, res) => res.ok({})
);
await server.start();

await supertest(innerServer.listener).put('/a').expect(200, {});
});

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

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 configured with a 5 minute timeout value for a GET', async () => {
const { server: innerServer, createRouter } = await server.setup(setupDeps);
const router = createRouter('/');

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, {});
});
});
});

describe('Cache-Control', () => {
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/http/router/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,14 @@ export class KibanaRequest<
private getRouteInfo(request: Request): KibanaRequestRoute<Method> {
const method = request.method as Method;
const { parse, maxBytes, allow, output } = request.route.settings.payload || {};
const timeout = request.route.settings.timeout?.socket;

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
body: isSafeMethod(method)
? undefined
: {
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/http/router/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
* Additional body options {@link RouteConfigOptionsBody}.
*/
body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody;

/**
* Timeouts for processing durations. Response timeout is in milliseconds.
* Default value: 2 minutes
*/
timeout?: number;
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1859,6 +1859,7 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
authRequired?: boolean | 'optional';
body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody;
tags?: readonly string[];
timeout?: number;
xsrfRequired?: Method extends 'get' ? never : boolean;
}

Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/lists/common/constants.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { EntriesArray } from './schemas/types';
import moment from 'moment';

import { EntriesArray } from './schemas/types';
export const DATE_NOW = '2020-04-20T15:25:31.830Z';
export const OLD_DATE_RELATIVE_TO_DATE_NOW = '2020-04-19T15:25:31.830Z';
export const USER = 'some user';
Expand Down Expand Up @@ -64,3 +65,4 @@ export const CURSOR = 'c29tZXN0cmluZ2ZvcnlvdQ==';
export const _VERSION = 'WzI5NywxXQ==';
export const VERSION = 1;
export const IMMUTABLE = false;
export const IMPORT_TIMEOUT = moment.duration(5, 'minutes');
2 changes: 2 additions & 0 deletions x-pack/plugins/lists/server/config.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import {
IMPORT_BUFFER_SIZE,
IMPORT_TIMEOUT,
LIST_INDEX,
LIST_ITEM_INDEX,
MAX_IMPORT_PAYLOAD_BYTES,
Expand All @@ -21,6 +22,7 @@ export const getConfigMock = (): Partial<ConfigType> => ({
export const getConfigMockDecoded = (): ConfigType => ({
enabled: true,
importBufferSize: IMPORT_BUFFER_SIZE,
importTimeout: IMPORT_TIMEOUT,
listIndex: LIST_INDEX,
listItemIndex: LIST_ITEM_INDEX,
maxImportPayloadBytes: MAX_IMPORT_PAYLOAD_BYTES,
Expand Down
Loading