[ResponseOps][Rules] Validate timezone in rule routes #201508

cnasikas · 2024-11-24T11:06:36Z

Summary

This PR adds validation only for internal routes that use the rRule schema.

Testing

Create a rule in main.
Snooze the rule by using the API as

POST /internal/alerting/rule/<ruleId>/_snooze
{
    "snooze_schedule": {
        "id": "e58e2340-dba6-454c-8308-b2ca66a7cf7b",
        "duration": 86400000,
        "rRule": {
            "dtstart": "2024-09-04T09:27:37.011Z",
            "tzid": "invalid",
            "freq": 2,
            "interval": 1,
            "byweekday": [
                "invalid"
            ]
        }
    }
}

Go to the rules page and verify that the rules are not loaded.
Switch to my PR.
Go to the rules page and verify that the rules load.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

cnasikas · 2024-11-24T11:07:28Z

x-pack/plugins/alerting/common/routes/rule/validation/validate_timezone/v1.ts


 export function validateTimezone(timezone: string) {
-  if (moment.tz.names().includes(timezone)) {
+  if (moment.tz.zone(timezone) != null) {


moment.tz.zone is faster than moment.tz.names().includes.

cnasikas · 2024-11-24T11:08:00Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts

@@ -478,6 +478,38 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
      });
    });

+    it('should return 400 if the timezone of an action is not valid', async () => {


Validation already existed. I added a test to avoid bugs in the future.

cnasikas · 2024-11-24T11:08:04Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts

@@ -182,6 +182,51 @@ export default function createUpdateTests({ getService }: FtrProviderContext) {
      });
    });

