Update responsibilities of migration finalize/delete endpoints

Discussions with @marshallmain lead to the following refactor: * finalize does not delete tasks * finalize only applies cleanup policy to a failed migration * delete takes an array of migration ids (like finalize) * delete hard-deletes the SavedObject of a completed (failed or successful) migration This gives a bit more flexibility with the endpoints, as well as disambiguates the semantics: it just deletes migrations!
elastic · Dec 15, 2020 · a1d2d6a · a1d2d6a
1 parent 082ec2c
commit a1d2d6a
Show file tree

Hide file tree

Showing 18 changed files with 120 additions and 164 deletions.
diff --git a/...urity_solution/common/detection_engine/schemas/request/delete_signals_migration_schema.ts b/...urity_solution/common/detection_engine/schemas/request/delete_signals_migration_schema.ts
@@ -6,11 +6,9 @@
 
 import * as t from 'io-ts';
 
-import { index } from '../common/schemas';
-
 export const deleteSignalsMigrationSchema = t.exact(
   t.type({
-    index,
+    migration_ids: t.array(t.string),
   })
 );
 

diff --git a/...plugins/security_solution/server/lib/detection_engine/migrations/delete_migration.test.ts b/...plugins/security_solution/server/lib/detection_engine/migrations/delete_migration.test.ts
@@ -8,9 +8,11 @@ import { ElasticsearchClient, SavedObjectsClientContract } from 'src/core/server
 import { elasticsearchServiceMock, savedObjectsClientMock } from 'src/core/server/mocks';
 import { deleteMigration } from './delete_migration';
 import { getSignalsMigrationSavedObjectMock } from './saved_objects_schema.mock';
-import { updateMigrationSavedObject } from './update_migration_saved_object';
+import { deleteMigrationSavedObject } from './delete_migration_saved_object';
+import { applyMigrationCleanupPolicy } from './migration_cleanup';
 
