Watcher accounts constructed lazily

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;
@@ -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 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,49 +103,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()) {
@@ -153,7 +154,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 + "]");
             }

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 {
+            }
+        };
+    }
 }

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"));
     }