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.

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

[Alerting] Corrects validation and errors handling in PagerDuty action #63954

Merged
merged 15 commits into from Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -142,6 +142,25 @@ describe('validateParams()', () => {
- [eventAction.2]: expected value to equal [acknowledge]"
`);
});

test('should validate and throw error when timestamp has spaces', () => {
const randoDate = new Date('1963-09-23T01:23:45Z').toISOString();
const timestamp = ` ${randoDate}`;
expect(() => {
validateParams(actionType, {
timestamp,
});
}).toThrowError(`error validating action params: error parsing timestamp "${timestamp}"`);
});

test('should validate and throw error when timestamp is invalid', () => {
const timestamp = `1963-09-55 90:23:45`;
expect(() => {
validateParams(actionType, {
timestamp,
});
}).toThrowError(`error validating action params: error parsing timestamp "${timestamp}"`);
});
});

describe('execute()', () => {
Expand Down
24 changes: 16 additions & 8 deletions x-pack/plugins/actions/server/builtin_action_types/pagerduty.ts
Expand Up @@ -70,18 +70,26 @@ const ParamsSchema = schema.object(

function validateParams(paramsObject: any): string | void {
const params: ActionParamsType = paramsObject;

const { timestamp } = params;
if (timestamp != null) {
let date;
try {
date = Date.parse(timestamp);
const date = Date.parse(timestamp);
if (isNaN(date)) {
return i18n.translate('xpack.actions.builtin.pagerduty.invalidTimestampErrorMessage', {
defaultMessage: `error parsing timestamp "{timestamp}"`,
values: {
timestamp,
},
});
}
} catch (err) {
return 'error parsing timestamp: ${err.message}';
}

if (isNaN(date)) {
return 'error parsing timestamp';
return i18n.translate('xpack.actions.builtin.pagerduty.timestampParsingFailedErrorMessage', {
defaultMessage: `error parsing timestamp "{timestamp}": {message}`,
values: {
timestamp,
message: err.message,
},
});
}
}
}
Expand Down
Expand Up @@ -90,7 +90,7 @@ describe('pagerduty action params validation', () => {
summary: '2323',
source: 'source',
severity: 'critical',
timestamp: '234654564654',
timestamp: new Date().toISOString(),
component: 'test',
group: 'group',
class: 'test class',
Expand All @@ -99,6 +99,7 @@ describe('pagerduty action params validation', () => {
expect(actionTypeModel.validateParams(actionParams)).toEqual({
errors: {
summary: [],
timestamp: [],
},
});
});
Expand Down Expand Up @@ -156,15 +157,15 @@ describe('PagerDutyParamsFields renders', () => {
summary: '2323',
source: 'source',
severity: SeverityActionOptions.CRITICAL,
timestamp: '234654564654',
timestamp: new Date().toISOString(),
component: 'test',
group: 'group',
class: 'test class',
};
const wrapper = mountWithIntl(
<ParamsFields
actionParams={actionParams}
errors={{ summary: [] }}
errors={{ summary: [], timestamp: [] }}
editAction={() => {}}
index={0}
/>
Expand Down
Expand Up @@ -14,6 +14,7 @@ import {
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import moment from 'moment';
import {
ActionTypeModel,
ActionConnectorFieldsProps,
Expand All @@ -23,6 +24,7 @@ import {
import { PagerDutyActionParams, PagerDutyActionConnector } from './types';
import pagerDutySvg from './pagerduty.svg';
import { AddMessageVariables } from '../add_message_variables';
import { hasMustacheTokens } from '../../lib/has_mustache_tokens';

export function getActionType(): ActionTypeModel {
return {
Expand Down Expand Up @@ -62,6 +64,7 @@ export function getActionType(): ActionTypeModel {
const validationResult = { errors: {} };
const errors = {
summary: new Array<string>(),
timestamp: new Array<string>(),
};
validationResult.errors = errors;
if (!actionParams.summary?.length) {
Expand All @@ -74,6 +77,24 @@ export function getActionType(): ActionTypeModel {
)
);
}
if (actionParams.timestamp && !hasMustacheTokens(actionParams.timestamp)) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it's parsing the action params to see if the date field doesn't have a mustache template, and then parsing it to see if it's a valid date. That makes sense, but I wonder how often people will be setting literal dates in their action parameters like this. Is it worth it?

Copy link
Contributor Author

@gmmorris gmmorris Apr 20, 2020

Choose a reason for hiding this comment

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

Well, we've already had a bug reported for not doing it so I get the feeling our UX isn't really good enough here.

If a specific format is required for this field, we should be able to validate that only content of that format is defined for it. The fact that we also want to support Mustache shouldn't mean the user can input bad data here... arguably we shouldn't allow any free values here at all, but rather we should only offer valid sources of Datetime strings, but when I looked into that I realised that was a huge change that didn't feel right as a patch bug fix. 🤷

if (isNaN(Date.parse(actionParams.timestamp))) {
const { nowShortFormat, nowLongFormat } = getValidTimestampExamples();
errors.timestamp.push(
i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.pagerDutyAction.error.invalidTimestamp',
{
defaultMessage:
'Timestamp must be a valid date, such as {nowShortFormat} or {nowLongFormat}.',
values: {
nowShortFormat,
nowLongFormat,
},
}
)
);
}
}
return validationResult;
},
actionConnectorFields: PagerDutyActionConnectorFields,
Expand Down Expand Up @@ -334,6 +355,8 @@ const PagerDutyParamsFields: React.FunctionComponent<ActionParamsProps<PagerDuty
<EuiFlexItem>
<EuiFormRow
fullWidth
error={errors.timestamp}
isInvalid={errors.timestamp.length > 0 && timestamp !== undefined}
label={i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.pagerDutyAction.timestampTextFieldLabel',
{
Expand All @@ -355,11 +378,14 @@ const PagerDutyParamsFields: React.FunctionComponent<ActionParamsProps<PagerDuty
name="timestamp"
data-test-subj="timestampInput"
value={timestamp || ''}
isInvalid={errors.timestamp.length > 0 && timestamp !== undefined}
onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
editAction('timestamp', e.target.value, index);
}}
onBlur={() => {
if (!timestamp) {
if (timestamp?.trim()) {
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about doing the trim() in the action params validation rather than in the UI here. Guessing here fixes it 99% of the time, but are there cases where it could be missed here, and still have leading/trailing whitespace when the validation occurs? Guessing possible, but very unlikely. In any case, the validation now prints the date value it's validating with dq's, so ... think this will be user-diagnose-able now where it wasn't before ...

Copy link
Contributor Author

@gmmorris gmmorris Apr 20, 2020

Choose a reason for hiding this comment

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

Yeah I originally wanted to do it in the action, but that forces us to do trim in multiple places as the validation is separate from where its used and it ends up forcing the null || string?.trim()) logic to repeat in multiple places unless a more major refactoring was done.

It would also mean it's very easy for us to think there's a timestamp value (as the SO wouldn't be null or even an empty string) but in practice we'd be treating it like we don't have one. Seems error prone.

editAction('timestamp', timestamp.trim(), index);
} else {
editAction('timestamp', '', index);
}
}}
Expand Down Expand Up @@ -534,3 +560,11 @@ const PagerDutyParamsFields: React.FunctionComponent<ActionParamsProps<PagerDuty
</Fragment>
);
};

function getValidTimestampExamples() {
const now = moment();
return {
nowShortFormat: now.format('YYYY-MM-DD'),
nowLongFormat: now.format('YYYY-MM-DD h:mm:ss'),
};
}
@@ -0,0 +1,23 @@
/*
* 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 uuid from 'uuid';

import { hasMustacheTokens } from './has_mustache_tokens';

describe('hasMustacheTokens', () => {
test('returns false for empty string', () => {
expect(hasMustacheTokens('')).toBe(false);
});

test('returns false for string without tokens', () => {
expect(hasMustacheTokens(`some random string ${uuid.v4()}`)).toBe(false);
});

test('returns true when a template token is present', () => {
expect(hasMustacheTokens('{{context.timestamp}}')).toBe(true);
});
});
@@ -0,0 +1,9 @@
/*
* 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.
*/

export function hasMustacheTokens(str: string): boolean {
return null !== str.match(/{{.*}}/);
}