Fix issue with feature flags where default value may not be honored (#…

…12849) (#12912) * 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> (cherry picked from commit 12f1487)
opensearch-project · Mar 26, 2024 · f96669a · f96669a
1 parent 3418137
commit f96669a
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,6 +21,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
 

diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java
@@ -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
@@ -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();
     }
 
     /**
@@ -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);
 }
diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java
@@ -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.";
@@ -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);
+    }
 }