Support scheme field when creating a Threshold alert with a Webhook action #53757

jkelastic · 2019-12-23T08:58:55Z

Summary

Fixed 44004 . Watcher API supports a scheme field when creating a webhook action, which accepts "http" and "https" as values. Added scheme to the UI and serialization logic to support this field.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-23T09:08:58Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

kibanamachine · 2019-12-23T10:15:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 05c907f44e8a8a4255114d806d784d7c2f5aaaca

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

jkelastic · 2019-12-27T23:44:49Z

@elasticmachine merge upstream

kibanamachine · 2019-12-28T01:03:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ec6106c5ee1a37c216b8ab5becdac67769f36ff4

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

jloleysens

@jkelastic Thanks for this contribution!

Code

The code changes look good to me! 👍

UX

When I opened the watcher threshold configuration I saw the scheme field but it could be cool to order the fields in the same way a conventional URL is ordered (moving scheme to before hostname). Let me know what you think:

When I tested the webhook action I saw the following in the notification (see bottom right undefined):

Could we clean this up?

jkelastic · 2019-12-30T16:26:14Z

@elasticmachine merge upstream

…ield

jkelastic · 2020-01-02T18:06:58Z

@elasticmachine merge upstream

alisonelizabeth

nice work @jkelastic! i left a couple minor comments, but overall looks great.

alisonelizabeth · 2020-01-02T18:58:23Z

.../sections/watch_edit/components/threshold_watch_edit/action_fields/webhook_action_fields.tsx