+    it('should return 400 if the timezone of an action is not valid', async () => {


Validation already existed. I added a test to avoid bugs in the future.

cnasikas · 2024-11-24T16:09:30Z

x-pack/plugins/alerting/common/routes/r_rule/request/schemas/v1.ts

The validations are used only in internal routes.

cnasikas · 2024-11-30T12:14:48Z

x-pack/plugins/alerting/server/lib/snooze/get_active_snooze_if_exist.ts

@@ -10,7 +10,7 @@ import { RuleSnoozeSchedule } from '../../types';

 const MAX_TIMESTAMP = 8640000000000000;

-export function isSnoozeActive(snooze: RuleSnoozeSchedule) {
+export function getActiveSnoozeIfExist(snooze: RuleSnoozeSchedule) {


The function returns an active snooze or null. I found the isSnoozeActive misleading and I renamed the function to something more intuitive. I am open to suggestions 🙂.

cnasikas · 2024-12-09T21:16:39Z

/ci

…apping'

…snooze_bug

cnasikas · 2025-01-20T15:38:42Z

@adcoelho @js-jankisalvi I pushed some commits based on your feedback and fixed the bug. Could you please review it again? Also, @Zacqary, I made some changes in the rule package. Could you please take a look?

js-jankisalvi

New approach looks good 🎉
Verified locally, bug is not reproducible anymore 👍

adcoelho

Verified the bug, it is now fixed 👍

Zacqary · 2025-01-21T20:17:54Z

src/platform/packages/shared/kbn-rrule/rrule.ts

+    try {
+      validateOptions(options);
+
+      return true;


I'd prefer to find a way to surface the error message that validateOptions produces instead of just turning it into a boolean. Not a blocker for this PR because I see we're just using it to display a notification badge. If there's a way to surface the error message without a huge rewrite I'd say give it a shot, otherwise feel free to merge.

Hey Zacq! In my use case, I do not want to surface the message but to know which rules have an invalid snooze schedule (very rare after the guardrails we put) and show an "Invalid Snooze" notification badge which the user can click and remove. Wdyt?

Sounds all right, we've got the bones in place for surfacing the error message in future use cases

elasticmachine · 2025-01-24T16:00:12Z

💚 Build Succeeded

Buildkite Build
Commit: 551687c

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	233	234	+1
`triggersActionsUi`	939	944	+5
total			+6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/rrule`	16	18	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	1.7MB	1.7MB	-18.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/rrule`	1	2	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	130.2KB	130.2KB	+22.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/rrule`	16	18	+2

History

💚 Build #269193 succeeded 2a040fa
💔 Build #268539 failed 8fc77c0
💔 Build #265102 failed afd0bfa
💔 Build #265091 failed 36baf63
💔 Build #262221 failed 2c189ec
💔 Build #259769 failed ce9e4da

cc @cnasikas

kibanamachine · 2025-01-24T17:46:42Z

Starting backport for target branches: 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12954783212

## Summary This PR adds validation only for internal routes that use the `rRule` schema. ## Testing 1. Create a rule in main. 2. Snooze the rule by using the API as ``` POST /internal/alerting/rule/<ruleId>/_snooze { "snooze_schedule": { "id": "e58e2340-dba6-454c-8308-b2ca66a7cf7b", "duration": 86400000, "rRule": { "dtstart": "2024-09-04T09:27:37.011Z", "tzid": "invalid", "freq": 2, "interval": 1, "byweekday": [ "invalid" ] } } } ``` 4. Go to the rules page and verify that the rules are not loaded. 5. Switch to my PR. 6. Go to the rules page and verify that the rules load. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 9a3fc89)

kibanamachine · 2025-01-24T18:01:41Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.16	Backport failed because of merge conflicts
❌	8.17	Backport failed because of merge conflicts
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 201508

Questions ?

Please refer to the Backport tool documentation

cnasikas · 2025-01-25T11:14:45Z

💚 All backports created successfully

Status	Branch	Result
✅	8.17
✅	8.16

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

## Summary This PR adds validation only for internal routes that use the `rRule` schema. ## Testing 1. Create a rule in main. 2. Snooze the rule by using the API as ``` POST /internal/alerting/rule/<ruleId>/_snooze { "snooze_schedule": { "id": "e58e2340-dba6-454c-8308-b2ca66a7cf7b", "duration": 86400000, "rRule": { "dtstart": "2024-09-04T09:27:37.011Z", "tzid": "invalid", "freq": 2, "interval": 1, "byweekday": [ "invalid" ] } } } ``` 4. Go to the rules page and verify that the rules are not loaded. 5. Switch to my PR. 6. Go to the rules page and verify that the rules load. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 9a3fc89) # Conflicts: # packages/kbn-rrule/validate.test.ts # packages/kbn-rrule/validate.ts # x-pack/packages/kbn-cloud-security-posture/common/types/graph/latest.ts # x-pack/platform/plugins/shared/alerting/server/lib/snooze/is_snooze_active.test.ts # x-pack/platform/plugins/shared/alerting/server/lib/snooze/is_snooze_active.ts # x-pack/plugins/alerting/common/routes/r_rule/request/schemas/v1.ts # x-pack/plugins/alerting/common/routes/r_rule/validation/index.ts # x-pack/plugins/alerting/common/routes/r_rule/validation/validate_recurrence_by/v1.ts # x-pack/plugins/alerting/server/application/rule/methods/snooze/snooze_rule.test.ts # x-pack/plugins/alerting/server/lib/snooze/get_active_snooze_if_exist.test.ts # x-pack/plugins/alerting/server/lib/snooze/get_active_snooze_if_exist.ts # x-pack/plugins/alerting/server/lib/snooze/is_snooze_active.test.ts # x-pack/plugins/alerting/server/lib/snooze/is_snooze_active.ts # x-pack/plugins/triggers_actions_ui/tsconfig.json

…#208252) # Backport This will backport the following commits from `main` to `8.x`: - [[ResponseOps][Rules] Validate timezone in rule routes (#201508)](#201508)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: Christos Nasikas <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Validate timezone in alerting routes

c3cc6cf

cnasikas self-assigned this Nov 24, 2024

cnasikas added the backport:skip This commit does not require backporting label Nov 24, 2024

cnasikas commented Nov 24, 2024

View reviewed changes

Validate snooze ID, rrule byweekday, bymonthday, and bymonth

5fe0245

cnasikas commented Nov 24, 2024

View reviewed changes

cnasikas added 4 commits November 25, 2024 11:42

Merge branch 'main' into fix_snooze_bug

aeeb8b1

Merge branch 'main' into fix_snooze_bug

58fd530

Do not throw an error when getting active snoozes

802b5b1

Fix cypress tests

2d2f446

cnasikas commented Nov 30, 2024

View reviewed changes

cnasikas added 3 commits December 2, 2024 18:23

Merge branch 'main' into fix_snooze_bug

f6418aa

Merge branch 'main' into fix_snooze_bug

b2c9f68

Merge branch 'main' into fix_snooze_bug

fd2b7c3

So warning in the UI

d63506b

cnasikas force-pushed the fix_snooze_bug branch from 396cd29 to d63506b Compare December 10, 2024 08:06

cnasikas added 3 commits December 10, 2024 10:08

Merge branch 'main' into fix_snooze_bug

bebe419

Improve text

f05fe77

Add unit tests

ce9e4da

cnasikas marked this pull request as ready for review December 12, 2024 12:56

cnasikas requested a review from a team as a code owner December 12, 2024 12:56

cnasikas added 3 commits January 13, 2025 17:00

Draft

e12c485

Merge branch 'main' into fix_snooze_bug

3639b49

Create validateOptions function

7ce30e1

cnasikas requested review from Zacqary and adcoelho January 20, 2025 15:16

kibanamachine and others added 3 commits January 20, 2025 15:23

[CI] Auto-commit changed files from 'node scripts/styled_components_m…

bb55b17

…apping'

Validate rule schedule in snooze_rule

8ab2768

Merge branch 'fix_snooze_bug' of github.com:cnasikas/kibana into fix_…

8fc77c0

…snooze_bug

js-jankisalvi approved these changes Jan 20, 2025

View reviewed changes

adcoelho approved these changes Jan 21, 2025

View reviewed changes

Zacqary reviewed Jan 21, 2025

View reviewed changes

cnasikas added 2 commits January 22, 2025 12:06

Fix integration tests

2a040fa

Merge branch 'main' into fix_snooze_bug

551687c

cnasikas merged commit 9a3fc89 into elastic:main Jan 24, 2025
9 checks passed

cnasikas deleted the fix_snooze_bug branch January 24, 2025 17:46

cnasikas added v8.16.4 v8.17.2 and removed v8.16.3 v8.17.1 labels Jan 24, 2025

kibanamachine mentioned this pull request Jan 24, 2025

[8.x] [ResponseOps][Rules] Validate timezone in rule routes (#201508) #208252

Merged

This was referenced Jan 25, 2025

[8.17] [ResponseOps][Rules] Validate timezone in rule routes (#201508) #208299

Open

[8.16] [ResponseOps][Rules] Validate timezone in rule routes (#201508) #208300

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps][Rules] Validate timezone in rule routes #201508

[ResponseOps][Rules] Validate timezone in rule routes #201508

cnasikas commented Nov 24, 2024 •

edited by kibanamachine

Loading

cnasikas Nov 24, 2024

cnasikas Nov 24, 2024

cnasikas Nov 24, 2024

cnasikas Nov 24, 2024 •

edited

Loading

cnasikas Nov 30, 2024 •

edited

Loading

cnasikas commented Dec 9, 2024

cnasikas commented Jan 20, 2025

js-jankisalvi left a comment

adcoelho left a comment

Zacqary Jan 21, 2025

cnasikas Jan 24, 2025

Zacqary Jan 24, 2025

elasticmachine commented Jan 24, 2025

API count

kibanamachine commented Jan 24, 2025

kibanamachine commented Jan 24, 2025

cnasikas commented Jan 25, 2025

[ResponseOps][Rules] Validate timezone in rule routes #201508

[ResponseOps][Rules] Validate timezone in rule routes #201508

Conversation

cnasikas commented Nov 24, 2024 • edited by kibanamachine Loading

Summary

Testing

Checklist

cnasikas Nov 24, 2024

Choose a reason for hiding this comment

cnasikas Nov 24, 2024

Choose a reason for hiding this comment

cnasikas Nov 24, 2024

Choose a reason for hiding this comment

cnasikas Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas commented Dec 9, 2024

cnasikas commented Jan 20, 2025

js-jankisalvi left a comment

Choose a reason for hiding this comment

adcoelho left a comment

Choose a reason for hiding this comment

Zacqary Jan 21, 2025

Choose a reason for hiding this comment

cnasikas Jan 24, 2025

Choose a reason for hiding this comment

Zacqary Jan 24, 2025

Choose a reason for hiding this comment

elasticmachine commented Jan 24, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

kibanamachine commented Jan 24, 2025

kibanamachine commented Jan 24, 2025

💔 Some backports could not be created

Manual backport

Questions ?

cnasikas commented Jan 25, 2025

💚 All backports created successfully

Questions ?

cnasikas commented Nov 24, 2024 •

edited by kibanamachine

Loading

cnasikas Nov 24, 2024 •

edited

Loading

cnasikas Nov 30, 2024 •

edited

Loading