Improved REST error handling by jbertouch · Pull Request #17916 · elastic/elasticsearch

jbertouch · 2016-04-21T18:28:41Z

First pass at closing #15335. The new behavior for the RestController#executeHandler method is as follows:

For a request to a valid endpoint with an unsupported HTTP method -> Return a 405 HTTP error including allowed methods for the endpoint in the Allow HTTP header. Refer to HTTP/1.1 - 10.4.6 - 405 Method Not Allowed.
For an OPTIONS HTTP method request to a valid endpoint -> Return a 200 HTTP response including allowed methods for the endpoint in the Allow HTTP header. Refer to HTTP/1.1 - 9.2 - Options.

The tests extend ESIntegTestCase rather than the ESSingleNodeTestCase, as http.enabled is set to false in the latter class.

Let me know if you think the tests should be broader - they could be improved by pulling in a random selection of REST endpoints to test, rather than the existing hardcoded endpoints.

Also improved OPTIONS http method handling to better conform with the http spec.

See elastic#15335

nik9000 · 2016-04-21T19:18:39Z

Thanks for changing the title! I use those to triage what I review and the old title didn't help!

jbertouch · 2016-04-21T19:27:33Z

No worries, I realized after I hit submit that the old title was pretty opaque, especially if you were scanning through the PR list.

nik9000 · 2016-04-21T19:29:49Z

core/src/main/java/org/elasticsearch/rest/RestController.java

