From 28ca159e3e297a52e4d2a13df9854e0bb3821f57 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 21 Jan 2023 01:50:33 +0000 Subject: [PATCH] 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 * Update Changelog Signed-off-by: Rabi Panda * Add feature flag check Signed-off-by: Rabi Panda * Use set instead of string arrays for allowed list Signed-off-by: Rabi Panda * Update Changelog Signed-off-by: Rabi Panda Signed-off-by: Rabi Panda (cherry picked from commit 011ff6353237fd3c23a5be1487dff67e8374a84d) Signed-off-by: github-actions[bot] --- CHANGELOG.md | 1 + .../snapshots/SearchableSnapshotIT.java | 45 ++++++++++++++++- .../put/TransportUpdateSettingsAction.java | 50 +++++++++++++++++++ 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b55b44fe646b1..01c939fc4e4a5 100644 --- 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 index 4e57ba1788f63..e06c220e2caf9 100644 --- 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/server/src/main/java/org/opensearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java b/server/src/main/java/org/opensearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java index a959fb043e4c6..a1c3b9341fd40 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java +++ b/server/src/main/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 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)); }