+        <EuiFlexItem>
+          <EuiFormRow
+            label={i18n.translate(
+              'xpack.watcher.sections.watchEdit.threshold.webhookAction.SchemeFieldLabel',


nit: camelcase the last segment - schemeFieldLabel

thank you, will do and upload the changes

@jkelastic this still needs to be fixed

@alisonelizabeth thanks, uploaded change

alisonelizabeth · 2020-01-02T19:38:55Z

.../sections/watch_edit/components/threshold_watch_edit/action_fields/webhook_action_fields.tsx

@@ -29,13 +29,15 @@ interface Props {

 const HTTP_VERBS = ['head', 'get', 'post', 'put', 'delete'];

+const SCHEME_VERBS = ['', 'http', 'https'];


I don't think it's necessary to have an empty first option. I believe http is the default value in ES, so I think it makes sense to have that selected by default in the UI.

nit: I might suggest renaming this constant. I wouldn't consider http and https as verbs.

I don't think it's necessary to have an empty first option. I believe http is the default value in ES, so I think it makes sense to have that selected by default in the UI.

I left an empty first option because when I set const SCHEME_VERBS = ['http', 'https']; and click on show request the scheme field doesn't show up on the generated request. It looks like it's because the EUI FieldText is set to update onChange. I can set const SCHEME_VERBS = ['http', 'https']; , but will have to add more code to deal with that.

nit: I might suggest renaming this constant. I wouldn't consider http and https as verbs.

No problem, I will sent it as const SCHEME = ['http', 'https'];

You should be able to provide a default value to scheme in the webhook_action.js model file.

For example:

this.scheme = get(props, 'scheme', 'http');

@alisonelizabeth thanks for the advice, i've uploaded the changes

Adds new Machine Learning Inference processor to Kibana Console. Also adds new Inference APIs generated from ES specs.

…lastic#53679)

…chema (elastic#51919) * [NP] Allow custom validations in HTTP Routes apart from @kbn/config-schema * API docs * Allow validate function in the route handler (run-code validation) * Prefix RouteXXX + Params and Body Validation Aliases * Fix test broken by lodash * Update API docs * Add default types for simpler manual declaration * Add run-time validation of the RouteValidateSpec * Expose RouteValidationError instead of SchemaTypeError * RouteValidator as a class to match config-schema interface * Test for not-inline handler (need to check IRouter for elastic#47047) * Add preValidation of the input for a safer custom validation * Better types for RouteHandlers * [NP] Move route validation to RouteValidator wrapper * Use the class only internally but maintain the same API * Fix types * Ensure RouteValidator instance in KibanaRequest.from * Fix validator.tests (Buffer.from instead of new Buffer) * Default precheck should allow null values * Also allow undefined in preChecks * MR feedback fixes * Provide RouteValidationResolver to the validation function * Add functional tests * Fix new functional tests * Fix validator additional test * Fix test with new resolver * Remove unused import * Rename ValidationResolver to ValidationResultFactory and change the interface to look more like the KibanaResponseFactory Co-authored-by: Elastic Machine <[email protected]>

* [SIEM] Migrating frontend services to NP (elastic#52783) * Remove legacy index_patterns import We'd already brought in the new interface in a previous commit; this was just used as an unnecessary type assertion. * Update snapshots following new docLink mocks * Remove unused manual mocks These are not picked up by jest; calling jest.mock('lib/compose/kibana_core') has the same effect whether or not these files exist. * WIP: Use kibana core mock everywhere we're doing it manually The timeline tests are the last place we're explicitly mocking useKibanaCore; removing the mocks cause tests to hang. I think hey're relying on the side effects of importing the mock/ui_settings file, but I'll figure that out next. * Replace ui/documentation_links with core NP service In most instances, this meant using the useContext hook with our NP core context. This also updates our mocks to leverage the factory so graciously provided by platform. There are a few failing tests, mostly due to links being previously undefined in tests. * Use new mocks on timeline test that doesn't hang The rest of these do, though. * Remove remaining uses of mockUiSettings in useKibanaCore mocks These have to be evaluated immediately so that we always return the same core object. Otherwise we get stuck in a loop between render/useEffect/setState due to the savedObjects client being different on each invocation. * Invoke platform's mock factory at mock time Previously, we were invoking it any time someone called `useKibanaCore`, getting a new object back each time. This both caused some bugs (looping with useEffect) and was not representative of how the actual hook worked. This also moves that invokation into the mock function, along with shaping the mocked module so that we don't have to do it in every call to jest.mock. * WIP: migrating to use kibana_react's provider and helpers We're re-exporting these locally to have more control around mocking them (until platform implements that). This breaks everything that was using the old mocks. Will fix. * WIP: Migrating to use kibana_react Instead of our homegrown hooks we can use these utilities instead. Unfortunately kibana_react doesn't yet have mocks, so we had to implement that ourselves. Luckily, we already had local mocks for the settings service. This migrates to a the new format. For clarity and consistency, we also re-export new platform's mocks here and use them to populate our kibana_react mocks. We started by migrating the UiSettings service to new platform, and let that drive the rest. With the mocks in place for kibana_react, removing the usage of useKibanaCore was a natural step as well. The next step is removing the usage of chrome.getUiSettingsClient with our useUiSetting$ hook, and with that (and maybe some config setup; I'm seeing errors at runtime), we should be ready to start migrating other services. * Bind a copy of kibana at mock creation We were previously returning a new copy any time e.g. useKibana was called, which is not the contract that consumers are expecting. and in fact caused looping with components employing useEffect etc. * Remove internal context providers and last usage We're now using kibana_react fully. * Fix tests failing due to wrong mocks Remaining failures are either due to a date format issue, or something being rendered differently. Those are up next. Still haven't touched use of chrome.getUiSettingsClient, that's after. * Fix test failures related to date formatting * mocks missing UI Setting (DEFAULT_TIMEZONE_BROWSER) which is required by our formatted_date utilities * mock timepicker ranges in the one test that uses it (SuperDatePicker) * Remove unnecessary and/or redundant mocks Since our TestProvider now mocks new platform, the only tests that should need to mock uiSettings related stuff (e.g. timezone preferences) would be the tests that (directly or no) use kibana_react to get it. * Refactor kibana_react mocks * adds a mock for the non-observable useUiSetting * removes the unmockable HOC withKibana * Replace usage of chrome.getUiSettingsClient with useUiSetting We're opting for the non-observable behavior here because I believe that's more analagous. There are a few remaining usages in non-react code. Tests are still using the mocks, those'll be removed next. * Remove ui_settings mocks Now that we're not using this hook there's no need for the mocks. Tests are green. * Remove siem's UI settings hook We're now using the ones provided by kibana_react. * Use withKibana HOC on our component classes React was kind enough to remind me that I can't put hooks in classes. Whoops. * Set defaults for some unknown UI settings The service claims not to know about these settings we're retrieving. Until I can figure out where they should come from, we're going to initialize them with what seem to be the defaults at plugin initialization. * Remove old hooks These have now been replaced with kibana_react's equivalents. * Fix type error on usage of useKibana hook This is one of the few places where we're using another plugin, which are not present in the default typings due to their opt-in nature. * Fix type error on ML call The indexPattern we get back is actually an array. The endpoint seems to handle this just fine (at least, it doesn't blow up), but once we started retrieving a typed value this error surfaced. * Export a 'bound' version of the useKibana function Rather than having to type this on each invocation. This requires us to define which plugins we depend on, which is a good thing. * Instantiate our mock function We aren't using these right now I didn't notice, but that wasn't the right reference. * Fix test that relies on unmocked service Our QueryBar component relies very (very, very) indirectly on a storage service that does not exist in New Platform, nor its corresponding mocks. To get it passing for now, we're just gonna pretend like it's there. * Remove use of ui/chrome in our charts Replaces with hooks that accomplish the same. * Remove last use of chrome.getUiSettingsClient This function is itself a hook, so we should be good here. * Remove unnecessary non-null assertions Now that we're using our typed version of the useKibana hook, typescript knows that these services will be available (once we actually enforce that in our kibana.json, of course). * Fix chart tests These rely on a kibana hook now, so we need to mock it out for these renders lest we blow up when the context isn't there. * Replace missing mock I deleted this in a previous commit, thinking it unneeded. However, getHostDetailsBreadcrumbs ultimately asks for some default date parameters for the timerange boundaries. * Add back tests for our theming hook * Style: cleanup * Remove unneeded default UI Settings values We were previously getting errors due to these values not being known to the client, but it looks like that was either fixed upstream, or a temporary issue caused by some improper context setup. * Simplify kibana_react mocks Let's leave JSX out of it. Co-authored-by: Elastic Machine <[email protected]> * Update references to now-deleted hooks These hooks were deleted on a recent branch, but new usages were merged to master in the meantime. * Fix remaining uses of hooks/chrome that were not merge conflicts * Use HOC on class component Co-authored-by: Elastic Machine <[email protected]>

## Summary * Adds more rules from detection groups ### Checklist Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR. ~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~ ~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~ ~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~ - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios ~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~ ### For maintainers ~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~ - [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

…alert / action (elastic#53333)

* Add kibanamachine support to Github PR comments * Temporary commit for quick successful pipeline * Only delete the last comment if it was made by kibanamachine * Revert "Temporary commit for quick successful pipeline" This reverts commit d31f579.

As we recently added react-use as a dependency, makes sense to clean up those generic hooks from Kibana repo. Removed custom hooks from kibana_react and other places: useObservable useUnmount useShallowCompareEffect react-use should be used instead: import useObservable from 'react-use/lib/useObservable'

…StyleProperty (elastic#53806)

…into bugfix/watcher

cjcenizal · 2020-01-07T17:19:19Z

@elasticmachine merge upstream

jloleysens

Re-tested, happy that my feedback was addressed. Feel free to merge once @alisonelizabeth 's feedback has been addressed 👍

jloleysens · 2020-01-08T10:02:26Z

x-pack/legacy/plugins/watcher/public/np_ready/application/models/action/webhook_action.js

    this.path = get(props, 'path');
    this.username = get(props, 'username');
    this.password = get(props, 'password');
    this.contentType = get(props, 'contentType');

-    this.fullPath = `${this.host}:${this.port}${this.path}`;
+    if (typeof this.path !== 'undefined') {


Fwiw, this is a very narrow check for the value of undefined (so values like null or 0 will pass the if check) which should be fine in this case but something like

this.fullPath = `${this.host}:${this.port}${this.path ? '/' + this.path : ''}`;

would be more robust because it's a default for all falsey values. I don't expect this.path to be anything other than undefined or a String in this case though 👍.

@jloleysens thanks for the advice, I've updated my code and uploaded it

…into bugfix/watcher

…name SCHEME_VERBS to SCHEME, and replaced this.fullPath logic

alisonelizabeth

@jkelastic great job! There is one minor comment that is still unaddressed, but otherwise LGTM.

alisonelizabeth · 2020-01-09T17:56:17Z

@elasticmachine merge upstream

alisonelizabeth · 2020-01-09T19:19:38Z

@elasticmachine merge upstream

alisonelizabeth · 2020-01-10T13:18:09Z

@elasticmachine merge upstream

… Webhook action (#53757) (#54459) Co-authored-by: Jimmy Kuang <[email protected]>

kibanamachine · 2020-01-16T01:22:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3ce5b89

History

💚 Build #19266 succeeded 3ce5b89
💔 Build #19084 failed 6a49dfb
💔 Build #19065 failed 7ce5d3f
💚 Build #19050 succeeded ff853ac
💚 Build #18891 succeeded ec24267

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

jkelastic added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Dec 23, 2019

jkelastic added Feature:Watcher release_note:enhancement v7.6.0 v8.0.0 labels Dec 23, 2019

jkelastic added 3 commits December 27, 2019 15:40

Adding Scheme field to Webhook

27092fe

Remove debugger breakpoint

d602d4d

Remove debugger tag

f3652ac

jkelastic force-pushed the bugfix/watcher branch from 05c907f to f3652ac Compare December 27, 2019 23:40

jloleysens reviewed Dec 30, 2019

View reviewed changes

jkelastic added 2 commits December 30, 2019 09:18

Fix undefined path variable and moved scheme field in front of host f…

8601dd6

…ield

Fixed Elint error

2b0a12b

jkelastic force-pushed the bugfix/watcher branch from 749a39f to 2b0a12b Compare December 30, 2019 17:24

Merge branch 'master' into bugfix/watcher

722b74f

alisonelizabeth reviewed Jan 2, 2020

View reviewed changes

benwtrent and others added 9 commits January 3, 2020 08:16

Add ml inference processor and APIs to console (elastic#53618)

c7843f9

Adds new Machine Learning Inference processor to Kibana Console. Also adds new Inference APIs generated from ES specs.

Use savedObjects provided by the platform instead of legacy shim. (e…

18d3b4c

…lastic#53679)

Enforce shimmed plugin boundaries (elastic#52633)

8cb7a80

Migrate kql telemetry and scripts api (elastic#52651)

7076524

NP Migration: Sample data (elastic#52753)

d8ba2d7

[DOCS] Fixes ML links (elastic#53713)

6dbb46c

brianseeders and others added 8 commits January 3, 2020 08:16

Skip failing test suite

b3a8150

Add tests to ensure AAD isn't broken after performing a change on an …

aca16b2

…alert / action (elastic#53333)

Bump year in NOTICE.txt

86418d9

Fix suggested value for time_zone in range query (elastic#53841)

18be159

[Maps] push mapbox value extraction from VectorStyle and into Dynamic…

47babdc

…StyleProperty (elastic#53806)

d Merge branch 'bugfix/watcher' of https://github.com/jkelastic/kibana …

6889692

…into bugfix/watcher

cjcenizal mentioned this pull request Jan 7, 2020

Pushover Webhook Interaction #53864

Closed

Merge branch 'master' into bugfix/watcher

bd40843

jloleysens approved these changes Jan 8, 2020

View reviewed changes

jloleysens reviewed Jan 8, 2020

View reviewed changes

jkelastic added 3 commits January 8, 2020 08:30

Merge branch 'bugfix/watcher' of https://github.com/jkelastic/kibana …

3b649c2

…into bugfix/watcher

Added default value to scheme in the webhook_action.js model file, re…

bebcb02

…name SCHEME_VERBS to SCHEME, and replaced this.fullPath logic

Removed return to make it prettier/prettier

ec24267

alisonelizabeth approved these changes Jan 9, 2020

View reviewed changes

alisonelizabeth mentioned this pull request Jan 9, 2020

Support scheme field when creating a Threshold alert with a Webhook action #44004

Closed

Corrected camel casing on L73

ff853ac

Merge branch 'master' into bugfix/watcher

7ce5d3f

Merge branch 'master' into bugfix/watcher

6a49dfb

Merge branch 'master' into bugfix/watcher

3ce5b89

alisonelizabeth merged commit 9191261 into elastic:master Jan 10, 2020

alisonelizabeth mentioned this pull request Jan 10, 2020

[7.x] [Watcher] Support scheme field when creating a Threshold alert with a Webhook action (#53757) #54459

Merged

alisonelizabeth added a commit that referenced this pull request Jan 10, 2020

[Watcher] Support scheme field when creating a Threshold alert with a…

4560a79

… Webhook action (#53757) (#54459) Co-authored-by: Jimmy Kuang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support scheme field when creating a Threshold alert with a Webhook action #53757

Support scheme field when creating a Threshold alert with a Webhook action #53757

jkelastic commented Dec 23, 2019 •

edited by cjcenizal

Loading

elasticmachine commented Dec 23, 2019

kibanamachine commented Dec 23, 2019

jkelastic commented Dec 27, 2019

kibanamachine commented Dec 28, 2019

jloleysens left a comment

jkelastic commented Dec 30, 2019

jkelastic commented Jan 2, 2020

alisonelizabeth left a comment

alisonelizabeth Jan 2, 2020

jkelastic Jan 2, 2020

alisonelizabeth Jan 9, 2020

jkelastic Jan 9, 2020

alisonelizabeth Jan 2, 2020

alisonelizabeth Jan 2, 2020

jkelastic Jan 2, 2020

alisonelizabeth Jan 7, 2020

jkelastic Jan 8, 2020

cjcenizal commented Jan 7, 2020

jloleysens left a comment

jloleysens Jan 8, 2020 •

edited

Loading

jkelastic Jan 8, 2020

alisonelizabeth left a comment

alisonelizabeth commented Jan 9, 2020

alisonelizabeth commented Jan 9, 2020

alisonelizabeth commented Jan 10, 2020

kibanamachine commented Jan 16, 2020

		@@ -29,13 +29,15 @@ interface Props {

		const HTTP_VERBS = ['head', 'get', 'post', 'put', 'delete'];

		const SCHEME_VERBS = ['', 'http', 'https'];

Support scheme field when creating a Threshold alert with a Webhook action #53757

Support scheme field when creating a Threshold alert with a Webhook action #53757

Conversation

jkelastic commented Dec 23, 2019 • edited by cjcenizal Loading

Summary

Checklist

For maintainers

elasticmachine commented Dec 23, 2019

kibanamachine commented Dec 23, 2019

💔 Build Failed

jkelastic commented Dec 27, 2019

kibanamachine commented Dec 28, 2019

💚 Build Succeeded

jloleysens left a comment

Choose a reason for hiding this comment

Code

UX

jkelastic commented Dec 30, 2019

jkelastic commented Jan 2, 2020

alisonelizabeth 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jan 7, 2020

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisonelizabeth left a comment

Choose a reason for hiding this comment

alisonelizabeth commented Jan 9, 2020

alisonelizabeth commented Jan 9, 2020

alisonelizabeth commented Jan 10, 2020

kibanamachine commented Jan 16, 2020

💚 Build Succeeded

History

jkelastic commented Dec 23, 2019 •

edited by cjcenizal

Loading

jloleysens Jan 8, 2020 •

edited

Loading