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.

Sign up for GitHub

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

[SECURITY SOLUTIONS][Alerts Actions] Fix migration from 7.11.0/7.11.1 to 7.12 #94722

Merged
merged 13 commits into from
Mar 18, 2021
Merged

[SECURITY SOLUTIONS][Alerts Actions] Fix migration from 7.11.0/7.11.1 to 7.12 #94722

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
391ed9f
do not modify connector with the right structure
XavierM Mar 16, 2021
0c5457c
review trying to bring back incident to live when we can
XavierM Mar 16, 2021
3d77532
manage custom action
XavierM Mar 17, 2021
1dc8580
Merge branch 'master' of github.com:elastic/kibana into 7-12-migratio…
XavierM Mar 17, 2021
88adb42
Merge branch 'master' into 7-12-migration-fix
kibanamachine Mar 17, 2021
94f0be3
fix cypress test
XavierM Mar 17, 2021
31a0dbe
Merge branch '7-12-migration-fix' of github.com:XavierM/kibana into 7…
XavierM Mar 17, 2021
a1ac3f5
Merge branch 'master' into 7-12-migration-fix
kibanamachine Mar 17, 2021
cbe95fb
Merge branch 'master' into 7-12-migration-fix
kibanamachine Mar 17, 2021
2881fc9
skip test unrelated to PR
XavierM Mar 18, 2021
8714e15
Merge branch 'master' of github.com:elastic/kibana into 7-12-migratio…
XavierM Mar 18, 2021
971c5a7
Merge branch '7-12-migration-fix' of github.com:XavierM/kibana into 7…
XavierM Mar 18, 2021
408ee6e
Merge branch 'master' into 7-12-migration-fix
kibanamachine Mar 18, 2021
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
126 changes: 126 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/migrations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,132 @@ describe('7.11.2', () => {
} as SavedObjectUnsanitizedDoc<RawAlert>;
expect(isAnyActionSupportIncidents(doc)).toBe(false);
});

test('it does not transforms alerts when the right structure connectors is already applied', () => {
Copy link
Member

Choose a reason for hiding this comment

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

If a customer upgrades to 7.11.2 with the current migration, they'll end up with some actions in alerts where incident is {}, right? Given the current schema, these won't validate when they're used, so should not be executed, but would presumably be candidates somehow fixing - if possible - in another migration (to 7.12.0?)?

In any case, whether we try to fix these or not, seems like we should add a test with an alert with a Jira/ServiceNow/IBM resilient action, where the incident is {}, since it seems like we'll have these in the field, in some cases.

Copy link
Contributor Author

@XavierM XavierM Mar 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh I see, what you are saying? Yes I will do that
I think there is two branch logic here:

  1. if a user went from 7.10.x to 7.11.0 -> they lost all their data in the connector that was the first bug that we tried to fix because we forget to write a migration and user will have to write them from scratch
    I think that we should be able to write them a script really familiar that what we did to bring everything back to normal in this situation too
  2. if a user went from 7.11.0/1 to 7.11.2 -> they will lost again their connector at this time they have two solution using the script from the SDH or re-creating them from scratch

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm a little curious how well these incident: {} actions will survive the Kibana UI experience. I'm pretty sure on the back-end we'll fail fast because we are always validating these before we use them productively. But wondering if you edit an alert using one of these actions - what happens? Scared of seeing some kind of null dereference somewhere, I guess ...

I think that would be the decision point as to whether we need to do something for these in a 7.12.0 migration. Like add a placeholder summary or other required fields in the incident, to keep other things from breaking.

Copy link
Contributor Author

@XavierM XavierM Mar 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh I tried it out to edit an alert when you have this incident: {} and we come back to normal. We know it is also working because our user who creates the SDH were able to get back to normal after editing all their alerts manually.

And yes when you have incident:{} like in the SDH, we know that the alert get trigger but we never create an action in the associated connector.

Copy link
Contributor

@rudolf rudolf Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really understand this bug, but just want to chime in and agree with @pmuellr . It doesn't usually make sense to fix a bug in an already released migration function (unless it's causing data loss) because that buggy code has already run on users's data. A 7.11.2 -> 7.12 upgrade won't get the bug fix applied to the data. So there needs to be a 7.12 migration to guarantee that all the upgrade paths get this bug fix applied.

I think it makes sense to keep the buggy 7.11.2 migration function just like it was (maybe a comment stating this code is buggy), this way the code makes it obvious that there are kibana 7.11.2 instances with buggy data. The 7.12 migration could then fix the bug with a comment "fixing our 7.11.2 migration bug"

Copy link
Contributor Author

@XavierM XavierM Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@rudolf we ask user to not migrate from 7.11.0/1 to 7.11.2 because if you do then user will loose the data actions for good. We will have now way to bring it back through migration and you will have to run this script to get it back. So we want to stop the bleeding by changing the 7.11.2 migration so user can move forward from 7.11.0/1 to 7.12.x. And of course let them migrate from earlier version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, if this was a data loss bug then yes, all we can do is fix the data loss bug in the 7.11.2 migration, it's too late for users who already upgraded...

const migration7112 = getMigrations(encryptedSavedObjectsSetup)['7.11.2'];
const alert = getMockData({
actions: [
{
actionTypeId: '.server-log',
group: 'threshold met',
params: {
level: 'info',
message: 'log message',
},
id: '99257478-e591-4560-b264-441bdd4fe1d9',
},
{
actionTypeId: '.servicenow',
group: 'threshold met',
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
short_description: 'SN short desc',
description: 'SN desc',
severity: '2',
impact: '2',
urgency: '2',
},
comments: [{ commentId: '1', comment: 'sn comment' }],
},
},
id: '1266562a-4e1f-4305-99ca-1b44c469b26e',
},
],
});

