Add support for /api/status before Kibana completes startup (elastic#…

…79012) Co-authored-by: Larry Gregory <larry.gregory@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
madirey · May 11, 2021 · 325a42d · 325a42d
1 parent e4e7619
commit 325a42d
Show file tree

Hide file tree

Showing 29 changed files with 363 additions and 74 deletions.
diff --git a/...server/kibana-plugin-core-server.customhttpresponseoptions.bypasserrorformat.md b/...server/kibana-plugin-core-server.customhttpresponseoptions.bypasserrorformat.md
@@ -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; [CustomHttpResponseOptions](./kibana-plugin-core-server.customhttpresponseoptions.md) &gt; [bypassErrorFormat](./kibana-plugin-core-server.customhttpresponseoptions.bypasserrorformat.md)
+
+## CustomHttpResponseOptions.bypassErrorFormat property
+
+Bypass the default error formatting
+
+<b>Signature:</b>
+
+```typescript
+bypassErrorFormat?: boolean;
+```
diff --git a/.../development/core/server/kibana-plugin-core-server.customhttpresponseoptions.md b/.../development/core/server/kibana-plugin-core-server.customhttpresponseoptions.md
@@ -17,6 +17,7 @@ export interface CustomHttpResponseOptions<T extends HttpResponsePayload | Respo
 |  Property | Type | Description |
 |  --- | --- | --- |
 |  [body](./kibana-plugin-core-server.customhttpresponseoptions.body.md) | <code>T</code> | HTTP message to send to the client |
+|  [bypassErrorFormat](./kibana-plugin-core-server.customhttpresponseoptions.bypasserrorformat.md) | <code>boolean</code> | Bypass the default error formatting |
 |  [headers](./kibana-plugin-core-server.customhttpresponseoptions.headers.md) | <code>ResponseHeaders</code> | HTTP Headers with additional information about response |
 |  [statusCode](./kibana-plugin-core-server.customhttpresponseoptions.statuscode.md) | <code>number</code> |  |
 
diff --git a/.../core/server/kibana-plugin-core-server.httpresponseoptions.bypasserrorformat.md b/.../core/server/kibana-plugin-core-server.httpresponseoptions.bypasserrorformat.md
@@ -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; [HttpResponseOptions](./kibana-plugin-core-server.httpresponseoptions.md) &gt; [bypassErrorFormat](./kibana-plugin-core-server.httpresponseoptions.bypasserrorformat.md)
+
+## HttpResponseOptions.bypassErrorFormat property
+
+Bypass the default error formatting
+
+<b>Signature:</b>
+
+```typescript
+bypassErrorFormat?: boolean;
+```
diff --git a/docs/development/core/server/kibana-plugin-core-server.httpresponseoptions.md b/docs/development/core/server/kibana-plugin-core-server.httpresponseoptions.md
@@ -17,5 +17,6 @@ export interface HttpResponseOptions
 |  Property | Type | Description |
 |  --- | --- | --- |
 |  [body](./kibana-plugin-core-server.httpresponseoptions.body.md) | <code>HttpResponsePayload</code> | HTTP message to send to the client |
+|  [bypassErrorFormat](./kibana-plugin-core-server.httpresponseoptions.bypasserrorformat.md) | <code>boolean</code> | Bypass the default error formatting |
 |  [headers](./kibana-plugin-core-server.httpresponseoptions.headers.md) | <code>ResponseHeaders</code> | HTTP Headers with additional information about response |
 
diff --git a/packages/kbn-test/src/functional_tests/lib/run_kibana_server.js b/packages/kbn-test/src/functional_tests/lib/run_kibana_server.js
@@ -38,7 +38,7 @@ export async function runKibanaServer({ procs, config, options }) {
       ...extendNodeOptions(installDir),
     },
     cwd: installDir || KIBANA_ROOT,
-    wait: /http server running/,
+    wait: /\[Kibana\]\[http\] http server running/,
   });
 }
 

diff --git a/packages/kbn-test/src/kbn_client/kbn_client_requester.ts b/packages/kbn-test/src/kbn_client/kbn_client_requester.ts
@@ -19,6 +19,10 @@ const isConcliftOnGetError = (error: any) => {
   );
 };
 
