[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors #194081

pmuellr · 2024-09-25T23:49:13Z

Extracted from #189027, commit c97afeb

Summary

Allows connector types to add functions to be called when connectors are created, updated, and deleted.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pmuellr · 2024-09-26T13:13:13Z

Here are some things to consider looking at the original commit:

naming: "event" implies to me all kinds of semantics that don't fit: truly async (no one is waiting for the return), that it won't affect the flow of the event producer, etc. We previously referenced "hook" before, and I still think that fits. Maybe "callback"? We've also used "middleware" in task manager, though I believe it's unused, and don't think that term fits either.
So maybe preSaveHook and postDeleteHook?
if an error occurs in the hook, should it fail the create/update/delete? At first I was a little worried about this, but I think it's probably the best - it easily allows a hook to fail the operation, and if a hook is really part of the connector so I'm not sure we want a lot of more "sophisticated" ways of indicating success/failure.
what to pass to the hooks. Currently scopedClusterClient is passed, but this doesn't seem right. ~~Currently we send a "Request" object in, this feels like it's good enough.~~ (note: see correction in comment below). Authors can also (I think) curry in their own objects when the connector is registered, as we do for other things
we should add doc for this, wherever we are keeping doc these days (at least the README I guess)

pmuellr · 2024-09-26T13:47:57Z

I was mistaken about the executor only getting passed a request. Here's the full signature for the executor:

kibana/x-pack/plugins/actions/server/types.ts

Lines 75 to 92 in bc576fe

    
           export interface ActionTypeExecutorOptions< 
        
             Config extends Record<string, unknown>, 
        
             Secrets extends Record<string, unknown>, 
        
             Params 
        
           > { 
        
             actionId: string; 
        
             services: Services | UnsecuredServices; 
        
             config: Config; 
        
             secrets: Secrets; 
        
             params: Params; 
        
             logger: Logger; 
        
             isEphemeral?: boolean; 
        
             taskInfo?: TaskInfo; 
        
             configurationUtilities: ActionsConfigurationUtilities; 
        
             source?: ActionExecutionSource<unknown>; 
        
             request?: KibanaRequest; 
        
             connectorUsageCollector: ConnectorUsageCollector; 
        
           }

I think we want most of these, but we could add them later as needed. Critically, we want services (or a subset).

Here's Services and UnsecuredServices:

kibana/x-pack/plugins/actions/server/types.ts

Lines 48 to 58 in bc576fe

    
           export interface Services { 
        
             savedObjectsClient: SavedObjectsClientContract; 
        
             scopedClusterClient: ElasticsearchClient; 
        
             connectorTokenClient: ConnectorTokenClient; 
        
           } 
        
           export interface UnsecuredServices { 
        
             savedObjectsClient: ISavedObjectsRepository; 
        
             scopedClusterClient: ElasticsearchClient; 
        
             connectorTokenClient: ConnectorTokenClient; 
        
           }

pmuellr · 2024-09-26T15:15:04Z

Thought it would be interesting to think about extending this in the future. Not in this PR :-)

Currently, the hooks really just allow the hook to allow or deny the operation - they don't return anything useful. What else could they do?

modify the config/secrets before the create/update
set some "connector state" object values; a concept we don't currently support

Modifying config/secrets seems dicey - in fact, we may want to provide deep copies of these to prevent modification. But could be useful to supply additional computed values in those objects. There are noconnector type provided functions invoked iduring create/update/delete, so this may make it easier to deal with non-trivial or expensive data before it hits the executor.

The concept of "connector state" feels like something we talked about a long time ago, but there are no issues open. The idea would be to add the capability for a state object ({}) to be provided to the executor and hooks, to store some data across invocations. The use case I keep coming to (but may not be completely relevant) is the Slack API channels bit. Because of the APIs involved, it's non-trivial to search for Slack channels to provide to the user. Could we do this once, to at least cache some of the relevant bits?

The state object would be persisted (pretend with the connector, and that's probably ok, but could be a new SO), and provided to the executor and hooks. We'd need to provide a way for those functions to "return" the new state to persist, not clear the best way to do that. We likely would NOT want to return this via HTTP, but probably via the ActionsClient is fine. Seems very likely some code outside of executor/hooks would want access to this data as well.

I don't think there's anything in the current "shape" of these hooks that would make it harder to implement these, if we want to go there.

pmuellr · 2024-09-26T17:56:41Z

In b0ec428c5be6717eb2076d6ff00086870506a372 I added a minimal services object to the hook parameters, similar to the executor options. But only provided the scopedClusterClient for now.

YulNaumenko · 2024-10-04T21:49:43Z

x-pack/plugins/actions/server/actions_client/actions_client.ts

@@ -255,6 +256,33 @@ export class ActionsClient {
    }
    this.context.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);

