Add check for invalid index in WildcardExpressionResolver by hub-cap · Pull Request #26409 · elastic/elasticsearch

hub-cap · 2017-08-28T19:22:13Z

Previous behavior in elasticsearch 5.x threw a 400 when accessing an
index that started with an underscore. In 6, this was changed to a 404,
an IndexNotFoundException. This is not a valid index however, and
should return the 400, as it is a bad request.

This commit adds validation to WildcardExpressionResolver
to ensure that indices cannot begin with an underscore. This change
still allows for behavior such as _foo,bar and will not mess with the
existing way we resolve indices. It will error given the example with an
InvalidIndexNameException.

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.

hub-cap · 2017-08-28T19:24:01Z

I wanted to get some eyes on this to make sure that I am doing this validation properly. I saw a few different spots where validation logic existed, so I wanted to extract it out to a common place for the index actions. Feel free to tell me Im 100% off base w/ this :D

bleskes · 2017-08-29T11:10:05Z

core/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java

+    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


I'm not sure where you're headed, but this thing is better done in org.elasticsearch.action.admin.indices.get.GetIndexRequest#validate

bleskes · 2017-08-29T11:11:47Z

.../src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java

+
+    @Override
+    protected void validateRequest(CreateIndexRequest request, ClusterState state) throws Exception {
+        MetaDataCreateIndexService.validateIndexName(request.index(), state);


I see where you're heading with the extra state validation, but name collisions are a rare occasion and I'm not sure it's worth it to have the extra API complexity in order to fail early. If we remove that we can move the name validation to org.elasticsearch.action.admin.indices.create.CreateIndexRequest#validate

The lack of validation in one of the calls actually caused behavior to break between major versions of ES. I think we should avoid this, but I am completely fine w keeping it out of a generic place.

This reverts commit d1e505e.

…tion_index_rest

…t place for it to be

hub-cap · 2017-09-05T15:31:36Z

ok, I have updated the commit to put the assertion in the actual WildcardExpressionResolver, after a check for _all, so the validation never has to special case that. This is the least intrusive place that will ensure that these indexes will no longer return IndexNotFoundException's.

hub-cap · 2017-09-05T15:41:37Z

I have also updated the comment / PR name to show what I am actually trying to achieve :)

jasontedor

This looks good, but I would like to see a test that would fail before this change yet would pass after this change that asserts the status here on an HTTP request would be 400. A REST test is fine for this.

jasontedor · 2017-09-05T20:32:53Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

                if (Strings.isEmpty(expression)) {
                    throw indexNotFoundException(expression);
                }
+                assertValidAliasOrIndex(expression);


Since this is not an actual assertion (like assert), let's change the name.

jasontedor · 2017-09-05T20:43:08Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java

+
+    public void testInvalidIndex() {
+        MetaData.Builder mdBuilder = MetaData.builder()
+            .put(indexBuilder("testXXX"));


It's okay to call the index "test", and to fit all of this on one line.

…tion_index_rest

martijnvg

👍

martijnvg · 2017-09-07T06:17:38Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java

        // only against the defined indices
        IndicesOptions ignoreAliasesOptions = IndicesOptions.fromOptions(false, false, true, false, true, false, true);
-        
+


unneeded change?

should i remove this? my intellij settings might be a bit aggressive :)

bleskes · 2017-09-07T09:21:08Z

@hub-cap can you elaborate a bit as to why you went to a new direction - i.e., we don't check this in the #validate method of requests as you had before?

jasontedor

I think the test should be tighter.

jasontedor · 2017-09-07T14:29:46Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

  - is_true: test_index_3.settings

+---
+"Should return an exception when querying invalid indices":


This test needs to assert on the HTTP status.

Oh I was unaware we could do that. I must've missed it in the docs!

You might have to add a specific catch for bad_request (400).

ill block this on it being merged

hub-cap · 2017-09-07T21:48:20Z

can you elaborate a bit ...

@bleskes initially I did start a generic validation on all of the MasterNode actions, but this proved problematic because there was not a single place the expressions were resolved. Some subclass TransportClusterInfoAction and some do not, and that subclass handles resolving for any of its children, but other actions handle it differently. Then I realized that I didnt care about every action, and the place that is actually causing an issue is in the resolvers themselves. So i went down the path of making it generic in the resolver, so a call to GET /foo,_bar would properly fail with a 400 on _bar, rather than the 404 index not found. And so i eventually found the place that has already decided the expression is not _all, so i could remove a conditional check on "if it starts with underscore and is *not* all".

In the end, I was trying to solve a specific case (indices should not start with underscore) in a generic way in any action, rather than validating it in the place that every action uses to resolve indices.

Edit