-            }
-        }
-    }
+	void executeHandler(RestRequest request, RestChannel channel) throws Exception {


Sneaky indentation change snuck in.

nik9000 · 2016-04-21T19:38:03Z

I like it! @jasontedor filed the original bug and will probably want to look too!

jbertouch · 2016-04-21T19:43:41Z

Thanks for the review @nik9000. Just realized my updates are tab indented - will change to space indents to match the ES code style. Sorry that snuck in, was working on something else with tab indents and forgot to change the style.

nik9000 · 2016-04-21T19:45:12Z

Sorry that snuck in,

It is cool! I suspect the build would warn you about that with gradle precommit.

jasontedor · 2016-04-21T20:23:24Z

@jbertouch I gave it a first pass and it looks great.

Regarding

The tests extend ESIntegTestCase rather than the ESSingleNodeTestCase, as http.enabled is set to false in the latter class.

you can override the ESSingleNodeTestCase#nodeSettings method to set http.enabled to true. Can you check if that would work?

jbertouch · 2016-04-21T23:22:21Z

@jasontedor Thank you for the review.

I can't see a ESSingleNodeTestCase#nodeSettings method. Did you mean the ESIntegTestCase#nodeSettings method? If so, that's the one I overrode in the the RestHTTPResponseHeadersIT class.

jasontedor · 2016-04-21T23:27:41Z

@jbertouch Nope, I'm referring to a protected method on ESSingleNodeTestCase that your test suite can override to return an instance of Settings that has set http.enabled to true.

jbertouch · 2016-04-21T23:38:48Z

Ah, sorry I see it now. I hadn't merged in the latest commits from master in a while.

jbertouch · 2016-04-22T15:22:23Z

@jasontedor Updated the test class to extend ESSingleNodeTestCase rather than ESIntegTestCase.

jasontedor · 2016-04-25T15:13:29Z

core/src/test/java/org/elasticsearch/rest/RestHTTPResponseHeadersTests.java

+     * <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
+     * 10.4.6 - 405 Method Not Allowed</a>).
+     */
+    public void testUnsupportedMethodResponseHTTPHeader() throws Exception {


Nit: HTTP -> Http

In this test, an OPTIONS method request is valid, and should not return a 405 error.

jbertouch · 2016-11-09T17:29:48Z

@jasontedor I had some time to review issue #17853 on a flight between LA and Sydney. Here's a high-level outline of my updated solution:

PathTrie

I added 4 trie matching modes for node retrieval: EXPLICIT_NODES_ONLY, WILDCARD_ROOT_NODES_ALLOWED, WILDCARD_LEAF_NODES_ALLOWED and WILDCARD_NODES_ALLOWED, and updated the recursive PathTrie#retrieve method to support them.
These modes allow for more nuanced path matching, and can be used to selectively prevent wildcard nodes from greedily matching requests.
Test cases for the updated matching functionality are in PathTrieTests#testWildcardMatchingModes and PathTrieTests#testExplicitMatchingMode.

RestController

Requests now cascade through the 4 new PathTrie.TrieMatchingMode modes, from the most restrictive (ie. only explicit nodes are matched), to the least restrictive (ie. wildcards are allowed anywhere). Requests were formerly matched allowing wildcards anywhere in the path, which was the cause of the various errors we were seeing.
If a matching mode fails to find a handler, or a handler for a different HTTP method using the same matching mode, then the request is passed to a more permissive matching mode.
If a match on a different HTTP method is found, the request is passed to RestController#handleUnsupportedHttpMethod.
If no matches are found, the request is passed to RestController#handleBadRequest.
There is a test for the edge case identified in issue POST index/_settings with body produces wrong exception #17853 in RestHttpResponseHeadersIT#testIndexSettingsPostRequest.

jbertouch · 2016-11-12T17:48:44Z

One other thing, I guess RestController#canTripCircuitBreaker should also be updated to use the new handler matching logic?

jbertouch · 2017-03-09T15:35:05Z

@jasontedor, any chance of reviewing these changes? I can close the PR if it's redundant.

elasticmachine · 2017-03-09T15:35:09Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jasontedor · 2017-04-12T16:25:43Z

@jbertouch I've asked @dakrone to take over reviewing this one; he'll work with you on getting this one across the finish line!

dakrone · 2017-04-12T16:33:29Z

Hi @jbertouch! We would really love to get this PR in as soon as we can, so it can be released! Would you be adverse to me helping out by cherry-picking your commits to a new branch and bringing this up to date with the current master branch? I know Github has a feature where repo members can push to a pull request's branch to collaboratively develop, but I think this PR was opened prior to that feature, so I can't push and help resolve the conflicts without a new branch/pr.

jbertouch · 2017-04-27T02:36:01Z

Hi @dakrone, that's great to hear - sorry about the delay in getting back to you. Please go ahead and move my commits to a new branch.

dakrone · 2017-05-02T18:27:15Z

Thanks! I've opened #24437 to continue this so I'm going to close this one.

jbertouch added 4 commits April 5, 2016 00:29

Improved REST endpoint exception handling, see elastic#15335

540a299

Also improved OPTIONS http method handling to better conform with the http spec.

Tidied up formatting and comments

0906aa1

See elastic#15335

Tests for elastic#15335

1d3dcdb

Cleaned up comments, added section number

550cc94

jbertouch changed the title ~~Closes issue #15335~~ Improved REST error handling (closes issue #15335) Apr 21, 2016

jbertouch changed the title ~~Improved REST error handling (closes issue #15335)~~ Improved REST error handling Apr 21, 2016

nik9000 reviewed Apr 21, 2016
View reviewed changes

nik9000 added >enhancement review :Core/Infra/REST API REST infrastructure and utilities v5.0.0-alpha2 labels Apr 21, 2016

Swapped out tab indents for space indents

321af02

Merge remote-tracking branch 'elastic/master' into issue_15335

f2af227

Test class now extends ESSingleNodeTestCase

20a2273

jasontedor reviewed Apr 25, 2016
View reviewed changes

jbertouch added 5 commits November 1, 2016 10:09

Removed redundant import

e6d2777

Updated integration tests

4d67a91

Refactoring to address issue elastic#17853

474d001

Cleaned up test assertions

55680ee

Fixed edge case if OPTIONS method randomly selected as invalid method

c10967d

In this test, an OPTIONS method request is valid, and should not return a 405 error.

clintongormley added v5.2.0 and removed v5.1.1 labels Dec 7, 2016

clintongormley added v5.3.0 and removed v5.2.0 labels Jan 24, 2017

clintongormley added v5.4.0 and removed v5.3.0 labels Feb 7, 2017

This was referenced Apr 4, 2017

null_pointer_exception when retrieving the Cluster state #23903

Closed

POST /_cat/indices causes a NullPointerException #24032

Closed

NullPointerException when trying a url with a typo #24047

Closed

jasontedor removed their assignment Apr 12, 2017

jasontedor requested a review from dakrone April 12, 2017 16:26

dakrone self-assigned this Apr 12, 2017

clintongormley added v5.4.1 and removed v5.4.0 labels Apr 28, 2017

dakrone mentioned this pull request May 2, 2017

Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support #24437

Merged

dakrone closed this May 2, 2017

clintongormley removed the v5.4.1 label May 8, 2017

Conversation

jbertouch commented Apr 21, 2016

Uh oh!

nik9000 commented Apr 21, 2016

Uh oh!

jbertouch commented Apr 21, 2016

Uh oh!

nik9000 Apr 21, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Apr 21, 2016

Uh oh!

jbertouch commented Apr 21, 2016

Uh oh!

nik9000 commented Apr 21, 2016

Uh oh!

jasontedor commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbertouch commented Apr 21, 2016

Uh oh!

jasontedor commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbertouch commented Apr 21, 2016

Uh oh!

jbertouch commented Apr 22, 2016

Uh oh!

jasontedor Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

jbertouch commented Nov 9, 2016

Uh oh!

jbertouch commented Nov 12, 2016

Uh oh!

jbertouch commented Mar 9, 2017

Uh oh!

elasticmachine commented Mar 9, 2017

Uh oh!

jasontedor commented Apr 12, 2017

Uh oh!

dakrone commented Apr 12, 2017

Uh oh!

jbertouch commented Apr 27, 2017

Uh oh!

dakrone commented May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasontedor commented Apr 21, 2016 •

edited

Loading

jasontedor commented Apr 21, 2016 •

edited

Loading