From dad60d77815224e3856b81e65f6711ccd57a576c Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 15 Apr 2025 12:31:18 -0700 Subject: [PATCH 1/4] Allow registration of new feature flags - For plugins. Signed-off-by: Finn Carroll --- .../org/opensearch/common/util/FeatureFlags.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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..4fd5376db69ca 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -180,6 +180,16 @@ static class FeatureFlagsImpl { initFromSysProperties(); } + /** + * Enables plugins to provide their own feature flags + * by registering new flags. New flags should be registered statically + * to ensure they are known previous to initializeFeatureFlags(). + * @param newFeatureFlag Feature flag to register. + */ + void registerFeatureFlag(Setting newFeatureFlag) { + featureFlags.put(newFeatureFlag, newFeatureFlag.getDefault(Settings.EMPTY)); + } + /** * Initialize feature flags map from the following sources: * (Each source overwrites previous feature flags) @@ -272,6 +282,10 @@ void set(String featureFlagName, Boolean value) { /** * Server module public API. */ + public static void registerFeatureFlag(Setting newFeatureFlag) { + featureFlagsImpl.registerFeatureFlag(newFeatureFlag); + } + public static void initializeFeatureFlags(Settings openSearchSettings) { featureFlagsImpl.initializeFeatureFlags(openSearchSettings); } From 4c4b535fd9ce1055483c7fa6d3d5bc0b37e3c555 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 15 Apr 2025 12:56:21 -0700 Subject: [PATCH 2/4] Test register new flag. Signed-off-by: Finn Carroll --- .../common/util/FeatureFlagTests.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) 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..399bb3f73d44d 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -9,6 +9,7 @@ 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; @@ -99,6 +100,30 @@ public void testFeatureDoesNotExist() { assertFalse(testFlagsImpl.isEnabled(DNE_FF)); } + @SuppressForbidden(reason = "Testing with system property") + public void testRegisterNewFlag() { + 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.registerFeatureFlag(NEW_FLAG_SETTING); + assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); + + // set with sys prop + setSystemPropertyTrue(NEW_FLAG); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertTrue(testFlagsImpl.isEnabled(NEW_FLAG)); + clearSystemProperty(NEW_FLAG); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); + + // set with settings + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(NEW_FLAG, true).build()); + assertTrue(testFlagsImpl.isEnabled(NEW_FLAG)); + testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); + } + /** * Test global feature flag instance. */ From d406859936d30013f9d1eb3d60edb8950ea95a28 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 15 Apr 2025 14:56:48 -0700 Subject: [PATCH 3/4] Add getFeatureFlags to Plugin API. Fetch aggregated plugin feature flags from PluginService. Register these feature flags only once in Node on init. Signed-off-by: Finn Carroll --- server/src/main/java/org/opensearch/node/Node.java | 4 ++++ server/src/main/java/org/opensearch/plugins/Plugin.java | 7 +++++++ .../main/java/org/opensearch/plugins/PluginsService.java | 4 ++++ 3 files changed, 15 insertions(+) diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 4137f1b37de2a..0820ca4ae8493 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -626,6 +626,10 @@ protected Node( additionalSettings.add(NODE_MASTER_SETTING); additionalSettings.add(NODE_REMOTE_CLUSTER_CLIENT); additionalSettings.addAll(pluginsService.getPluginSettings()); + for (Setting ff : pluginsService.getPluginFeatureFlags()) { + FeatureFlags.registerFeatureFlag(ff); + } + 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()); } From 488e00f34b237e9b4db88e18314e945ff7c31c46 Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Tue, 15 Apr 2025 16:48:07 -0700 Subject: [PATCH 4/4] Move registration of new feature flags to initializeFeatureFlags. Signed-off-by: Finn Carroll --- .../opensearch/common/util/FeatureFlags.java | 25 +++----- .../main/java/org/opensearch/node/Node.java | 5 +- .../common/util/FeatureFlagTests.java | 60 +++++++++++-------- .../startree/RangeAggregatorTests.java | 5 +- 4 files changed, 46 insertions(+), 49 deletions(-) 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 4fd5376db69ca..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; @@ -180,16 +181,6 @@ static class FeatureFlagsImpl { initFromSysProperties(); } - /** - * Enables plugins to provide their own feature flags - * by registering new flags. New flags should be registered statically - * to ensure they are known previous to initializeFeatureFlags(). - * @param newFeatureFlag Feature flag to register. - */ - void registerFeatureFlag(Setting newFeatureFlag) { - featureFlags.put(newFeatureFlag, newFeatureFlag.getDefault(Settings.EMPTY)); - } - /** * Initialize feature flags map from the following sources: * (Each source overwrites previous feature flags) @@ -197,8 +188,12 @@ void registerFeatureFlag(Setting newFeatureFlag) { * - 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); @@ -282,12 +277,8 @@ void set(String featureFlagName, Boolean value) { /** * Server module public API. */ - public static void registerFeatureFlag(Setting newFeatureFlag) { - featureFlagsImpl.registerFeatureFlag(newFeatureFlag); - } - - 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 0820ca4ae8493..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,9 +626,6 @@ protected Node( additionalSettings.add(NODE_MASTER_SETTING); additionalSettings.add(NODE_REMOTE_CLUSTER_CLIENT); additionalSettings.addAll(pluginsService.getPluginSettings()); - for (Setting ff : pluginsService.getPluginFeatureFlags()) { - FeatureFlags.registerFeatureFlag(ff); - } final List additionalSettingsFilter = new ArrayList<>(pluginsService.getPluginSettingsFilter()); for (final ExecutorBuilder builder : threadPool.builders()) { 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 399bb3f73d44d..7427295ff9eea 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -13,12 +13,19 @@ 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)); @@ -31,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)); } @@ -79,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)); } @@ -93,35 +100,29 @@ 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)); } - @SuppressForbidden(reason = "Testing with system property") - public void testRegisterNewFlag() { + 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.registerFeatureFlag(NEW_FLAG_SETTING); - assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); - - // set with sys prop - setSystemPropertyTrue(NEW_FLAG); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); + testFlagsImpl.initializeFeatureFlags(Settings.builder().put(NEW_FLAG, true).build(), List.of(NEW_FLAG_SETTING)); assertTrue(testFlagsImpl.isEnabled(NEW_FLAG)); - clearSystemProperty(NEW_FLAG); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); - assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); + } - // set with settings - testFlagsImpl.initializeFeatureFlags(Settings.builder().put(NEW_FLAG, true).build()); + @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)); - testFlagsImpl.initializeFeatureFlags(Settings.EMPTY); - assertFalse(testFlagsImpl.isEnabled(NEW_FLAG)); } /** @@ -131,7 +132,7 @@ public void testRegisterNewFlag() { 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 } } @@ -139,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 }); } @@ -147,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() {