From d1e505efa0d45bc0a7d58ddc45a359b68b25cb7e Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 28 Aug 2017 13:58:26 -0500 Subject: [PATCH 1/8] Add optional validation to master node actions This commit adds an optional override for validating a MasterNodeRequest. Cluster state can optionally be used for validation as well, to check the state of things such as index already existing in the validation logic. --- .../indices/create/TransportCreateIndexAction.java | 5 +++++ .../admin/indices/get/TransportGetIndexAction.java | 11 +++++++++++ .../support/master/TransportMasterNodeAction.java | 12 ++++++++++++ 3 files changed, 28 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java index 0ac8d02f97760..e069683c93045 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java @@ -83,4 +83,9 @@ protected void masterOperation(final CreateIndexRequest request, final ClusterSt listener::onFailure)); } + + @Override + protected void validateRequest(CreateIndexRequest request, ClusterState state) throws Exception { + MetaDataCreateIndexService.validateIndexName(request.index(), state); + } } diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java index 097444bcb68a9..13818f6627190 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java @@ -31,11 +31,13 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.MappingMetaData; +import org.elasticsearch.cluster.metadata.MetaDataCreateIndexService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.indices.InvalidIndexNameException; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -68,6 +70,15 @@ protected GetIndexResponse newResponse() { return new GetIndexResponse(); } + @Override + protected void validateRequest(GetIndexRequest request, ClusterState state) throws Exception { + for(String index: request.indices()) { + // Create index service also does validation on the index name which does not require + // cluster state, so it does not check if things like the index already exists in the cluster + MetaDataCreateIndexService.validateIndexOrAliasName(index, InvalidIndexNameException::new); + } + } + @Override protected void doMasterOperation(final GetIndexRequest request, String[] concreteIndices, final ClusterState state, final ActionListener listener) { diff --git a/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index f2bc4da423dea..3c7a1c3500bd2 100644 --- a/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -87,6 +87,12 @@ protected void masterOperation(Task task, Request request, ClusterState state, A masterOperation(request, state, listener); } + /** + * Override this operation to provide validation for an action + */ + protected void validateRequest(final Request request, ClusterState state) throws Exception { + } + protected boolean localExecute(Request request) { return false; } @@ -127,6 +133,12 @@ public void start() { } protected void doStart(ClusterState clusterState) { + try { + validateRequest(request, clusterState); + } catch (Exception e) { + listener.onFailure(e); + return; + } final Predicate masterChangePredicate = MasterNodeChangePredicate.build(clusterState); final DiscoveryNodes nodes = clusterState.nodes(); if (nodes.isLocalNodeElectedMaster() || localExecute(request)) { From 715b9357652be39b0f42748ea6d654c7822da448 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Thu, 31 Aug 2017 16:19:38 -0500 Subject: [PATCH 2/8] Revert "Add optional validation to master node actions" This reverts commit d1e505efa0d45bc0a7d58ddc45a359b68b25cb7e. --- .../indices/create/TransportCreateIndexAction.java | 5 ----- .../admin/indices/get/TransportGetIndexAction.java | 11 ----------- .../support/master/TransportMasterNodeAction.java | 12 ------------ 3 files changed, 28 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java index e069683c93045..0ac8d02f97760 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java @@ -83,9 +83,4 @@ protected void masterOperation(final CreateIndexRequest request, final ClusterSt listener::onFailure)); } - - @Override - protected void validateRequest(CreateIndexRequest request, ClusterState state) throws Exception { - MetaDataCreateIndexService.validateIndexName(request.index(), state); - } } diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java index 13818f6627190..097444bcb68a9 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java @@ -31,13 +31,11 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.MappingMetaData; -import org.elasticsearch.cluster.metadata.MetaDataCreateIndexService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.indices.InvalidIndexNameException; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -70,15 +68,6 @@ protected GetIndexResponse newResponse() { return new GetIndexResponse(); } - @Override - protected void validateRequest(GetIndexRequest request, ClusterState state) throws Exception { - for(String index: request.indices()) { - // Create index service also does validation on the index name which does not require - // cluster state, so it does not check if things like the index already exists in the cluster - MetaDataCreateIndexService.validateIndexOrAliasName(index, InvalidIndexNameException::new); - } - } - @Override protected void doMasterOperation(final GetIndexRequest request, String[] concreteIndices, final ClusterState state, final ActionListener listener) { diff --git a/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index 3c7a1c3500bd2..f2bc4da423dea 100644 --- a/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/core/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -87,12 +87,6 @@ protected void masterOperation(Task task, Request request, ClusterState state, A masterOperation(request, state, listener); } - /** - * Override this operation to provide validation for an action - */ - protected void validateRequest(final Request request, ClusterState state) throws Exception { - } - protected boolean localExecute(Request request) { return false; } @@ -133,12 +127,6 @@ public void start() { } protected void doStart(ClusterState clusterState) { - try { - validateRequest(request, clusterState); - } catch (Exception e) { - listener.onFailure(e); - return; - } final Predicate masterChangePredicate = MasterNodeChangePredicate.build(clusterState); final DiscoveryNodes nodes = clusterState.nodes(); if (nodes.isLocalNodeElectedMaster() || localExecute(request)) { From fd781af51f1667eb4b2987d76f73c34c9b47ec45 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Thu, 31 Aug 2017 16:20:50 -0500 Subject: [PATCH 3/8] Move the check to the resolver --- .../metadata/IndexNameExpressionResolver.java | 4 ++++ .../IndexNameExpressionResolverTests.java | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index 17b43efdf9d85..e47d6477e9fae 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -34,6 +34,7 @@ import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.indices.IndexClosedException; +import org.elasticsearch.indices.InvalidIndexNameException; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; @@ -174,6 +175,9 @@ Index[] concreteIndices(Context context, String... indexExpressions) { final Set concreteIndices = new HashSet<>(expressions.size()); for (String expression : expressions) { + if (expression.charAt(0) == '_' && MetaData.ALL.equals(expression) == false) { + throw new InvalidIndexNameException(expression, "must not start with '_'."); + } AliasOrIndex aliasOrIndex = metaData.getAliasAndIndexLookup().get(expression); if (aliasOrIndex == null ) { if (failNoIndices) { diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java index 5e04714552248..bd6cac092f69e 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java @@ -30,6 +30,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.indices.IndexClosedException; +import org.elasticsearch.indices.InvalidIndexNameException; import org.elasticsearch.test.ESTestCase; import java.util.Arrays; @@ -641,7 +642,7 @@ public void testConcreteIndicesWildcardAndAliases() { // when ignoreAliases option is set, concreteIndexNames resolves the provided expressions // only against the defined indices IndicesOptions ignoreAliasesOptions = IndicesOptions.fromOptions(false, false, true, false, true, false, true); - + String[] indexNamesIndexWildcard = indexNameExpressionResolver.concreteIndexNames(state, ignoreAliasesOptions, "foo*"); assertEquals(1, indexNamesIndexWildcard.length); @@ -1126,4 +1127,16 @@ public void testIndicesAliasesRequestIgnoresAliases() { assertEquals("test-index", indices[0]); } } + + public void testInvalidIndex() { + MetaData.Builder mdBuilder = MetaData.builder() + .put(indexBuilder("testXXX")); + ClusterState state = ClusterState.builder(new ClusterName("_name")).metaData(mdBuilder).build(); + IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, IndicesOptions.lenientExpandOpen()); + + InvalidIndexNameException iine = expectThrows(InvalidIndexNameException.class, + () -> indexNameExpressionResolver.concreteIndexNames(context, "_foo")); + assertEquals("Invalid index name [_foo], must not start with '_'.", + iine.getMessage()); + } } From c00bb806b6bfdc1b7f044bdd4b21acf4a5eda50f Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Sun, 3 Sep 2017 21:07:13 -0500 Subject: [PATCH 4/8] re-enable the check --- .../cluster/metadata/IndexNameExpressionResolver.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index e47d6477e9fae..5d652a8bebb24 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -159,6 +159,11 @@ Index[] concreteIndices(Context context, String... indexExpressions) { // or multiple indices are specified yield different behaviour. final boolean failNoIndices = indexExpressions.length == 1 ? !options.allowNoIndices() : !options.ignoreUnavailable(); List expressions = Arrays.asList(indexExpressions); + for (String expression : expressions) { + if (expression.charAt(0) == '_' && MetaData.ALL.equals(expression) == false) { + throw new InvalidIndexNameException(expression, "must not start with '_'."); + } + } for (ExpressionResolver expressionResolver : expressionResolvers) { expressions = expressionResolver.resolve(context, expressions); } @@ -175,9 +180,6 @@ Index[] concreteIndices(Context context, String... indexExpressions) { final Set concreteIndices = new HashSet<>(expressions.size()); for (String expression : expressions) { - if (expression.charAt(0) == '_' && MetaData.ALL.equals(expression) == false) { - throw new InvalidIndexNameException(expression, "must not start with '_'."); - } AliasOrIndex aliasOrIndex = metaData.getAliasAndIndexLookup().get(expression); if (aliasOrIndex == null ) { if (failNoIndices) { From bdcc09250756b696968733501f66f513d56fd21b Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Sun, 3 Sep 2017 21:53:31 -0500 Subject: [PATCH 5/8] Moving the check deeper into the resolver. this seems like the correct place for it to be --- .../metadata/IndexNameExpressionResolver.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index 5d652a8bebb24..51f03121ee2fe 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -159,11 +159,6 @@ Index[] concreteIndices(Context context, String... indexExpressions) { // or multiple indices are specified yield different behaviour. final boolean failNoIndices = indexExpressions.length == 1 ? !options.allowNoIndices() : !options.ignoreUnavailable(); List expressions = Arrays.asList(indexExpressions); - for (String expression : expressions) { - if (expression.charAt(0) == '_' && MetaData.ALL.equals(expression) == false) { - throw new InvalidIndexNameException(expression, "must not start with '_'."); - } - } for (ExpressionResolver expressionResolver : expressionResolvers) { expressions = expressionResolver.resolve(context, expressions); } @@ -607,6 +602,7 @@ private Set innerResolve(Context context, List expressions, Indi if (Strings.isEmpty(expression)) { throw indexNotFoundException(expression); } + assertValidAliasOrIndex(expression); if (aliasOrIndexExists(options, metaData, expression)) { if (result != null) { result.add(expression); @@ -660,6 +656,12 @@ private Set innerResolve(Context context, List expressions, Indi return result; } + private static void assertValidAliasOrIndex(String expression) { + if (expression.charAt(0) == '_') { + throw new InvalidIndexNameException(expression, "must not start with '_'."); + } + } + private static boolean aliasOrIndexExists(IndicesOptions options, MetaData metaData, String expression) { AliasOrIndex aliasOrIndex = metaData.getAliasAndIndexLookup().get(expression); //treat aliases as unavailable indices when ignoreAliases is set to true (e.g. delete index and update aliases api) From 201acf45e43158182a0240ed3f81ee61267c15f4 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 6 Sep 2017 13:53:04 -0500 Subject: [PATCH 6/8] Add rest test and fix up nits, add comment on _ check too --- .../cluster/metadata/IndexNameExpressionResolver.java | 8 ++++++-- .../metadata/IndexNameExpressionResolverTests.java | 3 +-- .../resources/rest-api-spec/test/indices.get/10_basic.yml | 8 ++++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index 51f03121ee2fe..6dc92a44bb08b 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -602,7 +602,7 @@ private Set innerResolve(Context context, List expressions, Indi if (Strings.isEmpty(expression)) { throw indexNotFoundException(expression); } - assertValidAliasOrIndex(expression); + validateAliasOrIndex(expression); if (aliasOrIndexExists(options, metaData, expression)) { if (result != null) { result.add(expression); @@ -656,7 +656,11 @@ private Set innerResolve(Context context, List expressions, Indi return result; } - private static void assertValidAliasOrIndex(String expression) { + private static void validateAliasOrIndex(String expression) { + // Expressions can not start with an underscore. This is reserved for APIs. If the check gets here, the API + // does not exist and the path is interpreted as an expression. If the expression begins with an underscore, + // throw a specific error that is different from the [[IndexNotFoundException]], which is typically thrown + // if the expression can't be found. if (expression.charAt(0) == '_') { throw new InvalidIndexNameException(expression, "must not start with '_'."); } diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java index bd6cac092f69e..f4cbfceab994a 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java @@ -1129,8 +1129,7 @@ public void testIndicesAliasesRequestIgnoresAliases() { } public void testInvalidIndex() { - MetaData.Builder mdBuilder = MetaData.builder() - .put(indexBuilder("testXXX")); + MetaData.Builder mdBuilder = MetaData.builder().put(indexBuilder("test")); ClusterState state = ClusterState.builder(new ClusterName("_name")).metaData(mdBuilder).build(); IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, IndicesOptions.lenientExpandOpen()); diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml index b6ac97eb91bfd..24ea50cd7121b 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml @@ -160,3 +160,11 @@ setup: - is_true: test_index_2.settings - is_true: test_index_3.settings +--- +"Should return an exception when querying invalid indices": + + - do: + catch: /Invalid index name \[_foo\], must not start with '_'\./ + indices.get: + index: _foo + From d1f0063cd87af553dccbd10ab356e8be77cfb16e Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 6 Sep 2017 13:55:23 -0500 Subject: [PATCH 7/8] Remove unneeded line --- .../cluster/metadata/IndexNameExpressionResolverTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java index f4cbfceab994a..0530bd617af63 100644 --- a/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java +++ b/core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java @@ -1135,7 +1135,6 @@ public void testInvalidIndex() { InvalidIndexNameException iine = expectThrows(InvalidIndexNameException.class, () -> indexNameExpressionResolver.concreteIndexNames(context, "_foo")); - assertEquals("Invalid index name [_foo], must not start with '_'.", - iine.getMessage()); + assertEquals("Invalid index name [_foo], must not start with '_'.", iine.getMessage()); } } From 4e41dd8b37a082f519a95d078960c49f8cf2bb99 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Thu, 14 Sep 2017 23:03:23 -0500 Subject: [PATCH 8/8] Use bad_request for validation --- .../main/resources/rest-api-spec/test/indices.get/10_basic.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml index 24ea50cd7121b..943cbcf65d144 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml @@ -164,7 +164,7 @@ setup: "Should return an exception when querying invalid indices": - do: - catch: /Invalid index name \[_foo\], must not start with '_'\./ + catch: bad_request indices.get: index: _foo