Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields #162425

mikecote · 2023-07-24T17:11:27Z

In this PR, I'm preparing the alerting rule task types for serverless by defining an explicit task state schema for the framework level fields. This schema is used to validate the task's state before saving but also when reading. In the scenario an older Kibana node runs a task after a newer Kibana node has stored additional task state, the unknown state properties will be dropped. Additionally, this will prompt developers to be aware that adding required fields to the task state is a breaking change that must be handled with care. (see #155764).

The PR also includes the following changes:

Modifying the @kbn/alerting-state-types package so the types are generated from config-schema, instead of io-ts. The schema is re-used for exporting a new stateSchemaByVersion property.
Removing DateFromString in favour of strings everywhere (config-schema doesn't support this conversion)
Add a v1 up migration that will ensure the types match the TypeScript interface on any existing alerting task. The migration assumes any type of data could exist and dropping parts that don't match the type expectation. The TypeScript interface uses schema.maybe() in a lot of places (as io-ts did), so safe if ever data gets dropped.
Cleanup the alerting/common/** exports to reduce bundle size. Because the @kbn/alerting-state-types package grew.
Since the new TypeScript interfaces / types are ReadOnly<...>, I created some Mutable... types for places that needed it (in order to avoid code refactoring).

To verify

Stack Monitoring:

To make TypeScript happy with the new ReadOnly RawAlertInstance type, I removed some redundant code and solved the issue.

Security Solution:

Changes to the alertInstanceFactoryStub set the alert's date to a string instead of a Date value. Note: The HTTP API response converted Date objects to string, so the HTTP API response will look the same with this change.

Response Ops:

In a fresh Kibana install, create alerting rules and ensure they run.
In a 8.9 version, create some rules, upgrade to this branch and ensure they still run.
Compare the io-ts definition with the new config-schema. They should match 1:1.
Look for ways the migration code could fail.

Note: The main changes are within the following areas:

x-pack/plugins/alerting/server/rule_type_registry.ts
x-pack/packages/kbn-alerting-state-types/*

…-ref HEAD~1..HEAD --fix'

….com:mikecote/kibana into alerting/onboard-task-types-state-validation

…d-task-types-state-validation

mikecote · 2023-07-31T12:39:42Z

x-pack/plugins/triggers_actions_ui/public/application/lib/rule_api/state.ts

-    .then((state: RuleTaskState) => {
-      return pipe(
-        ruleStateSchema.decode(state),
-        fold((e: Errors) => {
-          throw new Error(`Rule "${ruleId}" has invalid state`);
-        }, identity)
-      );
-    });


No longer necessary because:

The validation happens on task save

The decode would convert string dates into date objects. But would get converted back into strings when stringifying the JSON response.

Stack Monitoring who is using this API uses RuleTaskState to be type safe.

mikecote · 2023-07-31T12:45:28Z

x-pack/plugins/task_manager/server/saved_objects/migrations_880.test.ts

@@ -73,7 +74,7 @@ describe('successful migrations for 8.8.0', () => {
 });

 function checkMetaInRuleTaskState(
-  actual: RuleTaskState,
+  actual: MutableRuleTaskState,


To support code like this without having to rewrite.

delete copy?.alertInstances?.[id].meta?.uuid;

mikecote · 2023-07-31T12:45:56Z

x-pack/plugins/task_manager/server/saved_objects/migrations.ts

@@ -253,7 +254,7 @@ function addAlertUUID(doc: SavedObjectUnsanitizedDoc<SerializedConcreteTaskInsta

 // mutates alerts passed in
 function addAlertUUIDsToAlerts(
-  alerts: RuleTaskState['alertInstances'] | undefined,
+  alerts: MutableRuleTaskState['alertInstances'] | undefined,


To support code like this without having to rewrite

alert.meta.uuid = trackedAlert.alertUuid;

mikecote · 2023-07-31T12:48:08Z

x-pack/plugins/monitoring/server/alerts/base_rule.ts

-          if (filteredAlertInstance.state) {
-            accum[instanceId].state = {
-              alertStates: (filteredAlertInstance.state as AlertInstanceState).alertStates,
-            };
-          }


RawAlertInstance is now a ReadOnly type, since this code was doing a mutation, TypeScript was complaining. This code felt like it wasn't doing anything so I decided to remove it instead of fixing it..

Looks like state could have multiple properties set and this branch ensures we only keep alertStates, maybe to reduce payload sent to clients. Do you know if the state object can become big enough to be problematic ?

Ahh ok, that would make sense, thanks! I will keep this code around, at a first glance I wasn't sure what it was doing but it's clear now.

Do you know if the state object can become big enough to be problematic ?

It's possible for sure, there is a circuit breaker at the framework level to prevent more than 1,000 alert objects (and state objects) by default, so it is capped at some point but we haven't seen many issues around the storage size yet.

@klacabane I added the code back in base_rule.ts. I did it in a way that doesn't mutate the accum[instanceId] object to keep TypeScript happy (since RawAlertInstance is now a ReadOnly type). Let me know if this looks good to you 🙏

Looks great thanks!

mikecote · 2023-07-31T12:50:25Z

x-pack/plugins/alerting/server/task_runner/alert_task_instance.ts

-      ruleStateSchema.decode(taskInstance.state),
-      fold((e: t.Errors) => {
-        throw new Error(
-          `Task "${taskInstance.id}" ${
-            alert ? `(underlying Alert "${alert.id}") ` : ''
-          }has invalid state at ${enumerateErrorFields(e)}`
-        );
-      }, t.identity)
-    ),


Logic moved to task manager on read of the state object. This code also used to convert date strings into dates which we now type them as strings all over, so this is no longer needed.

mikecote · 2023-07-31T12:56:31Z

x-pack/plugins/alerting/server/alert/alert.ts

@@ -52,7 +52,7 @@ export class Alert<
  ActionGroupIds extends string = never
 > {
  private scheduledExecutionOptions?: ScheduledExecutionOptions<State, Context, ActionGroupIds>;
-  private meta: AlertInstanceMeta;
+  private meta: MutableAlertInstanceMeta;


The file does a bunch of mutations of the meta object, it felt best to continue that path for now instead of trying to fix it.

elasticmachine · 2023-08-09T11:58:56Z

Pinging @elastic/response-ops (Team:ResponseOps)

mikecote · 2023-08-14T11:29:48Z

@elasticmachine merge upstream

mikecote · 2023-08-16T16:42:50Z

@elasticmachine merge upstream

mikecote · 2023-08-22T15:57:02Z

@elasticmachine merge upstream

ymao1

LGTM! Verified that I was able to upgrade from earlier version (8.8) and see my detection/obs/stack rules with actions continue running and that I was able to create new rules that successfully ran.

mikecote · 2023-08-25T12:26:25Z

@elasticmachine merge upstream

e40pud

DE changes LGTM

mikecote · 2023-08-28T15:09:00Z

@elasticmachine merge upstream

kibana-ci · 2023-08-28T18:56:20Z

💚 Build Succeeded

Buildkite Build
Commit: 8259214

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	104	96	-8

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/alerting-state-types`	15	22	+7
`alerting`	755	749	-6
total			+1

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.5MB	1.4MB	-98.0KB

Page load bundle

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

id	before	after	diff
`alerting`	34.1KB	21.4KB	-12.7KB
`triggersActionsUi`	89.3KB	89.3KB	-2.0B
total			-12.7KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/alerting-state-types`	16	23	+7
`alerting`	786	780	-6
total			+1

async chunk count

id	before	after	diff
`triggersActionsUi`	63	61	-2

History

💔 Build #154192 failed c054572
💛 Build #154146 was flaky b5af1cd
💛 Build #153505 was flaky 95108ca
💛 Build #152431 was flaky 4d9fd20
💛 Build #151106 was flaky f227556
💛 Build #150023 was flaky 78030fc

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

cc @mikecote

klacabane

Infra changes LGTM

…idation of the framework fields (elastic#162425) Resolves elastic#159343 In this PR, I'm preparing the alerting rule task types for serverless by defining an explicit task state schema for the framework level fields. This schema is used to validate the task's state before saving but also when reading. In the scenario an older Kibana node runs a task after a newer Kibana node has stored additional task state, the unknown state properties will be dropped. Additionally, this will prompt developers to be aware that adding required fields to the task state is a breaking change that must be handled with care. (see elastic#155764). The PR also includes the following changes: - Modifying the `@kbn/alerting-state-types` package so the types are generated from `config-schema`, instead of `io-ts`. The schema is re-used for exporting a new `stateSchemaByVersion` property. - Removing `DateFromString` in favour of strings everywhere (config-schema doesn't support this conversion) - Add a v1 `up` migration that will ensure the types match the TypeScript interface on any existing alerting task. The migration assumes any type of data could exist and dropping parts that don't match the type expectation. The TypeScript interface uses `schema.maybe()` in a lot of places (as `io-ts` did), so safe if ever data gets dropped. - Cleanup the `alerting/common/**` exports to reduce bundle size. Because the `@kbn/alerting-state-types` package grew. - Since the new TypeScript interfaces / types are `ReadOnly<...>`, I created some `Mutable...` types for places that needed it (in order to avoid code refactoring). ## To verify Stack Monitoring: - To make TypeScript happy with the new ReadOnly `RawAlertInstance` type, I removed some redundant code and solved the issue. Security Solution: - Changes to the `alertInstanceFactoryStub` set the alert's date to a `string` instead of a `Date` value. Note: The HTTP API response converted `Date` objects to `string`, so the HTTP API response will look the same with this change. Response Ops: - In a fresh Kibana install, create alerting rules and ensure they run. - In a 8.9 version, create some rules, upgrade to this branch and ensure they still run. - Compare the `io-ts` definition with the new `config-schema`. They should match 1:1. - Look for ways the migration code could fail. Note: The main changes are within the following areas: - `x-pack/plugins/alerting/server/rule_type_registry.ts` - `x-pack/packages/kbn-alerting-state-types/*` --------- Co-authored-by: kibanamachine <[email protected]>

Initial commit

a3ee8a8

mikecote added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.10.0 labels Jul 24, 2023

mikecote self-assigned this Jul 24, 2023

kibanamachine and others added 16 commits July 24, 2023 17:17

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

aa82946

…-ref HEAD~1..HEAD --fix'

Keep hasFieldsForAAD

c1674dc

Merge branch 'alerting/onboard-task-types-state-validation' of github…

629e118

….com:mikecote/kibana into alerting/onboard-task-types-state-validation

Fix code merge

35c9f16

Merge branch 'main' of github.com:elastic/kibana into alerting/onboar…

88deca5

…d-task-types-state-validation

Fix typecheck failures

532a3dd

Merge branch 'main' of github.com:elastic/kibana into alerting/onboar…

613542b

…d-task-types-state-validation

Undo failing test

b6f9c7f

Fix test failure

7967331

Merge types and schemas into a single source

35bfc3b

Attempt reducing bundle size for alerting plugin

8fe518e

allow undefined

dcdd31e

Merge branch 'main' of github.com:elastic/kibana into alerting/onboar…

c0c9b30

…d-task-types-state-validation

Add migrations

2d8af0b

Merge branch 'main' of github.com:elastic/kibana into alerting/onboar…

78b5716

…d-task-types-state-validation

Add unit tests and fix type check

1b38ed8

mikecote commented Jul 31, 2023

View reviewed changes

Add back accidentally removed code

667e8b1

mikecote commented Jul 31, 2023

View reviewed changes

Merge branch 'main' into alerting/onboard-task-types-state-validation

475cb5c

mikecote marked this pull request as ready for review August 9, 2023 11:58

mikecote requested review from a team as code owners August 9, 2023 11:58

Merge branch 'main' into alerting/onboard-task-types-state-validation

78030fc

Merge branch 'main' into alerting/onboard-task-types-state-validation

f227556

mikecote added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

Merge branch 'main' into alerting/onboard-task-types-state-validation

4d9fd20

mikecote mentioned this pull request Aug 23, 2023

[Task Manager] Force validation on all tasks using state #164574

Merged

ymao1 approved these changes Aug 24, 2023

View reviewed changes

Merge branch 'main' into alerting/onboard-task-types-state-validation

c9a97b6

mikecote changed the title ~~Onboard alerting tasks to use stateSchemaByVersion for task state validation~~ Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields Aug 25, 2023

Remove some outdated code comments

95108ca

e40pud approved these changes Aug 25, 2023

View reviewed changes

kibanamachine and others added 3 commits August 28, 2023 11:09

Merge branch 'main' into alerting/onboard-task-types-state-validation

b5af1cd

Add back code that only keeps alertStates in the state

c054572

Modify code to make tests pass

8259214

klacabane approved these changes Aug 29, 2023

View reviewed changes

mikecote merged commit 82f551c into elastic:main Aug 29, 2023
34 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields #162425

Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields #162425

mikecote commented Jul 24, 2023 •

edited

Loading

mikecote Jul 31, 2023 •

edited

Loading

mikecote Jul 31, 2023

mikecote Jul 31, 2023

mikecote Jul 31, 2023 •

edited

Loading

klacabane Aug 28, 2023

mikecote Aug 28, 2023

mikecote Aug 28, 2023 •

edited

Loading

klacabane Aug 29, 2023

mikecote Jul 31, 2023 •

edited

Loading

mikecote Jul 31, 2023

elasticmachine commented Aug 9, 2023

mikecote commented Aug 14, 2023

mikecote commented Aug 16, 2023

mikecote commented Aug 22, 2023

ymao1 left a comment

mikecote commented Aug 25, 2023

e40pud left a comment

mikecote commented Aug 28, 2023

kibana-ci commented Aug 28, 2023

API count

async chunk count

klacabane left a comment

Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields #162425

Onboard alerting tasks to use stateSchemaByVersion for task state validation of the framework fields #162425

Conversation

mikecote commented Jul 24, 2023 • edited Loading

To verify

mikecote Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

mikecote Jul 31, 2023

Choose a reason for hiding this comment

mikecote Jul 31, 2023

Choose a reason for hiding this comment

mikecote Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

klacabane Aug 28, 2023

Choose a reason for hiding this comment

mikecote Aug 28, 2023

Choose a reason for hiding this comment

mikecote Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

klacabane Aug 29, 2023

Choose a reason for hiding this comment

mikecote Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

mikecote Jul 31, 2023

Choose a reason for hiding this comment

elasticmachine commented Aug 9, 2023

mikecote commented Aug 14, 2023

mikecote commented Aug 16, 2023

mikecote commented Aug 22, 2023

ymao1 left a comment

Choose a reason for hiding this comment

mikecote commented Aug 25, 2023

e40pud left a comment

Choose a reason for hiding this comment

mikecote commented Aug 28, 2023

kibana-ci commented Aug 28, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

History

klacabane left a comment

Choose a reason for hiding this comment

mikecote commented Jul 24, 2023 •

edited

Loading

mikecote Jul 31, 2023 •

edited

Loading

mikecote Jul 31, 2023 •

edited

Loading

mikecote Aug 28, 2023 •

edited

Loading

mikecote Jul 31, 2023 •

edited

Loading