[Backport 2.x] Reject invalid SearchBackpressureMode (opensearch-proj…

…ect#6832) (opensearch-project#7541) (opensearch-project#7571) * Reject invalid SearchBackpressureMode (opensearch-project#6832) (opensearch-project#7541) * Reject invalid SearchBackpressureMode (opensearch-project#6832) ClusterState updates of SearchBackpressureMode are persisted without validation which causes the entire cluster to become unstable or corrupted when a cluster tries to load invalid mode. Signed-off-by: Austin Lee <[email protected]> (cherry picked from commit ba35781) * Change version guard on rest-api-spec test Signed-off-by: Andrew Ross <[email protected]> * Add version guard for happy path test case Signed-off-by: Andrew Ross <[email protected]> --------- Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Austin Lee <[email protected]>
dreamer-89 · May 18, 2023 · 2d8f25f · 2d8f25f
1 parent 1222c17
commit 2d8f25f
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -52,6 +52,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ### Fixed
 - Replaces ZipInputStream with ZipFile to fix Zip Slip vulnerability ([#7230](https://github.com/opensearch-project/OpenSearch/pull/7230))
+- Add missing validation/parsing of SearchBackpressureMode of SearchBackpressureSettings ([#7541](https://github.com/opensearch-project/OpenSearch/pull/7541))
 
 ### Security
 

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml
@@ -69,3 +69,36 @@
         include_defaults: true
 
  - match: {defaults.node.attr.testattr: "test"}
+
+---
+"Test set search backpressure mode":
+
+  - skip:
+      version: "- 2.3.99"
+      reason: "Search backpressure was added in 2.4"
+
+  - do:
+      cluster.put_settings:
+        body:
+          persistent:
+            search_backpressure.mode: "monitor_only"
+
+  - match: {persistent: {search_backpressure: {mode: "monitor_only"}}}
+
+---
+"Test set invalid search backpressure mode":
+
+  - skip:
+      version: "- 2.7.99"
+      reason: "Parsing and validation of SearchBackpressureMode does not exist in versions < 2.8"
+
+  - do:
+      catch: bad_request
+      cluster.put_settings:
+        body:
+          persistent:
+            search_backpressure.mode: "monitor-only"
+
+  - match: {error.root_cause.0.type: "illegal_argument_exception"}
+  - match: { error.root_cause.0.reason: "Invalid SearchBackpressureMode: monitor-only" }
+  - match: { status: 400 }
diff --git a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureMode.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureMode.java
@@ -8,6 +8,8 @@
 
 package org.opensearch.search.backpressure.settings;
 
+import java.util.Locale;
+
 /**
  * Defines the search backpressure mode.
  */
@@ -38,7 +40,7 @@ public String getName() {
     }
 
     public static SearchBackpressureMode fromName(String name) {
-        switch (name) {
+        switch (name.toLowerCase(Locale.ROOT)) {
             case "disabled":
                 return DISABLED;
             case "monitor_only":

diff --git a/...src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java b/...src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java
@@ -43,9 +43,10 @@ private static class Defaults {
      * Defines the search backpressure mode. It can be either "disabled", "monitor_only" or "enforced".
      */
     private volatile SearchBackpressureMode mode;
-    public static final Setting<String> SETTING_MODE = Setting.simpleString(
+    public static final Setting<SearchBackpressureMode> SETTING_MODE = new Setting<>(
         "search_backpressure.mode",
         Defaults.MODE,
+        SearchBackpressureMode::fromName,
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -113,8 +114,8 @@ public SearchBackpressureSettings(Settings settings, ClusterSettings clusterSett
 
         interval = new TimeValue(SETTING_INTERVAL_MILLIS.get(settings));
 
-        mode = SearchBackpressureMode.fromName(SETTING_MODE.get(settings));
-        clusterSettings.addSettingsUpdateConsumer(SETTING_MODE, s -> this.setMode(SearchBackpressureMode.fromName(s)));
+        mode = SETTING_MODE.get(settings);
+        clusterSettings.addSettingsUpdateConsumer(SETTING_MODE, this::setMode);
         clusterSettings.addSettingsUpdateConsumer(SETTING_CANCELLATION_RATIO, searchShardTaskSettings::setCancellationRatio);
         clusterSettings.addSettingsUpdateConsumer(SETTING_CANCELLATION_RATE, searchShardTaskSettings::setCancellationRate);
         clusterSettings.addSettingsUpdateConsumer(SETTING_CANCELLATION_BURST, searchShardTaskSettings::setCancellationBurst);

diff --git a/...est/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java b/...est/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java
@@ -0,0 +1,40 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.search.backpressure.settings;
+
+import org.opensearch.common.settings.ClusterSettings;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.test.OpenSearchTestCase;
+
+public class SearchBackpressureSettingsTests extends OpenSearchTestCase {
+
+    /**
+     * Validate proper construction of SearchBackpressureSettings object with a valid mode.
+     */
+    public void testSearchBackpressureSettings() {
+        Settings settings = Settings.builder().put("search_backpressure.mode", "monitor_only").build();
+        ClusterSettings cs = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+        SearchBackpressureSettings sbs = new SearchBackpressureSettings(settings, cs);
+        assertEquals(SearchBackpressureMode.MONITOR_ONLY, sbs.getMode());
+        assertEquals(settings, sbs.getSettings());
+        assertEquals(cs, sbs.getClusterSettings());
+    }
+
+    /**
+     * Validate construction of SearchBackpressureSettings object gets rejected
+     * on invalid SearchBackpressureMode value.
+     */
+    public void testSearchBackpressureSettingValidateInvalidMode() {
+        Settings settings = Settings.builder().put("search_backpressure.mode", "foo").build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+    }
+}