-jest.mock('./update_migration_saved_object');
+jest.mock('./migration_cleanup');
+jest.mock('./delete_migration_saved_object');
 
 describe('deleteMigration', () => {
   let esClient: ElasticsearchClient;
@@ -19,24 +21,6 @@ describe('deleteMigration', () => {
   beforeEach(() => {
     esClient = elasticsearchServiceMock.createElasticsearchClient();
     soClient = savedObjectsClientMock.create();
-
-    // stub out our update call to just return the attributes we passed
-    (updateMigrationSavedObject as jest.Mock).mockImplementation(({ attributes }) => ({
-      attributes,
-    }));
-  });
-
-  it('does not delete an already-deleted migration', async () => {
-    const deletedMigration = getSignalsMigrationSavedObjectMock({ deleted: true });
-    await deleteMigration({
-      esClient,
-      migration: deletedMigration,
-      signalsAlias: 'my-signals-alias',
-      soClient,
-      username: 'username',
-    });
-
-    expect(updateMigrationSavedObject).not.toHaveBeenCalled();
   });
 
   it('does not delete a pending migration', async () => {
@@ -46,10 +30,9 @@ describe('deleteMigration', () => {
       migration: pendingMigration,
       signalsAlias: 'my-signals-alias',
       soClient,
-      username: 'username',
     });
 
-    expect(updateMigrationSavedObject).not.toHaveBeenCalled();
+    expect(deleteMigrationSavedObject).not.toHaveBeenCalled();
   });
 
   it('deletes a failed migration', async () => {
@@ -59,16 +42,19 @@ describe('deleteMigration', () => {
       migration: failedMigration,
       signalsAlias: 'my-signals-alias',
       soClient,
-      username: 'username',
     });
 
-    expect(updateMigrationSavedObject).toHaveBeenCalledWith(
+    expect(deletedMigration.id).toEqual(failedMigration.id);
+    expect(deleteMigrationSavedObject).toHaveBeenCalledWith(
       expect.objectContaining({
-        attributes: { deleted: true },
+        id: failedMigration.id,
+      })
+    );
+    expect(applyMigrationCleanupPolicy).toHaveBeenCalledWith(
+      expect.objectContaining({
+        index: failedMigration.attributes.destinationIndex,
       })
     );
-    expect(deletedMigration.id).toEqual(failedMigration.id);
-    expect(deletedMigration.attributes.deleted).toEqual(true);
   });
 
   it('deletes a successful migration', async () => {
@@ -78,16 +64,18 @@ describe('deleteMigration', () => {
       migration: successMigration,
       signalsAlias: 'my-signals-alias',
       soClient,
-      username: 'username',
     });
 
-    expect(updateMigrationSavedObject).toHaveBeenCalledWith(
+    expect(deletedMigration.id).toEqual(successMigration.id);
+    expect(deleteMigrationSavedObject).toHaveBeenCalledWith(
       expect.objectContaining({
-        attributes: { deleted: true },
+        id: successMigration.id,
+      })
+    );
+    expect(applyMigrationCleanupPolicy).toHaveBeenCalledWith(
+      expect.objectContaining({
+        index: successMigration.attributes.sourceIndex,
       })
     );
-
-    expect(deletedMigration.id).toEqual(successMigration.id);
-    expect(deletedMigration.attributes.deleted).toEqual(true);
   });
 });
diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/delete_migration.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/delete_migration.ts
@@ -5,22 +5,21 @@
  */
 
 import { ElasticsearchClient, SavedObjectsClientContract } from 'src/core/server';
-import { isMigrationDeleted, isMigrationFailed, isMigrationPending } from './helpers';
+import { deleteMigrationSavedObject } from './delete_migration_saved_object';
+import { isMigrationFailed, isMigrationPending, isMigrationSuccess } from './helpers';
 import { applyMigrationCleanupPolicy } from './migration_cleanup';
 import { SignalsMigrationSO } from './saved_objects_schema';
-import { updateMigrationSavedObject } from './update_migration_saved_object';
 
 /**
- * "Deletes" a completed migration:
- *   * soft-deletes the migration SO
+ * Deletes a completed migration:
+ *   * deletes the migration SO
  *   * deletes the underlying task document
  *   * applies deletion policy to the relevant index
  *
  * @param esClient An {@link ElasticsearchClient}
  * @param soClient An {@link SavedObjectsClientContract}
  * @param migration the migration to be finalized {@link SignalsMigrationSO}
  * @param signalsAlias the alias for signals indices
- * @param username name of the user initiating the deletion
  *
  * @returns the migration SavedObject {@link SignalsMigrationSO}
  * @throws if the migration is invalid or a client throws
@@ -30,19 +29,17 @@ export const deleteMigration = async ({
   migration,
   signalsAlias,
   soClient,
-  username,
 }: {
   esClient: ElasticsearchClient;
   migration: SignalsMigrationSO;
   signalsAlias: string;
   soClient: SavedObjectsClientContract;
-  username: string;
 }): Promise<SignalsMigrationSO> => {
-  if (isMigrationPending(migration) || isMigrationDeleted(migration)) {
+  if (isMigrationPending(migration)) {
     return migration;
   }
 
-  const { destinationIndex, taskId } = migration.attributes;
+  const { destinationIndex, sourceIndex, taskId } = migration.attributes;
 
   if (isMigrationFailed(migration)) {
     await applyMigrationCleanupPolicy({
@@ -51,30 +48,16 @@ export const deleteMigration = async ({
       index: destinationIndex,
     });
   }
-
-  try {
-    // task may have already have been deleted during finalization
-    await esClient.delete({ index: '.tasks', id: taskId });
-  } catch (error) {
-    if (error.statusCode !== 404) {
-      throw error;
-    }
+  if (isMigrationSuccess(migration)) {
+    await applyMigrationCleanupPolicy({
+      alias: signalsAlias,
+      esClient,
+      index: sourceIndex,
+    });
   }
 
-  const deletedMigration = await updateMigrationSavedObject({
-    username,
-    soClient,
-    id: migration.id,
-    attributes: {
-      deleted: true,
-    },
-  });
+  await esClient.delete({ index: '.tasks', id: taskId });
+  await deleteMigrationSavedObject({ id: migration.id, soClient });
 
-  return {
-    ...migration,
-    attributes: {
-      ...migration.attributes,
-      ...deletedMigration.attributes,
-    },
-  };
+  return migration;
 };
diff --git a/...security_solution/server/lib/detection_engine/migrations/delete_migration_saved_object.ts b/...security_solution/server/lib/detection_engine/migrations/delete_migration_saved_object.ts
@@ -0,0 +1,16 @@
+/*
+ * 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 { SavedObjectsClientContract } from 'src/core/server';
+import { signalsMigrationSOClient } from './saved_objects_client';
+
+export const deleteMigrationSavedObject = async ({
+  id,
+  soClient,
+}: {
+  id: string;
+  soClient: SavedObjectsClientContract;
+}): Promise<{}> => signalsMigrationSOClient(soClient).delete(id);
diff --git a/...ugins/security_solution/server/lib/detection_engine/migrations/finalize_migration.test.ts b/...ugins/security_solution/server/lib/detection_engine/migrations/finalize_migration.test.ts
@@ -31,22 +31,6 @@ describe('finalizeMigration', () => {
     }));
   });
 
-  it('does not finalize a soft-deleted migration', async () => {
-    const deletedMigration = getSignalsMigrationSavedObjectMock({
-      deleted: true,
-    });
-    const finalizedMigration = await finalizeMigration({
-      esClient,
-      migration: deletedMigration,
-      signalsAlias: 'my-signals-alias',
-      soClient,
-      username: 'username',
-    });
-
-    expect(updateMigrationSavedObject).not.toHaveBeenCalled();
-    expect(finalizedMigration).toEqual(deletedMigration);
-  });
-
   it('does not finalize a failed migration', async () => {
     const failedMigration = getSignalsMigrationSavedObjectMock({ status: 'failure' });
     const finalizedMigration = await finalizeMigration({

diff --git a/...ck/plugins/security_solution/server/lib/detection_engine/migrations/finalize_migration.ts b/...ck/plugins/security_solution/server/lib/detection_engine/migrations/finalize_migration.ts
@@ -6,7 +6,7 @@
 
 import { ElasticsearchClient, SavedObjectsClientContract } from 'src/core/server';
 import { getIndexCount } from '../index/get_index_count';
-import { isMigrationDeleted, isMigrationPending } from './helpers';
+import { isMigrationPending } from './helpers';
 import { applyMigrationCleanupPolicy } from './migration_cleanup';
 import { replaceSignalsIndexAlias } from './replace_signals_index_alias';
 import { SignalsMigrationSO } from './saved_objects_schema';
@@ -41,7 +41,7 @@ export const finalizeMigration = async ({
   soClient: SavedObjectsClientContract;
   username: string;
 }): Promise<SignalsMigrationSO> => {
-  if (isMigrationDeleted(migration) || !isMigrationPending(migration)) {
+  if (!isMigrationPending(migration)) {
     return migration;
   }
 
@@ -65,6 +65,12 @@ export const finalizeMigration = async ({
       },
     });
 
+    await applyMigrationCleanupPolicy({
+      alias: signalsAlias,
+      esClient,
+      index: destinationIndex,
+    });
+
     return {
       ...migration,
       attributes: {
@@ -80,12 +86,6 @@ export const finalizeMigration = async ({
     newIndex: destinationIndex,
     oldIndex: sourceIndex,
   });
-  await applyMigrationCleanupPolicy({
-    alias: signalsAlias,
-    esClient,
-    index: sourceIndex,
-  });
-  await esClient.delete({ index: '.tasks', id: taskId });
 
   const updatedMigration = await updateMigrationSavedObject({
     username,

diff --git a/...rity_solution/server/lib/detection_engine/migrations/get_migration_saved_objects_by_id.ts b/...rity_solution/server/lib/detection_engine/migrations/get_migration_saved_objects_by_id.ts
@@ -13,23 +13,23 @@ import { SignalsMigrationSO } from './saved_objects_schema';
  * Retrieves a list of migrations SOs by their ID
  *
  * @param soClient An {@link SavedObjectsClientContract}
- * @param migrationIds IDs of the migration SOs
+ * @param ids IDs of the migration SOs
  *
  * @returns a list of {@link SignalsMigrationSO[]}
  *
  * @throws if client returns an error
  */
 export const getMigrationSavedObjectsById = async ({
-  migrationIds,
+  ids,
   soClient,
 }: {
-  migrationIds: string[];
+  ids: string[];
   soClient: SavedObjectsClientContract;
 }): Promise<SignalsMigrationSO[]> =>
   findMigrationSavedObjects({
     soClient,
     options: {
-      search: migrationIds.map((id) => `${signalsMigrationType}:${id}`).join(' OR '),
+      search: ids.map((id) => `${signalsMigrationType}:${id}`).join(' OR '),
       rootSearchFields: ['_id'],
       sortField: 'updated',
       sortOrder: 'desc',

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/helpers.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/helpers.ts
@@ -20,9 +20,6 @@ export const isMigrationSuccess = (migration: SignalsMigrationSO): boolean =>
 export const isMigrationFailed = (migration: SignalsMigrationSO): boolean =>
   migration.attributes.status === 'failure';
 
-export const isMigrationDeleted = (migration: SignalsMigrationSO): boolean =>
-  migration.attributes.deleted === true;
-
 export const isOutdated = ({ current, target }: { current: number; target: number }): boolean =>
   current < target;
 

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/migration_service.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/migration_service.ts
@@ -53,7 +53,7 @@ export const signalsMigrationService = ({
       });
 
       return createMigrationSavedObject({
-        attributes: { ...migrationInfo, status: 'pending', error: null, deleted: false },
+        attributes: { ...migrationInfo, status: 'pending', error: null },
         soClient,
         username,
       });
@@ -72,7 +72,6 @@ export const signalsMigrationService = ({
         migration,
         signalsAlias,
         soClient,
-        username,
       }),
   };
 };
diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/saved_objects.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/migrations/saved_objects.ts
@@ -32,10 +32,6 @@ export const signalsMigrationMappings: SavedObjectsType['mappings'] = {
       type: 'keyword',
       index: false,
     },
-    deleted: {
-      type: 'boolean',
-      index: false,
-    },
     created: {
       type: 'date',
       index: false,

diff --git a/...ins/security_solution/server/lib/detection_engine/migrations/saved_objects_schema.mock.ts b/...ins/security_solution/server/lib/detection_engine/migrations/saved_objects_schema.mock.ts
@@ -15,7 +15,6 @@ export const getSignalsMigrationSavedObjectMock = (
   attributes: {
     destinationIndex: 'destinationIndex',
     sourceIndex: 'sourceIndex',
-    deleted: false,
     error: null,
     status: 'pending',
     taskId: 'taskid',

diff --git a/.../plugins/security_solution/server/lib/detection_engine/migrations/saved_objects_schema.ts b/.../plugins/security_solution/server/lib/detection_engine/migrations/saved_objects_schema.ts
@@ -13,7 +13,6 @@ const status = t.keyof({ success: null, failure: null, pending: null });
 
 const signalsMigrationSOWriteAttributes = {
   destinationIndex: t.string,
-  deleted: t.boolean,
   error: unionWithNullType(t.string),
   sourceIndex: t.string,
   status,