From daaabc4731db4ffff6f85d8e07f698eaf52b45f9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 21 Mar 2024 23:51:51 -0400 Subject: [PATCH 1/6] Fix issue with feature flags passed as system props where default value was not being honored Signed-off-by: Craig Perkins --- .../opensearch/common/util/FeatureFlags.java | 62 +++++++++++-------- .../common/util/FeatureFlagTests.java | 8 +++ 2 files changed, 43 insertions(+), 27 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 8633cf1fe25ea..ee0e67e41edde 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -65,10 +65,44 @@ public class FeatureFlags { */ public static final String PLUGGABLE_CACHE = "opensearch.experimental.feature.pluggable.caching.enabled"; + public static final Setting REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting( + REMOTE_STORE_MIGRATION_EXPERIMENTAL, + false, + Property.NodeScope + ); + + public static final Setting EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope); + + public static final Setting IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope); + + public static final Setting TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope); + + public static final Setting DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting( + DATETIME_FORMATTER_CACHING, + true, + Property.NodeScope + ); + + public static final Setting WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting( + WRITEABLE_REMOTE_INDEX, + false, + Property.NodeScope + ); + + public static final Setting PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope); + /** * Should store the settings from opensearch.yml. */ - private static Settings settings; + private static Settings settings = Settings.builder() + .put(REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING.getKey(), REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)) + .put(EXTENSIONS_SETTING.getKey(), EXTENSIONS_SETTING.getDefault(Settings.EMPTY)) + .put(IDENTITY_SETTING.getKey(), IDENTITY_SETTING.getDefault(Settings.EMPTY)) + .put(TELEMETRY_SETTING.getKey(), TELEMETRY_SETTING.getDefault(Settings.EMPTY)) + .put(DATETIME_FORMATTER_CACHING_SETTING.getKey(), DATETIME_FORMATTER_CACHING_SETTING.getDefault(Settings.EMPTY)) + .put(WRITEABLE_REMOTE_INDEX_SETTING.getKey(), WRITEABLE_REMOTE_INDEX_SETTING.getDefault(Settings.EMPTY)) + .put(PLUGGABLE_CACHE_SETTING.getKey(), PLUGGABLE_CACHE_SETTING.getDefault(Settings.EMPTY)) + .build();; /** * This method is responsible to map settings from opensearch.yml to local stored @@ -103,30 +137,4 @@ public static boolean isEnabled(Setting featureFlag) { return featureFlag.getDefault(Settings.EMPTY); } } - - public static final Setting REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting( - REMOTE_STORE_MIGRATION_EXPERIMENTAL, - false, - Property.NodeScope - ); - - public static final Setting EXTENSIONS_SETTING = Setting.boolSetting(EXTENSIONS, false, Property.NodeScope); - - public static final Setting IDENTITY_SETTING = Setting.boolSetting(IDENTITY, false, Property.NodeScope); - - public static final Setting TELEMETRY_SETTING = Setting.boolSetting(TELEMETRY, false, Property.NodeScope); - - public static final Setting DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting( - DATETIME_FORMATTER_CACHING, - true, - Property.NodeScope - ); - - public static final Setting WRITEABLE_REMOTE_INDEX_SETTING = Setting.boolSetting( - WRITEABLE_REMOTE_INDEX, - false, - Property.NodeScope - ); - - public static final Setting 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 index f175308482b15..5e95ac2e865b0 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -11,6 +11,8 @@ import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; +import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING; + public class FeatureFlagTests extends OpenSearchTestCase { private final String FLAG_PREFIX = "opensearch.experimental.feature."; @@ -33,4 +35,10 @@ 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)); + } } From 86769ec028cc57402124d83fe2d4d52332bbd7a8 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 22 Mar 2024 00:13:32 -0400 Subject: [PATCH 2/6] Add CHANGELOG entry Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index af20332c61146..b3ba6b4596765 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,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 From ea21ee88a0ed5d4e4480c733337ee4ce38808531 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 22 Mar 2024 11:38:47 -0400 Subject: [PATCH 3/6] Add test for default value of false Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/common/util/FeatureFlags.java | 2 +- .../java/org/opensearch/common/util/FeatureFlagTests.java | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) 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 ee0e67e41edde..a9a078acff1f0 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -102,7 +102,7 @@ public class FeatureFlags { .put(DATETIME_FORMATTER_CACHING_SETTING.getKey(), DATETIME_FORMATTER_CACHING_SETTING.getDefault(Settings.EMPTY)) .put(WRITEABLE_REMOTE_INDEX_SETTING.getKey(), WRITEABLE_REMOTE_INDEX_SETTING.getDefault(Settings.EMPTY)) .put(PLUGGABLE_CACHE_SETTING.getKey(), PLUGGABLE_CACHE_SETTING.getDefault(Settings.EMPTY)) - .build();; + .build(); /** * This method is responsible to map settings from opensearch.yml to local stored 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 5e95ac2e865b0..0fc4ddd15e5bd 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -12,6 +12,7 @@ import org.opensearch.test.OpenSearchTestCase; import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING; +import static org.opensearch.common.util.FeatureFlags.IDENTITY; public class FeatureFlagTests extends OpenSearchTestCase { @@ -41,4 +42,10 @@ public void testBooleanFeatureFlagWithDefaultSetToTrue() { assertNotNull(testFlag); assertTrue(FeatureFlags.isEnabled(testFlag)); } + + public void testBooleanFeatureFlagWithDefaultSetToFalse() { + final String testFlag = IDENTITY; + assertNotNull(testFlag); + assertFalse(FeatureFlags.isEnabled(testFlag)); + } } From c8885f9bdd71029b786a8a5c38c114482c14f36d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 22 Mar 2024 17:15:35 -0400 Subject: [PATCH 4/6] Fix issue when empty settings passed in initialization Signed-off-by: Craig Perkins --- .../opensearch/common/util/FeatureFlags.java | 37 +++++++++++++------ .../common/util/FeatureFlagTests.java | 9 +++++ 2 files changed, 35 insertions(+), 11 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 a9a078acff1f0..3a926721dad40 100644 --- 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 @@ -91,18 +93,27 @@ public class FeatureFlags { public static final Setting PLUGGABLE_CACHE_SETTING = Setting.boolSetting(PLUGGABLE_CACHE, false, Property.NodeScope); + private static final List> 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 = Settings.builder() - .put(REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING.getKey(), REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)) - .put(EXTENSIONS_SETTING.getKey(), EXTENSIONS_SETTING.getDefault(Settings.EMPTY)) - .put(IDENTITY_SETTING.getKey(), IDENTITY_SETTING.getDefault(Settings.EMPTY)) - .put(TELEMETRY_SETTING.getKey(), TELEMETRY_SETTING.getDefault(Settings.EMPTY)) - .put(DATETIME_FORMATTER_CACHING_SETTING.getKey(), DATETIME_FORMATTER_CACHING_SETTING.getDefault(Settings.EMPTY)) - .put(WRITEABLE_REMOTE_INDEX_SETTING.getKey(), WRITEABLE_REMOTE_INDEX_SETTING.getDefault(Settings.EMPTY)) - .put(PLUGGABLE_CACHE_SETTING.getKey(), PLUGGABLE_CACHE_SETTING.getDefault(Settings.EMPTY)) - .build(); + private static Settings settings; + + static { + Settings.Builder settingsBuilder = Settings.builder(); + for (Setting 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 @@ -111,7 +122,11 @@ public class FeatureFlags { * @param openSearchSettings The settings stored in opensearch.yml. */ public static void initializeFeatureFlags(Settings openSearchSettings) { - settings = openSearchSettings; + Settings.Builder settingsBuilder = Settings.builder(); + for (Setting ffSetting : ALL_FEATURE_FLAG_SETTINGS) { + settingsBuilder = settingsBuilder.put(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings)); + } + settings = settingsBuilder.build(); } /** 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 0fc4ddd15e5bd..c1c15a6cc84e6 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -8,6 +8,7 @@ package org.opensearch.common.util; +import org.opensearch.common.settings.Settings; import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.OpenSearchTestCase; @@ -45,7 +46,15 @@ public void testBooleanFeatureFlagWithDefaultSetToTrue() { 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)); + } } From 768e161af4b26bb69c090e4b6cbcc9db22b848f0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 22 Mar 2024 17:21:47 -0400 Subject: [PATCH 5/6] Get actual value from settings and default from ff setting Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/common/util/FeatureFlags.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 3a926721dad40..bdfce72d106d3 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -124,7 +124,10 @@ public class FeatureFlags { public static void initializeFeatureFlags(Settings openSearchSettings) { Settings.Builder settingsBuilder = Settings.builder(); for (Setting ffSetting : ALL_FEATURE_FLAG_SETTINGS) { - settingsBuilder = settingsBuilder.put(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings)); + settingsBuilder = settingsBuilder.put( + ffSetting.getKey(), + openSearchSettings.getAsBoolean(ffSetting.getKey(), ffSetting.getDefault(openSearchSettings)) + ); } settings = settingsBuilder.build(); } From 565109e991035227b116a0cd08be7d5c93826a46 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 22 Mar 2024 17:26:10 -0400 Subject: [PATCH 6/6] Add test with non-empty setting initialization Signed-off-by: Craig Perkins --- .../org/opensearch/common/util/FeatureFlagTests.java | 10 ++++++++++ 1 file changed, 10 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 c1c15a6cc84e6..88cb3782252b7 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -13,6 +13,7 @@ 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 { @@ -57,4 +58,13 @@ public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToTru 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); + } }