Improved REST error handling

[jbertouch]

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.

[nik9000]

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

[jbertouch]

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]

Sneaky indentation change snuck in.

[nik9000]

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

[jbertouch]

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]

Sorry that snuck in,

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

[jasontedor]

@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]

@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]

@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]

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

[jbertouch]

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

[jasontedor]

Nit: HTTP -> Http

[jbertouch]

@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]

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

[jbertouch]

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

[elasticmachine]

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]

@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]

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]

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]

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