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

Implemented actions server API for supporting preconfigured connectors #62382

Merged

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Apr 2, 2020

Summary

Current PR exposing server API for preconfigured connectors defined in kibana.yaml.

  1. extended api/actions/get/{id} to support return preconfigured connector by id
  2. add rejection for update and edit preconfigured connectors
  3. exposed endpoint api/actions/_getAll for getting all connectors - preconfigured and saved objects. This endpoint is supposed to be used instead of api/actions/_find on the UI side for building Connectors table (currently all paging, sorting and filtering is done on the UI side).

Checklist

Delete any items that are not applicable to this PR.

@YulNaumenko YulNaumenko added v8.0.0 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) v7.8 labels Apr 2, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner April 2, 2020 22:05
@YulNaumenko YulNaumenko self-assigned this Apr 2, 2020
@elasticmachine
Copy link
Contributor

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

@YulNaumenko YulNaumenko requested a review from a team as a code owner April 3, 2020 03:50
@mikecote mikecote self-requested a review April 3, 2020 12:59
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.

This is great, heading towards the right direction!

I think we should keep using the actions terminology in the server side (variable names, TS interfaces, etc) instead of introducing connectors (front-end term).

It looks like some tests need to be fixed / updated (changing expected result in switch statements). Let me know if you need help with those.

x-pack/plugins/actions/server/types.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/config.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/actions_client.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/actions_client.ts Outdated Show resolved Hide resolved
@YulNaumenko YulNaumenko requested review from a team and mikecote April 3, 2020 19:20
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM from stack monitoring!

…gured-connectors-api

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like to see that comment corrected or, ideally, covered by a unit test so that it can't go out of date again.

x-pack/plugins/actions/server/actions_client.ts Outdated Show resolved Hide resolved
…gured-connectors-api

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@pmuellr pmuellr self-assigned this Apr 6, 2020
@mikecote mikecote added v7.8.0 and removed v7.8 labels Apr 6, 2020
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM - made a few small comments/questions

I think there may be a couple more validation things we should do, in a follow-on issue/PR:

  • what happens if two pre-configured actions have the same id?
  • what happens if someone creates a pre-configured action, that has the same id as an non-pre-configured action. Thinking someone trying to hack the system. User A creates a new slack action. User B gets the id for that action, creates a pre-configured action with the same id. Which one will be used?

@@ -29,35 +36,12 @@ interface CreateOptions {
action: Action;
}

interface FindOptions {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we we never need this? :-)

Seems like if we do need something like this in the future, if listing everything is too much (even with client filtering), we could provide some simple search capabilities that would work in both ES and "manual" searches through the pre-configured ones. But I guess pagination is something we just can't support anymore ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this with @mikecote and didn't find any real usage of 'find' API method in the Kibana plugins our UI implementation - seems like we don't need this (at least for now)

@@ -83,6 +84,7 @@ describe('create()', () => {
});
expect(result).toEqual({
id: '1',
isPreconfigured: false,
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering why isPreconfigured is something we're returning here. Guessing it will be nice, eventually, to somehow indicate this to the user, with some kind of "lock" icon or something. But I don't think it's really needed for anything but that, is it? Just curious, I'm +1 on having it, now that I see it, but guessing I might not have returned it if I had implemented this PR.

const actionsConfig = (await this.config) as ActionsConfig;
const actionsConfigUtils = getActionsConfigurationUtilities(actionsConfig);

this.preconfiguredActions.push(
Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably be running these through the actionType validators, to make sure the config/secrets get validated by the actionType. I don't think that's happening right now. I made some comments in my earlier review about other validations we should be doing, I think it's fine to do them all in a separate issue/PR.

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

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! 👍

…gured-connectors-api

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / kibana-xpack-agent / Sorts by activated rules.Signal detection rules Sorts by activated rules

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 20 times on tracked branches: https:/elastic/kibana/issues/62763


Stack Trace

CypressError: Timed out retrying: expected '<a.euiLink.euiLink--primary>' to have text 'Hping Process Activity', but the text was 'User Discovery via Whoami'
    at cypressErr (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:138644:9)
    at throwErr (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:138577:11)
    at Object.throwErrByPath (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:138625:3)
    at retry (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:132905:19)
    at onFailFn (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:121122:16)
    at tryCatcher (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:165465:23)
    at Promise._settlePromiseFromHandler (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:163401:31)
    at Promise._settlePromise (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:163458:18)
    at Promise._settlePromise0 (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:163503:10)
    at Promise._settlePromises (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:163578:18)
    at Async../node_modules/bluebird/js/release/async.js.Async._drainQueue (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:160190:16)
    at Async../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:160200:10)
    at Async.drainQueues (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:160074:14)

History

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

@YulNaumenko YulNaumenko merged commit 730dcbf into elastic:master Apr 8, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Apr 8, 2020
elastic#62382)

* Implemented actions server API for supporting preconfigured connectors defined in kibana.yaml

* Fixed type check

* Fixed due to comments and extended functional tests

* Fixed tests and renamed connectors

* fixed jest tests

* Fixed type checks

* Fixed failing alert save

* Fixed alert client tests

* fixed type checks

* Fixed language check error

* Fixed jest tests

* Added missing comments and docs

* fixed due to comments

* Fixed json config for preconfigured

* fixed type check, reverted config

* config experiment with json stringify

* revert experiment

* Removed the spaces from connector names in config
YulNaumenko added a commit that referenced this pull request Apr 8, 2020
#62382) (#62980)

* Implemented actions server API for supporting preconfigured connectors defined in kibana.yaml

* Fixed type check

* Fixed due to comments and extended functional tests

* Fixed tests and renamed connectors

* fixed jest tests

* Fixed type checks

* Fixed failing alert save

* Fixed alert client tests

* fixed type checks

* Fixed language check error

* Fixed jest tests

* Added missing comments and docs

* fixed due to comments

* Fixed json config for preconfigured

* fixed type check, reverted config

* config experiment with json stringify

* revert experiment

* Removed the spaces from connector names in config
@pmuellr pmuellr removed their assignment Apr 9, 2020
@mikecote mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants