[Ingest Manager] Don't retain POST /setup results. fixes #74587 (#75372)

* add retries for registry requests. works, afaict. no tests. one TS issue. * Fix TS issue. Add link to node-fetch error docs * Restore some accidentally deleted code. * Add more comments. Remove logging. * Add tests for plugin setup service & handlers * Add tests for Registry retry logic * Extract setup retry logic to separate function/file * Add tests for setup retry logic ``` firstSuccessOrTryAgain ✓ reject/throws is called again & its value returned (18ms) ✓ the first success value is cached (2ms) ``` * More straightforward(?) tests for setup caching * Revert cached setup. Still limit 1 call at a time Terrible tests. Committing & pushing to see if it fixes failures like https://github.com/elastic/kibana/pull/74507/checks?check_run_id=980178887 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/67892/execution/node/663/log/ ``` 07:36:56 └-> "before all" hook 07:36:56 └-> should not allow to enroll an agent with a invalid enrollment 07:36:56 └-> "before each" hook: global before each 07:36:56 └-> "before each" hook: beforeSetupWithDockerRegistry 07:36:56 │ proc [kibana] error [11:36:56.369] Error: Internal Server Error 07:36:56 │ proc [kibana] at HapiResponseAdapter.toError (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:132:19) 07:36:56 │ proc [kibana] at HapiResponseAdapter.toHapiResponse (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:86:19) 07:36:56 │ proc [kibana] at HapiResponseAdapter.handle (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/response_adapter.js:81:17) 07:36:56 │ proc [kibana] at Router.handle (/dev/shm/workspace/parallel/5/kibana/build/kibana-build-xpack/src/core/server/http/router/router.js:164:34) 07:36:56 │ proc [kibana] at process._tickCallback (internal/process/next_tick.js:68:7) 07:36:56 │ proc [kibana] log [11:36:56.581] [info][authentication][plugins][security] Authentication attempt failed: [security_exception] missing authentication credentials for REST request [/_security/_authenticate], with { header={ WWW-Authenticate={ 0="ApiKey" & 1="Basic realm=\"security\" charset=\"UTF-8\"" } } } 07:36:56 └- ✓ pass (60ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a invalid enrollment" 07:36:56 └-> should not allow to enroll an agent with a shared id if it already exists 07:36:56 └-> "before each" hook: global before each 07:36:56 └-> "before each" hook: beforeSetupWithDockerRegistry 07:36:56 └- ✓ pass (111ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a shared id if it already exists " 07:36:56 └-> should not allow to enroll an agent with a version > kibana 07:36:56 └-> "before each" hook: global before each 07:36:56 └-> "before each" hook: beforeSetupWithDockerRegistry 07:36:56 └- ✓ pass (58ms) "Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should not allow to enroll an agent with a version > kibana" 07:36:56 └-> should allow to enroll an agent with a valid enrollment token 07:36:56 └-> "before each" hook: global before each 07:36:56 └-> "before each" hook: beforeSetupWithDockerRegistry 07:36:56 └- ✖ fail: Ingest Manager Endpoints Fleet Endpoints fleet_agents_enroll should allow to enroll an agent with a valid enrollment token 07:36:56 │ Error: expected 200 "OK", got 500 "Internal Server Error" 07:36:56 │ at Test._assertStatus (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:268:12) 07:36:56 │ at Test._assertFunction (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:283:11) 07:36:56 │ at Test.assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:173:18) 07:36:56 │ at assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:131:12) 07:36:56 │ at /dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:128:5 07:36:56 │ at Test.Request.callback (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:718:3) 07:36:56 │ at parser (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:906:18) 07:36:56 │ at IncomingMessage.res.on (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/parsers/json.js:19:7) 07:36:56 │ at endReadableNT (_stream_readable.js:1145:12) 07:36:56 │ at process._tickCallback (internal/process/next_tick.js:63:19) 07:36:56 │ 07:36:56 │ ``` * New name & tests for one-at-a-time /setup behavior `firstPromiseBlocksAndFufills` for "the first promise created blocks others from being created, then fufills all with that first result" * More (better?) renaming * Fix name in test description * Fix spelling typo. * Remove registry retry code & tests * Use async fn's .catch to avoid unhandled rejection Add explicit `isPending` value instead of overloading role of `status`. Could probably do without it, but it makes the intent more clear. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elastic · Aug 20, 2020 · caf3bac · caf3bac
1 parent 0f494f4
commit caf3bac
Show file tree

Hide file tree

