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] Allow rule types to extract/inject saved object references on rule CRU #101896

Merged
merged 37 commits into from
Jul 20, 2021

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Jun 10, 2021

Resolves #87992

Summary

This PR provides a framework level hook to allow individual rule types to register functions for extracting/injecting saved object references from and into a rule's parameters.

The AlertType interface has been updated to include a new ExtractedParams template type and optional useSavedObjectReferences functions. Both useSavedObjectReferences.extractReferences and useSavedObjectReferences.injectReferences must be defined if one is defined.

Updated `AlertType` interface
export interface AlertType<
  Params extends AlertTypeParams = never,
  ExtractedParams extends AlertTypeParams = never -----> this can be the same as Params if the rule type wants
  State extends AlertTypeState = never,
  InstanceState extends AlertInstanceState = never,
  InstanceContext extends AlertInstanceContext = never,
  ActionGroupIds extends string = never,
  RecoveryActionGroupId extends string = never
> {
  id: string;
  name: string;
  actionGroups: Array<ActionGroup<ActionGroupIds>>;
  defaultActionGroupId: ActionGroup<ActionGroupIds>['id'];
  recoveryActionGroup?: ActionGroup<RecoveryActionGroupId>;
  executor: ExecutorType;
  producer: string;
  actionVariables?: {};
  minimumLicenseRequired: LicenseType;
  /**
   * These are the new hooks
   **/
  useSavedObjectReferences?: {
    extractReferences: (params: Params) => RuleParamsAndRefs<ExtractedParams>;
    injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => Params;
  };
}

In the alerts_client, the extractReferences hook would be called (if defined) on create and update and the injectReferences hook would be called (if defined) on get and find. The framework will prefix any extracted SO reference names with a framework level prefix (currently param:) to delineate between framework level SO references and references extracted by the rule type.

Example hooks implementation:
useSavedObjectReferences: {
  extractReferences: (params: Params): RuleParamsAndRefs<ExtractedParams> => {
    const { testSavedObjectId, ...otherParams } = params;

    const testSavedObjectRef = 'testRef_0';
    const references = [
      {
        name: `testRef_0`,
        id: testSavedObjectId,
        type: 'index-pattern',
      },
    ];
    return { params: { ...otherParams, testSavedObjectRef }, references };
  },
  injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => {
    const { testSavedObjectRef, ...otherParams } = params;
    const reference = references.find((ref) => ref.name === testSavedObjectRef);
    if (!reference) {
      throw new Error(`Test reference "${testSavedObjectRef}"`);
    }
    return { ...otherParams, testSavedObjectId: reference.id } as Params;
  },
},

Checklist

Delete any items that are not applicable to this PR.

@ymao1
Copy link
Contributor Author

ymao1 commented Jun 15, 2021

@elasticmachine merge upstream

@ymao1 ymao1 changed the title Alerting/saved object references [Alerting] Allow rule types to extract/inject saved object references on rule CRU Jun 16, 2021
@ymao1 ymao1 self-assigned this Jun 21, 2021
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM

I did this:

  • Reading through the description and the example.
  • Making sure I understood the example.
  • In the example I saw that you can remove params as well as add them back if you only want to store them in the saved references which is 👍 great.
  • Read through the two changes in files changed

I did not do this:

  • I did not try to create a function that exercises this functionality and ensure it works. Assuming someone from kibana-alerting will do this part and/or a e2e test at some point from kibana-alerting.

I assume once approved we will then implement this for exception lists and then write a SO migration to upgrade existing exception lists. These two PR's could be separate from our end.

@mikecote mikecote self-requested a review July 8, 2021 13:07
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM! I tested locally with the index threshold rule type, I was able to see it work as expected 👍

