honeymoose/OpenSearch
1
0
Fork 0
mirror of https://github.com/honeymoose/OpenSearch.git synced 2025-02-17 10:25:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

Watcher accounts constructed lazily (#36656)

Browse Source 
This fixes two bugs about watcher notifications:
* registering accounts that had only secure settings was not possible before;
these accounts are very much practical for Slack and PagerDuty integrations.
* removes the limitation that, for an account with both secure and cluster settings,
the admin had to first change/add the secure settings and only then add the
dependent dynamic cluster settings. The reverse order would trigger a
SettingsException for an incomplete account.

The workaround is to lazily instantiate account objects, hoping that when accounts
are instantiated all the required settings are in place. Previously, the approach
was to greedily validate all the account settings by constructing the account objects,
even if they would not ever be used by actions. This made sense in a world where
all the settings were set by a single API. But given that accounts have dependent
settings (that must be used together) that have to be changed using different APIs
(POST _nodes/reload_secure_settings and PUT _cluster/settings), the settings group
would technically be in an invalid state in between the calls.
This fix builds account objects, and validates the settings, when they are
needed by actions.
This commit is contained in:
Albert Zaharovits 2018-12-17 14:45:55 +02:00 committed by GitHub
parent 2ed6ab9648
commit 6f038997e1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 180 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java
View File

@ -14,6 +14,7 @@ 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.common.util.LazyInitializable;
import java.io.IOException;
import java.io.InputStream;
@ -35,8 +36,8 @@ public abstract class NotificationService<Account> {
private final Settings bootSettings;
private final List<Setting<?>> pluginSecureSettings;
// all are guarded by this
private volatile Map<String, Account> accounts;
private volatile Account defaultAccount;
private volatile Map<String, LazyInitializable<Account, SettingsException>> accounts;
private volatile LazyInitializable<Account, SettingsException> 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 abstract class NotificationService<Account> {
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 @@ public abstract class NotificationService<Account> {
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<String, Account> accounts;
final Account defaultAccount;
final Map<String, LazyInitializable<Account, SettingsException>> accounts;
final LazyInitializable<Account, SettingsException> 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);
LazyInitializable<Account, SettingsException> 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 abstract class NotificationService<Account> {
if (theAccount == null) {
throw new IllegalArgumentException("no account found for name: [" + name + "]");
}
return theAccount;
return theAccount.getOrCompute();
}
private String getNotificationsAccountPrefix() {
@ -124,27 +125,27 @@ public abstract class NotificationService<Account> {
}
private Set<String> 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<String, Account> createAccounts(Settings settings, Set<String> accountNames,
private Map<String, LazyInitializable<Account, SettingsException>> createAccounts(Settings settings, Set<String> accountNames,
BiFunction<String, Settings, Account> accountFactory) {
final Map<String, Account> accounts = new HashMap<>();
final Map<String, LazyInitializable<Account, SettingsException>> 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, new LazyInitializable<>(() -> {
return accountFactory.apply(accountName, accountSettings);
}));
}
return Collections.unmodifiableMap(accounts);
}
private @Nullable Account findDefaultAccountOrNull(Settings settings, Map<String, Account> accounts) {
private @Nullable LazyInitializable<Account, SettingsException> findDefaultAccountOrNull(Settings settings,
Map<String, LazyInitializable<Account, SettingsException>> accounts) {
final String defaultAccountName = getDefaultAccountName(settings);
if (defaultAccountName == null) {
if (accounts.isEmpty()) {
@ -153,7 +154,7 @@ public abstract class NotificationService<Account> {
return accounts.values().iterator().next();
}
} else {
final Account account = accounts.get(defaultAccountName);
final LazyInitializable<Account, SettingsException> account = accounts.get(defaultAccountName);
if (account == null) {
throw new SettingsException("could not find default account [" + defaultAccountName + "]");
}

164
x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/NotificationServiceTests.java
View File

@ -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;
@ -25,6 +40,7 @@ public class NotificationServiceTests extends ESTestCase {
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() {
@ -80,16 +96,160 @@ public class NotificationServiceTests extends ESTestCase {
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<SecureString> secureSetting1 = SecureSetting.secureString("xpack.notification.test.account.secure_only", null);
final Setting<SecureString> secureSetting2 = SecureSetting.secureString("xpack.notification.test.account.mixed.secure", null);
final Map<String, char[]> 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 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<SecureString> secureSetting = SecureSetting.secureString("xpack.notification.test.account.x.secure", null);
final Setting<String> setting = Setting.simpleString("xpack.notification.test.account.x.dynamic", Setting.Property.Dynamic,
Setting.Property.NodeScope);
final AtomicReference<String> secureSettingValue = new AtomicReference<String>(randomAlphaOfLength(4));
final AtomicReference<String> settingValue = new AtomicReference<String>(randomAlphaOfLength(4));
final Map<String, char[]> 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
updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service);
assertThat(validationInvocationCount.get(), is(1));
service.getAccount(null);
assertThat(validationInvocationCount.get(), is(2));
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
updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service);
// update cluster second
updateDynamicClusterSetting(settingValue, setting, settingsBuilder, service);
} else {
// update cluster first
updateDynamicClusterSetting(settingValue, setting, settingsBuilder, service);
// update secure second
updateSecureSetting(secureSettingValue, secureSetting, secureSettingsMap, settingsBuilder, service);
}
assertThat(validationInvocationCount.get(), is(3));
service.getAccount(null);
assertThat(validationInvocationCount.get(), is(4));
}
private static void updateDynamicClusterSetting(AtomicReference<String> settingValue, Setting<String> 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<String> secureSettingValue, Setting<SecureString> secureSetting,
Map<String, char[]> 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<String> {
TestNotificationService(Settings settings) {
super("test", settings, Collections.emptyList());
private final BiConsumer<String, Settings> validator;
TestNotificationService(Settings settings, List<Setting<?>> secureSettings, BiConsumer<String, Settings> validator) {
super("test", settings, secureSettings);
this.validator = validator;
reload(settings);
}
TestNotificationService(Settings settings, List<Setting<?>> secureSettings) {
this(settings, secureSettings, (x, y) -> {});
}
TestNotificationService(Settings settings) {
this(settings, Collections.emptyList(), (x, y) -> {});
}
TestNotificationService(Settings settings, BiConsumer<String, Settings> validator) {
this(settings, Collections.emptyList(), validator);
}
@Override
protected String createAccount(String name, Settings accountSettings) {
validator.accept(name, accountSettings);
return name;
}
}
private static SecureSettings secureSettingsFromMap(Map<String, char[]> 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<String> getSettingNames() {
return secureSettingsMap.keySet();
}
@Override
public InputStream getFile(String setting) throws GeneralSecurityException {
return null;
}
@Override
public void close() throws IOException {
}
};
}
}

2
x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/HipChatServiceTests.java
View File

@ -128,7 +128,7 @@ public class HipChatServiceTests extends ESTestCase {
.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"));
}