Showing 6 changed files with 420 additions and 136 deletions.
diff --git a/x-pack/plugins/ingest_manager/server/routes/setup/handlers.test.ts b/x-pack/plugins/ingest_manager/server/routes/setup/handlers.test.ts
@@ -0,0 +1,83 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { xpackMocks } from '../../../../../../x-pack/mocks';
+import { httpServerMock } from 'src/core/server/mocks';
+import { PostIngestSetupResponse } from '../../../common';
+import { RegistryError } from '../../errors';
+import { createAppContextStartContractMock } from '../../mocks';
+import { ingestManagerSetupHandler } from './handlers';
+import { appContextService } from '../../services/app_context';
+import { setupIngestManager } from '../../services/setup';
+
+jest.mock('../../services/setup', () => {
+  return {
+    setupIngestManager: jest.fn(),
+  };
+});
+
+const mockSetupIngestManager = setupIngestManager as jest.MockedFunction<typeof setupIngestManager>;
+
+describe('ingestManagerSetupHandler', () => {
+  let context: ReturnType<typeof xpackMocks.createRequestHandlerContext>;
+  let response: ReturnType<typeof httpServerMock.createResponseFactory>;
+  let request: ReturnType<typeof httpServerMock.createKibanaRequest>;
+
+  beforeEach(async () => {
+    context = xpackMocks.createRequestHandlerContext();
+    response = httpServerMock.createResponseFactory();
+    request = httpServerMock.createKibanaRequest({
+      method: 'post',
+      path: '/api/ingest_manager/setup',
+    });
+    // prevents `Logger not set.` and other appContext errors
+    appContextService.start(createAppContextStartContractMock());
+  });
+
+  afterEach(async () => {
+    jest.clearAllMocks();
+    appContextService.stop();
+  });
+
+  it('POST /setup succeeds w/200 and body of resolved value', async () => {
+    mockSetupIngestManager.mockImplementation(() => Promise.resolve({ isIntialized: true }));
+    await ingestManagerSetupHandler(context, request, response);
+
+    const expectedBody: PostIngestSetupResponse = { isInitialized: true };
+    expect(response.customError).toHaveBeenCalledTimes(0);
+    expect(response.ok).toHaveBeenCalledWith({ body: expectedBody });
+  });
+
+  it('POST /setup fails w/500 on custom error', async () => {
+    mockSetupIngestManager.mockImplementation(() =>
+      Promise.reject(new Error('SO method mocked to throw'))
+    );
+    await ingestManagerSetupHandler(context, request, response);
+
+    expect(response.customError).toHaveBeenCalledTimes(1);
+    expect(response.customError).toHaveBeenCalledWith({
+      statusCode: 500,
+      body: {
+        message: 'SO method mocked to throw',
+      },
+    });
+  });
+
+  it('POST /setup fails w/502 on RegistryError', async () => {
+    mockSetupIngestManager.mockImplementation(() =>
+      Promise.reject(new RegistryError('Registry method mocked to throw'))
+    );
+
+    await ingestManagerSetupHandler(context, request, response);
+    expect(response.customError).toHaveBeenCalledTimes(1);
+    expect(response.customError).toHaveBeenCalledWith({
+      statusCode: 502,
+      body: {
+        message: 'Registry method mocked to throw',
+      },
+    });
+  });
+});
diff --git a/x-pack/plugins/ingest_manager/server/routes/setup/index.ts b/x-pack/plugins/ingest_manager/server/routes/setup/index.ts
@@ -14,8 +14,7 @@ import {
 } from './handlers';
 import { PostFleetSetupRequestSchema } from '../../types';
 
