Browse Source

Add utility class for feature flags (#94511)

This utility class is intended to simplify the registration of feature
flags within the main (non-test) Elasticsearch code.

It encapsulates the logic for:
1. Enabling feature flags automatically in snapshot builds
2. Reading the feature flag value from a system property in release
   builds
3. Validation of allowed flag values
Tim Vernum 2 years ago
parent
commit
822a7f4843

+ 3 - 9
server/src/main/java/org/elasticsearch/cluster/metadata/DataLifecycle.java

@@ -8,7 +8,6 @@
 
 package org.elasticsearch.cluster.metadata;
 
-import org.elasticsearch.Build;
 import org.elasticsearch.action.admin.indices.rollover.RolloverConditions;
 import org.elasticsearch.cluster.Diff;
 import org.elasticsearch.cluster.SimpleDiffable;
@@ -16,7 +15,7 @@ import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.Setting;
-import org.elasticsearch.core.Booleans;
+import org.elasticsearch.common.util.FeatureFlag;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
@@ -66,7 +65,7 @@ public class DataLifecycle implements SimpleDiffable<DataLifecycle>, ToXContentO
         }
     }
 
-    private static final boolean FEATURE_FLAG_ENABLED;
+    private static final FeatureFlag DLM_FEATURE_FLAG = new FeatureFlag("dlm");
 
     public static final DataLifecycle EMPTY = new DataLifecycle();
     public static final String DLM_ORIGIN = "data_lifecycle";
