Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

legrego · 2018-08-20T19:25:43Z

This PR introduces a new kibana.yml setting to allow users to set the email address used to receive cluster alert notifications:

xpack.monitoring.cluster_alerts.email_notifications.email_address

If this is not set, then a deprecation warning will be logged warning that it will be required for Kibana 7.0. For the remainder of 6.x, Kibana will fallback to using the xpack:defaultAdminEmail setting from the advanced settings. When Spaces is enabled, then the setting will be retrieved from the Default Space's advanced settings.

Fixes #22146

"Release note: The xpack:defaultAdminEmail UI Setting is deprecated for Monitoring, but is still fully supported for the Watcher UI. Users who wish to receive cluster alert notification emails should configure xpack.monitoring.cluster_alerts.email_notifications.email_address in kibana.yml instead."

elasticmachine · 2018-08-20T21:08:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2018-08-22T12:42:10Z

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.js

+
+let loggedDeprecationWarning = false;
+
+export function resetDeprecationWarning() {


This only exists to enable test cases to reset the module state. I realize how ugly this is, but I was weighing the tradeoff of this vs a larger, more complex solution. I chose this because it is a short-lived feature that we will remove with 7.0

elasticmachine · 2018-08-23T14:08:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2018-08-23T14:14:12Z

@tsullivan @pickypg
GH suggests the two of you as reviewers on this PR. Would one or both of you be available for a review, or do you know someone better suited for this?

tsullivan · 2018-08-27T18:05:02Z

x-pack/plugins/monitoring/server/lib/constants.js

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export const CLUSTER_ALERTS_ADDRESS_CONFIG_KEY = 'cluster_alerts.email_notifications.email_address';


Might be better to put this in x-pack/plugins/monitoring/common/constants.js

elasticmachine · 2018-08-27T20:21:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-08-27T21:43:06Z

x-pack/plugins/monitoring/config.js

@@ -49,7 +49,8 @@ export const config = (Joi) => {
      enabled: boolean().default(true),
      index: string().default('.monitoring-alerts-6'),
      email_notifications: object({
-        enabled: boolean().default(true)
+        enabled: boolean().default(true),
+        email_address: string().email(),


We probably want to keep a similar name as the setting getting deprecated. Maybe default_admin_email

I considered default_admin_email, but to me, that implies that the email address can be overridden somewhere else. If I understand it correctly, Monitoring doesn't allow the email address to be changed for cluster alerts.

The xpack:defaultAdminEmail advanced setting makes sense as a default option when creating new watcher jobs through the UI, because it really is the default setting, which the user can change.

The xpack:defaultAdminEmail advanced setting makes sense as a default option when creating new watcher jobs through the UI, because it really is the default setting, which the user can change.

Interesting, I didn't realize Watcher UI uses this setting!

I'm OK with how you had it.

tsullivan · 2018-08-27T21:57:45Z

I still need to look through in more depth, but one thing I want to test is that https://github.com/elastic/kibana/pull/22220/files isn't affected. I think this will need to get the master branch merged in

elasticmachine · 2018-08-28T15:33:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-08-28T21:15:15Z

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.js

+  // DEPRECATED (Remove below in 7.0): If an email address is not configured in kibana.yml, then fallback to xpack:defaultAdminEmail
+  if (!loggedDeprecationWarning) {
+    const message = (
+      `Monitoring is using ${XPACK_DEFAULT_ADMIN_EMAIL_UI_SETTING} for cluster alert notifications,` +


we need a space at end end of this line's string, so notifications, which has a space between

tsullivan · 2018-08-28T21:36:30Z

x-pack/plugins/monitoring/deprecations.js

+      const clusterAlertsEnabled = get(settings, 'cluster_alerts.enabled');
+      const emailNotificationsEnabled = clusterAlertsEnabled && get(settings, 'cluster_alerts.email_notifications.enabled');
+      if (emailNotificationsEnabled && !get(settings, CLUSTER_ALERTS_ADDRESS_CONFIG_KEY)) {
+        log(`Config key "${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}" will be required for email notifications to work in 7.0."`);


I'm running this PR branch and trying to get this log to show up, but I'm not seeing it. I see undefined for clusterAlertsEnabled, emailNotificationsEnabled. I have a trial license and cluster alerts are enabled

I discovered while putting this PR together that the depreciation system only passes through the settings that are explicitly defined in your kibana.yml. Settings that are defaulted via the plugin schema are not provided.

To say another way, you'll see this warning if and only if you define the following in your kibana.yml:

xpack.monitoring.cluster_alerts.enabled: true xpack.monitoring.cluster_alerts.email_notifications.enabled: true

This is what led me to implement the other deprecation warning, so that it would be more obvious/helpful to ordinary installations that don't redefine default configuration settings.

Good to know, thanks!

tsullivan

The comma thing should be fixed. It looks like the deprecation log does too, but that might be an external issue

elasticmachine · 2018-08-28T23:20:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2018-08-28T23:26:17Z

Seemingly unrelated viz test failure.
Jenkins, test this please

elasticmachine · 2018-08-29T01:09:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2018-09-04T12:02:12Z

@tsullivan do you have any other feedback for this PR? If not, are you available to give it a LGTM?

tsullivan

LGTM

elasticmachine · 2018-09-04T19:43:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

#22765) Backports the following commits to 6.x: - Deprecate xpack:defaultAdminEmail for monitoring alerts (#22195)

deprecate xpack:defaultAdminEmail for monitoring alerts

54c581f

legrego added the WIP Work in progress label Aug 20, 2018

add deprecation warning the first time xpack:defaultAdminEmail is used

3b0f486

legrego commented Aug 22, 2018

View reviewed changes

Merge branch 'master' into deprecate-admin-email-for-monitoring

c771602

legrego added v6.5.0 v7.0.0 Team:Monitoring Stack Monitoring team labels Aug 22, 2018

Merge branch 'master' into deprecate-admin-email-for-monitoring

2605f2f

elastic deleted a comment from elasticmachine Aug 22, 2018

legrego changed the title ~~[WIP] Deprecate xpack:defaultAdminEmail for monitoring alerts~~ Deprecate xpack:defaultAdminEmail for monitoring alerts Aug 22, 2018

Merge branch 'master' into deprecate-admin-email-for-monitoring

a8b186d

elastic deleted a comment from elasticmachine Aug 23, 2018

legrego mentioned this pull request Aug 23, 2018

[Spaces] Enable Per-Space Advanced Settings #19869

Closed

legrego added review and removed WIP Work in progress labels Aug 23, 2018

legrego requested a review from pickypg August 27, 2018 14:22

tsullivan self-requested a review August 27, 2018 18:03

tsullivan reviewed Aug 27, 2018

View reviewed changes

consolidate constants

5eacfe1

tsullivan reviewed Aug 27, 2018

View reviewed changes

Merge branch 'master' into deprecate-admin-email-for-monitoring

ca5a8a1

tsullivan reviewed Aug 28, 2018

View reviewed changes

add missing space to deprecation warning

bbbc344

tsullivan approved these changes Sep 4, 2018

View reviewed changes

legrego removed the request for review from pickypg September 4, 2018 16:19

update cluster alerts documentation

820d2c5

legrego added release_note:deprecation and removed review labels Sep 6, 2018

legrego merged commit 5f96c90 into elastic:master Sep 6, 2018

legrego mentioned this pull request Sep 6, 2018

[6.x] Deprecate xpack:defaultAdminEmail for monitoring alerts (#22195) #22765

Merged

legrego added a commit to legrego/kibana that referenced this pull request Sep 6, 2018

Deprecate xpack:defaultAdminEmail for monitoring alerts (elastic#22195)

fa4ef9c

legrego added a commit that referenced this pull request Sep 6, 2018

[6.x] Deprecate xpack:defaultAdminEmail for monitoring alerts (#22195) (

263c916

#22765) Backports the following commits to 6.x: - Deprecate xpack:defaultAdminEmail for monitoring alerts (#22195)

LeeDr mentioned this pull request Nov 8, 2018

xPack:defaultAdminEmail should be marked deprecated in the UI #25439

Closed

neptunian mentioned this pull request Mar 3, 2022

[DOCS] Adds missing monitoring.cluster_alerts.email_notifications.email_address monitoring setting #126826

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

legrego commented Aug 20, 2018 •

edited

Loading

elasticmachine commented Aug 20, 2018

legrego Aug 22, 2018

elasticmachine commented Aug 23, 2018

legrego commented Aug 23, 2018

tsullivan Aug 27, 2018

elasticmachine commented Aug 27, 2018

tsullivan Aug 27, 2018

legrego Aug 28, 2018

tsullivan Aug 28, 2018

tsullivan commented Aug 27, 2018

elasticmachine commented Aug 28, 2018

tsullivan Aug 28, 2018

tsullivan Aug 28, 2018

legrego Aug 28, 2018

tsullivan Aug 28, 2018

tsullivan left a comment

elasticmachine commented Aug 28, 2018

legrego commented Aug 28, 2018

elasticmachine commented Aug 29, 2018

legrego commented Sep 4, 2018

tsullivan left a comment

elasticmachine commented Sep 4, 2018


		let loggedDeprecationWarning = false;

		export function resetDeprecationWarning() {

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

Conversation

legrego commented Aug 20, 2018 • edited Loading

elasticmachine commented Aug 20, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Aug 23, 2018

💚 Build Succeeded

legrego commented Aug 23, 2018

Choose a reason for hiding this comment

elasticmachine commented Aug 27, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Aug 27, 2018

elasticmachine commented Aug 28, 2018

💚 Build Succeeded

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

tsullivan left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 28, 2018

💔 Build Failed

legrego commented Aug 28, 2018

elasticmachine commented Aug 29, 2018

💚 Build Succeeded

legrego commented Sep 4, 2018

tsullivan left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 4, 2018

💚 Build Succeeded

legrego commented Aug 20, 2018 •

edited

Loading