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

gmmorris · 2020-04-20T11:26:29Z

Summary

Improves the messaging when the Pager Duty action has trouble parsing the timestamp field and adds trimming on the timestamp's field to make us more flexible in handling the parsing and hence more likely to be forgiving of the input by the user.

As the timestamp relies on context variables provided via mustcahe templates, we can't reliably validate this field at alert creation time.
We address by:

Trimming the edges, which is required when parsing a date, should help prevent accidental spaces from breaking the parsing.
Checking for a mustache template on the client side and if there are none - we validate for a valid timestamp when the action is created.

closes #63758
closes https://github.com/elastic/siem-team/issues/607

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-20T11:26:31Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

…possible" This reverts commit 68359d0.

…ibana into actions/pagerduty-date-format * 'actions/pagerduty-date-format' of github.com:gmmorris/kibana: adds i18m to timestamp parsing errors

pmuellr

LGTM, but don't think we need the use of random() in one of the tests.

x-pack/plugins/actions/server/builtin_action_types/pagerduty.test.ts

pmuellr · 2020-04-20T14:21:55Z

...plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty.tsx

@@ -74,6 +76,21 @@ export function getActionType(): ActionTypeModel {
          )
        );
      }
+      if (actionParams.timestamp && !hasMustacheTokens(actionParams.timestamp)) {


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?

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. 🤷

pmuellr · 2020-04-20T14:25:29Z

...plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty.tsx

              onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
                editAction('timestamp', e.target.value, index);
              }}
              onBlur={() => {
-                if (!timestamp) {
+                if (timestamp?.trim()) {


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 ...

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.

YulNaumenko

LGTM

* master: (30 commits) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) [TSVB] Use default Kibana palette for split series (elastic#62241) Ingest overview page (elastic#63214) ...

gmmorris · 2020-04-21T11:33:15Z

This will be backported to 7.7.1 as well

…elastic#63954) Improves the messaging when the Pager Duty action has trouble parsing the timestamp field and adds trimming on the timestamp's field to make us more flexible in handling the parsing and hence more likely to be forgiving of the input by the user. As the timestamp relies on context variables provided via mustcahe templates, we can't reliably validate this field at alert creation time. We address by: 1. Trimming the edges, which is required when parsing a date, should help prevent accidental spaces from breaking the parsing. 2. Checking for a mustache template on the client side and if there are none - we validate for a valid timestamp when the action is created.

…#63954) (#64060) Improves the messaging when the Pager Duty action has trouble parsing the timestamp field and adds trimming on the timestamp's field to make us more flexible in handling the parsing and hence more likely to be forgiving of the input by the user. As the timestamp relies on context variables provided via mustcahe templates, we can't reliably validate this field at alert creation time. We address by: 1. Trimming the edges, which is required when parsing a date, should help prevent accidental spaces from breaking the parsing. 2. Checking for a mustache template on the client side and if there are none - we validate for a valid timestamp when the action is created. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-04-23T12:52:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: baab614

History

💚 Build #42097 succeeded baab614
💔 Build #41905 failed b4a821e
💔 Build #41900 failed a1df80f
💔 Build #41838 failed bfcd018
💚 Build #41831 succeeded 4f2e734

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…elastic#63954) Improves the messaging when the Pager Duty action has trouble parsing the timestamp field and adds trimming on the timestamp's field to make us more flexible in handling the parsing and hence more likely to be forgiving of the input by the user. As the timestamp relies on context variables provided via mustcahe templates, we can't reliably validate this field at alert creation time. We address by: 1. Trimming the edges, which is required when parsing a date, should help prevent accidental spaces from breaking the parsing. 2. Checking for a mustache template on the client side and if there are none - we validate for a valid timestamp when the action is created.

…#63954) (#66571) Improves the messaging when the Pager Duty action has trouble parsing the timestamp field and adds trimming on the timestamp's field to make us more flexible in handling the parsing and hence more likely to be forgiving of the input by the user. As the timestamp relies on context variables provided via mustcahe templates, we can't reliably validate this field at alert creation time. We address by: 1. Trimming the edges, which is required when parsing a date, should help prevent accidental spaces from breaking the parsing. 2. Checking for a mustache template on the client side and if there are none - we validate for a valid timestamp when the action is created.

gmmorris added 2 commits April 20, 2020 10:52

improve messaging around PD timestamp parsing

ffc0c29

trim ends off of timestamp to make it more likely we parse it correctly

34d0166

gmmorris added Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.0 v7.7.1 labels Apr 20, 2020

gmmorris requested a review from a team as a code owner April 20, 2020 11:26

gmmorris added 2 commits April 20, 2020 12:30

trim timestamp field in action to ensure were as flexible as possible

68359d0

Revert "trim timestamp field in action to ensure were as flexible as …

e7aa47d

…possible" This reverts commit 68359d0.

gmmorris added the release_note:fix label Apr 20, 2020

gmmorris added 5 commits April 20, 2020 12:37

adds i18m to timestamp parsing errors

d36324b

added missing test

f1d1cf7

Merge branch 'actions/pagerduty-date-format' of github.com:gmmorris/k…

4f2e734

…ibana into actions/pagerduty-date-format * 'actions/pagerduty-date-format' of github.com:gmmorris/kibana: adds i18m to timestamp parsing errors

added check for mustache tempalte in timestamp

02451b8

dont trim on change its bad ux

bfcd018

pmuellr approved these changes Apr 20, 2020

View reviewed changes

improve error message on timestamp

633f20f

gmmorris changed the title ~~[Alerting] improve error message and trimming in Pager Duty action~~ [Alerting] Improve validation and errors handling in PagerDuty action Apr 20, 2020

YulNaumenko approved these changes Apr 20, 2020

View reviewed changes

gmmorris added 5 commits April 20, 2020 16:19

removed randomness from test

a1df80f

fixed ui unit tests

5131cb2

removed unused import

b4a821e

fixed i18n lint error

28ac631

gmmorris merged commit 02d55db into elastic:master Apr 21, 2020

gmmorris mentioned this pull request Apr 21, 2020

[7.x] [Alerting] Improve validation and errors handling in PagerDuty action (#63954) #64060

Merged

gmmorris changed the title ~~[Alerting] Improve validation and errors handling in PagerDuty action~~ [Alerting] Corrects validation and errors handling in PagerDuty action Apr 23, 2020

tsg mentioned this pull request Apr 30, 2020

[SIEM] PagerDuty timestamp causes rule to fail #64871

Closed

gmmorris mentioned this pull request May 14, 2020

[7.7] [Alerting] Improve validation and errors handling in PagerDuty action (#63954) #66571

Merged

romain-chanu mentioned this pull request Jun 4, 2020

Adding an alert variable in PagerDuty action and selecting a value results in an extra space character #68208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gmmorris commented Apr 20, 2020 •

edited

elasticmachine commented Apr 20, 2020

pmuellr left a comment

pmuellr Apr 20, 2020

gmmorris Apr 20, 2020 •

edited

pmuellr Apr 20, 2020

gmmorris Apr 20, 2020 •

edited

YulNaumenko left a comment

gmmorris commented Apr 21, 2020

kibanamachine commented Apr 23, 2020

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

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

Conversation

gmmorris commented Apr 20, 2020 • edited

Summary

Checklist

For maintainers

elasticmachine commented Apr 20, 2020

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Apr 20, 2020

Choose a reason for hiding this comment

gmmorris Apr 20, 2020 • edited

Choose a reason for hiding this comment

pmuellr Apr 20, 2020

Choose a reason for hiding this comment

gmmorris Apr 20, 2020 • edited

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

gmmorris commented Apr 21, 2020

kibanamachine commented Apr 23, 2020

💚 Build Succeeded

History

gmmorris commented Apr 20, 2020 •

edited

gmmorris Apr 20, 2020 •

edited

gmmorris Apr 20, 2020 •

edited