@@ -81,11 +80,6 @@ public class DataLifecycle implements SimpleDiffable<DataLifecycle>, ToXContentO
     );
 
     static {
-        final String property = System.getProperty("es.dlm_feature_flag_enabled");
-        if (Build.CURRENT.isSnapshot() && property != null) {
-            throw new IllegalArgumentException("es.dlm_feature_flag_enabled is only supported in non-snapshot builds");
-        }
-        FEATURE_FLAG_ENABLED = Booleans.parseBoolean(property, false);
         PARSER.declareField(
             ConstructingObjectParser.optionalConstructorArg(),
             (p, c) -> TimeValue.parseTimeValue(p.textOrNull(), DATA_RETENTION_FIELD.getPreferredName()),
@@ -95,7 +89,7 @@ public class DataLifecycle implements SimpleDiffable<DataLifecycle>, ToXContentO
     }
 
     public static boolean isEnabled() {
-        return Build.CURRENT.isSnapshot() || FEATURE_FLAG_ENABLED;
+        return DLM_FEATURE_FLAG.isEnabled();
     }
 
     @Nullable

+ 103 - 0
server/src/main/java/org/elasticsearch/common/util/FeatureFlag.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.util;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.elasticsearch.Build;
+import org.elasticsearch.core.Booleans;
+
+import java.util.function.Function;
+
+/**
+ * A utility class for registering feature flags in Elasticsearch code.
+ * <br/>
+ * Typical usage:
+ * <pre>{
+ *    public static final FeatureFlag XYZZY_FEATURE_FLAG = new FeatureFlag("xyzzy");
+ *    // ...
+ *    if (XYZZY_FEATURE_FLAG.isEnabled()) {
+ *        addRestHandlers();
+ *        registerSettings();
+ *        // etc
+ *    }
+ * }</pre>
+ * <p>
+ * The feature flag will be enabled automatically in all {@link Build#isSnapshot() snapshot} builds.
+ * The feature flag can be enabled in release builds by setting the system property "es.{name}_feature_flag_enabled" to {@code true}
+ * (e.g. {@code -Des.xyzzy_feature_flag_enabled=true}
+ * </p>
+ */
+public class FeatureFlag {
+
+    private final Logger logger = LogManager.getLogger(FeatureFlag.class);
+
+    private final String name;
+    private final boolean enabled;
+
+    private static final Function<String, String> GET_SYSTEM_PROPERTY = System::getProperty;
+
+    public FeatureFlag(String name) {
+        this(name, "enabled", Build.CURRENT, GET_SYSTEM_PROPERTY);
+    }
+
+    /**
+     * This method exists to register feature flags that use the old {@code {name}_feature_flag_registered} naming for their system
+     * property, instead of the preferred {@code {name}_feature_flag_enabled} name.
+     * It exists so that old style feature flag implementations can be converted to use this utility class without changing the name of
+     * their system property (which would affect consumers of the feature).
+     * @deprecated Use {@link FeatureFlag#FeatureFlag(String)} instead
+     */
+    @Deprecated
+    public static FeatureFlag legacyRegisteredFlag(String name) {
+        return new FeatureFlag(name, "registered", Build.CURRENT, GET_SYSTEM_PROPERTY);
+    }
+
+    /**
+     * Accessible for testing only
+     */
+    FeatureFlag(String name, String suffix, Build build, Function<String, String> getSystemProperty) {
+        this.name = name;
+        assert name.indexOf('.') == -1 : "Feature flag names may not contain a '.' character";
+        assert name.contains("feature_flag") == false : "Feature flag names may not contain the string 'feature_flag'";
+
+        final String propertyName = "es." + name + "_feature_flag_" + suffix;
+        if (build.isSnapshot()) {
+            enabled = parseSystemProperty(getSystemProperty, propertyName, true);
+            if (enabled == false) {
+                throw new IllegalArgumentException(
+                    "Feature flag " + name + " (via system property '" + propertyName + "') cannot be disabled in snapshot builds"
+                );
+            }
+            logger.info("The current build is a snapshot, feature flag [{}] is enabled", name);
+        } else {
+            enabled = parseSystemProperty(getSystemProperty, propertyName, false);
+            logger.debug("The current build is a not snapshot, feature flag [{}] is {}", name, enabled ? "enabled" : "disabled");
+        }
+    }
+
+    private boolean parseSystemProperty(Function<String, String> getProperty, String propertyName, boolean defaultValue) {
+        final String propertyValue = getProperty.apply(propertyName);
+        logger.trace("Feature flag system property [{}] is set to [{}]", propertyName, propertyValue);
+        try {
+            return Booleans.parseBoolean(propertyValue, defaultValue);
+        } catch (IllegalArgumentException e) {
+            throw new IllegalArgumentException("Invalid value [" + propertyValue + "] for system property [" + propertyName + "]", e);
+        }
+    }
+
+    public boolean isEnabled() {
+        return enabled;
+    }
+
+    @Override
+    public String toString() {
+        return "Feature-Flag(" + name + "=" + enabled + ")";
+    }
+}

+ 4 - 13
server/src/main/java/org/elasticsearch/index/IndexSettings.java

@@ -10,7 +10,6 @@ package org.elasticsearch.index;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.util.Strings;
 import org.apache.lucene.index.MergePolicy;
-import org.elasticsearch.Build;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.routing.IndexRouting;
@@ -23,7 +22,7 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.time.DateUtils;
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
-import org.elasticsearch.core.Booleans;
+import org.elasticsearch.common.util.FeatureFlag;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.index.translog.Translog;
 import org.elasticsearch.ingest.IngestService;
@@ -474,21 +473,13 @@ public final class IndexSettings {
     );
 
     /**
-     * Is the {@code index.mode} enabled? It should only be enbaled if you
+     * Is the {@code index.mode} enabled? It should only be enabled if you
      * pass a jvm parameter or are running a snapshot build.
      */
-    private static final Boolean TIME_SERIES_MODE_FEATURE_FLAG_REGISTERED;
-
-    static {
-        final String property = System.getProperty("es.index_mode_feature_flag_registered");
-        if (Build.CURRENT.isSnapshot() && property != null) {
-            throw new IllegalArgumentException("es.index_mode_feature_flag_registered is only supported in non-snapshot builds");
-        }
-        TIME_SERIES_MODE_FEATURE_FLAG_REGISTERED = Booleans.parseBoolean(property, null);
-    }
+    private static final FeatureFlag TIME_SERIES_MODE_FEATURE_FLAG = FeatureFlag.legacyRegisteredFlag("index_mode");
 
     public static boolean isTimeSeriesModeEnabled() {
-        return Build.CURRENT.isSnapshot() || (TIME_SERIES_MODE_FEATURE_FLAG_REGISTERED != null && TIME_SERIES_MODE_FEATURE_FLAG_REGISTERED);
+        return TIME_SERIES_MODE_FEATURE_FLAG.isEnabled();
     }
 
     /**

+ 3 - 15
server/src/main/java/org/elasticsearch/transport/TcpTransport.java

@@ -11,7 +11,6 @@ import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.lucene.util.BytesRef;
-import org.elasticsearch.Build;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.TransportVersion;
 import org.elasticsearch.Version;
@@ -40,10 +39,10 @@ import org.elasticsearch.common.transport.NetworkExceptionHelper;
 import org.elasticsearch.common.transport.PortsRange;
 import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.common.unit.ByteSizeValue;
+import org.elasticsearch.common.util.FeatureFlag;
 import org.elasticsearch.common.util.PageCacheRecycler;
 import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
 import org.elasticsearch.common.util.concurrent.CountDown;
-import org.elasticsearch.core.Booleans;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.indices.breaker.CircuitBreakerService;
 import org.elasticsearch.monitor.jvm.JvmInfo;
@@ -108,21 +107,10 @@ public abstract class TcpTransport extends AbstractLifecycleComponent implements
         Setting.Property.NodeScope
     );
 
-    private static final Boolean UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG_REGISTERED;
-
-    static {
-        final String property = System.getProperty("es.untrusted_remote_cluster_feature_flag_registered");
-        if (Build.CURRENT.isSnapshot() && property != null) {
-            throw new IllegalArgumentException(
-                "es.untrusted_remote_cluster_feature_flag_registered " + "is only supported in non-snapshot builds"
-            );
-        }
-        UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG_REGISTERED = Booleans.parseBoolean(property, null);
-    }
+    private static final FeatureFlag UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG = FeatureFlag.legacyRegisteredFlag("untrusted_remote_cluster");
 
     public static boolean isUntrustedRemoteClusterEnabled() {
-        return Build.CURRENT.isSnapshot()
-            || (UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG_REGISTERED != null && UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG_REGISTERED);
+        return UNTRUSTED_REMOTE_CLUSTER_FEATURE_FLAG.isEnabled();
     }
 
     private final boolean ignoreDeserializationErrors;

+ 88 - 0
server/src/test/java/org/elasticsearch/common/util/FeatureFlagTests.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.util;
+
+import org.apache.commons.codec.binary.Hex;
+import org.elasticsearch.Build;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.VersionUtils;
+
+import java.time.Instant;
+import java.util.Properties;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+public class FeatureFlagTests extends ESTestCase {
+
+    public void testSetFeatureFlagEnabledInReleaseBuild() {
+        final Properties properties = setProperty("es.test_feature_flag_enabled", "true");
+        final FeatureFlag flag = newFeatureFlag(properties, false);
+        assertThat(flag.isEnabled(), is(true));
+    }
+
+    public void testSetFeatureFlagRegisteredInReleaseBuild() {
+        final Properties properties = setProperty("es.test_feature_flag_registered", "true");
+        final Build build = randomBuild(false);
+        final FeatureFlag flag = new FeatureFlag("test", "registered", build, properties::getProperty);
+        assertThat(flag.isEnabled(), is(true));
+    }
+
+    public void testSetFeatureFlagExplicitlyDisabledInReleaseBuild() {
+        final Properties properties = setProperty("es.test_feature_flag_enabled", "false");
+        final FeatureFlag flag = newFeatureFlag(properties, false);
+        assertThat(flag.isEnabled(), is(false));
+    }
+
+    public void testSetFeatureFlagDefaultDisabledInReleaseBuild() {
+        final Properties properties = new Properties();
+        final FeatureFlag flag = newFeatureFlag(properties, false);
+        assertThat(flag.isEnabled(), is(false));
+    }
+
+    public void testSetFeatureFlagDefaultEnabledInSnapshotBuild() {
+        final Properties properties = new Properties();
+        final FeatureFlag flag = newFeatureFlag(properties, true);
+        assertThat(flag.isEnabled(), is(true));
+    }
+
+    public void testSetFeatureFlagExplicitlyEnabledInSnapshotBuild() {
+        final Properties properties = setProperty("es.test_feature_flag_enabled", "true");
+        final FeatureFlag flag = newFeatureFlag(properties, true);
+        assertThat(flag.isEnabled(), is(true));
+    }
+
+    public void testSetFeatureFlagCannotBeDisabledInSnapshotBuild() {
+        final Properties properties = setProperty("es.test_feature_flag_enabled", "false");
+        final IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> newFeatureFlag(properties, true));
+        assertThat(ex.getMessage(), containsString("cannot be disabled in snapshot builds"));
+    }
+
+    private static FeatureFlag newFeatureFlag(Properties properties, boolean isSnapshot) {
+        final Build build = randomBuild(isSnapshot);
+        return new FeatureFlag("test", "enabled", build, properties::getProperty);
+    }
+
+    private static Properties setProperty(String key, String value) {
+        Properties properties = new Properties();
+        properties.setProperty(key, value);
+        return properties;
+    }
+
+    private static Build randomBuild(boolean isSnapshot) {
+        return new Build(
+            randomFrom(Build.Type.values()),
+            Hex.encodeHexString(randomByteArrayOfLength(20)),
+            Instant.now().toString(),
+            isSnapshot,
+            VersionUtils.randomVersion(random()).toString()
+        );
+    }
+
+}