Add update settings allowlist for searchable snapshot (#5907)

* Add update settings allowlist for searchable snapshot Allow search related settings to be updated for searchable snapshots. Signed-off-by: Rabi Panda <adnapibar@gmail.com> * Update Changelog Signed-off-by: Rabi Panda <adnapibar@gmail.com> * Add feature flag check Signed-off-by: Rabi Panda <adnapibar@gmail.com> * Use set instead of string arrays for allowed list Signed-off-by: Rabi Panda <adnapibar@gmail.com> * Update Changelog Signed-off-by: Rabi Panda <adnapibar@gmail.com> Signed-off-by: Rabi Panda <adnapibar@gmail.com> (cherry picked from commit 011ff63) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Jan 21, 2023 · 28ca159 · 28ca159
1 parent 3c7d1b4
commit 28ca159
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Added cluster manager throttling stats in nodes/stats API ([#5790](https://github.com/opensearch-project/OpenSearch/pull/5790))
 - Added support for feature flags in opensearch.yml ([#4959](https://github.com/opensearch-project/OpenSearch/pull/4959))
 - Add query for initialized extensions ([#5658](https://github.com/opensearch-project/OpenSearch/pull/5658))
+- Add update-index-settings allowlist for searchable snapshot ([#5907](https://github.com/opensearch-project/OpenSearch/pull/5907))
 
 ### Dependencies
 - Update nebula-publishing-plugin to 19.2.0 ([#5704](https://github.com/opensearch-project/OpenSearch/pull/5704))

diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java
@@ -12,6 +12,7 @@
 import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest;
 import org.opensearch.action.admin.indices.settings.put.UpdateSettingsRequestBuilder;
 import org.opensearch.action.index.IndexRequestBuilder;
+import org.opensearch.action.support.master.AcknowledgedResponse;
 import org.opensearch.client.Client;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.block.ClusterBlockException;
@@ -37,6 +38,7 @@
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest.Metric.FS;
 import static org.opensearch.common.util.CollectionUtils.iterableAsArrayList;
 
@@ -210,7 +212,6 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception {
         restoreSnapshotAndEnsureGreen(client, snapshotName, repoName);
 
         assertIndexingBlocked(restoredIndexName);
-        assertIndexSettingChangeBlocked(restoredIndexName);
         assertTrue(client.admin().indices().prepareDelete(restoredIndexName).get().isAcknowledged());
         assertThrows(
             "Expect index to not exist",
@@ -321,7 +322,27 @@ private void assertIndexingBlocked(String index) {
         }
     }
 
-    private void assertIndexSettingChangeBlocked(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));
@@ -332,6 +353,26 @@ private void assertIndexSettingChangeBlocked(String index) {
         }
     }
 
+    private void testUpdateIndexSettingsOnlyAllowedSettings(String index) {
+        final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index);
+        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
@@ -49,11 +49,17 @@
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.inject.Inject;
 import org.opensearch.common.io.stream.StreamInput;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.index.Index;
+import org.opensearch.index.IndexModule;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.TransportService;
 
 import java.io.IOException;
+import java.util.stream.Stream;
+import java.util.Set;
+
+import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING;
 
 /**
  * Transport action for updating index settings
@@ -64,6 +70,19 @@ public class TransportUpdateSettingsAction extends TransportClusterManagerNodeAc
 
     private static final Logger logger = LogManager.getLogger(TransportUpdateSettingsAction.class);
 
+    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"
+    );
+
+    private final static String[] ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES = { "index.search.slowlog" };
+
     private final MetadataUpdateSettingsService updateSettingsService;
 
     @Inject
@@ -106,6 +125,37 @@ protected ClusterBlockException checkBlock(UpdateSettingsRequest request, Cluste
             || IndexMetadata.INDEX_BLOCKS_READ_ONLY_ALLOW_DELETE_SETTING.exists(request.settings())) {
             return null;
         }
+
+        if (FeatureFlags.isEnabled(FeatureFlags.SEARCHABLE_SNAPSHOT)) {
+            final Index[] requestIndices = indexNameExpressionResolver.concreteIndices(state, request);
+            boolean allowSearchableSnapshotSettingsUpdate = true;
+            // check if all indices in the request are remote snapshot
+            for (Index index : requestIndices) {
+                if (state.blocks().indexBlocked(ClusterBlockLevel.METADATA_WRITE, index.getName())) {
+                    allowSearchableSnapshotSettingsUpdate = allowSearchableSnapshotSettingsUpdate
+                        && IndexModule.Type.REMOTE_SNAPSHOT.match(
+                            state.getMetadata().getIndexSafe(index).getSettings().get(INDEX_STORE_TYPE_SETTING.getKey())
+                        );
+                }
+            }
+            // check if all settings in the request are in the allow list
+            if (allowSearchableSnapshotSettingsUpdate) {
+                for (String setting : request.settings().keySet()) {
+                    allowSearchableSnapshotSettingsUpdate = allowSearchableSnapshotSettingsUpdate
+                        && (ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS.contains(setting)
+                            || Stream.of(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES).anyMatch(setting::startsWith));
+                }
+            }
+
+            return allowSearchableSnapshotSettingsUpdate
+                ? null
+                : state.blocks()
+                    .indicesBlockedException(
+                        ClusterBlockLevel.METADATA_WRITE,
+                        indexNameExpressionResolver.concreteIndexNames(state, request)
+                    );
+        }
+
         return state.blocks()
             .indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request));
     }