Skip to content

Commit

Permalink
Fix issue with feature flags where default value may not be honored (#…
Browse files Browse the repository at this point in the history 
…12849)

* Fix issue with feature flags passed as system props where default value was not being honored

Signed-off-by: Craig Perkins <craig5008@gmail.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <craig5008@gmail.com>

* Add test for default value of false

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Fix issue when empty settings passed in initialization

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Get actual value from settings and default from ff setting

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add test with non-empty setting initialization

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <craig5008@gmail.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
  • Loading branch information
@cwperks
cwperks authored Mar 25, 2024
1 parent 70711cf commit 12f1487
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 28 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Removed

### Fixed
- Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849))

### Security

Expand Down
82 changes: 54 additions & 28 deletions server/src/main/java/org/opensearch/common/util/FeatureFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
import org.opensearch.common.settings.Setting.Property;
import org.opensearch.common.settings.Settings;

import java.util.List;

/**
* Utility class to manage feature flags. Feature flags are system properties that must be set on the JVM.
* These are used to gate the visibility/availability of incomplete features. Fore more information, see
* These are used to gate the visibility/availability of incomplete features. For more information, see
* https://featureflags.io/feature-flag-introduction/
*
* @opensearch.internal
Expand Down Expand Up @@ -65,19 +67,69 @@ public class FeatureFlags {
*/
public static final String PLUGGABLE_CACHE = "opensearch.experimental.feature.pluggable.caching.enabled";

public static final Setting<Boolean> REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
REMOTE_STORE_MIGRATION_EXPERIMENTAL,
false,
Property.NodeScope
);

public static final Setting<Boolean> EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope);

public static final Setting<Boolean> IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope);

public static final Setting<Boolean> TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope);

public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
DATETIME_FORMATTER_CACHING,
true,
Property.NodeScope
);

public static final Setting<Boolean> WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting(
WRITEABLE_REMOTE_INDEX,
false,
Property.NodeScope
);

public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);

private static final List<Setting<Boolean>> ALL_FEATURE_FLAG_SETTINGS = List.of(
REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING,
EXTENSIONS_SETTING,
IDENTITY_SETTING,
TELEMETRY_SETTING,
DATETIME_FORMATTER_CACHING_SETTING,
WRITEABLE_REMOTE_INDEX_SETTING,
PLUGGABLE_CACHE_SETTING
);
/**
* Should store the settings from opensearch.yml.
*/
private static Settings settings;

static {
Settings.Builder settingsBuilder = Settings.builder();
for (Setting<Boolean> ffSetting : ALL_FEATURE_FLAG_SETTINGS) {
settingsBuilder = settingsBuilder.put(ffSetting.getKey(), ffSetting.getDefault(Settings.EMPTY));
}
settings = settingsBuilder.build();
}

/**
* This method is responsible to map settings from opensearch.yml to local stored
* settings value. That is used for the existing isEnabled method.
*
* @param openSearchSettings The settings stored in opensearch.yml.
*/
public static void initializeFeatureFlags(Settings openSearchSettings) {
settings = openSearchSettings;
Settings.Builder settingsBuilder = Settings.builder();
for (Setting<Boolean> ffSetting : ALL_FEATURE_FLAG_SETTINGS) {
settingsBuilder = settingsBuilder.put(
ffSetting.getKey(),
openSearchSettings.getAsBoolean(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings))
);
}
settings = settingsBuilder.build();
}

/**
Expand All @@ -103,30 +155,4 @@ public static boolean isEnabled(Setting<Boolean> featureFlag) {
return featureFlag.getDefault(Settings.EMPTY);
}
}

public static final Setting<Boolean> REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
REMOTE_STORE_MIGRATION_EXPERIMENTAL,
false,
Property.NodeScope
);

public static final Setting<Boolean> EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope);

public static final Setting<Boolean> IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope);

public static final Setting<Boolean> TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope);

public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
DATETIME_FORMATTER_CACHING,
true,
Property.NodeScope
);

public static final Setting<Boolean> WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting(
WRITEABLE_REMOTE_INDEX,
false,
Property.NodeScope
);

public static final Setting<Boolean> PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope);
}
34 changes: 34 additions & 0 deletions server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,14 @@

package org.opensearch.common.util;

import org.opensearch.common.settings.Settings;
import org.opensearch.test.FeatureFlagSetter;
import org.opensearch.test.OpenSearchTestCase;

import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING;
import static org.opensearch.common.util.FeatureFlags.EXTENSIONS;
import static org.opensearch.common.util.FeatureFlags.IDENTITY;

public class FeatureFlagTests extends OpenSearchTestCase {

private final String FLAG_PREFIX = "opensearch.experimental.feature.";
Expand All @@ -33,4 +38,33 @@ public void testNonBooleanFeatureFlag() {
assertNotNull(System.getProperty(javaVersionProperty));
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
}

public void testBooleanFeatureFlagWithDefaultSetToTrue() {
final String testFlag = DATETIME_FORMATTER_CACHING;
assertNotNull(testFlag);
assertTrue(FeatureFlags.isEnabled(testFlag));
}

public void testBooleanFeatureFlagWithDefaultSetToFalse() {
final String testFlag = IDENTITY;
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
assertNotNull(testFlag);
assertFalse(FeatureFlags.isEnabled(testFlag));
}

public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToTrue() {
final String testFlag = DATETIME_FORMATTER_CACHING;
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
assertNotNull(testFlag);
assertTrue(FeatureFlags.isEnabled(testFlag));
}

public void testInitializeFeatureFlagsWithExperimentalSettings() {
FeatureFlags.initializeFeatureFlags(Settings.builder().put(IDENTITY, true).build());
assertTrue(FeatureFlags.isEnabled(IDENTITY));
assertTrue(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING));
assertFalse(FeatureFlags.isEnabled(EXTENSIONS));
// reset FeatureFlags to defaults
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
}
}

0 comments on commit 12f1487

Please sign in to comment.