Use set instead of string arrays for allowed list

Signed-off-by: Rabi Panda <[email protected]>
opensearch-project · Jan 20, 2023 · 542a680 · 542a680
1 parent b6784c1
commit 542a680
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 10 deletions.
diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java
@@ -212,8 +212,6 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception {
         restoreSnapshotAndEnsureGreen(client, snapshotName, repoName);
 
         assertIndexingBlocked(restoredIndexName);
-        assertUpdateIndexSettingsDefault(restoredIndexName);
-        assertUpdateIndexSettingsAllowList(restoredIndexName);
         assertTrue(client.admin().indices().prepareDelete(restoredIndexName).get().isAcknowledged());
         assertThrows(
             "Expect index to not exist",
@@ -324,7 +322,27 @@ private void assertIndexingBlocked(String index) {
         }
     }
 
-    private void assertUpdateIndexSettingsDefault(String index) {
+    public void testUpdateIndexSettings() throws InterruptedException {
+        final String indexName = "test-index";
+        final String restoredIndexName = indexName + "-copy";
+        final String repoName = "test-repo";
+        final String snapshotName = "test-snap";
+        final Client client = client();
+
+        createIndexWithDocsAndEnsureGreen(0, 100, indexName);
+        createRepositoryWithSettings(null, repoName);
+        takeSnapshot(client, snapshotName, repoName, indexName);
+        deleteIndicesAndEnsureGreen(client, indexName);
+
+        internalCluster().ensureAtLeastNumSearchNodes(1);
+        restoreSnapshotAndEnsureGreen(client, snapshotName, repoName);
+
+        testUpdateIndexSettingsOnlyNotAllowedSettings(restoredIndexName);
+        testUpdateIndexSettingsOnlyAllowedSettings(restoredIndexName);
+        testUpdateIndexSettingsAtLeastOneNotAllowedSettings(restoredIndexName);
+    }
+
+    private void testUpdateIndexSettingsOnlyNotAllowedSettings(String index) {
         try {
             final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index);
             builder.setSettings(Map.of("index.refresh_interval", 10));
@@ -335,13 +353,26 @@ private void assertUpdateIndexSettingsDefault(String index) {
         }
     }
 
-    private void assertUpdateIndexSettingsAllowList(String index) {
+    private void testUpdateIndexSettingsOnlyAllowedSettings(String index) {
         final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index);
-        builder.setSettings(Map.of("index.max_result_window", 100, "index.search.slowlog.threshold.query.warn", "10s"));
+        builder.setSettings(Map.of("index.max_result_window", 1000, "index.search.slowlog.threshold.query.warn", "10s"));
         AcknowledgedResponse settingsResponse = builder.execute().actionGet();
         assertThat(settingsResponse, notNullValue());
     }
 
+    private void testUpdateIndexSettingsAtLeastOneNotAllowedSettings(String index) {
+        try {
+            final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index);
+            builder.setSettings(
+                Map.of("index.max_result_window", 5000, "index.search.slowlog.threshold.query.warn", "15s", "index.refresh_interval", 10)
+            );
+            builder.execute().actionGet();
+            fail("Expected operation to throw an exception");
+        } catch (ClusterBlockException e) {
+            MatcherAssert.assertThat(e.blocks(), contains(IndexMetadata.REMOTE_READ_ONLY_ALLOW_DELETE));
+        }
+    }
+
     /**
      * Picks a shard out of the cluster state for each given index and asserts
      * that the 'index' directory does not exist in the node's file system.

diff --git a/.../java/org/opensearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java b/.../java/org/opensearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java
@@ -56,8 +56,8 @@
 import org.opensearch.transport.TransportService;
 
 import java.io.IOException;
-import java.util.Arrays;
 import java.util.stream.Stream;
+import java.util.Set;
 
 import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING;
 
@@ -70,15 +70,16 @@ public class TransportUpdateSettingsAction extends TransportClusterManagerNodeAc
 
     private static final Logger logger = LogManager.getLogger(TransportUpdateSettingsAction.class);
 
-    private final static String[] ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS = {
+    private final static Set<String> ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS = Set.of(
         "index.max_result_window",
         "index.max_inner_result_window",
         "index.max_rescore_window",
         "index.max_docvalue_fields_search",
         "index.max_script_fields",
         "index.max_terms_count",
         "index.max_regex_length",
-        "index.highlight.max_analyzed_offset" };
+        "index.highlight.max_analyzed_offset"
+    );
 
     private final static String[] ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES = { "index.search.slowlog" };
 
@@ -141,8 +142,8 @@ protected ClusterBlockException checkBlock(UpdateSettingsRequest request, Cluste
             if (allowSearchableSnapshotSettingsUpdate) {
                 for (String setting : request.settings().keySet()) {
                     allowSearchableSnapshotSettingsUpdate = allowSearchableSnapshotSettingsUpdate
-                        && (Stream.of(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES).anyMatch(setting::startsWith)
-                            || (Arrays.asList(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS).contains(setting)));
+                        && (ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS.contains(setting)
+                            || Stream.of(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES).anyMatch(setting::startsWith));
                 }
             }