From adf9510b48732d5d2f5a210666d36a926b6f33ed Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 14 Dec 2018 21:07:30 +0200 Subject: [PATCH 1/3] Lazy reload --- .../notification/NotificationService.java | 32 ++-- .../NotificationServiceTests.java | 138 +++++++++++++++++- 2 files changed, 152 insertions(+), 18 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 f62de14b931c6..193b4841ea3d5 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 @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Set; import java.util.function.BiFunction; +import java.util.function.Supplier; /** * Basic notification service @@ -35,8 +36,8 @@ public abstract class NotificationService { private final Settings bootSettings; private final List> pluginSecureSettings; // all are guarded by this - private volatile Map accounts; - private volatile Account defaultAccount; + private volatile Map> accounts; + private volatile Supplier defaultAccount; // cached cluster setting, required when recreating the notification clients // using the new "reloaded" secure settings private volatile Settings cachedClusterSettings; @@ -59,7 +60,7 @@ public NotificationService(String type, Settings settings, ClusterSettings clust this.pluginSecureSettings = pluginSecureSettings; } - private synchronized void clusterSettingsConsumer(Settings settings) { + protected synchronized void clusterSettingsConsumer(Settings settings) { // update cached cluster settings this.cachedClusterSettings = settings; // use these new dynamic cluster settings together with the previously cached @@ -102,13 +103,13 @@ private void buildAccounts() { 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... - final Map accounts; - final Account defaultAccount; + final Map> accounts; + final Supplier defaultAccount; synchronized (this) { // must read under sync block otherwise it might be inconsistent accounts = this.accounts; defaultAccount = this.defaultAccount; } - Account theAccount = accounts.getOrDefault(name, defaultAccount); + Supplier theAccount = accounts.getOrDefault(name, defaultAccount); if (theAccount == null && name == null) { throw new IllegalArgumentException("no accounts of type [" + type + "] configured. " + "Please set up an account using the [xpack.notification." + type +"] settings"); @@ -116,7 +117,7 @@ public Account getAccount(String name) { if (theAccount == null) { throw new IllegalArgumentException("no account found for name: [" + name + "]"); } - return theAccount; + return theAccount.get(); } private String getNotificationsAccountPrefix() { @@ -124,27 +125,26 @@ private String getNotificationsAccountPrefix() { } private Set getAccountNames(Settings settings) { - // secure settings are not responsible for the client names - final Settings noSecureSettings = Settings.builder().put(settings, false).build(); - return noSecureSettings.getByPrefix(getNotificationsAccountPrefix()).names(); + 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, + private Map> createAccounts(Settings settings, Set accountNames, BiFunction accountFactory) { - final Map accounts = new HashMap<>(); + 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); + accounts.put(accountName, () -> { + return accountFactory.apply(accountName, accountSettings); + }); } return Collections.unmodifiableMap(accounts); } - private @Nullable Account findDefaultAccountOrNull(Settings settings, Map accounts) { + private @Nullable Supplier findDefaultAccountOrNull(Settings settings, Map> accounts) { final String defaultAccountName = getDefaultAccountName(settings); if (defaultAccountName == null) { if (accounts.isEmpty()) { @@ -153,7 +153,7 @@ private Map createAccounts(Settings settings, Set accou return accounts.values().iterator().next(); } } else { - final Account account = accounts.get(defaultAccountName); + final Supplier account = accounts.get(defaultAccountName); if (account == null) { throw new SettingsException("could not find default account [" + defaultAccountName + "]"); } 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 184ff56c21300..53878e289afc7 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 @@ -5,12 +5,27 @@ */ package org.elasticsearch.xpack.watcher.notification; +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 org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.watcher.notification.NotificationService; +import java.io.IOException; +import java.io.InputStream; +import java.security.GeneralSecurityException; +import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiConsumer; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.is; @@ -80,16 +95,135 @@ public void testAccountDoesNotExist() throws Exception{ is("no accounts of type [test] configured. Please set up an account using the [xpack.notification.test] settings")); } + public void testAccountWithSecureSettings() throws Exception { + final Setting secureSetting1 = SecureSetting.secureString("xpack.notification.test.account.secure_only", null); + final Setting secureSetting2 = SecureSetting.secureString("xpack.notification.test.account.mixed.secure", null); + final Map secureSettingsMap = new HashMap<>(); + secureSettingsMap.put(secureSetting1.getKey(), "secure_only".toCharArray()); + secureSettingsMap.put(secureSetting2.getKey(), "mixed_secure".toCharArray()); + Settings settings = Settings.builder() + .put("xpack.notification.test.account.unsecure_only", "bar") + .put("xpack.notification.test.account.mixed.unsecure", "mixed_unsecure") + .setSecureSettings(secureSettingsFromMap(secureSettingsMap)) + .build(); + TestNotificationService service = new TestNotificationService(settings, Arrays.asList(secureSetting1, secureSetting2)); + assertThat(service.getAccount("secure_only"), is("secure_only")); + assertThat(service.getAccount("unsecure_only"), is("unsecure_only")); + assertThat(service.getAccount("mixed"), is("mixed")); + assertThat(service.getAccount(null), anyOf(is("secure_only"), is("unsecure_only"), is("mixed"))); + } + + public void testAccountUpdateSettings() throws Exception { + final Setting secureSetting = SecureSetting.secureString("xpack.notification.test.account.x.secure", null); + final Setting setting = Setting.simpleString("xpack.notification.test.account.x.dynamic", Setting.Property.Dynamic, + Setting.Property.NodeScope); + final AtomicReference secureSettingValue = new AtomicReference(randomAlphaOfLength(4)); + final AtomicReference settingValue = new AtomicReference(randomAlphaOfLength(4)); + final Map secureSettingsMap = new HashMap<>(); + final AtomicInteger validationInvocationCount = new AtomicInteger(0); + secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); + final Settings.Builder settingsBuilder = Settings.builder() + .put(setting.getKey(), settingValue.get()) + .setSecureSettings(secureSettingsFromMap(secureSettingsMap)); + final TestNotificationService service = new TestNotificationService(settingsBuilder.build(), Arrays.asList(secureSetting), + (String name, Settings accountSettings) -> { + assertThat(accountSettings.get("dynamic"), is(settingValue.get())); + assertThat(SecureSetting.secureString("secure", null).get(accountSettings), is(secureSettingValue.get())); + validationInvocationCount.incrementAndGet(); + }); + assertThat(validationInvocationCount.get(), is(0)); + service.getAccount(null); + assertThat(validationInvocationCount.get(), is(1)); + // update secure setting only + secureSettingValue.set(randomAlphaOfLength(4)); + secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); + service.reload(settingsBuilder.build()); + assertThat(validationInvocationCount.get(), is(1)); + service.getAccount(null); + assertThat(validationInvocationCount.get(), is(2)); + // update dynamic cluster setting only + settingValue.set(randomAlphaOfLength(4)); + settingsBuilder.put(setting.getKey(), settingValue.get()); + service.clusterSettingsConsumer(settingsBuilder.build()); + assertThat(validationInvocationCount.get(), is(2)); + service.getAccount(null); + assertThat(validationInvocationCount.get(), is(3)); + // update both + if (randomBoolean()) { + // update secure first + secureSettingValue.set(randomAlphaOfLength(4)); + secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); + service.reload(settingsBuilder.build()); + // update cluster second + settingValue.set(randomAlphaOfLength(4)); + settingsBuilder.put(setting.getKey(), settingValue.get()); + service.clusterSettingsConsumer(settingsBuilder.build()); + } else { + // update cluster first + settingValue.set(randomAlphaOfLength(4)); + settingsBuilder.put(setting.getKey(), settingValue.get()); + service.clusterSettingsConsumer(settingsBuilder.build()); + // update secure second + secureSettingValue.set(randomAlphaOfLength(4)); + secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); + service.reload(settingsBuilder.build()); + } + assertThat(validationInvocationCount.get(), is(3)); + service.getAccount(null); + assertThat(validationInvocationCount.get(), is(4)); + } + private static class TestNotificationService extends NotificationService { - TestNotificationService(Settings settings) { - super("test", settings, Collections.emptyList()); + private final BiConsumer validator; + + TestNotificationService(Settings settings, List> secureSettings, BiConsumer validator) { + super("test", settings, secureSettings); + this.validator = validator; reload(settings); } + TestNotificationService(Settings settings, List> secureSettings) { + this(settings, secureSettings, (x, y) -> {}); + } + + TestNotificationService(Settings settings) { + this(settings, Collections.emptyList(), (x, y) -> {}); + } + @Override protected String createAccount(String name, Settings accountSettings) { + validator.accept(name, accountSettings); return name; } } + + private static SecureSettings secureSettingsFromMap(Map secureSettingsMap) { + return new SecureSettings() { + + @Override + public boolean isLoaded() { + return true; + } + + @Override + public SecureString getString(String setting) throws GeneralSecurityException { + return new SecureString(secureSettingsMap.get(setting)); + } + + @Override + public Set getSettingNames() { + return secureSettingsMap.keySet(); + } + + @Override + public InputStream getFile(String setting) throws GeneralSecurityException { + return null; + } + + @Override + public void close() throws IOException { + } + }; + } } From 73292dffdd7473837440f2f1554ce69a60125f54 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 14 Dec 2018 23:28:32 +0200 Subject: [PATCH 2/3] Replace Supplier with LazyInitilizable --- .../notification/NotificationService.java | 26 ++++---- .../NotificationServiceTests.java | 64 +++++++++++++------ .../hipchat/HipChatServiceTests.java | 2 +- 3 files changed, 59 insertions(+), 33 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 193b4841ea3d5..6cd3bb8dbade2 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 @@ -14,6 +14,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; +import org.elasticsearch.common.util.LazyInitializable; import java.io.IOException; import java.io.InputStream; @@ -24,7 +25,6 @@ import java.util.Map; import java.util.Set; import java.util.function.BiFunction; -import java.util.function.Supplier; /** * Basic notification service @@ -36,8 +36,8 @@ public abstract class NotificationService { private final Settings bootSettings; private final List> pluginSecureSettings; // all are guarded by this - private volatile Map> accounts; - private volatile Supplier defaultAccount; + private volatile Map> accounts; + private volatile LazyInitializable defaultAccount; // cached cluster setting, required when recreating the notification clients // using the new "reloaded" secure settings private volatile Settings cachedClusterSettings; @@ -103,13 +103,13 @@ private void buildAccounts() { 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... - final Map> accounts; - final Supplier defaultAccount; + final Map> accounts; + final LazyInitializable defaultAccount; synchronized (this) { // must read under sync block otherwise it might be inconsistent accounts = this.accounts; defaultAccount = this.defaultAccount; } - Supplier theAccount = accounts.getOrDefault(name, defaultAccount); + LazyInitializable theAccount = accounts.getOrDefault(name, defaultAccount); if (theAccount == null && name == null) { throw new IllegalArgumentException("no accounts of type [" + type + "] configured. " + "Please set up an account using the [xpack.notification." + type +"] settings"); @@ -117,7 +117,7 @@ public Account getAccount(String name) { if (theAccount == null) { throw new IllegalArgumentException("no account found for name: [" + name + "]"); } - return theAccount.get(); + return theAccount.getOrCompute(); } private String getNotificationsAccountPrefix() { @@ -132,19 +132,19 @@ private Set getAccountNames(Settings settings) { return settings.get("xpack.notification." + type + ".default_account"); } - private Map> createAccounts(Settings settings, Set accountNames, + private Map> createAccounts(Settings settings, Set accountNames, BiFunction accountFactory) { - final Map> accounts = new HashMap<>(); + final Map> accounts = new HashMap<>(); for (final String accountName : accountNames) { final Settings accountSettings = settings.getAsSettings(getNotificationsAccountPrefix() + accountName); - accounts.put(accountName, () -> { + accounts.put(accountName, new LazyInitializable<>(() -> { return accountFactory.apply(accountName, accountSettings); - }); + })); } return Collections.unmodifiableMap(accounts); } - private @Nullable Supplier findDefaultAccountOrNull(Settings settings, Map> accounts) { + private @Nullable LazyInitializable findDefaultAccountOrNull(Settings settings, Map> accounts) { final String defaultAccountName = getDefaultAccountName(settings); if (defaultAccountName == null) { if (accounts.isEmpty()) { @@ -153,7 +153,7 @@ private Map> createAccounts(Settings settings, Set account = accounts.get(defaultAccountName); + final LazyInitializable account = accounts.get(defaultAccountName); if (account == null) { throw new SettingsException("could not find default account [" + defaultAccountName + "]"); } 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 53878e289afc7..efbefdd640893 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 @@ -40,6 +40,7 @@ public void testSingleAccount() { assertThat(service.getAccount(accountName), is(accountName)); // single account, this will also be the default assertThat(service.getAccount("non-existing"), is(accountName)); + assertThat(service.getAccount(null), is(accountName)); } public void testMultipleAccountsWithExistingDefault() { @@ -113,6 +114,26 @@ public void testAccountWithSecureSettings() throws Exception { assertThat(service.getAccount(null), anyOf(is("secure_only"), is("unsecure_only"), is("mixed"))); } + public void testAccountCreationCached() { + String accountName = randomAlphaOfLength(10); + Settings settings = Settings.builder().put("xpack.notification.test.account." + accountName, "bar").build(); + final AtomicInteger validationInvocationCount = new AtomicInteger(0); + + TestNotificationService service = new TestNotificationService(settings, (String name, Settings accountSettings) -> { + validationInvocationCount.incrementAndGet(); + }); + assertThat(validationInvocationCount.get(), is(0)); + assertThat(service.getAccount(accountName), is(accountName)); + assertThat(validationInvocationCount.get(), is(1)); + if (randomBoolean()) { + assertThat(service.getAccount(accountName), is(accountName)); + } else { + assertThat(service.getAccount(null), is(accountName)); + } + // counter is still 1 because the account is cached + assertThat(validationInvocationCount.get(), is(1)); + } + public void testAccountUpdateSettings() throws Exception { final Setting secureSetting = SecureSetting.secureString("xpack.notification.test.account.x.secure", null); final Setting setting = Setting.simpleString("xpack.notification.test.account.x.dynamic", Setting.Property.Dynamic, @@ -135,44 +156,45 @@ public void testAccountUpdateSettings() throws Exception { service.getAccount(null); assertThat(validationInvocationCount.get(), is(1)); // update secure setting only - secureSettingValue.set(randomAlphaOfLength(4)); - secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); - service.reload(settingsBuilder.build()); + updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service); assertThat(validationInvocationCount.get(), is(1)); service.getAccount(null); assertThat(validationInvocationCount.get(), is(2)); - // update dynamic cluster setting only - settingValue.set(randomAlphaOfLength(4)); - settingsBuilder.put(setting.getKey(), settingValue.get()); - service.clusterSettingsConsumer(settingsBuilder.build()); + updateDynamicClusterSetting(settingValue, setting, settingsBuilder, service); assertThat(validationInvocationCount.get(), is(2)); service.getAccount(null); assertThat(validationInvocationCount.get(), is(3)); // update both if (randomBoolean()) { // update secure first - secureSettingValue.set(randomAlphaOfLength(4)); - secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); - service.reload(settingsBuilder.build()); + updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service); // update cluster second - settingValue.set(randomAlphaOfLength(4)); - settingsBuilder.put(setting.getKey(), settingValue.get()); - service.clusterSettingsConsumer(settingsBuilder.build()); + updateDynamicClusterSetting(settingValue, setting, settingsBuilder, service); } else { // update cluster first - settingValue.set(randomAlphaOfLength(4)); - settingsBuilder.put(setting.getKey(), settingValue.get()); - service.clusterSettingsConsumer(settingsBuilder.build()); + updateDynamicClusterSetting(settingValue, setting, settingsBuilder, service); // update secure second - secureSettingValue.set(randomAlphaOfLength(4)); - secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); - service.reload(settingsBuilder.build()); + updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service); } assertThat(validationInvocationCount.get(), is(3)); service.getAccount(null); assertThat(validationInvocationCount.get(), is(4)); } + private static void updateDynamicClusterSetting(AtomicReference settingValue, Setting setting, + Settings.Builder settingsBuilder, TestNotificationService service) { + settingValue.set(randomAlphaOfLength(4)); + settingsBuilder.put(setting.getKey(), settingValue.get()); + service.clusterSettingsConsumer(settingsBuilder.build()); + } + + private static void updateSecureSetting(AtomicReference secureSettingValue, Setting secureSetting, + Map secureSettingsMap, Settings.Builder settingsBuilder, TestNotificationService service) { + secureSettingValue.set(randomAlphaOfLength(4)); + secureSettingsMap.put(secureSetting.getKey(), secureSettingValue.get().toCharArray()); + service.reload(settingsBuilder.build()); + } + private static class TestNotificationService extends NotificationService { private final BiConsumer validator; @@ -191,6 +213,10 @@ private static class TestNotificationService extends NotificationService this(settings, Collections.emptyList(), (x, y) -> {}); } + TestNotificationService(Settings settings, BiConsumer validator) { + this(settings, Collections.emptyList(), validator); + } + @Override protected String createAccount(String name, Settings accountSettings) { validator.accept(name, accountSettings); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatServiceTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatServiceTests.java index 7d3960a93449f..7b5d6c7f081a4 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatServiceTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatServiceTests.java @@ -128,7 +128,7 @@ public void testSingleAccountIntegrationNoRoomSetting() throws Exception { .put("xpack.notification.hipchat.account." + accountName + ".auth_token", "_token"); SettingsException e = expectThrows(SettingsException.class, () -> new HipChatService(settingsBuilder.build(), httpClient, - new ClusterSettings(settingsBuilder.build(), new HashSet<>(HipChatService.getSettings())))); + new ClusterSettings(settingsBuilder.build(), new HashSet<>(HipChatService.getSettings()))).getAccount(null)); assertThat(e.getMessage(), containsString("missing required [room] setting for [integration] account profile")); } From b16cdceb0dcafdacf562368b350750ca235fdef4 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Sat, 15 Dec 2018 00:48:05 +0200 Subject: [PATCH 3/3] Checkstyle --- .../xpack/watcher/notification/NotificationService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 6cd3bb8dbade2..c2a079e519f0f 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 @@ -144,7 +144,8 @@ private Map> createAccount return Collections.unmodifiableMap(accounts); } - private @Nullable LazyInitializable findDefaultAccountOrNull(Settings settings, Map> accounts) { + private @Nullable LazyInitializable findDefaultAccountOrNull(Settings settings, + Map> accounts) { final String defaultAccountName = getDefaultAccountName(settings); if (defaultAccountName == null) { if (accounts.isEmpty()) {