expect(migration7112(alert, migrationContext)).toEqual(alert);
});

test('if incident attribute is an empty object, copy back the related attributes from subActionParams back to incident', () => {
const migration7112 = getMigrations(encryptedSavedObjectsSetup)['7.11.2'];
const alert = getMockData({
actions: [
{
actionTypeId: '.server-log',
group: 'threshold met',
params: {
level: 'info',
message: 'log message',
},
id: '99257478-e591-4560-b264-441bdd4fe1d9',
},
{
actionTypeId: '.servicenow',
group: 'threshold met',
params: {
subAction: 'pushToService',
subActionParams: {
short_description: 'SN short desc',
description: 'SN desc',
severity: '2',
impact: '2',
urgency: '2',
incident: {},
XavierM marked this conversation as resolved.
Show resolved Hide resolved
comments: [{ commentId: '1', comment: 'sn comment' }],
},
},
id: '1266562a-4e1f-4305-99ca-1b44c469b26e',
},
],
});

expect(migration7112(alert, migrationContext)).toEqual({
...alert,
attributes: {
...alert.attributes,
actions: [
alert.attributes.actions![0],
{
actionTypeId: '.servicenow',
group: 'threshold met',
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
short_description: 'SN short desc',
description: 'SN desc',
severity: '2',
impact: '2',
urgency: '2',
},
comments: [{ commentId: '1', comment: 'sn comment' }],
},
},
id: '1266562a-4e1f-4305-99ca-1b44c469b26e',
},
],
},
});
});

test('custom action does not get migrated/loss', () => {
const migration7112 = getMigrations(encryptedSavedObjectsSetup)['7.11.2'];
const alert = getMockData({
actions: [
{
actionTypeId: '.mike',
group: 'threshold met',
params: {
subAction: 'pushToService',
subActionParams: {
short_description: 'SN short desc',
description: 'SN desc',
severity: '2',
impact: '2',
urgency: '2',
incident: {},
comments: [{ commentId: '1', comment: 'sn comment' }],
},
},
id: '1266562a-4e1f-4305-99ca-1b44c469b26e',
},
],
});

expect(migration7112(alert, migrationContext)).toEqual(alert);
});
});

function getUpdatedAt(): string {
Expand Down
217 changes: 126 additions & 91 deletions x-pack/plugins/alerting/server/saved_objects/migrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
SavedObjectUnsanitizedDoc,
SavedObjectMigrationFn,
SavedObjectMigrationContext,
SavedObjectAttributes,
} from '../../../../../src/core/server';
import { RawAlert, RawAlertAction } from '../types';
import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server';
Expand Down Expand Up @@ -180,113 +181,147 @@ function initializeExecutionStatus(
};
}

