From 1caee81debdb4ccafb884fe51ca4d013a0a7622e Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Tue, 17 Jan 2023 17:05:40 +0000 Subject: [PATCH 1/5] Add update settings allowlist for searchable snapshot Allow search related settings to be updated for searchable snapshots. Signed-off-by: Rabi Panda --- .../snapshots/SearchableSnapshotIT.java | 10 +++++ .../put/TransportUpdateSettingsAction.java | 44 ++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index 4e57ba1788f63..d0658945e1065 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; @@ -211,6 +213,7 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { assertIndexingBlocked(restoredIndexName); assertIndexSettingChangeBlocked(restoredIndexName); + assertAllowedIndexSettingUpdate(restoredIndexName); assertTrue(client.admin().indices().prepareDelete(restoredIndexName).get().isAcknowledged()); assertThrows( "Expect index to not exist", @@ -332,6 +335,13 @@ private void assertIndexSettingChangeBlocked(String index) { } } + private void assertAllowedIndexSettingUpdate(String index) { + final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index); + builder.setSettings(Map.of("index.max_result_window", 100)); + AcknowledgedResponse settingsResponse = builder.execute().actionGet(); + assertThat(settingsResponse, notNullValue()); + } + /** * 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..f25a505839f42 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 @@ -50,10 +50,15 @@ import org.opensearch.common.inject.Inject; import org.opensearch.common.io.stream.StreamInput; 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.Arrays; +import java.util.stream.Stream; + +import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; /** * Transport action for updating index settings @@ -64,6 +69,18 @@ public class TransportUpdateSettingsAction extends TransportClusterManagerNodeAc private static final Logger logger = LogManager.getLogger(TransportUpdateSettingsAction.class); + private final static String[] ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS = { + "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,8 +123,31 @@ protected ClusterBlockException checkBlock(UpdateSettingsRequest request, Cluste || IndexMetadata.INDEX_BLOCKS_READ_ONLY_ALLOW_DELETE_SETTING.exists(request.settings())) { return null; } - return state.blocks() - .indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request)); + + 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 + && (Stream.of(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES).anyMatch(setting::startsWith) + || (Arrays.asList(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS).contains(setting))); + } + } + + return allowSearchableSnapshotSettingsUpdate + ? null + : state.blocks() + .indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request)); } @Override From cc30f4cf7b89791b7be9147a92546f460e852dae Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Wed, 18 Jan 2023 23:33:14 +0000 Subject: [PATCH 2/5] Update Changelog Signed-off-by: Rabi Panda --- CHANGELOG.md | 1 + .../org/opensearch/snapshots/SearchableSnapshotIT.java | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc3c83f64a157..feb1e47fc7d2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151)) - Add support for ppc64le architecture ([#5459](https://github.com/opensearch-project/OpenSearch/pull/5459)) - Add support to disallow search request with preference parameter with strict weighted shard routing([#5874](https://github.com/opensearch-project/OpenSearch/pull/5874)) +- Add update settings allowlist for searchable snapshot ([#5907](https://github.com/opensearch-project/OpenSearch/pull/5907)) ### Dependencies - Bumps `log4j-core` from 2.18.0 to 2.19.0 diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index d0658945e1065..af2b425baf14a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java @@ -212,8 +212,8 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertIndexingBlocked(restoredIndexName); - assertIndexSettingChangeBlocked(restoredIndexName); - assertAllowedIndexSettingUpdate(restoredIndexName); + assertUpdateIndexSettingsDefault(restoredIndexName); + assertUpdateIndexSettingsAllowList(restoredIndexName); assertTrue(client.admin().indices().prepareDelete(restoredIndexName).get().isAcknowledged()); assertThrows( "Expect index to not exist", @@ -324,7 +324,7 @@ private void assertIndexingBlocked(String index) { } } - private void assertIndexSettingChangeBlocked(String index) { + private void assertUpdateIndexSettingsDefault(String index) { try { final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index); builder.setSettings(Map.of("index.refresh_interval", 10)); @@ -335,9 +335,9 @@ private void assertIndexSettingChangeBlocked(String index) { } } - private void assertAllowedIndexSettingUpdate(String index) { + private void assertUpdateIndexSettingsAllowList(String index) { final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index); - builder.setSettings(Map.of("index.max_result_window", 100)); + builder.setSettings(Map.of("index.max_result_window", 100, "index.search.slowlog.threshold.query.warn", "10s")); AcknowledgedResponse settingsResponse = builder.execute().actionGet(); assertThat(settingsResponse, notNullValue()); } From b6784c14b9731e5933d5118d6826b67febf907e6 Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Thu, 19 Jan 2023 23:24:51 +0000 Subject: [PATCH 3/5] Add feature flag check Signed-off-by: Rabi Panda --- .../put/TransportUpdateSettingsAction.java | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) 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 f25a505839f42..e1ba9b93e51a8 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,6 +49,7 @@ 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; @@ -124,30 +125,38 @@ protected ClusterBlockException checkBlock(UpdateSettingsRequest request, Cluste return null; } - 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()) - ); + 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 - && (Stream.of(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS_PREFIXES).anyMatch(setting::startsWith) - || (Arrays.asList(ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS).contains(setting))); + // check if all settings in the request are in the allow list + 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))); + } } + + return allowSearchableSnapshotSettingsUpdate + ? null + : state.blocks() + .indicesBlockedException( + ClusterBlockLevel.METADATA_WRITE, + indexNameExpressionResolver.concreteIndexNames(state, request) + ); } - return allowSearchableSnapshotSettingsUpdate - ? null - : state.blocks() - .indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request)); + return state.blocks() + .indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request)); } @Override From 542a6808f26887418989a326a8e4c37980f32431 Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Fri, 20 Jan 2023 09:58:19 -0800 Subject: [PATCH 4/5] Use set instead of string arrays for allowed list Signed-off-by: Rabi Panda --- .../snapshots/SearchableSnapshotIT.java | 41 ++++++++++++++++--- .../put/TransportUpdateSettingsAction.java | 11 ++--- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index af2b425baf14a..e06c220e2caf9 100644 --- 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/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 e1ba9b93e51a8..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 @@ -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,7 +70,7 @@ 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 ALLOWLIST_REMOTE_SNAPSHOT_SETTINGS = Set.of( "index.max_result_window", "index.max_inner_result_window", "index.max_rescore_window", @@ -78,7 +78,8 @@ public class TransportUpdateSettingsAction extends TransportClusterManagerNodeAc "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)); } } From 79a211e2258e22328c346f95afdbe0b29cc95e63 Mon Sep 17 00:00:00 2001 From: Rabi Panda Date: Fri, 20 Jan 2023 10:25:21 -0800 Subject: [PATCH 5/5] Update Changelog Signed-off-by: Rabi Panda --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index feb1e47fc7d2a..af34a840c837b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151)) - Add support for ppc64le architecture ([#5459](https://github.com/opensearch-project/OpenSearch/pull/5459)) - Add support to disallow search request with preference parameter with strict weighted shard routing([#5874](https://github.com/opensearch-project/OpenSearch/pull/5874)) -- Add update settings allowlist for searchable snapshot ([#5907](https://github.com/opensearch-project/OpenSearch/pull/5907)) ### Dependencies - Bumps `log4j-core` from 2.18.0 to 2.19.0 @@ -83,6 +82,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