-export const registerRoutes = (router: IRouter, config: IngestManagerConfigType) => {
-  // Ingest manager setup
+export const registerIngestManagerSetupRoute = (router: IRouter) => {
   router.post(
     {
       path: SETUP_API_ROUTE,
@@ -26,12 +25,20 @@ export const registerRoutes = (router: IRouter, config: IngestManagerConfigType)
     },
     ingestManagerSetupHandler
   );
+};
 
-  if (!config.fleet.enabled) {
-    return;
-  }
+export const registerCreateFleetSetupRoute = (router: IRouter) => {
+  router.post(
+    {
+      path: FLEET_SETUP_API_ROUTES.CREATE_PATTERN,
+      validate: PostFleetSetupRequestSchema,
+      options: { tags: [`access:${PLUGIN_ID}-all`] },
+    },
+    createFleetSetupHandler
+  );
+};
 
-  // Get Fleet setup
+export const registerGetFleetStatusRoute = (router: IRouter) => {
   router.get(
     {
       path: FLEET_SETUP_API_ROUTES.INFO_PATTERN,
@@ -40,14 +47,19 @@ export const registerRoutes = (router: IRouter, config: IngestManagerConfigType)
     },
     getFleetStatusHandler
   );
+};
+
+export const registerRoutes = (router: IRouter, config: IngestManagerConfigType) => {
+  // Ingest manager setup
+  registerIngestManagerSetupRoute(router);
+
+  if (!config.fleet.enabled) {
+    return;
+  }
+
+  // Get Fleet setup
+  registerGetFleetStatusRoute(router);
 
   // Create Fleet setup
-  router.post(
-    {
-      path: FLEET_SETUP_API_ROUTES.CREATE_PATTERN,
-      validate: PostFleetSetupRequestSchema,
-      options: { tags: [`access:${PLUGIN_ID}-all`] },
-    },
-    createFleetSetupHandler
-  );
+  registerCreateFleetSetupRoute(router);
 };
diff --git a/x-pack/plugins/ingest_manager/server/services/setup.test.ts b/x-pack/plugins/ingest_manager/server/services/setup.test.ts
@@ -4,41 +4,59 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { xpackMocks } from '../../../../../x-pack/mocks';
+import { createAppContextStartContractMock } from '../mocks';
+import { appContextService } from './app_context';
 import { setupIngestManager } from './setup';
-import { savedObjectsClientMock } from 'src/core/server/mocks';
 
-describe('setupIngestManager', () => {
-  it('returned promise should reject if errors thrown', async () => {
-    const { savedObjectsClient, callClusterMock } = makeErrorMocks();
-    const setupPromise = setupIngestManager(savedObjectsClient, callClusterMock);
-    await expect(setupPromise).rejects.toThrow('mocked');
+const mockedMethodThrowsError = () =>
+  jest.fn().mockImplementation(() => {
+    throw new Error('SO method mocked to throw');
   });
-});
 
-function makeErrorMocks() {
-  jest.mock('./app_context'); // else fails w/"Logger not set."
-  jest.mock('./epm/registry/registry_url', () => {
-    return {
-      fetchUrl: () => {
-        throw new Error('mocked registry#fetchUrl');
-      },
-    };
+class CustomTestError extends Error {}
+const mockedMethodThrowsCustom = () =>
+  jest.fn().mockImplementation(() => {
+    throw new CustomTestError('method mocked to throw');
   });
 
-  const callClusterMock = jest.fn();
-  const savedObjectsClient = savedObjectsClientMock.create();
-  savedObjectsClient.find = jest.fn().mockImplementation(() => {
-    throw new Error('mocked SO#find');
-  });
-  savedObjectsClient.get = jest.fn().mockImplementation(() => {
-    throw new Error('mocked SO#get');
+describe('setupIngestManager', () => {
+  let context: ReturnType<typeof xpackMocks.createRequestHandlerContext>;
+
+  beforeEach(async () => {
+    context = xpackMocks.createRequestHandlerContext();
+    // prevents `Logger not set.` and other appContext errors
+    appContextService.start(createAppContextStartContractMock());
   });
-  savedObjectsClient.update = jest.fn().mockImplementation(() => {
-    throw new Error('mocked SO#update');
+
+  afterEach(async () => {
+    jest.clearAllMocks();
+    appContextService.stop();
   });
 
-  return {
-    savedObjectsClient,
-    callClusterMock,
-  };
-}
+  describe('should reject with any error thrown underneath', () => {
+    it('SO client throws plain Error', async () => {
+      const soClient = context.core.savedObjects.client;
+      soClient.create = mockedMethodThrowsError();
+      soClient.find = mockedMethodThrowsError();
+      soClient.get = mockedMethodThrowsError();
+      soClient.update = mockedMethodThrowsError();
+
+      const setupPromise = setupIngestManager(soClient, jest.fn());
+      await expect(setupPromise).rejects.toThrow('SO method mocked to throw');
+      await expect(setupPromise).rejects.toThrow(Error);
+    });
+
+    it('SO client throws other error', async () => {
+      const soClient = context.core.savedObjects.client;
+      soClient.create = mockedMethodThrowsCustom();
+      soClient.find = mockedMethodThrowsCustom();
+      soClient.get = mockedMethodThrowsCustom();
+      soClient.update = mockedMethodThrowsCustom();
+
+      const setupPromise = setupIngestManager(soClient, jest.fn());
+      await expect(setupPromise).rejects.toThrow('method mocked to throw');
+      await expect(setupPromise).rejects.toThrow(CustomTestError);
+    });
+  });
+});