function isEmptyObject(obj: {}) {
for (const attr in obj) {
if (Object.prototype.hasOwnProperty.call(obj, attr)) {
return false;
}
}
return true;
XavierM marked this conversation as resolved.
Show resolved Hide resolved
}

function restructureConnectorsThatSupportIncident(
doc: SavedObjectUnsanitizedDoc<RawAlert>
): SavedObjectUnsanitizedDoc<RawAlert> {
const { actions } = doc.attributes;
const newActions = actions.reduce((acc, action) => {
if (action.params.subAction !== 'pushToService') {
return [...acc, action];
}

if (action.actionTypeId === '.servicenow') {
const { title, comments, comment, description, severity, urgency, impact } = action.params
.subActionParams as {
title: string;
description?: string;
severity?: string;
urgency?: string;
impact?: string;
comment?: string;
comments?: Array<{ commentId: string; comment: string }>;
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
short_description: title,
description,
severity,
urgency,
impact,
if (
['.servicenow', '.jira', '.resilient'].includes(action.actionTypeId) &&
action.params.subAction === 'pushToService'
) {
// Future developer, we needed to do that because when we created this migration
// we forget to think about user already using 7.11.0 and having an incident attribute build the right way
// IMPORTANT -> if you change this code please do the same inside of this file
// x-pack/plugins/alerting/server/saved_objects/migrations.ts
const subActionParamsIncident =
(action.params?.subActionParams as SavedObjectAttributes)?.incident ?? null;
if (subActionParamsIncident != null && !isEmptyObject(subActionParamsIncident)) {
return [...acc, action];
}
if (action.actionTypeId === '.servicenow') {
const {
title,
comments,
comment,
description,
severity,
urgency,
impact,
short_description: shortDescription,
} = action.params.subActionParams as {
title: string;
description?: string;
severity?: string;
urgency?: string;
impact?: string;
comment?: string;
comments?: Array<{ commentId: string; comment: string }>;
short_description?: string;
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
short_description: shortDescription ?? title,
description,
severity,
urgency,
impact,
},
comments: [
...(comments ?? []),
...(comment != null ? [{ commentId: '1', comment }] : []),
],
},
comments: [
...(comments ?? []),
...(comment != null ? [{ commentId: '1', comment }] : []),
],
},
},
},
] as RawAlertAction[];
}

if (action.actionTypeId === '.jira') {
const { title, comments, description, issueType, priority, labels, parent } = action.params
.subActionParams as {
title: string;
description: string;
issueType: string;
priority?: string;
labels?: string[];
parent?: string;
comments?: unknown[];
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
summary: title,
description,
issueType,
priority,
labels,
parent,
] as RawAlertAction[];
} else if (action.actionTypeId === '.jira') {
const {
title,
comments,
description,
issueType,
priority,
labels,
parent,
summary,
} = action.params.subActionParams as {
title: string;
description: string;
issueType: string;
priority?: string;
labels?: string[];
parent?: string;
comments?: unknown[];
summary?: string;
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
summary: summary ?? title,
description,
issueType,
priority,
labels,
parent,
},
comments,
},
comments,
},
},
},
] as RawAlertAction[];
}

if (action.actionTypeId === '.resilient') {
const { title, comments, description, incidentTypes, severityCode } = action.params
.subActionParams as {
title: string;
description: string;
incidentTypes?: number[];
severityCode?: number;
comments?: unknown[];
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
name: title,
description,
incidentTypes,
severityCode,
] as RawAlertAction[];
} else if (action.actionTypeId === '.resilient') {
const { title, comments, description, incidentTypes, severityCode, name } = action.params
.subActionParams as {
title: string;
description: string;
incidentTypes?: number[];
severityCode?: number;
comments?: unknown[];
name?: string;
};
return [
...acc,
{
...action,
params: {
subAction: 'pushToService',
subActionParams: {
incident: {
name: name ?? title,
description,
incidentTypes,
severityCode,
},
comments,
},
comments,
},
},
},
] as RawAlertAction[];
] as RawAlertAction[];
}
}

return acc;
return [...acc, action];
}, [] as RawAlertAction[]);

return {
Expand Down
Loading