-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add update settings allowlist for searchable snapshot #5907
Changes from 3 commits
1caee81
cc30f4c
b6784c1
542a680
79a211e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,8 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { | |
restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); | ||
|
||
assertIndexingBlocked(restoredIndexName); | ||
assertIndexSettingChangeBlocked(restoredIndexName); | ||
assertUpdateIndexSettingsDefault(restoredIndexName); | ||
assertUpdateIndexSettingsAllowList(restoredIndexName); | ||
assertTrue(client.admin().indices().prepareDelete(restoredIndexName).get().isAcknowledged()); | ||
assertThrows( | ||
"Expect index to not exist", | ||
|
@@ -321,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)); | ||
|
@@ -332,6 +335,13 @@ private void assertIndexSettingChangeBlocked(String index) { | |
} | ||
} | ||
|
||
private void assertUpdateIndexSettingsAllowList(String index) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a case for a request that contains both an allowed setting and a disallowed setting? |
||
final UpdateSettingsRequestBuilder builder = client().admin().indices().prepareUpdateSettings(index); | ||
builder.setSettings(Map.of("index.max_result_window", 100, "index.search.slowlog.threshold.query.warn", "10s")); | ||
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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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.Arrays; | ||
import java.util.stream.Stream; | ||
|
||
import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; | ||
|
||
/** | ||
* Transport action for updating index settings | ||
|
@@ -64,6 +70,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" }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick, but for both of these I'd use |
||
|
||
private final MetadataUpdateSettingsService updateSettingsService; | ||
|
||
@Inject | ||
|
@@ -106,6 +124,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 | ||
&& (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 state.blocks() | ||
.indicesBlockedException(ClusterBlockLevel.METADATA_WRITE, indexNameExpressionResolver.concreteIndexNames(state, request)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to go in the
[Unreleased 2.x]
section since it will be released on the 2.x line.