Skip to content

Allow new feature flags registered from plugin #17952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<Setting<Boolean>> registerFlags) {
for (Setting<Boolean> flag : registerFlags) {
featureFlags.put(flag, flag.getDefault(Settings.EMPTY));
}
initFromDefaults();
initFromSysProperties();
initFromSettings(openSearchSettings);
Expand Down Expand Up @@ -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<Setting<Boolean>> registerFlags) {
featureFlagsImpl.initializeFeatureFlags(openSearchSettings, registerFlags);
}

public static boolean isEnabled(String featureFlagName) {
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/node/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<IdentityPlugin> identityPlugins = new ArrayList<>();
identityPlugins.addAll(pluginsService.filterPlugins(IdentityPlugin.class));
Expand Down Expand Up @@ -626,6 +626,7 @@ protected Node(
additionalSettings.add(NODE_MASTER_SETTING);
additionalSettings.add(NODE_REMOTE_CLUSTER_CLIENT);
additionalSettings.addAll(pluginsService.getPluginSettings());

final List<String> additionalSettingsFilter = new ArrayList<>(pluginsService.getPluginSettingsFilter());
for (final ExecutorBuilder<?> builder : threadPool.builders()) {
additionalSettings.addAll(builder.getRegisteredSettings());
Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/opensearch/plugins/Plugin.java
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,13 @@ public List<Setting<?>> getSettings() {
return Collections.emptyList();
}

/**
* Returns a list of additional {@link org.opensearch.common.util.FeatureFlags} settings for this plugin.
*/
public List<Setting<Boolean>> getFeatureFlags() {
return Collections.emptyList();
}

/**
* Returns a list of additional settings filter for this plugin
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ public List<Setting<?>> getPluginSettings() {
return plugins.stream().flatMap(p -> p.v2().getSettings().stream()).collect(Collectors.toList());
}

public List<Setting<Boolean>> getPluginFeatureFlags() {
return plugins.stream().flatMap(p -> p.v2().getFeatureFlags().stream()).collect(Collectors.toList());
}

public List<String> getPluginSettingsFilter() {
return plugins.stream().flatMap(p -> p.v2().getSettingsFilter().stream()).collect(Collectors.toList());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boolean> 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));
Expand All @@ -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));
}

Expand Down Expand Up @@ -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));
}

Expand All @@ -92,37 +100,62 @@ 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<Boolean> 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<Boolean> 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.
*/

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
}
}

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

@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));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down
Loading