[BUG] Unable to flip feature flags defined in FeatureFlags.java

[noCharger]

Describe the bug

A clear and concise description of what the bug is.

The feature flags used for experimental are defined here. This below example creates an setting static object with the feature flag name and default value.

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

However, if the feature flag is not specified as an env variable or written in the yml file, the FeatureFlags.isEnabled method does not read the value from these static settings and instead sets to 'false' by default.

    /**
     * Used to test feature flags whose values are expected to be booleans.
     * This method returns true if the value is "true" (case-insensitive),
     * and false otherwise.
     */
    public static boolean isEnabled(String featureFlagName) {
        if ("true".equalsIgnoreCase(System.getProperty(featureFlagName))) {
            // TODO: Remove the if condition once FeatureFlags are only supported via opensearch.yml
            return true;
        }

        return settings != null && settings.getAsBoolean(featureFlagName, false);
    }

Because these feature flag settings are never initialized / registered during node boostrap, it causes the difficulty when adding a static setting with the default value of true to keep the feature enabled but still gated behind the flag.

For example on node.java, if the default value on FeatureFlags.IDENTITY is set to true, the FeatureFlags.isEnabled will always return false.

    final Settings settings = pluginsService.updatedSettings();

    // Ensure to initialize Feature Flags via the settings from opensearch.yml
    FeatureFlags.initializeFeatureFlags(settings);

    final List<IdentityPlugin> identityPlugins = new ArrayList<>();
    if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) {
        ...
       # this condition will never entered
    }

To Reproduce
Steps to reproduce the behavior:

1. Set any feature flag (for example IDENTITY_SETTING) default value to true

   OpenSearch/server/src/main/java/org/opensearch/common/util/FeatureFlags.java

   Line 103 in 8cfde6c

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

2. Add logs around FeatureFlags.isEnabled method usage during node bootstrap

   OpenSearch/server/src/main/java/org/opensearch/node/Node.java

   Lines 467 to 473 in 8cfde6c

   	final Settings settings = pluginsService.updatedSettings();
   	// Ensure to initialize Feature Flags via the settings from opensearch.yml
   	FeatureFlags.initializeFeatureFlags(settings);
   	final List<IdentityPlugin> identityPlugins = new ArrayList<>();
   	if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) {

    # Example logging
    logger.info("louis-feature-flag-test: " + FeatureFlags.IDENTITY + " value: " + settings.get(FeatureFlags.IDENTITY));
    if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) {
        // If identity is enabled load plugins implementing the extension point
        logger.info("Identity on so found plugins implementing: " + pluginsService.filterPlugins(IdentityPlugin.class).toString());
        identityPlugins.addAll(pluginsService.filterPlugins(IdentityPlugin.class));
    }

3. Start cluster

./gradlew run --debug-server-jvm --info 

4. See error

[2023-08-25T12:46:29,937][INFO ][o.o.n.Node               ] [runTask-0] louis-feature-flag-test: opensearch.experimental.feature.identity.enabled value: null
[2023-08-25T12:46:29,938][INFO ][o.o.n.Node               ] [runTask-0] louis-feature-flag-test: opensearch.experimental.feature.extensions.enabled value: null

Expected behavior

A clear and concise description of what you expected to happen.

Some options here

1. Disallow the use case of adding a static setting with the default value of 'true' in order to keep the feature enabled but still gated behind the flag: We could limit the feature flag's functionality by concealing the default_value during static setting construction. The default value is then set to always false. On public doc and java doc, we should explictly mention that flags are required to be removed when you want to enable the feature.
2. FeatureFlags.isEnabled reads the setting object's real default_value.

[dblock]

I think that a feature flag that allows a default value should ... respect that default value, so this is a real bug. Do you think you can turn it into a unit test?

[jayeshathila]

I guess we can use the existing BUILT_IN_FEATURE_FLAGS set and modify isEnabled .

The behaviour of settings.get(FeatureFlags.IDENTITY) to return null is seeming correct only, we only need to modify the isEnable function.

Proposal draft PR: #9691 [Pending UT and log line removal]

Thoughts ?

[dblock]

I am not sure, feels like a workaround for the problem instead of fixing the actual problem.

I'd really start by writing tests that reproduce the various states of feature flags, then it becomes easy to try and produce a fix that works in all cases.

[jayeshathila]

@dblock, can you confirm what is expectation out of the below ones:

1. UT for isEnabled, which tests for all the possibility of returned values and hence covering this use case of default value (in public static fields) being true as well.

2. UT for public static feature flags and their different values should be returned from isEnabled.

3. General UT which will test settings Setting.boolSetting, floatSetting etc ?

[dblock]

I don't have the answers to these without digging deep into what's supposed to be returned or existing tests. I was noting that you found one case where the flag doesn't work as one would expect, so I was commenting on wanting tests (some may exist) that cover all permutations.

PS: Do note that settings can also be declared in plugins.

[noCharger]

        if (settings != null && FeatureFlagSettings.FEATURE_FLAG_MAPPING.get(featureFlagName) != null) {
            defaultValue = (boolean) FeatureFlagSettings.FEATURE_FLAG_MAPPING.get(featureFlagName).getDefault(settings);
        }

If I understand correctly, getDefault(settings) will retrieve the value of a setting from settings, which does not contain FeatureFlagSettings.FEATURE_FLAG_MAPPING unless any of them included in opensearch.yml. As a result, it will still return null.

[jayeshathila]

@noCharger No getDefaults implementation is little un-intuitive in this case, it will return the default value of FeatureFlagSettings.FEATURE_FLAG_MAPPING .

getDefaultRaw implementation

    public String getDefault(Settings settings) {
        return defaultValue.apply(settings);
    }

here ^ the argument (settings ) value will be ignored because the implementation of defaultValue function is no-op for all practical purposes and this method will return defaultValue of featureFlag.

The defaultValue function implementation while initializing the featureFlag Setting.boolSetting is

 (s) -> Boolean.toString(defaultValue)

[msfroh]

@jayeshathila -- can we assign this issue to you, since it looks like you've been working on a fix?

[jayeshathila]

@msfroh Sure, please assign it to me.

[cwperks]

Closing this issue as #12849 was merged.