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.

Sign up for GitHub

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

Jump to bottom

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

Merged
merged 9 commits into from
Sep 6, 2018
Merged

Deprecate xpack:defaultAdminEmail for monitoring alerts #22195

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
54c581f
deprecate xpack:defaultAdminEmail for monitoring alerts
legrego Aug 20, 2018
3b0f486
add deprecation warning the first time xpack:defaultAdminEmail is used
legrego Aug 22, 2018
c771602
Merge branch 'master' into deprecate-admin-email-for-monitoring
legrego Aug 22, 2018
2605f2f
Merge branch 'master' into deprecate-admin-email-for-monitoring
legrego Aug 22, 2018
a8b186d
Merge branch 'master' into deprecate-admin-email-for-monitoring
legrego Aug 23, 2018
5eacfe1
consolidate constants
legrego Aug 27, 2018
ca5a8a1
Merge branch 'master' into deprecate-admin-email-for-monitoring
legrego Aug 28, 2018
bbbc344
add missing space to deprecation warning
legrego Aug 28, 2018
820d2c5
update cluster alerts documentation
legrego Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions x-pack/plugins/monitoring/__tests__/deprecations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,66 @@ describe('monitoring plugin deprecations', function () {
expect(log.called).to.be(false);
});

describe('cluster_alerts.email_notifications.email_address', function () {
it(`shouldn't log when email notifications are disabled`, function () {
const settings = {
cluster_alerts: {
email_notifications: {
enabled: false
}
}
};

const log = sinon.spy();
transformDeprecations(settings, log);
expect(log.called).to.be(false);
});

it(`shouldn't log when cluster alerts are disabled`, function () {
const settings = {
cluster_alerts: {
enabled: false,
email_notifications: {
enabled: true
}
}
};

const log = sinon.spy();
transformDeprecations(settings, log);
expect(log.called).to.be(false);
});

it(`shouldn't log when email_address is specified`, function () {
const settings = {
cluster_alerts: {
enabled: true,
email_notifications: {
enabled: true,
email_address: 'foo@bar.com'
}
}
};

const log = sinon.spy();
transformDeprecations(settings, log);
expect(log.called).to.be(false);
});

it(`should log when email_address is missing, but alerts/notifications are both enabled`, function () {
const settings = {
cluster_alerts: {
enabled: true,
email_notifications: {
enabled: true
}
}
};

const log = sinon.spy();
transformDeprecations(settings, log);
expect(log.called).to.be(true);
});
});

});
5 changes: 3 additions & 2 deletions x-pack/plugins/monitoring/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { XPACK_INFO_API_DEFAULT_POLL_FREQUENCY_IN_MILLIS } from '../../server/li
*/
export const config = (Joi) => {
const { array, boolean, number, object, string } = Joi;
const DEFAULT_REQUEST_HEADERS = [ 'authorization' ];
const DEFAULT_REQUEST_HEADERS = ['authorization'];

return object({
ccs: object({
Expand Down Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}).default()
}).default(),
xpack_api_polling_frequency_millis: number().default(XPACK_INFO_API_DEFAULT_POLL_FREQUENCY_IN_MILLIS),
Expand Down
12 changes: 10 additions & 2 deletions x-pack/plugins/monitoring/deprecations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { get, has, set } from 'lodash';
import { CLUSTER_ALERTS_ADDRESS_CONFIG_KEY } from './server/lib/constants';

/**
* Re-writes deprecated user-defined config settings and logs warnings as a
Expand All @@ -29,12 +30,19 @@ export const deprecations = ({ rename }) => {
delete settings.elasticsearch.ssl.verify;

log('Config key "xpack.monitoring.elasticsearch.ssl.verify" is deprecated. ' +
'It has been replaced with "xpack.monitoring.elasticsearch.ssl.verificationMode"');
'It has been replaced with "xpack.monitoring.elasticsearch.ssl.verificationMode"');
},
(settings, log) => {
if (has(settings, 'report_stats')) {
log('Config key "xpack.monitoring.report_stats" is deprecated and will be removed in 7.0. ' +
'Use "xpack.xpack_main.telemetry.enabled" instead.');
'Use "xpack.xpack_main.telemetry.enabled" instead.');
}
},
(settings, log) => {
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."`);
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 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know, thanks!

}
},
];
Expand Down
12 changes: 8 additions & 4 deletions ...plugins/monitoring/server/kibana_monitoring/collectors/__tests__/check_for_email_value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,31 @@ import expect from 'expect.js';
import { checkForEmailValue } from '../get_settings_collector';

describe('getSettingsCollector / checkForEmailValue', () => {
const mockLogger = {
warn: () => { }
};

it('ignores shouldUseNull=true value and returns email if email value if one is set', async () => {
const shouldUseNull = true;
const getDefaultAdminEmailMock = () => 'test@elastic.co';
expect(await checkForEmailValue(undefined, undefined, shouldUseNull, getDefaultAdminEmailMock)).to.be('test@elastic.co');
expect(await checkForEmailValue(undefined, undefined, mockLogger, shouldUseNull, getDefaultAdminEmailMock)).to.be('test@elastic.co');
});

it('ignores shouldUseNull=false value and returns email if email value if one is set', async () => {
const shouldUseNull = false;
const getDefaultAdminEmailMock = () => 'test@elastic.co';
expect(await checkForEmailValue(undefined, undefined, shouldUseNull, getDefaultAdminEmailMock)).to.be('test@elastic.co');
expect(await checkForEmailValue(undefined, undefined, mockLogger, shouldUseNull, getDefaultAdminEmailMock)).to.be('test@elastic.co');
});

it('returns a null if no email value is set and null is allowed', async () => {
const shouldUseNull = true;
const getDefaultAdminEmailMock = () => null;
expect(await checkForEmailValue(undefined, undefined, shouldUseNull, getDefaultAdminEmailMock)).to.be(null);
expect(await checkForEmailValue(undefined, undefined, mockLogger, shouldUseNull, getDefaultAdminEmailMock)).to.be(null);
});

it('returns undefined if no email value is set and null is not allowed', async () => {
const shouldUseNull = false;
const getDefaultAdminEmailMock = () => null;
expect(await checkForEmailValue(undefined, undefined, shouldUseNull, getDefaultAdminEmailMock)).to.be(undefined);
expect(await checkForEmailValue(undefined, undefined, mockLogger, shouldUseNull, getDefaultAdminEmailMock)).to.be(undefined);
});
});
151 changes: 124 additions & 27 deletions ...ugins/monitoring/server/kibana_monitoring/collectors/__tests__/get_default_admin_email.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,23 @@ import sinon from 'sinon';
import { set } from 'lodash';

import { XPACK_DEFAULT_ADMIN_EMAIL_UI_SETTING } from '../../../../../../server/lib/constants';
import { getDefaultAdminEmail } from '../get_settings_collector';
import { getDefaultAdminEmail, resetDeprecationWarning } from '../get_settings_collector';
import { CLUSTER_ALERTS_ADDRESS_CONFIG_KEY } from '../../../lib/constants';

describe('getSettingsCollector / getDefaultAdminEmail', () => {
function setup({ enabled = true, docExists = true, adminEmail = 'admin@email.com' }) {
function setup({ enabled = true, docExists = true, defaultAdminEmail = 'default-admin@email.com', adminEmail = null }) {
const config = { get: sinon.stub() };

config.get
.withArgs('xpack.monitoring.cluster_alerts.email_notifications.enabled')
.returns(enabled);

if (adminEmail) {
config.get
.withArgs(`xpack.monitoring.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}`)
.returns(adminEmail);
}

config.get
.withArgs('kibana.index')
.returns('.kibana');
Expand All @@ -29,8 +36,8 @@ describe('getSettingsCollector / getDefaultAdminEmail', () => {

const doc = {};
if (docExists) {
if (adminEmail) {
set(doc, ['_source', 'config', XPACK_DEFAULT_ADMIN_EMAIL_UI_SETTING], adminEmail);
if (defaultAdminEmail) {
set(doc, ['_source', 'config', XPACK_DEFAULT_ADMIN_EMAIL_UI_SETTING], defaultAdminEmail);
} else {
set(doc, '_source.config', {});
}
Expand All @@ -46,41 +53,131 @@ describe('getSettingsCollector / getDefaultAdminEmail', () => {
}))
.returns(doc);

const log = {
warn: sinon.stub()
};

return {
config,
callCluster
callCluster,
log,
};
}

describe('xpack.monitoring.cluster_alerts.email_notifications.enabled = false', () => {
it('returns null', async () => {
const { config, callCluster } = setup({ enabled: false });
expect(await getDefaultAdminEmail(config, callCluster)).to.be(null);
sinon.assert.notCalled(callCluster);
describe('using xpack:defaultAdminEmail', () => {
beforeEach(() => {
resetDeprecationWarning();
});
});

describe('doc does not exist', () => {
it('returns null', async () => {
const { config, callCluster } = setup({ docExists: false });
expect(await getDefaultAdminEmail(config, callCluster)).to.be(null);
sinon.assert.calledOnce(callCluster);
describe('xpack.monitoring.cluster_alerts.email_notifications.enabled = false', () => {

it('returns null', async () => {
const { config, callCluster, log } = setup({ enabled: false });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be(null);
sinon.assert.notCalled(callCluster);
});

it('does not log a deprecation warning', async () => {
const { config, callCluster, log } = setup({ enabled: false });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.notCalled(log.warn);
});
});
});

describe('value is not defined', () => {
it('returns null', async () => {
const { config, callCluster } = setup({ adminEmail: false });
expect(await getDefaultAdminEmail(config, callCluster)).to.be(null);
sinon.assert.calledOnce(callCluster);
describe('doc does not exist', () => {
it('returns null', async () => {
const { config, callCluster, log } = setup({ docExists: false });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be(null);
sinon.assert.calledOnce(callCluster);
});

it('logs a deprecation warning', async () => {
const { config, callCluster, log } = setup({ docExists: false });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.calledOnce(log.warn);
});
});

describe('value is not defined', () => {
it('returns null', async () => {
const { config, callCluster, log } = setup({ defaultAdminEmail: false });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be(null);
sinon.assert.calledOnce(callCluster);
});

it('logs a deprecation warning', async () => {
const { config, callCluster, log } = setup({ defaultAdminEmail: false });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.calledOnce(log.warn);
});
});

describe('value is defined', () => {
it('returns value', async () => {
const { config, callCluster, log } = setup({ defaultAdminEmail: 'hello@world' });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be('hello@world');
sinon.assert.calledOnce(callCluster);
});

it('logs a deprecation warning', async () => {
const { config, callCluster, log } = setup({ defaultAdminEmail: 'hello@world' });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.calledOnce(log.warn);
});
});
});

describe('value is defined', () => {
it('returns value', async () => {
const { config, callCluster } = setup({ adminEmail: 'hello@world' });
expect(await getDefaultAdminEmail(config, callCluster)).to.be('hello@world');
sinon.assert.calledOnce(callCluster);
describe('using xpack.monitoring.cluster_alerts.email_notifications.email_address', () => {
beforeEach(() => {
resetDeprecationWarning();
});

describe('xpack.monitoring.cluster_alerts.email_notifications.enabled = false', () => {
it('returns null', async () => {
const { config, callCluster, log } = setup({ enabled: false });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be(null);
sinon.assert.notCalled(callCluster);
});

it('does not log a deprecation warning', async () => {
const { config, callCluster, log } = setup({ enabled: false });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.notCalled(log.warn);
});
});

describe('value is not defined', () => {
it('returns value from xpack:defaultAdminEmail', async () => {
const { config, callCluster, log } = setup({
defaultAdminEmail: 'default-admin@email.com',
adminEmail: false
});
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be('default-admin@email.com');
sinon.assert.calledOnce(callCluster);
});

it('logs a deprecation warning', async () => {
const { config, callCluster, log } = setup({
defaultAdminEmail: 'default-admin@email.com',
adminEmail: false
});
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.calledOnce(log.warn);
});
});

describe('value is defined', () => {
it('returns value', async () => {
const { config, callCluster, log } = setup({ adminEmail: 'hello@world' });
expect(await getDefaultAdminEmail(config, callCluster, log)).to.be('hello@world');
sinon.assert.notCalled(callCluster);
});

it('does not log a deprecation warning', async () => {
const { config, callCluster, log } = setup({ adminEmail: 'hello@world' });
await getDefaultAdminEmail(config, callCluster, log);
sinon.assert.notCalled(log.warn);
});
});
});
});
Loading