From 296a1824fd91a0c6b52ae6ae31b996e75953e375 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sun, 20 Jan 2019 19:37:39 -0500 Subject: [PATCH 1/4] Do not allow put mapping directly to follower Today, the mapping on the follower is managed and replicated from its leader index by the ShardFollowTask. In other words, users should not modify the mapping on the follower directly. --- .../elasticsearch/action/ActionModule.java | 6 ++ .../put/MappingRequestOriginValidator.java | 40 ++++++++ .../mapping/put/PutMappingRequest.java | 19 ++++ .../put/TransportPutMappingAction.java | 36 ++++++- .../elasticsearch/plugins/ActionPlugin.java | 10 ++ .../put/ValidateMappingRequestOriginIT.java | 99 +++++++++++++++++++ .../java/org/elasticsearch/xpack/ccr/Ccr.java | 7 ++ .../xpack/ccr/action/CcrRequests.java | 23 +++++ .../xpack/ccr/IndexFollowingIT.java | 19 ++++ .../core/LocalStateCompositeXPackPlugin.java | 7 ++ 10 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java create mode 100644 server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index a3d3c615162dc..abde49bb40de1 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -361,6 +361,7 @@ public class ActionModule extends AbstractModule { private final AutoCreateIndex autoCreateIndex; private final DestructiveOperations destructiveOperations; private final RestController restController; + private final TransportPutMappingAction.RequestOriginValidators mappingRequestOriginValidators; public ActionModule(boolean transportClient, Settings settings, IndexNameExpressionResolver indexNameExpressionResolver, IndexScopedSettings indexScopedSettings, ClusterSettings clusterSettings, SettingsFilter settingsFilter, @@ -392,6 +393,10 @@ public ActionModule(boolean transportClient, Settings settings, IndexNameExpress restWrapper = newRestWrapper; } } + mappingRequestOriginValidators = new TransportPutMappingAction.RequestOriginValidators( + actionPlugins.stream().flatMap(p -> p.mappingRequestOriginValidators().stream()).collect(Collectors.toList()) + ); + if (transportClient) { restController = null; } else { @@ -684,6 +689,7 @@ public void initRestHandlers(Supplier nodesInCluster) { protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); bind(DestructiveOperations.class).toInstance(destructiveOperations); + bind(TransportPutMappingAction.RequestOriginValidators.class).toInstance(mappingRequestOriginValidators); if (false == transportClient) { // Supporting classes only used when not a transport client diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java new file mode 100644 index 0000000000000..e1a4f9af84e59 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java @@ -0,0 +1,40 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.action.admin.indices.mapping.put; + +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.index.Index; + +/** + * A validator that validates the origin of a {@link PutMappingRequest} before executing it. + * @see TransportPutMappingAction.RequestOriginValidators + */ +public interface MappingRequestOriginValidator { + + /** + * Validates the origin a given put mapping request with its associated concrete indices and the current state. + * + * @param request the request to validate + * @param state the current cluster state + * @param indices the concrete indices that associated with the given put mapping request + * @return a non-null exception indicates a reason that the given request should be aborted; otherwise returns null. + */ + Exception validateRequestOrigin(PutMappingRequest request, ClusterState state, Index[] indices); +} diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java index 926ae175d65ad..b9d3204ebbc9f 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java @@ -74,6 +74,7 @@ public class PutMappingRequest extends AcknowledgedRequest im private String type; private String source; + private String origin = ""; private Index concreteIndex; @@ -184,6 +185,16 @@ public PutMappingRequest source(Object... source) { return source(buildFromSimplifiedDef(type, source)); } + public String origin() { + return origin; + } + + public PutMappingRequest origin(String origin) { + // reserve "null" for bwc. + this.origin = Objects.requireNonNull(origin); + return this; + } + /** * @param type * the mapping type @@ -301,6 +312,11 @@ public void readFrom(StreamInput in) throws IOException { in.readBoolean(); // updateAllTypes } concreteIndex = in.readOptionalWriteable(Index::new); + if (in.getVersion().onOrAfter(Version.V_7_0_0)) { + origin = in.readOptionalString(); + } else { + origin = null; + } } @Override @@ -314,6 +330,9 @@ public void writeTo(StreamOutput out) throws IOException { out.writeBoolean(true); // updateAllTypes } out.writeOptionalWriteable(concreteIndex); + if (out.getVersion().onOrAfter(Version.V_7_0_0)) { + out.writeOptionalString(origin); + } } @Override diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java index 565fd0616d028..5bff9b33a1d06 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java @@ -37,20 +37,25 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import java.util.Collection; + /** * Put mapping action. */ public class TransportPutMappingAction extends TransportMasterNodeAction { private final MetaDataMappingService metaDataMappingService; + private final RequestOriginValidators requestOriginValidators; @Inject public TransportPutMappingAction(TransportService transportService, ClusterService clusterService, ThreadPool threadPool, MetaDataMappingService metaDataMappingService, - ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver) { + ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver, + RequestOriginValidators requestOriginValidators) { super(PutMappingAction.NAME, transportService, clusterService, threadPool, actionFilters, indexNameExpressionResolver, PutMappingRequest::new); this.metaDataMappingService = metaDataMappingService; + this.requestOriginValidators = requestOriginValidators; } @Override @@ -82,6 +87,13 @@ protected void masterOperation(final PutMappingRequest request, final ClusterSta final Index[] concreteIndices = request.getConcreteIndex() == null ? indexNameExpressionResolver.concreteIndices(state, request) : new Index[] {request.getConcreteIndex()}; + if (request.origin() != null) { + final Exception validationException = requestOriginValidators.validateRequestOrigin(request, state, concreteIndices); + if (validationException != null) { + listener.onFailure(validationException); + return; + } + } PutMappingClusterStateUpdateRequest updateRequest = new PutMappingClusterStateUpdateRequest() .ackTimeout(request.timeout()).masterNodeTimeout(request.masterNodeTimeout()) .indices(concreteIndices).type(request.type()) @@ -107,4 +119,26 @@ public void onFailure(Exception t) { throw ex; } } + + + public static class RequestOriginValidators { + private final Collection validators; + + public RequestOriginValidators(Collection validators) { + this.validators = validators; + } + + private Exception validateRequestOrigin(PutMappingRequest request, ClusterState state, Index[] indices) { + Exception firstException = null; + for (MappingRequestOriginValidator validator : validators) { + final Exception e = validator.validateRequestOrigin(request, state, indices); + if (firstException == null) { + firstException = e; + } else { + firstException.addSuppressed(e); + } + } + return firstException; + } + } } diff --git a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java index c0d94c3f000c8..29b5a3bdcd415 100644 --- a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java +++ b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java @@ -22,6 +22,8 @@ import org.elasticsearch.action.Action; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.action.admin.indices.mapping.put.TransportPutMappingAction; import org.elasticsearch.action.support.ActionFilter; import org.elasticsearch.action.support.TransportAction; import org.elasticsearch.action.support.TransportActions; @@ -179,4 +181,12 @@ public int hashCode() { return Objects.hash(action, transportAction, supportTransportActions); } } + + /** + * Returns a collection of validators that are used by {@link TransportPutMappingAction.RequestOriginValidators} to validate + * the origin of a {@link org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest} before the executing it. + */ + default Collection mappingRequestOriginValidators() { + return Collections.emptyList(); + } } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java new file mode 100644 index 0000000000000..6484344fd7045 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java @@ -0,0 +1,99 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.action.admin.indices.mapping.put; + +import org.elasticsearch.common.util.concurrent.ConcurrentCollections; +import org.elasticsearch.index.Index; +import org.elasticsearch.plugins.ActionPlugin; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.ESSingleNodeTestCase; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Map; + +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; + +public class ValidateMappingRequestOriginIT extends ESSingleNodeTestCase { + static final Map> allowedOrigins = ConcurrentCollections.newConcurrentMap(); + public static class TestPlugin extends Plugin implements ActionPlugin { + @Override + public Collection mappingRequestOriginValidators() { + return Collections.singletonList((request, state, indices) -> { + for (Index index : indices) { + if (allowedOrigins.getOrDefault(index.getName(), Collections.emptySet()).contains(request.origin()) == false) { + return new IllegalStateException("not allowed: index[" + index.getName() + "] origin[" + request.origin() + "]"); + } + } + return null; + }); + } + } + + @Override + protected Collection> getPlugins() { + return Collections.singletonList(TestPlugin.class); + } + + public void testValidateMappingRequestOrigin() { + createIndex("index_1"); + createIndex("index_2"); + allowedOrigins.put("index_1", Arrays.asList("1", "2")); + allowedOrigins.put("index_2", Arrays.asList("2", "3")); + { + String origin = randomFrom("", "3", "4", "5"); + PutMappingRequest request = new PutMappingRequest().indices("index_1").type("doc").source("t1", "type=keyword").origin(origin); + Exception e = expectThrows(IllegalStateException.class, () -> client().admin().indices().putMapping(request).actionGet()); + assertThat(e.getMessage(), equalTo("not allowed: index[index_1] origin[" + origin + "]")); + } + { + PutMappingRequest request = new PutMappingRequest().indices("index_1").origin(randomFrom("1", "2")) + .type("doc").source("t1", "type=keyword"); + assertAcked(client().admin().indices().putMapping(request).actionGet()); + } + + { + String origin = randomFrom("", "1", "4", "5"); + PutMappingRequest request = new PutMappingRequest().indices("index_2").type("doc").source("t2", "type=keyword").origin(origin); + Exception e = expectThrows(IllegalStateException.class, () -> client().admin().indices().putMapping(request).actionGet()); + assertThat(e.getMessage(), equalTo("not allowed: index[index_2] origin[" + origin + "]")); + } + { + PutMappingRequest request = new PutMappingRequest().indices("index_2").origin(randomFrom("2", "3")) + .type("doc").source("t1", "type=keyword"); + assertAcked(client().admin().indices().putMapping(request).actionGet()); + } + + { + String origin = randomFrom("", "1", "3", "4"); + PutMappingRequest request = new PutMappingRequest().indices("*").type("doc").source("t3", "type=keyword").origin(origin); + Exception e = expectThrows(IllegalStateException.class, () -> client().admin().indices().putMapping(request).actionGet()); + assertThat(e.getMessage(), containsString("not allowed:")); + } + { + PutMappingRequest request = new PutMappingRequest().indices("index_2").origin("2") + .type("doc").source("t3", "type=keyword"); + assertAcked(client().admin().indices().putMapping(request).actionGet()); + } + } +} diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java index ab0f995803762..6388c31b04587 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java @@ -9,6 +9,7 @@ import org.apache.lucene.util.SetOnce; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNodes; @@ -45,6 +46,7 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.watcher.ResourceWatcherService; import org.elasticsearch.xpack.ccr.action.AutoFollowCoordinator; +import org.elasticsearch.xpack.ccr.action.CcrRequests; import org.elasticsearch.xpack.ccr.action.ShardChangesAction; import org.elasticsearch.xpack.ccr.action.ShardFollowTask; import org.elasticsearch.xpack.ccr.action.ShardFollowTasksExecutor; @@ -313,4 +315,9 @@ public void onIndexModule(IndexModule indexModule) { protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); } + @Override + public Collection mappingRequestOriginValidators() { + return Collections.singletonList(CcrRequests.CCR_PUT_MAPPING_REQUEST_VALIDATOR); + } + } diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java index 12432c740a701..7d2fa6d8b4563 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java @@ -7,8 +7,16 @@ import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.index.Index; +import org.elasticsearch.xpack.ccr.CcrSettings; + +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; public final class CcrRequests { @@ -24,8 +32,23 @@ public static ClusterStateRequest metaDataRequest(String leaderIndex) { public static PutMappingRequest putMappingRequest(String followerIndex, MappingMetaData mappingMetaData) { PutMappingRequest putMappingRequest = new PutMappingRequest(followerIndex); + putMappingRequest.origin("ccr"); putMappingRequest.type(mappingMetaData.type()); putMappingRequest.source(mappingMetaData.source().string(), XContentType.JSON); return putMappingRequest; } + + public static final MappingRequestOriginValidator CCR_PUT_MAPPING_REQUEST_VALIDATOR = (request, state, indices) -> { + final List followingIndices = Arrays.stream(indices) + .filter(index -> { + final IndexMetaData indexMetaData = state.metaData().index(index); + return indexMetaData != null && CcrSettings.CCR_FOLLOWING_INDEX_SETTING.get(indexMetaData.getSettings()); + }).collect(Collectors.toList()); + if (followingIndices.isEmpty() == false && "ccr".equals(request.origin()) == false) { + return new IllegalStateException("can't put mapping to the following indices [" + + followingIndices.stream().map(Index::getName).collect(Collectors.joining(", ")) + "]; " + + "the mapping of the following indices are self-replicated from its leader indices"); + } + return null; + }; } diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java index 857445ad88de8..d3bbe315ec0c3 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java @@ -34,6 +34,7 @@ import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.CheckedRunnable; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; @@ -209,6 +210,24 @@ public void testNoMappingDefined() throws Exception { assertThat(XContentMapValues.extractValue("properties.k", mappingMetaData.sourceAsMap()), nullValue()); } + public void testDoNotAllowPutMappingToFollower() throws Exception { + final String leaderIndexSettings = getIndexSettings(between(1, 2), between(0, 1), + singletonMap(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); + assertAcked(leaderClient().admin().indices().prepareCreate("index-1").setSource(leaderIndexSettings, XContentType.JSON)); + final PutFollowAction.Request followRequest = putFollow("index-1", "index-2"); + followerClient().execute(PutFollowAction.INSTANCE, followRequest).get(); + PutMappingRequest putMappingRequest = new PutMappingRequest("index-2").type("doc").source("new_field", "type=keyword"); + assertThat(expectThrows(IllegalStateException.class, + () -> followerClient().admin().indices().putMapping(putMappingRequest).actionGet()).getMessage(), + equalTo("can't put mapping to the following indices [index-2]; " + + "the mapping of the following indices are self-replicated from its leader indices")); + pauseFollow("index-2"); + followerClient().admin().indices().close(new CloseIndexRequest("index-2")).actionGet(); + assertAcked(followerClient().execute(UnfollowAction.INSTANCE, new UnfollowAction.Request("index-2")).actionGet()); + followerClient().admin().indices().open(new OpenIndexRequest("index-2")).actionGet(); + assertAcked(followerClient().admin().indices().putMapping(putMappingRequest).actionGet()); + } + public void testFollowIndex_backlog() throws Exception { int numberOfShards = between(1, 5); String leaderIndexSettings = getIndexSettings(numberOfShards, between(0, 1), diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java index 2b19eea5b567f..0ec5d7bb131b3 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java @@ -7,6 +7,7 @@ import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; import org.elasticsearch.action.support.ActionFilter; import org.elasticsearch.bootstrap.BootstrapCheck; import org.elasticsearch.client.Client; @@ -427,6 +428,12 @@ public Optional getEngineFactory(IndexSettings indexSettings) { } } + @Override + public Collection mappingRequestOriginValidators() { + return filterPlugins(ActionPlugin.class).stream().flatMap(p -> p.mappingRequestOriginValidators().stream()) + .collect(Collectors.toList()); + } + private List filterPlugins(Class type) { return plugins.stream().filter(x -> type.isAssignableFrom(x.getClass())).map(p -> ((T)p)) .collect(Collectors.toList()); From 1b8131a842a816432a6088c950a339665d5789a8 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Mon, 21 Jan 2019 21:34:28 -0500 Subject: [PATCH 2/4] stylecheck --- .../test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java index d3bbe315ec0c3..67fce971cca52 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java @@ -34,7 +34,6 @@ import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.CheckedRunnable; -import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; From e83679dae5415f575c6dce8e0e02995ce599ef29 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 22 Jan 2019 10:45:59 -0500 Subject: [PATCH 3/4] return forbidden rest status --- .../elasticsearch/xpack/ccr/action/CcrRequests.java | 9 ++++++--- .../org/elasticsearch/xpack/ccr/IndexFollowingIT.java | 11 +++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java index 7d2fa6d8b4563..13b8fe3313c43 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.ccr.action; +import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; @@ -12,6 +13,7 @@ import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.ccr.CcrSettings; import java.util.Arrays; @@ -45,9 +47,10 @@ public static PutMappingRequest putMappingRequest(String followerIndex, MappingM return indexMetaData != null && CcrSettings.CCR_FOLLOWING_INDEX_SETTING.get(indexMetaData.getSettings()); }).collect(Collectors.toList()); if (followingIndices.isEmpty() == false && "ccr".equals(request.origin()) == false) { - return new IllegalStateException("can't put mapping to the following indices [" + - followingIndices.stream().map(Index::getName).collect(Collectors.joining(", ")) + "]; " - + "the mapping of the following indices are self-replicated from its leader indices"); + final String errorMessage = "can't put mapping to the following indices " + + "[" + followingIndices.stream().map(Index::getName).collect(Collectors.joining(", ")) + "]; " + + "the mapping of the following indices are self-replicated from its leader indices"; + return new ElasticsearchStatusException(errorMessage, RestStatus.FORBIDDEN); } return null; }; diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java index 67fce971cca52..e811480e1b1a0 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/IndexFollowingIT.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.ccr; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksRequest; import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse; import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; @@ -46,6 +47,7 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.persistent.PersistentTasksCustomMetaData; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.tasks.TaskInfo; import org.elasticsearch.xpack.CcrIntegTestCase; import org.elasticsearch.xpack.ccr.action.ShardFollowTask; @@ -213,13 +215,14 @@ public void testDoNotAllowPutMappingToFollower() throws Exception { final String leaderIndexSettings = getIndexSettings(between(1, 2), between(0, 1), singletonMap(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); assertAcked(leaderClient().admin().indices().prepareCreate("index-1").setSource(leaderIndexSettings, XContentType.JSON)); - final PutFollowAction.Request followRequest = putFollow("index-1", "index-2"); - followerClient().execute(PutFollowAction.INSTANCE, followRequest).get(); + followerClient().execute(PutFollowAction.INSTANCE, putFollow("index-1", "index-2")).get(); PutMappingRequest putMappingRequest = new PutMappingRequest("index-2").type("doc").source("new_field", "type=keyword"); - assertThat(expectThrows(IllegalStateException.class, - () -> followerClient().admin().indices().putMapping(putMappingRequest).actionGet()).getMessage(), + ElasticsearchStatusException forbiddenException = expectThrows(ElasticsearchStatusException.class, + () -> followerClient().admin().indices().putMapping(putMappingRequest).actionGet()); + assertThat(forbiddenException.getMessage(), equalTo("can't put mapping to the following indices [index-2]; " + "the mapping of the following indices are self-replicated from its leader indices")); + assertThat(forbiddenException.status(), equalTo(RestStatus.FORBIDDEN)); pauseFollow("index-2"); followerClient().admin().indices().close(new CloseIndexRequest("index-2")).actionGet(); assertAcked(followerClient().execute(UnfollowAction.INSTANCE, new UnfollowAction.Request("index-2")).actionGet()); From d2a61d52a8214f4b348dd1694aa1087d05f88437 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 23 Jan 2019 19:10:37 -0500 Subject: [PATCH 4/4] remove origin --- .../elasticsearch/action/ActionModule.java | 8 +++--- ...ator.java => MappingRequestValidator.java} | 10 +++---- .../put/TransportPutMappingAction.java | 28 +++++++++---------- .../elasticsearch/plugins/ActionPlugin.java | 8 +++--- ...va => ValidateMappingRequestPluginIT.java} | 6 ++-- .../java/org/elasticsearch/xpack/ccr/Ccr.java | 4 +-- .../xpack/ccr/action/CcrRequests.java | 7 +++-- .../core/LocalStateCompositeXPackPlugin.java | 6 ++-- 8 files changed, 39 insertions(+), 38 deletions(-) rename server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/{MappingRequestOriginValidator.java => MappingRequestValidator.java} (75%) rename server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/{ValidateMappingRequestOriginIT.java => ValidateMappingRequestPluginIT.java} (95%) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 64de8c38e31f0..8a8cea82b0a4d 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -357,7 +357,7 @@ public class ActionModule extends AbstractModule { private final AutoCreateIndex autoCreateIndex; private final DestructiveOperations destructiveOperations; private final RestController restController; - private final TransportPutMappingAction.RequestOriginValidators mappingRequestOriginValidators; + private final TransportPutMappingAction.RequestValidators mappingRequestValidators; public ActionModule(boolean transportClient, Settings settings, IndexNameExpressionResolver indexNameExpressionResolver, IndexScopedSettings indexScopedSettings, ClusterSettings clusterSettings, SettingsFilter settingsFilter, @@ -389,8 +389,8 @@ public ActionModule(boolean transportClient, Settings settings, IndexNameExpress restWrapper = newRestWrapper; } } - mappingRequestOriginValidators = new TransportPutMappingAction.RequestOriginValidators( - actionPlugins.stream().flatMap(p -> p.mappingRequestOriginValidators().stream()).collect(Collectors.toList()) + mappingRequestValidators = new TransportPutMappingAction.RequestValidators( + actionPlugins.stream().flatMap(p -> p.mappingRequestValidators().stream()).collect(Collectors.toList()) ); if (transportClient) { @@ -683,7 +683,7 @@ public void initRestHandlers(Supplier nodesInCluster) { protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); bind(DestructiveOperations.class).toInstance(destructiveOperations); - bind(TransportPutMappingAction.RequestOriginValidators.class).toInstance(mappingRequestOriginValidators); + bind(TransportPutMappingAction.RequestValidators.class).toInstance(mappingRequestValidators); if (false == transportClient) { // Supporting classes only used when not a transport client diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestValidator.java similarity index 75% rename from server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java rename to server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestValidator.java index e1a4f9af84e59..8d6608c575874 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestOriginValidator.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/MappingRequestValidator.java @@ -23,18 +23,18 @@ import org.elasticsearch.index.Index; /** - * A validator that validates the origin of a {@link PutMappingRequest} before executing it. - * @see TransportPutMappingAction.RequestOriginValidators + * A validator that validates a {@link PutMappingRequest} before executing it. + * @see TransportPutMappingAction.RequestValidators */ -public interface MappingRequestOriginValidator { +public interface MappingRequestValidator { /** - * Validates the origin a given put mapping request with its associated concrete indices and the current state. + * Validates a given put mapping request with its associated concrete indices and the current state. * * @param request the request to validate * @param state the current cluster state * @param indices the concrete indices that associated with the given put mapping request * @return a non-null exception indicates a reason that the given request should be aborted; otherwise returns null. */ - Exception validateRequestOrigin(PutMappingRequest request, ClusterState state, Index[] indices); + Exception validateRequest(PutMappingRequest request, ClusterState state, Index[] indices); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java index 5bff9b33a1d06..acd0d10281463 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java @@ -45,17 +45,17 @@ public class TransportPutMappingAction extends TransportMasterNodeAction { private final MetaDataMappingService metaDataMappingService; - private final RequestOriginValidators requestOriginValidators; + private final RequestValidators requestValidators; @Inject public TransportPutMappingAction(TransportService transportService, ClusterService clusterService, ThreadPool threadPool, MetaDataMappingService metaDataMappingService, ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver, - RequestOriginValidators requestOriginValidators) { + RequestValidators requestValidators) { super(PutMappingAction.NAME, transportService, clusterService, threadPool, actionFilters, indexNameExpressionResolver, PutMappingRequest::new); this.metaDataMappingService = metaDataMappingService; - this.requestOriginValidators = requestOriginValidators; + this.requestValidators = requestValidators; } @Override @@ -87,12 +87,10 @@ protected void masterOperation(final PutMappingRequest request, final ClusterSta final Index[] concreteIndices = request.getConcreteIndex() == null ? indexNameExpressionResolver.concreteIndices(state, request) : new Index[] {request.getConcreteIndex()}; - if (request.origin() != null) { - final Exception validationException = requestOriginValidators.validateRequestOrigin(request, state, concreteIndices); - if (validationException != null) { - listener.onFailure(validationException); - return; - } + final Exception validationException = requestValidators.validateRequest(request, state, concreteIndices); + if (validationException != null) { + listener.onFailure(validationException); + return; } PutMappingClusterStateUpdateRequest updateRequest = new PutMappingClusterStateUpdateRequest() .ackTimeout(request.timeout()).masterNodeTimeout(request.masterNodeTimeout()) @@ -121,17 +119,17 @@ public void onFailure(Exception t) { } - public static class RequestOriginValidators { - private final Collection validators; + public static class RequestValidators { + private final Collection validators; - public RequestOriginValidators(Collection validators) { + public RequestValidators(Collection validators) { this.validators = validators; } - private Exception validateRequestOrigin(PutMappingRequest request, ClusterState state, Index[] indices) { + private Exception validateRequest(PutMappingRequest request, ClusterState state, Index[] indices) { Exception firstException = null; - for (MappingRequestOriginValidator validator : validators) { - final Exception e = validator.validateRequestOrigin(request, state, indices); + for (MappingRequestValidator validator : validators) { + final Exception e = validator.validateRequest(request, state, indices); if (firstException == null) { firstException = e; } else { diff --git a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java index 29b5a3bdcd415..adc2fa8f0b282 100644 --- a/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java +++ b/server/src/main/java/org/elasticsearch/plugins/ActionPlugin.java @@ -22,7 +22,7 @@ import org.elasticsearch.action.Action; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestValidator; import org.elasticsearch.action.admin.indices.mapping.put.TransportPutMappingAction; import org.elasticsearch.action.support.ActionFilter; import org.elasticsearch.action.support.TransportAction; @@ -183,10 +183,10 @@ public int hashCode() { } /** - * Returns a collection of validators that are used by {@link TransportPutMappingAction.RequestOriginValidators} to validate - * the origin of a {@link org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest} before the executing it. + * Returns a collection of validators that are used by {@link TransportPutMappingAction.RequestValidators} to + * validate a {@link org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest} before the executing it. */ - default Collection mappingRequestOriginValidators() { + default Collection mappingRequestValidators() { return Collections.emptyList(); } } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestPluginIT.java similarity index 95% rename from server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java rename to server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestPluginIT.java index 6484344fd7045..b25c9ecb5fc91 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestOriginIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/mapping/put/ValidateMappingRequestPluginIT.java @@ -34,11 +34,11 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -public class ValidateMappingRequestOriginIT extends ESSingleNodeTestCase { +public class ValidateMappingRequestPluginIT extends ESSingleNodeTestCase { static final Map> allowedOrigins = ConcurrentCollections.newConcurrentMap(); public static class TestPlugin extends Plugin implements ActionPlugin { @Override - public Collection mappingRequestOriginValidators() { + public Collection mappingRequestValidators() { return Collections.singletonList((request, state, indices) -> { for (Index index : indices) { if (allowedOrigins.getOrDefault(index.getName(), Collections.emptySet()).contains(request.origin()) == false) { @@ -55,7 +55,7 @@ protected Collection> getPlugins() { return Collections.singletonList(TestPlugin.class); } - public void testValidateMappingRequestOrigin() { + public void testValidateMappingRequest() { createIndex("index_1"); createIndex("index_2"); allowedOrigins.put("index_1", Arrays.asList("1", "2")); diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java index 7eaef8615297b..acda8d06dc550 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java @@ -9,7 +9,7 @@ import org.apache.lucene.util.SetOnce; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestValidator; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNodes; @@ -315,7 +315,7 @@ public void onIndexModule(IndexModule indexModule) { protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); } @Override - public Collection mappingRequestOriginValidators() { + public Collection mappingRequestValidators() { return Collections.singletonList(CcrRequests.CCR_PUT_MAPPING_REQUEST_VALIDATOR); } diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java index 13b8fe3313c43..87d913c337642 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CcrRequests.java @@ -8,7 +8,7 @@ import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; -import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestValidator; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.common.xcontent.XContentType; @@ -40,7 +40,10 @@ public static PutMappingRequest putMappingRequest(String followerIndex, MappingM return putMappingRequest; } - public static final MappingRequestOriginValidator CCR_PUT_MAPPING_REQUEST_VALIDATOR = (request, state, indices) -> { + public static final MappingRequestValidator CCR_PUT_MAPPING_REQUEST_VALIDATOR = (request, state, indices) -> { + if (request.origin() == null) { + return null; // a put-mapping-request on old versions does not have origin. + } final List followingIndices = Arrays.stream(indices) .filter(index -> { final IndexMetaData indexMetaData = state.metaData().index(index); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java index 0ec5d7bb131b3..1dd07a5df81ff 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java @@ -7,7 +7,7 @@ import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestOriginValidator; +import org.elasticsearch.action.admin.indices.mapping.put.MappingRequestValidator; import org.elasticsearch.action.support.ActionFilter; import org.elasticsearch.bootstrap.BootstrapCheck; import org.elasticsearch.client.Client; @@ -429,8 +429,8 @@ public Optional getEngineFactory(IndexSettings indexSettings) { } @Override - public Collection mappingRequestOriginValidators() { - return filterPlugins(ActionPlugin.class).stream().flatMap(p -> p.mappingRequestOriginValidators().stream()) + public Collection mappingRequestValidators() { + return filterPlugins(ActionPlugin.class).stream().flatMap(p -> p.mappingRequestValidators().stream()) .collect(Collectors.toList()); }