From 6cc612123898ba487c2a50198ab3606c253f916a Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Fri, 27 Jul 2018 17:52:13 -0400 Subject: [PATCH 1/2] Reject follow request if following setting not enabled Today we do not check if the `following_index` setting of the follower is enabled or not when processing a follow-request. If that setting is disabled, the follower will use the default engine, not the following engine. This change checks and rejects such invalid follow requests. --- .../xpack/ccr/action/FollowIndexAction.java | 5 +- .../xpack/ccr/ShardChangesIT.java | 31 ++++- .../ccr/action/FollowIndexActionTests.java | 115 ++++++++++-------- 3 files changed, 98 insertions(+), 53 deletions(-) diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java index c7b2c58d5e036..0988282e10fbd 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowIndexAction.java @@ -487,7 +487,10 @@ static void validate(Request request, IndexMetaData leaderIndex, IndexMetaData f if (leaderIndex.getState() != IndexMetaData.State.OPEN || followIndex.getState() != IndexMetaData.State.OPEN) { throw new IllegalArgumentException("leader and follow index must be open"); } - + if (CcrSettings.CCR_FOLLOWING_INDEX_SETTING.get(followIndex.getSettings()) == false) { + throw new IllegalArgumentException("the following index [" + request.followerIndex + "] is not ready " + + "to follow; the setting [" + CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey() + "] must be enabled."); + } // Make a copy, remove settings that are allowed to be different and then compare if the settings are equal. Settings leaderSettings = filter(leaderIndex.getSettings()); Settings followerSettings = filter(followIndex.getSettings()); diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java index 6fcf1f62e62c2..33c3a735d5814 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java @@ -60,10 +60,7 @@ import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.*; @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, transportClientRatio = 0) public class ShardChangesIT extends ESIntegTestCase { @@ -427,6 +424,32 @@ public void testFollowNonExistentIndex() throws Exception { expectThrows(IllegalArgumentException.class, () -> client().execute(FollowIndexAction.INSTANCE, followRequest3).actionGet()); } + public void testValidateFollowingIndexSettings() throws Exception { + assertAcked(client().admin().indices().prepareCreate("test-leader") + .setSettings(Settings.builder().put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true))); + // TODO: indexing should be optional but the current mapping logic requires for now. + client().prepareIndex("test-leader", "doc", "id").setSource("{\"f\": \"v\"}", XContentType.JSON).get(); + assertAcked(client().admin().indices().prepareCreate("test-follower").get()); + IllegalArgumentException followError = expectThrows(IllegalArgumentException.class, () -> client().execute( + FollowIndexAction.INSTANCE, createFollowRequest("test-leader", "test-follower")).actionGet()); + assertThat(followError.getMessage(), equalTo("the following index [test-follower] is not ready to follow;" + + " the setting [index.xpack.ccr.following_index] must be enabled.")); + // updating the `following_index` with an open index must not be allowed. + IllegalArgumentException updateError = expectThrows(IllegalArgumentException.class, () -> { + client().admin().indices().prepareUpdateSettings("test-follower") + .setSettings(Settings.builder().put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true)).get(); + }); + assertThat(updateError.getMessage(), containsString("Can't update non dynamic settings " + + "[[index.xpack.ccr.following_index]] for open indices [[test-follower/")); + assertAcked(client().admin().indices().prepareClose("test-follower")); + assertAcked(client().admin().indices().prepareUpdateSettings("test-follower") + .setSettings(Settings.builder().put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true))); + assertAcked(client().admin().indices().prepareOpen("test-follower")); + assertAcked(client().execute(FollowIndexAction.INSTANCE, + createFollowRequest("test-leader", "test-follower")).actionGet()); + unfollowIndex("test-follower"); + } + public void testFollowIndex_lowMaxTranslogBytes() throws Exception { final String leaderIndexSettings = getIndexSettings(1, singletonMap(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); assertAcked(client().admin().indices().prepareCreate("index1").setSource(leaderIndexSettings, XContentType.JSON)); diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexActionTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexActionTests.java index 31a6eb31c81b2..3088cf740a110 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexActionTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexActionTests.java @@ -8,12 +8,12 @@ import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.IndexMetaData.State; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.MapperTestUtils; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ccr.CcrSettings; import org.elasticsearch.xpack.ccr.ShardChangesIT; import java.io.IOException; @@ -31,22 +31,23 @@ public void testValidation() throws IOException { } { // should fail, because follow index does not exist - IndexMetaData leaderIMD = createIMD("index1", 5); + IndexMetaData leaderIMD = createIMD("index1", 5, Settings.EMPTY); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowIndexAction.validate(request, leaderIMD, null, null)); assertThat(e.getMessage(), equalTo("follow index [index2] does not exist")); } { // should fail because leader index does not have soft deletes enabled - IndexMetaData leaderIMD = createIMD("index1", 5); - IndexMetaData followIMD = createIMD("index2", 5); + IndexMetaData leaderIMD = createIMD("index1", 5, Settings.EMPTY); + IndexMetaData followIMD = createIMD("index2", 5, Settings.EMPTY); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowIndexAction.validate(request, leaderIMD, followIMD, null)); assertThat(e.getMessage(), equalTo("leader index [index1] does not have soft deletes enabled")); } { // should fail because the number of primary shards between leader and follow index are not equal - IndexMetaData leaderIMD = createIMD("index1", 5, new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); - IndexMetaData followIMD = createIMD("index2", 4); + IndexMetaData leaderIMD = createIMD("index1", 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); + IndexMetaData followIMD = createIMD("index2", 4, Settings.EMPTY); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowIndexAction.validate(request, leaderIMD, followIMD, null)); assertThat(e.getMessage(), @@ -54,10 +55,10 @@ public void testValidation() throws IOException { } { // should fail, because leader index is closed - IndexMetaData leaderIMD = createIMD("index1", State.CLOSE, "{}", 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); - IndexMetaData followIMD = createIMD("index2", State.OPEN, "{}", 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); + IndexMetaData leaderIMD = createIMD("index1", State.CLOSE, "{}", 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); + IndexMetaData followIMD = createIMD("index2", State.OPEN, "{}", 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowIndexAction.validate(request, leaderIMD, followIMD, null)); assertThat(e.getMessage(), equalTo("leader and follow index must be open")); @@ -65,8 +66,9 @@ public void testValidation() throws IOException { { // should fail, because leader has a field with the same name mapped as keyword and follower as text IndexMetaData leaderIMD = createIMD("index1", State.OPEN, "{\"properties\": {\"field\": {\"type\": \"keyword\"}}}", 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); - IndexMetaData followIMD = createIMD("index2", State.OPEN, "{\"properties\": {\"field\": {\"type\": \"text\"}}}", 5); + Settings.builder().put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); + IndexMetaData followIMD = createIMD("index2", State.OPEN, "{\"properties\": {\"field\": {\"type\": \"text\"}}}", 5, + Settings.builder().put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true).build()); MapperService mapperService = MapperTestUtils.newMapperService(xContentRegistry(), createTempDir(), Settings.EMPTY, "index2"); mapperService.updateMapping(followIMD); Exception e = expectThrows(IllegalArgumentException.class, @@ -76,21 +78,39 @@ public void testValidation() throws IOException { { // should fail because of non whitelisted settings not the same between leader and follow index String mapping = "{\"properties\": {\"field\": {\"type\": \"text\", \"analyzer\": \"my_analyzer\"}}}"; - IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"), - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "whitespace")); - IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "standard")); + IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true") + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "whitespace").build()); + IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, Settings.builder() + .put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true) + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "standard").build()); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowIndexAction.validate(request, leaderIMD, followIMD, null)); assertThat(e.getMessage(), equalTo("the leader and follower index settings must be identical")); } + { + // should fail because the following index does not have the following_index settings + IndexMetaData leaderIMD = createIMD("index1", 5, + Settings.builder().put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); + Settings followingIndexSettings = randomBoolean() ? Settings.EMPTY : + Settings.builder().put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), false).build(); + IndexMetaData followIMD = createIMD("index2", 5, followingIndexSettings); + MapperService mapperService = MapperTestUtils.newMapperService(xContentRegistry(), createTempDir(), + followingIndexSettings, "index2"); + mapperService.updateMapping(followIMD); + IllegalArgumentException error = expectThrows(IllegalArgumentException.class, + () -> FollowIndexAction.validate(request, leaderIMD, followIMD, mapperService)); + assertThat(error.getMessage(), equalTo("the following index [index2] is not ready to follow; " + + "the setting [index.xpack.ccr.following_index] must be enabled.")); + } { // should succeed - IndexMetaData leaderIMD = createIMD("index1", 5, new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); - IndexMetaData followIMD = createIMD("index2", 5); + IndexMetaData leaderIMD = createIMD("index1", 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true").build()); + IndexMetaData followIMD = createIMD("index2", 5, Settings.builder() + .put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true).build()); MapperService mapperService = MapperTestUtils.newMapperService(xContentRegistry(), createTempDir(), Settings.EMPTY, "index2"); mapperService.updateMapping(followIMD); FollowIndexAction.validate(request, leaderIMD, followIMD, mapperService); @@ -98,13 +118,14 @@ public void testValidation() throws IOException { { // should succeed, index settings are identical String mapping = "{\"properties\": {\"field\": {\"type\": \"text\", \"analyzer\": \"my_analyzer\"}}}"; - IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"), - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "standard")); - IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "standard")); + IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true") + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "standard").build()); + IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, Settings.builder() + .put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true) + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "standard").build()); MapperService mapperService = MapperTestUtils.newMapperService(xContentRegistry(), createTempDir(), followIMD.getSettings(), "index2"); mapperService.updateMapping(followIMD); @@ -113,15 +134,16 @@ public void testValidation() throws IOException { { // should succeed despite whitelisted settings being different String mapping = "{\"properties\": {\"field\": {\"type\": \"text\", \"analyzer\": \"my_analyzer\"}}}"; - IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, - new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"), - new Tuple<>(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "1s"), - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "standard")); - IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, - new Tuple<>(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "10s"), - new Tuple<>("index.analysis.analyzer.my_analyzer.type", "custom"), - new Tuple<>("index.analysis.analyzer.my_analyzer.tokenizer", "standard")); + IndexMetaData leaderIMD = createIMD("index1", State.OPEN, mapping, 5, Settings.builder() + .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true") + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "1s") + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "standard").build()); + IndexMetaData followIMD = createIMD("index2", State.OPEN, mapping, 5, Settings.builder() + .put(CcrSettings.CCR_FOLLOWING_INDEX_SETTING.getKey(), true) + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "10s") + .put("index.analysis.analyzer.my_analyzer.type", "custom") + .put("index.analysis.analyzer.my_analyzer.tokenizer", "standard").build()); MapperService mapperService = MapperTestUtils.newMapperService(xContentRegistry(), createTempDir(), followIMD.getSettings(), "index2"); mapperService.updateMapping(followIMD); @@ -129,21 +151,18 @@ public void testValidation() throws IOException { } } - private static IndexMetaData createIMD(String index, int numShards, Tuple... settings) throws IOException { - return createIMD(index, State.OPEN, "{\"properties\": {}}", numShards, settings); + private static IndexMetaData createIMD(String index, int numberOfShards, Settings settings) throws IOException { + return createIMD(index, State.OPEN, "{\"properties\": {}}", numberOfShards, settings); } - private static IndexMetaData createIMD(String index, State state, String mapping, int numShards, - Tuple... settings) throws IOException { - Settings.Builder settingsBuilder = settings(Version.CURRENT); - for (Tuple setting : settings) { - settingsBuilder.put((String) setting.v1(), (String) setting.v2()); - } - return IndexMetaData.builder(index).settings(settingsBuilder) - .numberOfShards(numShards) + private static IndexMetaData createIMD(String index, State state, String mapping, int numberOfShards, + Settings settings) throws IOException { + return IndexMetaData.builder(index) + .settings(settings(Version.CURRENT).put(settings)) + .numberOfShards(numberOfShards) .state(state) .numberOfReplicas(0) - .setRoutingNumShards(numShards) + .setRoutingNumShards(numberOfShards) .putMapping("_doc", mapping) .build(); } From 1cd58194e1b948d6bad99d9b37b521e99fe1744f Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sat, 28 Jul 2018 10:00:12 -0400 Subject: [PATCH 2/2] fix stylecheck --- .../java/org/elasticsearch/xpack/ccr/ShardChangesIT.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java index 33c3a735d5814..7b2032f2eac39 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java @@ -60,7 +60,11 @@ import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, transportClientRatio = 0) public class ShardChangesIT extends ESIntegTestCase {