@@ -146,6 +152,10 @@ export interface AlertType<
params?: ActionVariable[];
};
minimumLicenseRequired: LicenseType;
useSavedObjectReferences?: {
extractReferences: (params: Params) => RuleParamsAndRefs<ExtractedParams>;
injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => Params;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking there's a reason for it to be SavedObjectAttributes but I figured I'd ask, should this be ExtractedParams?

Suggested change
injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => Params;
injectReferences: (params: ExtractedParams, references: SavedObjectReference[]) => Params;

@@ -183,6 +184,8 @@ export interface GetAlertInstanceSummaryParams {
dateStart?: string;
}

const extractedSavedObjectParamReferenceNamePrefix = 'param:';
Copy link
Contributor

Choose a reason for hiding this comment

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

optional nit: should we add a comment about modifying this value will require a migration for existing SOs with references?

@ymao1 ymao1 removed the request for review from spong July 12, 2021 11:42
@ymao1
Copy link
Contributor Author

ymao1 commented Jul 12, 2021

@elasticmachine merge upstream

Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Maps team owned file updates lgtm!

@ymao1
Copy link
Contributor Author

ymao1 commented Jul 16, 2021

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Jul 20, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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
alerting 230 234 +4
Unknown metric groups

API count

id before after diff
alerting 238 242 +4

History

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

cc @ymao1

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome new capabilities for the rule types. Tested this manually on the extending Index Threshold with the new hooks - it works!

@ymao1 ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 20, 2021
@ymao1 ymao1 merged commit e2aacdc into elastic:master Jul 20, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 20, 2021
… on rule CRU (elastic#101896)

* Adding function hooks into rule type definition and call extract fn on rule create

* Adding hooks for extracting and injecting saved object references. Adding extractReferences to create and update workflow

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Calling injectReferences function if defined. Finishing unit tests for create and update

* Adding tests for get

* Adding tests for find

* Cleanup

* Fixing types check

* Fixing functional tests

* Fixing functional tests

* Fixing tests

* Updating README

* Throwing boom error instead of normal error

* Adding framework level prefix to extracted saved object reference names

* Fixing types

* Fixing types

* PR fixes

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 20, 2021
… on rule CRU (#101896) (#106333)

* Adding function hooks into rule type definition and call extract fn on rule create

* Adding hooks for extracting and injecting saved object references. Adding extractReferences to create and update workflow

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Calling injectReferences function if defined. Finishing unit tests for create and update

* Adding tests for get

* Adding tests for find

* Cleanup

* Fixing types check

* Fixing functional tests

* Fixing functional tests

* Fixing tests

* Updating README

* Throwing boom error instead of normal error

* Adding framework level prefix to extracted saved object reference names

* Fixing types

* Fixing types

* PR fixes

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: ymao1 <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 21, 2021
…y-show-migrate-to-authzd-users

* 'master' of github.com:elastic/kibana: (48 commits)
  [Canvas] Expression shape (elastic#103219)
  [FTR] Skips Vega tests
  [Sample data] Use Lens in ecommerce data (elastic#106039)
  [APM] Backends inventory & overview page routes (elastic#106223)
  [TSVB] Add more functional tests for Gauge and TopN (elastic#105361)
  Add toggle to enable/disable rule install from SOs (elastic#106189)
  Improve unit test coverage of FS API calls (elastic#106242)
  Remove recursive plugin status in meta field (elastic#106286)
  [Ingest pipelines] add community id processor (elastic#103863)
  [XY axis] Fixes the values inside bar charts (elastic#106198)
  [data.search] Set default expiration to 1m if search sessions are disabled (elastic#105329)
  set the doc title when navigating to reporting and unset when navigating away (elastic#106253)
  [Lens] Display legend inside chart (elastic#105571)
  [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid` (elastic#106199)
  [Security Solutions] Removes the elastic legacy client from lists and security_solution plugins (elastic#106130)
  [Enterprise Search] Require security plugin in 8.0 (elastic#106307)
  [DOCS] Updates screenshots in Dev Tools docs (elastic#105859)
  [DOCS] Updates text and screenshots in tags doc (elastic#105853)
  [Alerting] Allow rule types to extract/inject saved object references on rule CRU (elastic#101896)
  Jest and Storybook fixes (elastic#104991)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/plugin.ts
ymao1 added a commit to ymao1/kibana that referenced this pull request Jul 23, 2021
… on rule CRU (elastic#101896)

* Adding function hooks into rule type definition and call extract fn on rule create

* Adding hooks for extracting and injecting saved object references. Adding extractReferences to create and update workflow

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Adding type template for extracted params

* Calling injectReferences function if defined. Finishing unit tests for create and update

* Adding tests for get

* Adding tests for find

* Cleanup

* Fixing types check

* Fixing functional tests

* Fixing functional tests

* Fixing tests

* Updating README

* Throwing boom error instead of normal error

* Adding framework level prefix to extracted saved object reference names

* Fixing types

* Fixing types

* PR fixes

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow usage of saved object references in alerts
10 participants