And, the Expression Resolver was actually throwing the 404 not found, so the "Fix" would have to wrap that and fix the handling, or throw the proper exception in the resolver itself.

bleskes · 2017-09-08T16:21:27Z

@hub-cap thanks for the explanation. I see the hassle of going through all relevant requests and adapt their validate() method. I'm OK with compromising and putting it deeper in the index resolver (with the hope it won't bubble up other trouble).

hub-cap · 2017-09-08T16:36:39Z

I think about it this way, but I am also new to the codebase, so take it with a grain of salt :)

The index resolver already throws a not found for an index that does resolve to something existing, and now it also throws an invalid index if you try to resolve a bad index.

…tion_index_rest

hub-cap · 2017-09-15T04:06:19Z

I have changed the test to use the bad_request

jasontedor

LGTM.

…tion_index_rest

hub-cap · 2017-09-15T20:33:49Z

@elasticmachine test this please

This commit adds validation to the resolving of indexes in the wildcard expression resolver. It no longer throws a 404 Not Found when resolving invalid indices. It throws a 400 instead, as it is an invalid index. This was the behavior of 5.x.

* master: fix testSniffNodes to use the new error message Add check for invalid index in WildcardExpressionResolver (elastic#26409) Docs: Use single-node discovery.type for dev example Filter unsupported relation for range query builder (elastic#26620) Fix kuromoji default stoptags (elastic#26600) [Docs] Add description for missing fields in Reindex/Update/Delete By Query (elastic#26618) [Docs] Update ingest.asciidoc (elastic#26599) Better message text for ResponseException [DOCS] Remove edit link from ML node enable bwc testing fix StartRecoveryRequestTests.testSerialization Add bad_request to the rest-api-spec catch params (elastic#26539) Introduce a History UUID as a requirement for ops based recovery (elastic#26577) Add missing catch arguments to the rest api spec (elastic#26536)

* master: (278 commits) Move pre-6.0 node checkpoint to SequenceNumbers Invalid JSON request body caused endless loop (elastic#26680) added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery Filter pre-6.0 nodes for checkpoint invariants Skip bad request REST test on pre-6.0 Reenable BWC tests after disabling for backport Add global checkpoint tracking on the primary [Test] Fix reference/cat/allocation/line_8 test failure [Docs] improved description for fs.total.available_in_bytes (elastic#26657) Fix discovery-file plugin to use custom config path fix testSniffNodes to use the new error message Add check for invalid index in WildcardExpressionResolver (elastic#26409) ...

Add optional validation to master node actions

d1e505e

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.

hub-cap added WIP :Core/Infra/REST API REST infrastructure and utilities labels Aug 28, 2017

bleskes reviewed Aug 29, 2017

View reviewed changes

hub-cap added 6 commits August 31, 2017 16:19

Revert "Add optional validation to master node actions"

715b935

This reverts commit d1e505e.

Move the check to the resolver

fd781af

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

36362dd

…tion_index_rest

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

7e10ece

…tion_index_rest

re-enable the check

c00bb80

Moving the check deeper into the resolver. this seems like the correc…

bdcc092

…t place for it to be

hub-cap removed the WIP label Sep 5, 2017

hub-cap changed the title ~~Add optional validation to master node actions~~ Add check for invalid index in WildcardExpressionResolver Sep 5, 2017

hub-cap requested a review from bleskes September 5, 2017 15:42

jasontedor requested changes Sep 5, 2017

View reviewed changes

hub-cap added 3 commits September 5, 2017 21:49

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

692e31f

…tion_index_rest

Add rest test and fix up nits, add comment on _ check too

201acf4

Remove unneeded line

d1f0063

hub-cap requested a review from jasontedor September 6, 2017 18:56

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

d16c70f

…tion_index_rest

martijnvg approved these changes Sep 7, 2017

View reviewed changes

jasontedor requested changes Sep 7, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

fc21e26

…tion_index_rest

Use bad_request for validation

4e41dd8

hub-cap requested a review from jasontedor September 15, 2017 04:05

jasontedor approved these changes Sep 15, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into issues/fix_valida…

94d359d

…tion_index_rest

hub-cap merged commit 296c239 into elastic:master Sep 15, 2017

		// only against the defined indices
		IndicesOptions ignoreAliasesOptions = IndicesOptions.fromOptions(false, false, true, false, true, false, true);

Conversation

hub-cap commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hub-cap commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Sep 5, 2017

Uh oh!

hub-cap commented Sep 5, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleskes commented Sep 7, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit

Uh oh!

bleskes commented Sep 8, 2017

Uh oh!

hub-cap commented Sep 8, 2017

Uh oh!

hub-cap commented Sep 15, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hub-cap commented Aug 28, 2017 •

edited

Loading

hub-cap commented Sep 7, 2017 •

edited

Loading