+const isIgnorableError = (error: any, ignorableErrors: number[] = []) => {
+  return isAxiosResponseError(error) && ignorableErrors.includes(error.response.status);
+};
+
 export const uriencode = (
   strings: TemplateStringsArray,
   ...values: Array<string | number | boolean>
@@ -53,6 +57,7 @@ export interface ReqOptions {
   body?: any;
   retries?: number;
   headers?: Record<string, string>;
+  ignoreErrors?: number[];
   responseType?: ResponseType;
 }
 
@@ -125,6 +130,10 @@ export class KbnClientRequester {
         const requestedRetries = options.retries !== undefined;
         const failedToGetResponse = isAxiosRequestError(error);
 
+        if (isIgnorableError(error, options.ignoreErrors)) {
+          return error.response;
+        }
+
         let errorMessage;
         if (conflictOnGet) {
           errorMessage = `Conflict on GET (path=${options.path}, attempt=${attempt}/${maxAttempts})`;

diff --git a/packages/kbn-test/src/kbn_client/kbn_client_status.ts b/packages/kbn-test/src/kbn_client/kbn_client_status.ts
@@ -44,6 +44,8 @@ export class KbnClientStatus {
     const { data } = await this.requester.request<ApiResponseStatus>({
       method: 'GET',
       path: 'api/status',
+      // Status endpoint returns 503 if any services are in an unavailable state
+      ignoreErrors: [503],
     });
     return data;
   }

diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts
@@ -138,6 +138,40 @@ test('log listening address after started when configured with BasePath and rewr
   `);
 });
 
+test('does not allow router registration after server is listening', async () => {
+  expect(server.isListening()).toBe(false);
+
+  const { registerRouter } = await server.setup(config);
+
+  const router1 = new Router('/foo', logger, enhanceWithContext);
+  expect(() => registerRouter(router1)).not.toThrowError();
+
+  await server.start();
+
+  expect(server.isListening()).toBe(true);
+
+  const router2 = new Router('/bar', logger, enhanceWithContext);
+  expect(() => registerRouter(router2)).toThrowErrorMatchingInlineSnapshot(
+    `"Routers can be registered only when HTTP server is stopped."`
+  );
+});
+
+test('allows router registration after server is listening via `registerRouterAfterListening`', async () => {
+  expect(server.isListening()).toBe(false);
+
+  const { registerRouterAfterListening } = await server.setup(config);
+
+  const router1 = new Router('/foo', logger, enhanceWithContext);
+  expect(() => registerRouterAfterListening(router1)).not.toThrowError();
+
+  await server.start();
+
+  expect(server.isListening()).toBe(true);
+
+  const router2 = new Router('/bar', logger, enhanceWithContext);
+  expect(() => registerRouterAfterListening(router2)).not.toThrowError();
+});
+
 test('valid params', async () => {
   const router = new Router('/foo', logger, enhanceWithContext);
 

diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts
@@ -33,6 +33,7 @@ import {
   KibanaRouteOptions,
   KibanaRequestState,
   isSafeMethod,
+  RouterRoute,
 } from './router';
 import {
   SessionStorageCookieOptions,
@@ -52,6 +53,13 @@ export interface HttpServerSetup {
    * @param router {@link IRouter} - a router with registered route handlers.
    */
   registerRouter: (router: IRouter) => void;
+  /**
+   * Add all the routes registered with `router` to HTTP server request listeners.
+   * Unlike `registerRouter`, this function allows routes to be registered even after the server
+   * has started listening for requests.
+   * @param router {@link IRouter} - a router with registered route handlers.
+   */
+  registerRouterAfterListening: (router: IRouter) => void;
   registerStaticDir: (path: string, dirPath: string) => void;
   basePath: HttpServiceSetup['basePath'];
   csp: HttpServiceSetup['csp'];
@@ -114,6 +122,17 @@ export class HttpServer {
     this.registeredRouters.add(router);
   }
 
+  private registerRouterAfterListening(router: IRouter) {
+    if (this.isListening()) {
+      for (const route of router.getRoutes()) {
+        this.configureRoute(route);
+      }
+    } else {
+      // Not listening yet, add to set of registeredRouters so that it can be added after listening has started.
+      this.registeredRouters.add(router);
+    }
+  }
+
   public async setup(config: HttpConfig): Promise<HttpServerSetup> {
     const serverOptions = getServerOptions(config);
     const listenerOptions = getListenerOptions(config);
@@ -130,6 +149,7 @@ export class HttpServer {
 
     return {
       registerRouter: this.registerRouter.bind(this),
+      registerRouterAfterListening: this.registerRouterAfterListening.bind(this),
       registerStaticDir: this.registerStaticDir.bind(this),
       registerOnPreRouting: this.registerOnPreRouting.bind(this),
       registerOnPreAuth: this.registerOnPreAuth.bind(this),
@@ -170,45 +190,7 @@ export class HttpServer {
 
     for (const router of this.registeredRouters) {
       for (const route of router.getRoutes()) {
-        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 = {}, timeout } = route.options;
-        const { accepts: allow, maxBytes, output, parse } = body;
-
-        const kibanaRouteOptions: KibanaRouteOptions = {
-          xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
-        };
-
-        this.server.route({
-          handler: route.handler,
-          method: route.method,
-          path: route.path,
-          options: {
-            auth: this.getAuthOption(authRequired),
-            app: kibanaRouteOptions,
-            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,
-            // @ts-expect-error Types are outdated and doesn't allow `payload.multipart` to be `true`
-            payload: [allow, maxBytes, output, parse, timeout?.payload].some((x) => x !== undefined)
-              ? {
-                  allow,
-                  maxBytes,
-                  output,
-                  parse,
-                  timeout: timeout?.payload,
-                  multipart: true,
-                }
-              : undefined,
-            timeout: {
-              socket: timeout?.idleSocket ?? this.config!.socketTimeout,
-            },
-          },
-        });
+        this.configureRoute(route);
       }
     }
 
@@ -486,4 +468,46 @@ export class HttpServer {
       options: { auth: false },
     });
   }
+
+  private configureRoute(route: RouterRoute) {
+    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 = {}, timeout } = route.options;
+    const { accepts: allow, maxBytes, output, parse } = body;
+
+    const kibanaRouteOptions: KibanaRouteOptions = {
+      xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
+    };
+
+    this.server!.route({
+      handler: route.handler,
+      method: route.method,
+      path: route.path,
+      options: {
+        auth: this.getAuthOption(authRequired),
+        app: kibanaRouteOptions,
+        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,
+        // @ts-expect-error Types are outdated and doesn't allow `payload.multipart` to be `true`
+        payload: [allow, maxBytes, output, parse, timeout?.payload].some((x) => x !== undefined)
+          ? {
+              allow,
+              maxBytes,
+              output,
+              parse,
+              timeout: timeout?.payload,
+              multipart: true,
+            }
+          : undefined,
+        timeout: {
+          socket: timeout?.idleSocket ?? this.config!.socketTimeout,
+        },
+      },
+    });
+  }
 }
diff --git a/src/core/server/http/http_service.test.ts b/src/core/server/http/http_service.test.ts
@@ -68,20 +68,32 @@ test('creates and sets up http server', async () => {
     start: jest.fn(),
     stop: jest.fn(),
   };
-  mockHttpServer.mockImplementation(() => httpServer);
+  const notReadyHttpServer = {
+    isListening: () => false,
+    setup: jest.fn().mockReturnValue({ server: fakeHapiServer }),
+    start: jest.fn(),
+    stop: jest.fn(),
+  };
+  mockHttpServer.mockImplementationOnce(() => httpServer);
+  mockHttpServer.mockImplementationOnce(() => notReadyHttpServer);
 
   const service = new HttpService({ coreId, configService, env, logger });
 
   expect(mockHttpServer.mock.instances.length).toBe(1);
 
   expect(httpServer.setup).not.toHaveBeenCalled();
+  expect(notReadyHttpServer.setup).not.toHaveBeenCalled();
 
   await service.setup(setupDeps);
   expect(httpServer.setup).toHaveBeenCalled();
   expect(httpServer.start).not.toHaveBeenCalled();
 
+  expect(notReadyHttpServer.setup).toHaveBeenCalled();
+  expect(notReadyHttpServer.start).toHaveBeenCalled();
+
   await service.start();
   expect(httpServer.start).toHaveBeenCalled();
+  expect(notReadyHttpServer.stop).toHaveBeenCalled();
 });
 
 test('spins up notReady server until started if configured with `autoListen:true`', async () => {
@@ -102,6 +114,8 @@ test('spins up notReady server until started if configured with `autoListen:true
     .mockImplementationOnce(() => httpServer)
     .mockImplementationOnce(() => ({
       setup: () => ({ server: notReadyHapiServer }),
+      start: jest.fn(),
+      stop: jest.fn().mockImplementation(() => notReadyHapiServer.stop()),
     }));
 
   const service = new HttpService({
@@ -163,14 +177,22 @@ test('stops http server', async () => {
     start: noop,
     stop: jest.fn(),
   };
-  mockHttpServer.mockImplementation(() => httpServer);
+  const notReadyHttpServer = {
+    isListening: () => false,
+    setup: jest.fn().mockReturnValue({ server: fakeHapiServer }),
+    start: noop,
+    stop: jest.fn(),
+  };
+  mockHttpServer.mockImplementationOnce(() => httpServer);
+  mockHttpServer.mockImplementationOnce(() => notReadyHttpServer);
 
   const service = new HttpService({ coreId, configService, env, logger });
 
   await service.setup(setupDeps);
   await service.start();
 
   expect(httpServer.stop).toHaveBeenCalledTimes(0);
+  expect(notReadyHttpServer.stop).toHaveBeenCalledTimes(1);
 
   await service.stop();
 
@@ -188,7 +210,7 @@ test('stops not ready server if it is running', async () => {
     isListening: () => false,
     setup: jest.fn().mockReturnValue({ server: mockHapiServer }),
     start: noop,
-    stop: jest.fn(),
+    stop: jest.fn().mockImplementation(() => mockHapiServer.stop()),
   };
   mockHttpServer.mockImplementation(() => httpServer);
 
@@ -198,7 +220,7 @@ test('stops not ready server if it is running', async () => {
 
   await service.stop();
 
-  expect(mockHapiServer.stop).toHaveBeenCalledTimes(1);
+  expect(mockHapiServer.stop).toHaveBeenCalledTimes(2);
 });
 
 test('register route handler', async () => {
@@ -231,6 +253,7 @@ test('returns http server contract on setup', async () => {
   mockHttpServer.mockImplementation(() => ({
     isListening: () => false,
     setup: jest.fn().mockReturnValue(httpServer),
+    start: noop,
     stop: noop,
   }));