+    const hookServices: HookServices = {
+      scopedClusterClient: this.context.scopedClusterClient.asCurrentUser,


For my specific use case I need scopedClusterClient.asInternalUser, so can we just return scopedClusterClient to have a choice or expose two client?

I plan on sending this.context.scopedClusterClient, so both will be available. Easy to imagine both might be needed.

YulNaumenko · 2024-10-04T23:25:56Z

Using post/preSave hooks in the commit 6f9504f

…onnectors Extracted from elastic#189027, commit c97afeb Allows connector types to add functions to be called when connectors are created, updated, and deleted.

kibana-ci · 2024-10-07T17:25:11Z

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #84 / alerting api integration security and spaces enabled - Group 2 Connectors create no_kibana_privileges at space1 should call the pre-save hook appropriately
[job] [logs] FTR Configs #86 / alerting api integration security and spaces enabled - Group 2 Connectors create no_kibana_privileges at space1 should call the pre-save hook appropriately
[job] [logs] FTR Configs #86 / alerting api integration security and spaces enabled - Group 2 Connectors create no_kibana_privileges at space1 should call the pre-save hook appropriately
[job] [logs] FTR Configs #84 / alerting api integration security and spaces enabled - Group 2 Connectors create no_kibana_privileges at space1 should call the pre-save hook appropriately
[job] [logs] FTR Configs #82 / Reporting Functional Tests with Security enabled Access to Management > Reporting Download report user can access download link for export type that is no longer supported
[job] [logs] Jest Tests #3 / update() updates an action with all given properties
[job] [logs] Jest Tests #3 / update() updates an action with all given properties

History

💔 Build #238598 failed 447fa3d6284628f6be92dddae39dde139b40c25c
💚 Build #238208 succeeded f037dba9cab5cdfd1935c39a9f39409af0af80e0
💚 Build #237461 succeeded b0ec428c5be6717eb2076d6ff00086870506a372
💛 Build #237393 was flaky 74f8e98d2c848623690b4f4abb621c5df8078356

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

- re-added the update code to the new update module (update body removed from actionsClient) - propertly set `scopedClusterClient` instead of the `asCurrentUser` flavor - moved `tryCatch()` to `lib`

elasticmachine · 2024-10-08T22:07:20Z

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

pmuellr · 2024-10-09T11:52:47Z

x-pack/plugins/actions/server/lib/try_catch.ts

+// functional version of try/catch, allows you to not have to use
+// `let` vars initialied to `undefined` to capture the result value
+
+export async function tryCatch<T>(fn: () => Promise<T>): Promise<T | Error> {


Handy little function to avoid having to let define a variable that would need to be set within a try / catch.

Happy to use some alternative, didn't find anything simple.

ymao1 · 2024-10-09T13:45:25Z

x-pack/plugins/actions/server/actions_client/actions_client.ts

+    const label = `connectorId: "${id}"; type: ${actionTypeId}`;
+    const tags = ['post-save-hook', id];
+
+    if (actionType.postSaveHook) {


is the idea here that by passing in wasSuccessful the postSave hook could potentially undo whatever it did in the preSave hook if the create failed? Might be useful to be explicit about this in the README

That is the exact reason, and you're right, I'll add it to the doc. Yuliia will need to make use of it, as she is calling the ES /inference endpoint in the pre hook, but if the connector create/update fails, she'll need to "undo" it ...

ymao1

LGTM

ymao1 · 2024-10-09T14:14:59Z

x-pack/test/alerting_api_integration/common/plugins/alerts/server/action_types.ts

+    async executor({ config, secrets, params, services, actionId }) {
+      return { status: 'ok', actionId };
+    },
+    async preSaveHook({ connectorId, config, secrets, services, isUpdate, logger }) {


nit: could add a flag to the config to force the hook to throw an error to test the error condition

ah, thanks for reminding me. I do think we should have some FT that test the error conditions, but didn't think they were absolutely required, since we do test in the jest tests. And given the time frame. Also not sure how to test for the SO create/update failing in FT, but could likely do it in a jest integration test.

So was planning on opening an issue to add those later.

added add additional function tests for pre/post save/delete hooks #195663

YulNaumenko

Looks great! Thank you @pmuellr for shaping it and making through the line! Docs are awesome 💅

pmuellr · 2024-10-09T20:58:44Z

I thought it wouldn't do a full re-build if just an .md file changed, but maybe I'm thinking .asciidoc :-)

I'll plan on merging this after THAT build completes ...

elasticmachine · 2024-10-09T21:48:23Z

💚 Build Succeeded

Buildkite Build
Commit: e0d6e01

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
`actions`	308	314	+6

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
`actions`	33	36	+3

Unknown metric groups

API count

id	before	after	diff
`actions`	314	320	+6

History

💚 Build #240841 succeeded dfcacaf
💔 Build #240473 failed 2df18f2
💔 Build #240055 failed 5ee1eb3
💔 Build #239963 failed c182b1b

kibanamachine · 2024-10-09T21:52:26Z

Starting backport for target branches: 8.x

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

…onnectors (elastic#194081) Allows connector types to add functions to be called when connectors are created, updated, and deleted. Extracted from elastic#189027, commit c97afeb Co-authored-by: Yuliia Naumenko <[email protected]> (cherry picked from commit 57096d1)

kibanamachine · 2024-10-09T21:56:56Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

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

Questions ?

Please refer to the Backport tool documentation

… for connectors (#194081) (#195686) # Backport This will backport the following commits from `main` to `8.x`: - [[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors (#194081)](#194081)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Patrick Mueller <[email protected]>

YulNaumenko reviewed Oct 4, 2024

View reviewed changes

pmuellr force-pushed the 189027-just-actions-servers branch from 6ab5755 to 4599d00 Compare October 7, 2024 16:09

pmuellr added 2 commits October 7, 2024 12:14

[ResponseOps] add pre-create, pre-update, and post-delete hooks for c…

fd3ce86

…onnectors Extracted from elastic#189027, commit c97afeb Allows connector types to add functions to be called when connectors are created, updated, and deleted.

fix license in FTR rule type added for this PR

c182b1b

pmuellr force-pushed the 189027-just-actions-servers branch from 4599d00 to c182b1b Compare October 7, 2024 16:20

pmuellr added 4 commits October 7, 2024 17:11

- refactored hook jest tests into a separate file

5ee1eb3

- re-added the update code to the new update module (update body removed from actionsClient) - propertly set `scopedClusterClient` instead of the `asCurrentUser` flavor - moved `tryCatch()` to `lib`

more tests

5edb82b

should be it for the jest tests, now function tests

668bdfc

add function tests

2df18f2

pmuellr added release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 8, 2024

pmuellr marked this pull request as ready for review October 8, 2024 22:07

pmuellr requested a review from a team as a code owner October 8, 2024 22:07

pmuellr requested review from YulNaumenko, ymao1 and umbopepato October 8, 2024 22:08

prettier fixes, clean up some tests

c0b6ee3

pmuellr commented Oct 9, 2024

View reviewed changes

fix typos, add a detail to the README

483a47a

ymao1 reviewed Oct 9, 2024

View reviewed changes

ymao1 approved these changes Oct 9, 2024

View reviewed changes

YulNaumenko approved these changes Oct 9, 2024

View reviewed changes

YulNaumenko mentioned this pull request Oct 9, 2024

[Connectors][GenAI] Inference Service Kibana connector #189027

Merged

YulNaumenko and others added 2 commits October 9, 2024 10:36

Merge branch 'main' into 189027-just-actions-servers

dfcacaf

update README from PR comments

e0d6e01

pmuellr mentioned this pull request Oct 9, 2024

[ResponseOps][Connectors] add additional function tests for pre/post save/delete hooks #195663

Open

pmuellr added v9.0.0 v8.16.0 labels Oct 9, 2024

pmuellr merged commit 57096d1 into elastic:main Oct 9, 2024
26 checks passed

kibanamachine mentioned this pull request Oct 9, 2024

[8.x] [ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors (#194081) #195686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors #194081

[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors #194081

pmuellr commented Sep 25, 2024 •

edited by kibanamachine

Loading

pmuellr commented Sep 26, 2024 •

edited

Loading

pmuellr commented Sep 26, 2024

pmuellr commented Sep 26, 2024 •

edited

Loading

pmuellr commented Sep 26, 2024

YulNaumenko Oct 4, 2024

pmuellr Oct 7, 2024

YulNaumenko commented Oct 4, 2024

kibana-ci commented Oct 7, 2024 •

edited

Loading

elasticmachine commented Oct 8, 2024

pmuellr Oct 9, 2024

ymao1 Oct 9, 2024

pmuellr Oct 9, 2024

ymao1 left a comment

ymao1 Oct 9, 2024

pmuellr Oct 9, 2024

pmuellr Oct 9, 2024

YulNaumenko left a comment

pmuellr commented Oct 9, 2024

elasticmachine commented Oct 9, 2024

API count

kibanamachine commented Oct 9, 2024

kibanamachine commented Oct 9, 2024

[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors #194081

[ResponseOps] add pre-create, pre-update, and post-delete hooks for connectors #194081

Conversation

pmuellr commented Sep 25, 2024 • edited by kibanamachine Loading

Summary

Checklist

pmuellr commented Sep 26, 2024 • edited Loading

pmuellr commented Sep 26, 2024

pmuellr commented Sep 26, 2024 • edited Loading

pmuellr commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YulNaumenko commented Oct 4, 2024

kibana-ci commented Oct 7, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

History

elasticmachine commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

pmuellr commented Oct 9, 2024

elasticmachine commented Oct 9, 2024

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

kibanamachine commented Oct 9, 2024

kibanamachine commented Oct 9, 2024

💚 All backports created successfully

Questions ?

pmuellr commented Sep 25, 2024 •

edited by kibanamachine

Loading

pmuellr commented Sep 26, 2024 •

edited

Loading

pmuellr commented Sep 26, 2024 •

edited

Loading

kibana-ci commented Oct 7, 2024 •

edited

Loading