Skip to content
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

Adding Index Settings validation before starting replication #461

Merged
merged 2 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import org.opensearch.action.support.HandledTransportAction
import org.opensearch.action.support.IndicesOptions
import org.opensearch.client.Client
import org.opensearch.cluster.ClusterState
import org.opensearch.cluster.metadata.IndexMetadata
import org.opensearch.cluster.metadata.MetadataCreateIndexService
import org.opensearch.common.inject.Inject
import org.opensearch.common.settings.Settings
import org.opensearch.env.Environment
Expand All @@ -48,7 +48,8 @@ class TransportReplicateIndexAction @Inject constructor(transportService: Transp
val threadPool: ThreadPool,
actionFilters: ActionFilters,
private val client : Client,
private val environment: Environment) :
private val environment: Environment,
private val metadataCreateIndexService: MetadataCreateIndexService) :
HandledTransportAction<ReplicateIndexRequest, ReplicateIndexResponse>(ReplicateIndexAction.NAME,
transportService, actionFilters, ::ReplicateIndexRequest),
CoroutineScope by GlobalScope {
Expand Down Expand Up @@ -102,7 +103,13 @@ class TransportReplicateIndexAction @Inject constructor(transportService: Transp
throw IllegalArgumentException("Cannot replicate k-NN index - ${request.leaderIndex}")
}

ValidationUtil.validateAnalyzerSettings(environment, leaderSettings, request.settings)
ValidationUtil.validateIndexSettings(
environment,
request.followerIndex,
leaderSettings,
request.settings,
metadataCreateIndexService
)

// Setup checks are successful and trigger replication for the index
// permissions evaluation to trigger replication is based on the current security context set
Expand All @@ -128,7 +135,7 @@ class TransportReplicateIndexAction @Inject constructor(transportService: Transp

private suspend fun getLeaderIndexSettings(leaderAlias: String, leaderIndex: String): Settings {
val remoteClient = client.getRemoteClusterClient(leaderAlias)
val getSettingsRequest = GetSettingsRequest().includeDefaults(false).indices(leaderIndex)
val getSettingsRequest = GetSettingsRequest().includeDefaults(true).indices(leaderIndex)
val settingsResponse = remoteClient.suspending(remoteClient.admin().indices()::getSettings,
injectSecurityContext = true)(getSettingsRequest)
return settingsResponse.indexToSettings.get(leaderIndex) ?: throw IndexNotFoundException("${leaderAlias}:${leaderIndex}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,27 @@ object ValidationUtil {

private val log = LogManager.getLogger(ValidationUtil::class.java)

fun validateIndexSettings(
environment: Environment,
followerIndex: String,
leaderSettings: Settings,
overriddenSettings: Settings,
metadataCreateIndexService: MetadataCreateIndexService
) {
val settingsList = arrayOf(leaderSettings, overriddenSettings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please validate to see if the behavior is as expected, due to open bug on replication side: #298?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed this bug in this PR as well. Now we are retrieving the default settings as well .

val desiredSettingsBuilder = Settings.builder()
// Desired settings are taking leader Settings and then overriding them with desired settings
for (settings in settingsList) {
for (key in settings.keySet()) {
desiredSettingsBuilder.copy(key, settings);
}
}
val desiredSettings = desiredSettingsBuilder.build()

metadataCreateIndexService.validateIndexSettings(followerIndex,desiredSettings, false)
validateAnalyzerSettings(environment, leaderSettings, overriddenSettings)
}

fun validateAnalyzerSettings(environment: Environment, leaderSettings: Settings, overriddenSettings: Settings) {
val analyserSettings = leaderSettings.filter { k: String? -> k!!.matches(Regex("index.analysis.*path")) }
for (analyserSetting in analyserSettings.keySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,26 @@ class StartReplicationIT: MultiClusterRestTestCase() {
}
}

fun `test start replication invalid settings`() {
val followerClient = getClientForCluster(FOLLOWER)
val leaderClient = getClientForCluster(LEADER)

createConnectionBetweenClusters(FOLLOWER, LEADER)

val createIndexResponse = leaderClient.indices().create(CreateIndexRequest(leaderIndexName), RequestOptions.DEFAULT)
assertThat(createIndexResponse.isAcknowledged).isTrue()
val settings = Settings.builder()
.put("index.data_path", "/random-path/invalid-setting")
.build()

try {
followerClient.startReplication(StartReplicationRequest("source", leaderIndexName, followerIndexName, settings = settings))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we validated status code as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done .

} catch (e: ResponseException) {
Assert.assertEquals(400, e.response.statusLine.statusCode)
Assert.assertTrue(e.message!!.contains("Validation Failed: 1: custom path [/random-path/invalid-setting] is not a sub-path of path.shared_data"))
}
}

fun `test that replication is not started when all primary shards are not in active state`() {
val followerClient = getClientForCluster(FOLLOWER)
val leaderClient = getClientForCluster(LEADER)
Expand Down