Watcher accounts constructed lazily (#36656)

albertzaharovits · albertzaharovits · commit 436336e7e6ac · 2018-12-17T16:24:23.000+02:00
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.
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
@@ -13,6 +13,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;
@@ -33,8 +34,8 @@ public abstract class NotificationService<Account> extends AbstractComponent {
     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;
@@ -56,7 +57,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
@@ -99,49 +100,49 @@ 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<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");
         }
         if (theAccount == null) {
             throw new IllegalArgumentException("no account found for name: [" + name + "]");
         }
-        return theAccount;
+        return theAccount.getOrCompute();
     }
 
     private String getNotificationsAccountPrefix() {
         return "xpack.notification." + type + ".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()) {
@@ -150,7 +151,7 @@ private Map<String, Account> createAccounts(Settings settings, Set<String> accou
                 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 + "]");
             }
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
@@ -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 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() {
@@ -80,16 +96,160 @@ 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<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 {
+            }
+        };
+    }
 }
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
@@ -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"));
     }
 

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ public void testSingleAccountIntegrationNoRoomSetting() throws Exception {`
`128`	`128`	`.put("xpack.notification.hipchat.account." + accountName + ".auth_token", "_token");`
`129`	`129`	`SettingsException e = expectThrows(SettingsException.class, () ->`
`130`	`130`	`new HipChatService(settingsBuilder.build(), httpClient,`
`131`		`- new ClusterSettings(settingsBuilder.build(), new HashSet<>(HipChatService.getSettings()))));`
	`131`	`+ new ClusterSettings(settingsBuilder.build(), new HashSet<>(HipChatService.getSettings()))).getAccount(null));`
`132`	`132`	`assertThat(e.getMessage(), containsString("missing required [room] setting for [integration] account profile"));`
`133`	`133`	`}`
`134`	`134`