From 2d8f25f64872aaffda1f6d7d2ba201e1955af39d Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Thu, 18 May 2023 11:21:18 -0500 Subject: [PATCH] [Backport 2.x] Reject invalid SearchBackpressureMode (#6832) (#7541) (#7571) * Reject invalid SearchBackpressureMode (#6832) (#7541) * Reject invalid SearchBackpressureMode (#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 (cherry picked from commit ba35781610b52daa852d93fb037bb10d9096177a) * Change version guard on rest-api-spec test Signed-off-by: Andrew Ross * Add version guard for happy path test case Signed-off-by: Andrew Ross --------- Signed-off-by: Andrew Ross Co-authored-by: Austin Lee --- CHANGELOG.md | 1 + .../test/cluster.put_settings/10_basic.yml | 33 +++++++++++++++ .../settings/SearchBackpressureMode.java | 4 +- .../settings/SearchBackpressureSettings.java | 7 ++-- .../SearchBackpressureSettingsTests.java | 40 +++++++++++++++++++ 5 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index b98c6f94f4813..3e58d6bbaeba0 100644 --- 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 index 825bac9f91649..60081783bb346 100644 --- 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 index a0e4e3c0d25aa..72b58f1d3de02 100644 --- 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/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java index a80f399460ff4..d20e3e50d419f 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java +++ b/server/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 SETTING_MODE = Setting.simpleString( + public static final Setting 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/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java b/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java new file mode 100644 index 0000000000000..a02ca3cf877ad --- /dev/null +++ b/server/src/test/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)) + ); + } +}