diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 5633fe91d51a9..6a682a17cecb1 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -13,6 +13,7 @@ import org.opensearch.common.settings.Settings; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -187,8 +188,12 @@ static class FeatureFlagsImpl { * - Set from JVM system property if flag exists * - Set from provided settings if flag exists * @param openSearchSettings The settings stored in opensearch.yml. + * @param registerFlags new feature flags to register on initialization. */ - void initializeFeatureFlags(Settings openSearchSettings) { + void initializeFeatureFlags(Settings openSearchSettings, List> registerFlags) { + for (Setting flag : registerFlags) { + featureFlags.put(flag, flag.getDefault(Settings.EMPTY)); + } initFromDefaults(); initFromSysProperties(); initFromSettings(openSearchSettings); @@ -272,8 +277,8 @@ void set(String featureFlagName, Boolean value) { /** * Server module public API. */ - public static void initializeFeatureFlags(Settings openSearchSettings) { - featureFlagsImpl.initializeFeatureFlags(openSearchSettings); + public static void initializeFeatureFlags(Settings openSearchSettings, List> registerFlags) { + featureFlagsImpl.initializeFeatureFlags(openSearchSettings, registerFlags); } public static boolean isEnabled(String featureFlagName) { diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 4137f1b37de2a..fc69b95a7d441 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -537,7 +537,7 @@ protected Node( final Settings settings = pluginsService.updatedSettings(); // Ensure to initialize Feature Flags via the settings from opensearch.yml - FeatureFlags.initializeFeatureFlags(settings); + FeatureFlags.initializeFeatureFlags(settings, pluginsService.getPluginFeatureFlags()); final List identityPlugins = new ArrayList<>(); identityPlugins.addAll(pluginsService.filterPlugins(IdentityPlugin.class)); @@ -626,6 +626,7 @@ protected Node( additionalSettings.add(NODE_MASTER_SETTING); additionalSettings.add(NODE_REMOTE_CLUSTER_CLIENT); additionalSettings.addAll(pluginsService.getPluginSettings()); + final List additionalSettingsFilter = new ArrayList<>(pluginsService.getPluginSettingsFilter()); for (final ExecutorBuilder builder : threadPool.builders()) { additionalSettings.addAll(builder.getRegisteredSettings()); diff --git a/server/src/main/java/org/opensearch/plugins/Plugin.java b/server/src/main/java/org/opensearch/plugins/Plugin.java index 4d3830a55059c..254ee3824031f 100644 --- a/server/src/main/java/org/opensearch/plugins/Plugin.java +++ b/server/src/main/java/org/opensearch/plugins/Plugin.java @@ -192,6 +192,13 @@ public List> getSettings() { return Collections.emptyList(); } + /** + * Returns a list of additional {@link org.opensearch.common.util.FeatureFlags} settings for this plugin. + */ + public List> getFeatureFlags() { + return Collections.emptyList(); + } + /** * Returns a list of additional settings filter for this plugin */ diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java index 769494204cc49..c6dab6886a44a 100644 --- a/server/src/main/java/org/opensearch/plugins/PluginsService.java +++ b/server/src/main/java/org/opensearch/plugins/PluginsService.java @@ -114,6 +114,10 @@ public List> getPluginSettings() { return plugins.stream().flatMap(p -> p.v2().getSettings().stream()).collect(Collectors.toList()); } + public List> getPluginFeatureFlags() { + return plugins.stream().flatMap(p -> p.v2().getFeatureFlags().stream()).collect(Collectors.toList()); + } + public List getPluginSettingsFilter() { return plugins.stream().flatMap(p -> p.v2().getSettingsFilter().stream()).collect(Collectors.toList()); } diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index f3751e98f5b60..7427295ff9eea 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -9,15 +9,23 @@ package org.opensearch.common.util; import org.opensearch.common.SuppressForbidden; +import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; +import java.util.Collections; +import java.util.List; + import static org.opensearch.common.util.FeatureFlags.FEATURE_FLAG_PREFIX; public class FeatureFlagTests extends OpenSearchTestCase { // Evergreen test flag private static final String TEST_FLAG = "test.flag.enabled"; + // For testing registration of new feature flags + final String NEW_FLAG = FEATURE_FLAG_PREFIX + "newflag"; + Setting NEW_FLAG_SETTING = Setting.boolSetting(NEW_FLAG, false, Setting.Property.NodeScope); + public void testFeatureFlagsNotInitialized() { FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); @@ -30,15 +38,15 @@ public void testFeatureFlagsFromDefault() { public void testFeatureFlagFromEmpty() { FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY, Collections.emptyList()); assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); } public void testFeatureFlagFromSettings() { FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); - testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, true).build()); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, true).build(), Collections.emptyList()); assertTrue(testFlagsImpl.isEnabled(TEST_FLAG)); - testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build(), Collections.emptyList()); assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); } @@ -78,11 +86,11 @@ public void testFeatureFlagSettingOverwritesSystemProperties() { FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); synchronized (TEST_FLAG) { // sync for sys property setSystemPropertyTrue(TEST_FLAG); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY, Collections.emptyList()); assertTrue(testFlagsImpl.isEnabled(TEST_FLAG)); clearSystemProperty(TEST_FLAG); } - testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build(), Collections.emptyList()); assertFalse(testFlagsImpl.isEnabled(TEST_FLAG)); } @@ -92,13 +100,31 @@ public void testFeatureDoesNotExist() { FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); assertFalse(testFlagsImpl.isEnabled(DNE_FF)); setSystemPropertyTrue(DNE_FF); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY, Collections.emptyList()); assertFalse(testFlagsImpl.isEnabled(DNE_FF)); clearSystemProperty(DNE_FF); - testFlagsImpl.initializeFeatureFlags(Settings.builder().put(DNE_FF, true).build()); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(DNE_FF, true).build(), Collections.emptyList()); assertFalse(testFlagsImpl.isEnabled(DNE_FF)); } + public void testRegisterNewFlagSetWithSettings() { + final String NEW_FLAG = FEATURE_FLAG_PREFIX + "newflag"; + Setting NEW_FLAG_SETTING = Setting.boolSetting(NEW_FLAG, false, Setting.Property.NodeScope); + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(NEW_FLAG, true).build(), List.of(NEW_FLAG_SETTING)); + assertTrue(testFlagsImpl.isEnabled(NEW_FLAG)); + } + + @SuppressForbidden(reason = "Testing with system property") + public void testRegisterNewFlagSetWithSysProp() { + final String NEW_FLAG = FEATURE_FLAG_PREFIX + "newflag"; + Setting NEW_FLAG_SETTING = Setting.boolSetting(NEW_FLAG, false, Setting.Property.NodeScope); + FeatureFlags.FeatureFlagsImpl testFlagsImpl = new FeatureFlags.FeatureFlagsImpl(); + setSystemPropertyTrue(NEW_FLAG); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY, List.of(NEW_FLAG_SETTING)); + assertTrue(testFlagsImpl.isEnabled(NEW_FLAG)); + } + /** * Test global feature flag instance. */ @@ -106,7 +132,7 @@ public void testFeatureDoesNotExist() { public void testLockFeatureFlagWithFlagLock() { try (FeatureFlags.TestUtils.FlagWriteLock ignore = new FeatureFlags.TestUtils.FlagWriteLock(TEST_FLAG)) { assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); - FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build(), Collections.emptyList()); assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked } } @@ -114,7 +140,7 @@ public void testLockFeatureFlagWithFlagLock() { public void testLockFeatureFlagWithHelper() throws Exception { FeatureFlags.TestUtils.with(TEST_FLAG, () -> { assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); - FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build(), Collections.emptyList()); assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked }); } @@ -122,7 +148,14 @@ public void testLockFeatureFlagWithHelper() throws Exception { @LockFeatureFlag(TEST_FLAG) public void testLockFeatureFlagAnnotation() { assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); - FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build()); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(TEST_FLAG, false).build(), Collections.emptyList()); assertTrue(FeatureFlags.isEnabled(TEST_FLAG)); // flag is locked } + + public void testRegisterNewFlagSetWithWriteLock() { + FeatureFlags.initializeFeatureFlags(Settings.EMPTY, List.of(NEW_FLAG_SETTING)); + try (FeatureFlags.TestUtils.FlagWriteLock ignore = new FeatureFlags.TestUtils.FlagWriteLock(NEW_FLAG)) { + assertTrue(FeatureFlags.isEnabled(NEW_FLAG)); + } + } } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/RangeAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/RangeAggregatorTests.java index 68d8423338f01..a5c8e2664bddb 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/RangeAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/RangeAggregatorTests.java @@ -50,6 +50,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Random; @@ -73,12 +74,12 @@ public class RangeAggregatorTests extends AggregatorTestCase { @Before public void setup() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build()); + FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.STAR_TREE_INDEX, true).build(), Collections.emptyList()); } @After public void teardown() throws IOException { - FeatureFlags.initializeFeatureFlags(Settings.EMPTY); + FeatureFlags.initializeFeatureFlags(Settings.EMPTY, Collections.emptyList()); } protected Codec getCodec() {