Allow routes to specify the idle socket timeout in addition to the pa…

…yload 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 <me@joshdover.com> * 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 <elasticmachine@users.noreply.github.com> Co-authored-by: Josh Dover <me@joshdover.com>
elastic · Aug 18, 2020 · 83bc500 · 83bc500
1 parent f0430f2
commit 83bc500
Show file tree

Hide file tree

Showing 17 changed files with 843 additions and 217 deletions.
diff --git 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<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 |
+|  [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | <code>{</code><br/><code>        payload?: Method extends 'get' &#124; 'options' ? undefined : number;</code><br/><code>        idleSocket?: number;</code><br/><code>    }</code> | Defines per-route timeouts. |
 |  [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 |
 
diff --git a/...development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md b/...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.
 
 <b>Signature:</b>
 
 ```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
@@ -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,
-    });
   });
 });