From 5b943d00e2cff77d64156fd7b4e1a1fe7149bf83 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 15 Nov 2018 21:53:55 +0200 Subject: [PATCH 1/8] WIP --- .../notification/NotificationService.java | 135 ++++++++++++++++-- .../notification/slack/SlackService.java | 6 +- 2 files changed, 122 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index 0b545c7942821..b011b645a0805 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -5,18 +5,27 @@ */ package org.elasticsearch.xpack.watcher.notification; +import org.elasticsearch.common.Nullable; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.SecureSetting; +import org.elasticsearch.common.settings.SecureSettings; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; +import java.io.IOException; +import java.io.InputStream; +import java.security.GeneralSecurityException; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.BiFunction; +import java.util.stream.Collectors; /** * Basic notification service @@ -24,14 +33,30 @@ public abstract class NotificationService extends AbstractComponent { private final String type; - // both are guarded by this - private Map accounts; - private Account defaultAccount; - - public NotificationService(String type, - ClusterSettings clusterSettings, List> pluginSettings) { - this(type); - clusterSettings.addSettingsUpdateConsumer(this::reload, pluginSettings); + // all are guarded by this + private volatile Map accounts; + private volatile Account defaultAccount; + private volatile Settings cachedClusterSettings = null; + private volatile SecureSettings cachedSecureSettings = null; + + public NotificationService(String type, ClusterSettings clusterSettings, List> pluginSettings) { + this.type = type; + clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginSettings); + } + + private synchronized void clusterSettingsConsumer(Settings settings) { + // update cached settings + this.cachedClusterSettings = settings; + final Set accountNames = getAccountNames(settings); + // build new settings from the previously cached secure settings + final Settings.Builder completeSettingsBuilder = Settings.builder().put(settings, false); + if (cachedSecureSettings != null) { + completeSettingsBuilder.setSecureSettings(cachedSecureSettings); + } + final Settings completeSettings = completeSettingsBuilder.build(); + // create new accounts from the latest + this.accounts = createAccounts(completeSettings, accountNames, this::createAccount); + this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); } // Used for testing only @@ -40,8 +65,8 @@ public NotificationService(String type, } public synchronized void reload(Settings settings) { - Tuple, Account> accounts = buildAccounts(settings, this::createAccount); - this.accounts = Collections.unmodifiableMap(accounts.v1()); + Tuple, Account> accounts = buildAccounts(settings, type, this::createAccount); + this.accounts = accounts.v1(); this.defaultAccount = accounts.v2(); } @@ -67,15 +92,55 @@ public Account getAccount(String name) { return theAccount; } - private Tuple, A> buildAccounts(Settings settings, BiFunction accountFactory) { - Settings accountsSettings = settings.getByPrefix("xpack.notification." + type + ".").getAsSettings("account"); + private String getNotificationsAccountPrefix() { + return "xpack.notification." + type + ".account."; + } + + private Set getAccountNames(Settings settings) { + return settings.getByPrefix(getNotificationsAccountPrefix()).names(); + } + + private @Nullable String getDefaultAccountName(Settings settings) { + return settings.get("xpack.notification." + type + ".default_account"); + } + + private Map createAccounts(Settings settings, Set accountNames, + BiFunction accountFactory) { + final Map accounts = new HashMap<>(); + for (final String accountName : accountNames) { + final Settings accountSettings = settings.getAsSettings(getNotificationsAccountPrefix() + accountName); + final Account account = accountFactory.apply(accountName, accountSettings); + accounts.put(accountName, account); + } + return Collections.unmodifiableMap(accounts); + } + + private @Nullable Account selectDefaultAccount(Settings settings, Map accounts) { + final String defaultAccountName = getDefaultAccountName(settings); + if (defaultAccountName == null) { + if (accounts.isEmpty()) { + return null; + } else { + return accounts.values().iterator().next(); + } + } else { + final Account account = accounts.get(defaultAccountName); + if (account == null) { + throw new SettingsException("could not find default account [" + defaultAccountName + "]"); + } + return account; + } + } + + private static Tuple, A> buildAccounts(Settings settings, String type, + BiFunction accountFactory) { + Settings accountsSettings = settings.getByPrefix("xpack.notification." + type + ".account."); Map accounts = new HashMap<>(); for (String name : accountsSettings.names()) { Settings accountSettings = accountsSettings.getAsSettings(name); A account = accountFactory.apply(name, accountSettings); accounts.put(name, account); } - final String defaultAccountName = settings.get("xpack.notification." + type + ".default_account"); A defaultAccount; if (defaultAccountName == null) { @@ -84,7 +149,6 @@ private Tuple, A> buildAccounts(Settings settings, BiFunction } else { A account = accounts.values().iterator().next(); defaultAccount = account; - } } else { defaultAccount = accounts.get(defaultAccountName); @@ -92,6 +156,47 @@ private Tuple, A> buildAccounts(Settings settings, BiFunction throw new SettingsException("could not find default account [" + defaultAccountName + "]"); } } - return new Tuple<>(accounts, defaultAccount); + return new Tuple<>(Collections.unmodifiableMap(accounts), defaultAccount); + } + + private SecureSettings cacheSecureSettings(Settings source, List> secureSettingsList) { + for (final SecureSetting secureSetting : secureSettingsList) { + secureSetting.getA + } + source.get("a"); + source.filter(settingKey -> { + for (final SecureSetting secureSetting : secureSettingsList) { + if (secureSetting.match(settingKey)) { + return true; + } + } + return false; + }); + return new SecureSettings() { + + @Override + public boolean isLoaded() { + return true; + } + + @Override + public SecureString getString(String setting) throws GeneralSecurityException { + return null; + } + + @Override + public Set getSettingNames() { + return null; + } + + @Override + public InputStream getFile(String setting) throws GeneralSecurityException { + return null; + } + + @Override + public void close() throws IOException { + } + }; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java index 888da55430a8b..802155359ca87 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java @@ -30,8 +30,7 @@ public class SlackService extends NotificationService { (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered, Property.Deprecated)); private static final Setting.AffixSetting SETTING_URL_SECURE = - Setting.affixKeySetting("xpack.notification.slack.account.", "secure_url", - (key) -> SecureSetting.secureString(key, null)); + Setting.affixKeySetting("xpack.notification.slack.account.", "secure_url", (key) -> SecureSetting.secureString(key, null)); private static final Setting.AffixSetting SETTING_DEFAULTS = Setting.affixKeySetting("xpack.notification.slack.account.", "message_defaults", @@ -40,11 +39,10 @@ public class SlackService extends NotificationService { private final HttpClient httpClient; public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("slack", clusterSettings, SlackService.getSettings()); + super("slack", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_URL, SETTING_DEFAULTS)); this.httpClient = httpClient; clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_URL, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_URL_SECURE, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {}); reload(settings); } From fb887a1339f16da646843b3f6d2cdb3b9303fe8f Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 16 Nov 2018 01:06:53 +0200 Subject: [PATCH 2/8] Done! --- .../notification/NotificationService.java | 100 ++++++++---------- .../notification/email/EmailService.java | 7 +- .../notification/hipchat/HipChatService.java | 4 +- .../notification/jira/JiraService.java | 6 +- .../pagerduty/PagerDutyService.java | 3 +- 5 files changed, 53 insertions(+), 67 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index b011b645a0805..cbae50589dac5 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -6,10 +6,8 @@ package org.elasticsearch.xpack.watcher.notification; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.ClusterSettings; -import org.elasticsearch.common.settings.SecureSetting; import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; @@ -25,7 +23,6 @@ import java.util.Map; import java.util.Set; import java.util.function.BiFunction; -import java.util.stream.Collectors; /** * Basic notification service @@ -36,18 +33,21 @@ public abstract class NotificationService extends AbstractComponent { // all are guarded by this private volatile Map accounts; private volatile Account defaultAccount; - private volatile Settings cachedClusterSettings = null; - private volatile SecureSettings cachedSecureSettings = null; - - public NotificationService(String type, ClusterSettings clusterSettings, List> pluginSettings) { + // cached cluster setting, required when recreating the notification clients + // using the new "reloaded" secure settings + private volatile Settings cachedClusterSettings; + // cached secure settings, required when recreating the notification clients + // using the new updated cluster settings + private volatile SecureSettings cachedSecureSettings; + + public NotificationService(String type, ClusterSettings clusterSettings, List> pluginClusterSettings) { this.type = type; - clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginSettings); + clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginClusterSettings); } private synchronized void clusterSettingsConsumer(Settings settings) { - // update cached settings + // update cached cluster settings this.cachedClusterSettings = settings; - final Set accountNames = getAccountNames(settings); // build new settings from the previously cached secure settings final Settings.Builder completeSettingsBuilder = Settings.builder().put(settings, false); if (cachedSecureSettings != null) { @@ -55,6 +55,7 @@ private synchronized void clusterSettingsConsumer(Settings settings) { } final Settings completeSettings = completeSettingsBuilder.build(); // create new accounts from the latest + final Set accountNames = getAccountNames(completeSettings); this.accounts = createAccounts(completeSettings, accountNames, this::createAccount); this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); } @@ -65,13 +66,28 @@ private synchronized void clusterSettingsConsumer(Settings settings) { } public synchronized void reload(Settings settings) { - Tuple, Account> accounts = buildAccounts(settings, type, this::createAccount); - this.accounts = accounts.v1(); - this.defaultAccount = accounts.v2(); + // `SecureSettings` are available here! + // cache the `SecureSettings` + try { + this.cachedSecureSettings = cacheSecureSettings(settings); + } catch (GeneralSecurityException e) { + logger.error("Keystore exception while reloading watcher notification service", e); + return; + } + // build new settings from the previously cached cluster settings + final Settings.Builder completeSettingsBuilder = Settings.builder().put(settings, true); + if (cachedClusterSettings != null) { + completeSettingsBuilder.put(cachedClusterSettings); + } + final Settings completeSettings = completeSettingsBuilder.build(); + // create new accounts from the latest + final Set accountNames = getAccountNames(completeSettings); + this.accounts = createAccounts(completeSettings, accountNames, this::createAccount); + this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); } protected abstract Account createAccount(String name, Settings accountSettings); - + public Account getAccount(String name) { // note this is not final since we mock it in tests and that causes // trouble since final methods can't be mocked... @@ -97,7 +113,9 @@ private String getNotificationsAccountPrefix() { } private Set getAccountNames(Settings settings) { - return settings.getByPrefix(getNotificationsAccountPrefix()).names(); + // secure settings don't account for the client names + final Settings noSecureSettings = Settings.builder().put(settings, false).build(); + return noSecureSettings.getByPrefix(getNotificationsAccountPrefix()).names(); } private @Nullable String getDefaultAccountName(Settings settings) { @@ -132,46 +150,14 @@ private Map createAccounts(Settings settings, Set accou } } - private static Tuple, A> buildAccounts(Settings settings, String type, - BiFunction accountFactory) { - Settings accountsSettings = settings.getByPrefix("xpack.notification." + type + ".account."); - Map accounts = new HashMap<>(); - for (String name : accountsSettings.names()) { - Settings accountSettings = accountsSettings.getAsSettings(name); - A account = accountFactory.apply(name, accountSettings); - accounts.put(name, account); + private SecureSettings cacheSecureSettings(Settings source) throws GeneralSecurityException { + // get the secure settings out + final SecureSettings sourceSecureSettings = Settings.builder().put(source, true).getSecureSettings(); + // cache them... + final Map cache = new HashMap<>(); + for (final String settingKey : sourceSecureSettings.getSettingNames()) { + cache.put(settingKey, sourceSecureSettings.getString(settingKey)); } - final String defaultAccountName = settings.get("xpack.notification." + type + ".default_account"); - A defaultAccount; - if (defaultAccountName == null) { - if (accounts.isEmpty()) { - defaultAccount = null; - } else { - A account = accounts.values().iterator().next(); - defaultAccount = account; - } - } else { - defaultAccount = accounts.get(defaultAccountName); - if (defaultAccount == null) { - throw new SettingsException("could not find default account [" + defaultAccountName + "]"); - } - } - return new Tuple<>(Collections.unmodifiableMap(accounts), defaultAccount); - } - - private SecureSettings cacheSecureSettings(Settings source, List> secureSettingsList) { - for (final SecureSetting secureSetting : secureSettingsList) { - secureSetting.getA - } - source.get("a"); - source.filter(settingKey -> { - for (final SecureSetting secureSetting : secureSettingsList) { - if (secureSetting.match(settingKey)) { - return true; - } - } - return false; - }); return new SecureSettings() { @Override @@ -181,17 +167,17 @@ public boolean isLoaded() { @Override public SecureString getString(String setting) throws GeneralSecurityException { - return null; + return cache.get(setting); } @Override public Set getSettingNames() { - return null; + return cache.keySet(); } @Override public InputStream getFile(String setting) throws GeneralSecurityException { - return null; + throw new IllegalStateException("A NotificationService setting cannot be File."); } @Override diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java index 70922e57bd078..d091083613411 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java @@ -104,7 +104,11 @@ public class EmailService extends NotificationService { private final CryptoService cryptoService; public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { - super("email", clusterSettings, EmailService.getSettings()); + super("email", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, + SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_STARTTLS_REQUIRED, SETTING_SMTP_HOST, SETTING_SMTP_PORT, SETTING_SMTP_USER, + SETTING_SMTP_PASSWORD, SETTING_SMTP_TIMEOUT, SETTING_SMTP_CONNECTION_TIMEOUT, SETTING_SMTP_WRITE_TIMEOUT, + SETTING_SMTP_SSL_TRUST_ADDRESS, SETTING_SMTP_LOCAL_ADDRESS, SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, + SETTING_SMTP_WAIT_ON_QUIT)); this.cryptoService = cryptoService; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -117,7 +121,6 @@ public EmailService(Settings settings, @Nullable CryptoService cryptoService, Cl clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_PORT, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_USER, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_PASSWORD, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_PASSWORD, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_TIMEOUT, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_CONNECTION_TIMEOUT, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_WRITE_TIMEOUT, (s, o) -> {}, (s, o) -> {}); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java index 58840aec9775b..fa15d98b6e2a9 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java @@ -65,14 +65,14 @@ public class HipChatService extends NotificationService { private HipChatServer defaultServer; public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("hipchat", clusterSettings, HipChatService.getSettings()); + super("hipchat", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, + SETTING_AUTH_TOKEN, SETTING_PROFILE, SETTING_ROOM, SETTING_HOST, SETTING_PORT, SETTING_MESSAGE_DEFAULTS)); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_HOST, (s) -> {}); clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_PORT, (s) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_AUTH_TOKEN, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_AUTH_TOKEN_SECURE, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_PROFILE, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_ROOM, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_HOST, (s, o) -> {}, (s, o) -> {}); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java index d7b7fe2003b95..4d2a2ad6f39c5 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java @@ -62,7 +62,8 @@ public class JiraService extends NotificationService { private final HttpClient httpClient; public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("jira", clusterSettings, JiraService.getSettings()); + super("jira", clusterSettings, + Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS)); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -70,9 +71,6 @@ public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clu clusterSettings.addAffixUpdateConsumer(SETTING_URL, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_USER, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_PASSWORD, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_USER, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_URL, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_PASSWORD, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {}); // do an initial load reload(settings); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java index 6834fcd4e2e1a..996fb92bc356e 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java @@ -40,11 +40,10 @@ public class PagerDutyService extends NotificationService { private final HttpClient httpClient; public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("pagerduty", clusterSettings, PagerDutyService.getSettings()); + super("pagerduty", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_SERVICE_API_KEY, SETTING_DEFAULTS)); this.httpClient = httpClient; clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {}); - clusterSettings.addAffixUpdateConsumer(SETTING_SECURE_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {}); reload(settings); } From 47b378ab594702d60675995933eb82cfc86c8afa Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 16 Nov 2018 01:07:56 +0200 Subject: [PATCH 3/8] trims --- .../xpack/watcher/notification/NotificationService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index cbae50589dac5..0494a1f1b9318 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -37,7 +37,7 @@ public abstract class NotificationService extends AbstractComponent { // using the new "reloaded" secure settings private volatile Settings cachedClusterSettings; // cached secure settings, required when recreating the notification clients - // using the new updated cluster settings + // using the new updated cluster settings private volatile SecureSettings cachedSecureSettings; public NotificationService(String type, ClusterSettings clusterSettings, List> pluginClusterSettings) { @@ -87,7 +87,7 @@ public synchronized void reload(Settings settings) { } protected abstract Account createAccount(String name, Settings accountSettings); - + public Account getAccount(String name) { // note this is not final since we mock it in tests and that causes // trouble since final methods can't be mocked... From d6507d4b6b3a542fc16ca9ed9c61a602f5f63d66 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Wed, 28 Nov 2018 17:16:58 +0200 Subject: [PATCH 4/8] Address review --- .../notification/NotificationService.java | 79 ++++++++++++------- .../notification/email/EmailService.java | 19 +++-- .../notification/hipchat/HipChatService.java | 16 ++-- .../notification/jira/JiraService.java | 15 +++- .../pagerduty/PagerDutyService.java | 13 ++- .../notification/slack/SlackService.java | 13 ++- 6 files changed, 106 insertions(+), 49 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index 5969d1e9a151b..576d6bf427f22 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.xpack.watcher.notification; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.SecureSettings; @@ -29,6 +31,8 @@ public abstract class NotificationService { private final String type; + private final Logger logger; + private final Settings bootSettings; // all are guarded by this private volatile Map accounts; private volatile Account defaultAccount; @@ -39,50 +43,57 @@ public abstract class NotificationService { // using the new updated cluster settings private volatile SecureSettings cachedSecureSettings; - public NotificationService(String type, ClusterSettings clusterSettings, List> pluginClusterSettings) { + public NotificationService(String type, Settings settings, ClusterSettings clusterSettings, List> pluginClusterSettings) { this.type = type; + this.logger = LogManager.getLogger(); + this.bootSettings = settings; + // register a grand updater for the whole group, as settings are usable together clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginClusterSettings); } - private synchronized void clusterSettingsConsumer(Settings settings) { - // update cached cluster settings - this.cachedClusterSettings = settings; - // build new settings from the previously cached secure settings - final Settings.Builder completeSettingsBuilder = Settings.builder().put(settings, false); - if (cachedSecureSettings != null) { - completeSettingsBuilder.setSecureSettings(cachedSecureSettings); - } - final Settings completeSettings = completeSettingsBuilder.build(); - // create new accounts from the latest - final Set accountNames = getAccountNames(completeSettings); - this.accounts = createAccounts(completeSettings, accountNames, this::createAccount); - this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); - } - // Used for testing only NotificationService(String type) { this.type = type; + this.logger = LogManager.getLogger(); + this.bootSettings = Settings.EMPTY; + } + + private synchronized void clusterSettingsConsumer(Settings settings) { + // update cached cluster settings + this.cachedClusterSettings = settings; + // use these new dynamic cluster settings together with the previously cached + // secure settings + buildAccounts(); } public synchronized void reload(Settings settings) { - // `SecureSettings` are available here! - // cache the `SecureSettings` + // `SecureSettings` are available here! cache them as they will be needed + // whenever dynamic cluster settings change and we have to rebuild the accounts try { - this.cachedSecureSettings = cacheSecureSettings(settings); + this.cachedSecureSettings = extractSecureSettings(settings); } catch (GeneralSecurityException e) { logger.error("Keystore exception while reloading watcher notification service", e); return; } - // build new settings from the previously cached cluster settings - final Settings.Builder completeSettingsBuilder = Settings.builder().put(settings, true); - if (cachedClusterSettings != null) { - completeSettingsBuilder.put(cachedClusterSettings); + // use these new secure settings together with the previously cached dynamic + // cluster settings + buildAccounts(); + } + + private void buildAccounts() { + // build complete settings combining cluster and secure settings + final Settings.Builder completeSettingsBuilder = Settings.builder().put(bootSettings, false); + if (this.cachedClusterSettings != null) { + completeSettingsBuilder.put(this.cachedClusterSettings, false); + } + if (this.cachedSecureSettings != null) { + completeSettingsBuilder.setSecureSettings(this.cachedSecureSettings); } final Settings completeSettings = completeSettingsBuilder.build(); - // create new accounts from the latest + // obtain account names and create accounts final Set accountNames = getAccountNames(completeSettings); this.accounts = createAccounts(completeSettings, accountNames, this::createAccount); - this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); + this.defaultAccount = findDefaultAccountOrNull(completeSettings, this.accounts); } protected abstract Account createAccount(String name, Settings accountSettings); @@ -112,7 +123,7 @@ private String getNotificationsAccountPrefix() { } private Set getAccountNames(Settings settings) { - // secure settings don't account for the client names + // secure settings are not responsible for the client names final Settings noSecureSettings = Settings.builder().put(settings, false).build(); return noSecureSettings.getByPrefix(getNotificationsAccountPrefix()).names(); } @@ -132,7 +143,7 @@ private Map createAccounts(Settings settings, Set accou return Collections.unmodifiableMap(accounts); } - private @Nullable Account selectDefaultAccount(Settings settings, Map accounts) { + private @Nullable Account findDefaultAccountOrNull(Settings settings, Map accounts) { final String defaultAccountName = getDefaultAccountName(settings); if (defaultAccountName == null) { if (accounts.isEmpty()) { @@ -149,7 +160,19 @@ private Map createAccounts(Settings settings, Set accou } } - private SecureSettings cacheSecureSettings(Settings source) throws GeneralSecurityException { + /** + * Extracts the {@link SecureSettings}` out of the passed in {@link Settings} object. The + * {@code Setting} argument has to have the {@code SecureSettings} open/available. Normally + * {@code SecureSettings} are available only under specific callstacks (eg. during node + * initialization or during a `reload` call). The returned copy can be reused freely as it + * will never be closed (this is a bit of cheating, but it is necessary in this specific + * circumstance). Only works for secure settings of type string (not file). + * + * @param source + * A {@code Settings} object with its {@code SecureSettings} open/available. + * @return A copy of the {@code SecureSettings} of the passed in {@code Settings} argument. + */ + private static SecureSettings extractSecureSettings(Settings source) throws GeneralSecurityException { // get the secure settings out final SecureSettings sourceSecureSettings = Settings.builder().put(source, true).getSecureSettings(); // cache them... diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java index ffc6dfa97ee11..1041350fc020c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java @@ -19,6 +19,8 @@ import org.elasticsearch.xpack.watcher.notification.NotificationService; import javax.mail.MessagingException; + +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -108,11 +110,7 @@ public class EmailService extends NotificationService { private final CryptoService cryptoService; public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { - super("email", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, - SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_STARTTLS_REQUIRED, SETTING_SMTP_HOST, SETTING_SMTP_PORT, SETTING_SMTP_USER, - SETTING_SMTP_PASSWORD, SETTING_SMTP_TIMEOUT, SETTING_SMTP_CONNECTION_TIMEOUT, SETTING_SMTP_WRITE_TIMEOUT, - SETTING_SMTP_SSL_TRUST_ADDRESS, SETTING_SMTP_LOCAL_ADDRESS, SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, - SETTING_SMTP_WAIT_ON_QUIT)); + super("email", settings, clusterSettings, getClusterSettings()); this.cryptoService = cryptoService; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -182,12 +180,17 @@ public Email email() { } } - public static List> getSettings() { + private static List> getClusterSettings() { return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, SETTING_SMTP_HOST, SETTING_SMTP_PASSWORD, SETTING_SMTP_PORT, SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_USER, SETTING_SMTP_STARTTLS_REQUIRED, SETTING_SMTP_TIMEOUT, SETTING_SMTP_CONNECTION_TIMEOUT, SETTING_SMTP_WRITE_TIMEOUT, SETTING_SMTP_LOCAL_ADDRESS, - SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, SETTING_SMTP_WAIT_ON_QUIT, SETTING_SMTP_SSL_TRUST_ADDRESS, - SETTING_SECURE_PASSWORD); + SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, SETTING_SMTP_WAIT_ON_QUIT, SETTING_SMTP_SSL_TRUST_ADDRESS); + } + + public static List> getSettings() { + List> allSettings = new ArrayList>(getClusterSettings()); + allSettings.add(SETTING_SECURE_PASSWORD); + return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java index 89839e5563c4b..5f5d2f7db47c1 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java @@ -16,6 +16,7 @@ import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -68,8 +69,7 @@ public class HipChatService extends NotificationService { private HipChatServer defaultServer; public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("hipchat", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, - SETTING_AUTH_TOKEN, SETTING_PROFILE, SETTING_ROOM, SETTING_HOST, SETTING_PORT, SETTING_MESSAGE_DEFAULTS)); + super("hipchat", settings, clusterSettings, getClusterSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -81,7 +81,7 @@ public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings.addAffixUpdateConsumer(SETTING_HOST, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_PORT, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_MESSAGE_DEFAULTS, (s, o) -> {}, (s, o) -> {}); - + // do an initial load reload(settings); } @@ -100,8 +100,14 @@ protected HipChatAccount createAccount(String name, Settings accountSettings) { return profile.createAccount(name, accountSettings, defaultServer, httpClient, logger); } + private static List> getClusterSettings() { + return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_AUTH_TOKEN, SETTING_PROFILE, SETTING_ROOM, SETTING_MESSAGE_DEFAULTS, + SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, SETTING_HOST, SETTING_PORT); + } + public static List> getSettings() { - return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_AUTH_TOKEN, SETTING_AUTH_TOKEN_SECURE, SETTING_PROFILE, SETTING_ROOM, - SETTING_MESSAGE_DEFAULTS, SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, SETTING_HOST, SETTING_PORT); + List> allSettings = new ArrayList>(getClusterSettings()); + allSettings.add(SETTING_AUTH_TOKEN_SECURE); + return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java index 4d2a2ad6f39c5..9bb8bf70cb29b 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -62,8 +63,7 @@ public class JiraService extends NotificationService { private final HttpClient httpClient; public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("jira", clusterSettings, - Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS)); + super("jira", settings, clusterSettings, getClusterSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -81,8 +81,15 @@ protected JiraAccount createAccount(String name, Settings settings) { return new JiraAccount(name, settings, httpClient); } + private static List> getClusterSettings() { + return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS); + } + public static List> getSettings() { - return Arrays.asList(SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_SECURE_USER, - SETTING_SECURE_PASSWORD, SETTING_SECURE_URL, SETTING_DEFAULTS, SETTING_DEFAULT_ACCOUNT); + List> allSettings = new ArrayList>(getClusterSettings()); + allSettings.add(SETTING_SECURE_USER); + allSettings.add(SETTING_SECURE_PASSWORD); + allSettings.add(SETTING_SECURE_URL); + return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java index 996fb92bc356e..f9ed67b4dbd5f 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -40,11 +41,13 @@ public class PagerDutyService extends NotificationService { private final HttpClient httpClient; public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("pagerduty", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_SERVICE_API_KEY, SETTING_DEFAULTS)); + super("pagerduty", settings, clusterSettings, getClusterSettings()); this.httpClient = httpClient; + // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_SERVICE_API_KEY, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {}); + // do an initial load reload(settings); } @@ -53,7 +56,13 @@ protected PagerDutyAccount createAccount(String name, Settings accountSettings) return new PagerDutyAccount(name, accountSettings, accountSettings, httpClient); } + private static List> getClusterSettings() { + return Arrays.asList(SETTING_SERVICE_API_KEY, SETTING_DEFAULTS, SETTING_DEFAULT_ACCOUNT); + } + public static List> getSettings() { - return Arrays.asList(SETTING_SERVICE_API_KEY, SETTING_SECURE_SERVICE_API_KEY, SETTING_DEFAULTS, SETTING_DEFAULT_ACCOUNT); + List> allSettings = new ArrayList>(getClusterSettings()); + allSettings.add(SETTING_SECURE_SERVICE_API_KEY); + return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java index 038da8755e5dc..a6ceacceaf447 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java @@ -16,6 +16,7 @@ import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -43,11 +44,13 @@ public class SlackService extends NotificationService { private final HttpClient httpClient; public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("slack", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_URL, SETTING_DEFAULTS)); + super("slack", settings, clusterSettings, getClusterSettings()); this.httpClient = httpClient; + // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_URL, (s, o) -> {}, (s, o) -> {}); clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {}); + // do an initial load reload(settings); } @@ -56,7 +59,13 @@ protected SlackAccount createAccount(String name, Settings accountSettings) { return new SlackAccount(name, accountSettings, accountSettings, httpClient, logger); } + private static List> getClusterSettings() { + return Arrays.asList(SETTING_URL, SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULTS); + } + public static List> getSettings() { - return Arrays.asList(SETTING_URL, SETTING_URL_SECURE, SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULTS); + List> allSettings = new ArrayList>(getClusterSettings()); + allSettings.add(SETTING_URL_SECURE); + return allSettings; } } From ba02a20e59eb799b876098eab7f2d32ec8048e2a Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Wed, 28 Nov 2018 18:22:38 +0200 Subject: [PATCH 5/8] Fix NPE --- .../xpack/watcher/notification/NotificationService.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index 576d6bf427f22..44c26ee69c339 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -177,8 +177,10 @@ private static SecureSettings extractSecureSettings(Settings source) throws Gene final SecureSettings sourceSecureSettings = Settings.builder().put(source, true).getSecureSettings(); // cache them... final Map cache = new HashMap<>(); - for (final String settingKey : sourceSecureSettings.getSettingNames()) { - cache.put(settingKey, sourceSecureSettings.getString(settingKey)); + if (sourceSecureSettings != null) { + for (final String settingKey : sourceSecureSettings.getSettingNames()) { + cache.put(settingKey, sourceSecureSettings.getString(settingKey)); + } } return new SecureSettings() { From 459e6e7af478c5fb072a07f729e65c8847a60df8 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 29 Nov 2018 10:43:16 +0200 Subject: [PATCH 6/8] Trim --- .../xpack/watcher/notification/NotificationService.java | 8 +++----- .../watcher/notification/NotificationServiceTests.java | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index 44c26ee69c339..8fffd867551e7 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -44,18 +44,16 @@ public abstract class NotificationService { private volatile SecureSettings cachedSecureSettings; public NotificationService(String type, Settings settings, ClusterSettings clusterSettings, List> pluginClusterSettings) { - this.type = type; - this.logger = LogManager.getLogger(); - this.bootSettings = settings; + this(type, settings); // register a grand updater for the whole group, as settings are usable together clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginClusterSettings); } // Used for testing only - NotificationService(String type) { + NotificationService(String type, Settings settings) { this.type = type; this.logger = LogManager.getLogger(); - this.bootSettings = Settings.EMPTY; + this.bootSettings = settings; } private synchronized void clusterSettingsConsumer(Settings settings) { diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java index ddf45de816367..1c8b21ecf3121 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java @@ -81,7 +81,7 @@ public void testAccountDoesNotExist() throws Exception{ private static class TestNotificationService extends NotificationService { TestNotificationService(Settings settings) { - super("test"); + super("test", settings); reload(settings); } From af230384a1a8203ade7fcbddb92e34db1b3336ae Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 29 Nov 2018 10:51:06 +0200 Subject: [PATCH 7/8] Renames --- .../xpack/watcher/notification/email/EmailService.java | 6 +++--- .../xpack/watcher/notification/hipchat/HipChatService.java | 6 +++--- .../xpack/watcher/notification/jira/JiraService.java | 6 +++--- .../watcher/notification/pagerduty/PagerDutyService.java | 6 +++--- .../xpack/watcher/notification/slack/SlackService.java | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java index 1041350fc020c..ffed96ffe8277 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java @@ -110,7 +110,7 @@ public class EmailService extends NotificationService { private final CryptoService cryptoService; public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { - super("email", settings, clusterSettings, getClusterSettings()); + super("email", settings, clusterSettings, getDynamicSettings()); this.cryptoService = cryptoService; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -180,7 +180,7 @@ public Email email() { } } - private static List> getClusterSettings() { + private static List> getDynamicSettings() { return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, SETTING_SMTP_HOST, SETTING_SMTP_PASSWORD, SETTING_SMTP_PORT, SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_USER, SETTING_SMTP_STARTTLS_REQUIRED, SETTING_SMTP_TIMEOUT, SETTING_SMTP_CONNECTION_TIMEOUT, SETTING_SMTP_WRITE_TIMEOUT, SETTING_SMTP_LOCAL_ADDRESS, @@ -188,7 +188,7 @@ private static List> getClusterSettings() { } public static List> getSettings() { - List> allSettings = new ArrayList>(getClusterSettings()); + List> allSettings = new ArrayList>(getDynamicSettings()); allSettings.add(SETTING_SECURE_PASSWORD); return allSettings; } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java index 5f5d2f7db47c1..e24ff2f1ce0f3 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java @@ -69,7 +69,7 @@ public class HipChatService extends NotificationService { private HipChatServer defaultServer; public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("hipchat", settings, clusterSettings, getClusterSettings()); + super("hipchat", settings, clusterSettings, getDynamicSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -100,13 +100,13 @@ protected HipChatAccount createAccount(String name, Settings accountSettings) { return profile.createAccount(name, accountSettings, defaultServer, httpClient, logger); } - private static List> getClusterSettings() { + private static List> getDynamicSettings() { return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_AUTH_TOKEN, SETTING_PROFILE, SETTING_ROOM, SETTING_MESSAGE_DEFAULTS, SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, SETTING_HOST, SETTING_PORT); } public static List> getSettings() { - List> allSettings = new ArrayList>(getClusterSettings()); + List> allSettings = new ArrayList>(getDynamicSettings()); allSettings.add(SETTING_AUTH_TOKEN_SECURE); return allSettings; } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java index 9bb8bf70cb29b..884768bd40c10 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java @@ -63,7 +63,7 @@ public class JiraService extends NotificationService { private final HttpClient httpClient; public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("jira", settings, clusterSettings, getClusterSettings()); + super("jira", settings, clusterSettings, getDynamicSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -81,12 +81,12 @@ protected JiraAccount createAccount(String name, Settings settings) { return new JiraAccount(name, settings, httpClient); } - private static List> getClusterSettings() { + private static List> getDynamicSettings() { return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS); } public static List> getSettings() { - List> allSettings = new ArrayList>(getClusterSettings()); + List> allSettings = new ArrayList>(getDynamicSettings()); allSettings.add(SETTING_SECURE_USER); allSettings.add(SETTING_SECURE_PASSWORD); allSettings.add(SETTING_SECURE_URL); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java index f9ed67b4dbd5f..8639574557358 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java @@ -41,7 +41,7 @@ public class PagerDutyService extends NotificationService { private final HttpClient httpClient; public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("pagerduty", settings, clusterSettings, getClusterSettings()); + super("pagerduty", settings, clusterSettings, getDynamicSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -56,12 +56,12 @@ protected PagerDutyAccount createAccount(String name, Settings accountSettings) return new PagerDutyAccount(name, accountSettings, accountSettings, httpClient); } - private static List> getClusterSettings() { + private static List> getDynamicSettings() { return Arrays.asList(SETTING_SERVICE_API_KEY, SETTING_DEFAULTS, SETTING_DEFAULT_ACCOUNT); } public static List> getSettings() { - List> allSettings = new ArrayList>(getClusterSettings()); + List> allSettings = new ArrayList>(getDynamicSettings()); allSettings.add(SETTING_SECURE_SERVICE_API_KEY); return allSettings; } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java index a6ceacceaf447..48e919c40ff76 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java @@ -44,7 +44,7 @@ public class SlackService extends NotificationService { private final HttpClient httpClient; public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("slack", settings, clusterSettings, getClusterSettings()); + super("slack", settings, clusterSettings, getDynamicSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -59,12 +59,12 @@ protected SlackAccount createAccount(String name, Settings accountSettings) { return new SlackAccount(name, accountSettings, accountSettings, httpClient, logger); } - private static List> getClusterSettings() { + private static List> getDynamicSettings() { return Arrays.asList(SETTING_URL, SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULTS); } public static List> getSettings() { - List> allSettings = new ArrayList>(getClusterSettings()); + List> allSettings = new ArrayList>(getDynamicSettings()); allSettings.add(SETTING_URL_SECURE); return allSettings; } From 55a6a93f6da8ae8fdc163311de45a3010e0a5f08 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 29 Nov 2018 19:58:13 +0200 Subject: [PATCH 8/8] Oopsie daisy, extract and cached only selected settings --- .../notification/NotificationService.java | 38 +++++++++++-------- .../notification/email/EmailService.java | 10 +++-- .../notification/hipchat/HipChatService.java | 8 +++- .../notification/jira/JiraService.java | 10 +++-- .../pagerduty/PagerDutyService.java | 8 +++- .../notification/slack/SlackService.java | 8 +++- .../NotificationServiceTests.java | 4 +- 7 files changed, 57 insertions(+), 29 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java index 8fffd867551e7..f62de14b931c6 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java @@ -33,6 +33,7 @@ public abstract class NotificationService { private final String type; private final Logger logger; private final Settings bootSettings; + private final List> pluginSecureSettings; // all are guarded by this private volatile Map accounts; private volatile Account defaultAccount; @@ -43,17 +44,19 @@ public abstract class NotificationService { // using the new updated cluster settings private volatile SecureSettings cachedSecureSettings; - public NotificationService(String type, Settings settings, ClusterSettings clusterSettings, List> pluginClusterSettings) { - this(type, settings); + public NotificationService(String type, Settings settings, ClusterSettings clusterSettings, List> pluginDynamicSettings, + List> pluginSecureSettings) { + this(type, settings, pluginSecureSettings); // register a grand updater for the whole group, as settings are usable together - clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginClusterSettings); + clusterSettings.addSettingsUpdateConsumer(this::clusterSettingsConsumer, pluginDynamicSettings); } // Used for testing only - NotificationService(String type, Settings settings) { + NotificationService(String type, Settings settings, List> pluginSecureSettings) { this.type = type; this.logger = LogManager.getLogger(); this.bootSettings = settings; + this.pluginSecureSettings = pluginSecureSettings; } private synchronized void clusterSettingsConsumer(Settings settings) { @@ -68,7 +71,7 @@ public synchronized void reload(Settings settings) { // `SecureSettings` are available here! cache them as they will be needed // whenever dynamic cluster settings change and we have to rebuild the accounts try { - this.cachedSecureSettings = extractSecureSettings(settings); + this.cachedSecureSettings = extractSecureSettings(settings, pluginSecureSettings); } catch (GeneralSecurityException e) { logger.error("Keystore exception while reloading watcher notification service", e); return; @@ -159,25 +162,30 @@ private Map createAccounts(Settings settings, Set accou } /** - * Extracts the {@link SecureSettings}` out of the passed in {@link Settings} object. The - * {@code Setting} argument has to have the {@code SecureSettings} open/available. Normally - * {@code SecureSettings} are available only under specific callstacks (eg. during node - * initialization or during a `reload` call). The returned copy can be reused freely as it - * will never be closed (this is a bit of cheating, but it is necessary in this specific - * circumstance). Only works for secure settings of type string (not file). + * Extracts the {@link SecureSettings}` out of the passed in {@link Settings} object. The {@code Setting} argument has to have the + * {@code SecureSettings} open/available. Normally {@code SecureSettings} are available only under specific callstacks (eg. during node + * initialization or during a `reload` call). The returned copy can be reused freely as it will never be closed (this is a bit of + * cheating, but it is necessary in this specific circumstance). Only works for secure settings of type string (not file). * * @param source * A {@code Settings} object with its {@code SecureSettings} open/available. + * @param securePluginSettings + * The list of settings to copy. * @return A copy of the {@code SecureSettings} of the passed in {@code Settings} argument. */ - private static SecureSettings extractSecureSettings(Settings source) throws GeneralSecurityException { + private static SecureSettings extractSecureSettings(Settings source, List> securePluginSettings) + throws GeneralSecurityException { // get the secure settings out final SecureSettings sourceSecureSettings = Settings.builder().put(source, true).getSecureSettings(); - // cache them... + // filter and cache them... final Map cache = new HashMap<>(); - if (sourceSecureSettings != null) { + if (sourceSecureSettings != null && securePluginSettings != null) { for (final String settingKey : sourceSecureSettings.getSettingNames()) { - cache.put(settingKey, sourceSecureSettings.getString(settingKey)); + for (final Setting secureSetting : securePluginSettings) { + if (secureSetting.match(settingKey)) { + cache.put(settingKey, sourceSecureSettings.getString(settingKey)); + } + } } } return new SecureSettings() { diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java index ffed96ffe8277..0933ba4616280 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailService.java @@ -110,7 +110,7 @@ public class EmailService extends NotificationService { private final CryptoService cryptoService; public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { - super("email", settings, clusterSettings, getDynamicSettings()); + super("email", settings, clusterSettings, EmailService.getDynamicSettings(), EmailService.getSecureSettings()); this.cryptoService = cryptoService; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -187,9 +187,13 @@ private static List> getDynamicSettings() { SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, SETTING_SMTP_WAIT_ON_QUIT, SETTING_SMTP_SSL_TRUST_ADDRESS); } + private static List> getSecureSettings() { + return Arrays.asList(SETTING_SECURE_PASSWORD); + } + public static List> getSettings() { - List> allSettings = new ArrayList>(getDynamicSettings()); - allSettings.add(SETTING_SECURE_PASSWORD); + List> allSettings = new ArrayList>(EmailService.getDynamicSettings()); + allSettings.addAll(EmailService.getSecureSettings()); return allSettings; } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java index e24ff2f1ce0f3..39b1f0cb61709 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatService.java @@ -69,7 +69,7 @@ public class HipChatService extends NotificationService { private HipChatServer defaultServer; public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("hipchat", settings, clusterSettings, getDynamicSettings()); + super("hipchat", settings, clusterSettings, HipChatService.getDynamicSettings(), HipChatService.getSecureSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -105,9 +105,13 @@ private static List> getDynamicSettings() { SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, SETTING_HOST, SETTING_PORT); } + private static List> getSecureSettings() { + return Arrays.asList(SETTING_AUTH_TOKEN_SECURE); + } + public static List> getSettings() { List> allSettings = new ArrayList>(getDynamicSettings()); - allSettings.add(SETTING_AUTH_TOKEN_SECURE); + allSettings.addAll(getSecureSettings()); return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java index 884768bd40c10..425ef0ee44fd1 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraService.java @@ -63,7 +63,7 @@ public class JiraService extends NotificationService { private final HttpClient httpClient; public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("jira", settings, clusterSettings, getDynamicSettings()); + super("jira", settings, clusterSettings, JiraService.getDynamicSettings(), JiraService.getSecureSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -85,11 +85,13 @@ private static List> getDynamicSettings() { return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS); } + private static List> getSecureSettings() { + return Arrays.asList(SETTING_SECURE_USER, SETTING_SECURE_PASSWORD, SETTING_SECURE_URL); + } + public static List> getSettings() { List> allSettings = new ArrayList>(getDynamicSettings()); - allSettings.add(SETTING_SECURE_USER); - allSettings.add(SETTING_SECURE_PASSWORD); - allSettings.add(SETTING_SECURE_URL); + allSettings.addAll(getSecureSettings()); return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java index 8639574557358..6a0fa5b5bf45b 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyService.java @@ -41,7 +41,7 @@ public class PagerDutyService extends NotificationService { private final HttpClient httpClient; public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("pagerduty", settings, clusterSettings, getDynamicSettings()); + super("pagerduty", settings, clusterSettings, PagerDutyService.getDynamicSettings(), PagerDutyService.getSecureSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -60,9 +60,13 @@ private static List> getDynamicSettings() { return Arrays.asList(SETTING_SERVICE_API_KEY, SETTING_DEFAULTS, SETTING_DEFAULT_ACCOUNT); } + private static List> getSecureSettings() { + return Arrays.asList(SETTING_SECURE_SERVICE_API_KEY); + } + public static List> getSettings() { List> allSettings = new ArrayList>(getDynamicSettings()); - allSettings.add(SETTING_SECURE_SERVICE_API_KEY); + allSettings.addAll(getSecureSettings()); return allSettings; } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java index 48e919c40ff76..0d8d0bc67faf6 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/SlackService.java @@ -44,7 +44,7 @@ public class SlackService extends NotificationService { private final HttpClient httpClient; public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { - super("slack", settings, clusterSettings, getDynamicSettings()); + super("slack", settings, clusterSettings, SlackService.getDynamicSettings(), SlackService.getSecureSettings()); this.httpClient = httpClient; // ensure logging of setting changes clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {}); @@ -63,9 +63,13 @@ private static List> getDynamicSettings() { return Arrays.asList(SETTING_URL, SETTING_DEFAULT_ACCOUNT, SETTING_DEFAULTS); } + private static List> getSecureSettings() { + return Arrays.asList(SETTING_URL_SECURE); + } + public static List> getSettings() { List> allSettings = new ArrayList>(getDynamicSettings()); - allSettings.add(SETTING_URL_SECURE); + allSettings.addAll(getSecureSettings()); return allSettings; } } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java index 1c8b21ecf3121..184ff56c21300 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java @@ -10,6 +10,8 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.util.Collections; + import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.is; @@ -81,7 +83,7 @@ public void testAccountDoesNotExist() throws Exception{ private static class TestNotificationService extends NotificationService { TestNotificationService(Settings settings) { - super("test", settings); + super("test", settings, Collections.emptyList()); reload(settings); }