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

* Improved REST endpoint exception handling, see #15335

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

* Tidied up formatting and comments

See #15335

* Tests for #15335

* Cleaned up comments, added section number

* Swapped out tab indents for space indents

* Test class now extends ESSingleNodeTestCase

* Capture RestResponse so it can be examined in test cases

Simple addition to surface the RestResponse object so we can run tests
against it (see issue #15335).

* Refactored class name, included feedback

See #15335.

* Unit test for REST error handling enhancements

Randomizing unit test for enhanced REST response error handling. See
issue #15335 for more details.

* Cleaned up formatting

* New constructor to set HTTP method

Constructor added to support RestController test cases.

* Refactored FakeRestRequest, streamlined test case.

* Cleaned up conflicts

* Tests for #15335

* Added functionality to ignore or include path wildcards

See #15335

* Further enhancements to request handling

Refactored executeHandler to prioritize explicit path matches. See
#15335 for more information.

* Cosmetic fixes

* Refactored method handlers

* Removed redundant import

* Updated integration tests

* Refactoring to address issue #17853

* Cleaned up test assertions

* Fixed edge case if OPTIONS method randomly selected as invalid method

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

* Remove redundant static modifier

* Hook the multiple PathTrie attempts into RestHandler.dispatchRequest

* Add missing space

* Correctly retrieve new handler for each Trie strategy

* Only copy headers to threadcontext once

* Fix test after REST header copying moved higher up

* Restore original params when trying the next trie candidate

* Remove OPTIONS for invalidHttpMethodArray so a 405 is guaranteed in tests

* Re-add the fix I already added and got removed during merge :-/

* Add missing GET method to test

* Add documentation to migration guide about breaking 404 -> 405 changes

* Explain boolean response, pull into local var

* fixup! Explain boolean response, pull into local var

* Encapsulate multiple HTTP methods into PathTrie<MethodHandlers>

* Add PathTrie.retrieveAll where all matching modes can be retrieved

Then TrieMatchingMode can be package private and not leak into RestController

* Include body of error with 405 responses to give hint about valid methods

* Fix missing usageService handler addition

I accidentally removed this :X

* Initialize PathTrieIterator modes with Arrays.asList

* Use "== false" instead of !

* Missing paren :-/
This commit is contained in:
Lee Hinman 2017-07-07 09:01:23 -06:00 committed by GitHub
parent 1ff2c13472
commit 8aa0a5c111
6 changed files with 369 additions and 19 deletions

View File

@ -56,10 +56,12 @@ import java.util.function.Supplier;
import java.util.function.UnaryOperator;
import static org.elasticsearch.rest.RestStatus.BAD_REQUEST;
import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
import static org.elasticsearch.rest.RestStatus.FORBIDDEN;
import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
import static org.elasticsearch.rest.RestStatus.NOT_ACCEPTABLE;
import static org.elasticsearch.rest.RestStatus.OK;
import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
public class RestController extends AbstractComponent implements HttpServerTransport.Dispatcher {
@ -141,11 +143,11 @@ public class RestController extends AbstractComponent implements HttpServerTrans
}
/**
* Registers a REST handler to be executed when the provided method and path match the request.
* Registers a REST handler to be executed when one of the provided methods and path match the request.
*
* @param method GET, POST, etc.
* @param path Path to handle (e.g., "/{index}/{type}/_bulk")
* @param handler The handler to actually execute
* @param method GET, POST, etc.
*/
public void registerHandler(RestRequest.Method method, String path, RestHandler handler) {
if (handler instanceof BaseRestHandler) {
@ -183,11 +185,8 @@ public class RestController extends AbstractComponent implements HttpServerTrans
}
@Override
public void dispatchBadRequest(
final RestRequest request,
final RestChannel channel,
final ThreadContext threadContext,
final Throwable cause) {
public void dispatchBadRequest(final RestRequest request, final RestChannel channel,
final ThreadContext threadContext, final Throwable cause) {
try {
final Exception e;
if (cause == null) {
@ -211,7 +210,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans
* Dispatch the request, if possible, returning true if a response was sent or false otherwise.
*/
boolean dispatchRequest(final RestRequest request, final RestChannel channel, final NodeClient client,
ThreadContext threadContext, final Optional<RestHandler> mHandler) throws Exception {
final Optional<RestHandler> mHandler) throws Exception {
final int contentLength = request.hasContent() ? request.content().length() : 0;
RestChannel responseChannel = channel;
@ -228,6 +227,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans
"] does not support stream parsing. Use JSON or SMILE instead"));
requestHandled = true;
} else if (mHandler.isPresent()) {
try {
if (canTripCircuitBreaker(mHandler)) {
inFlightRequestsBreaker(circuitBreakerService).addEstimateBytesAndMaybeBreak(contentLength, "<http_request>");
@ -246,10 +246,19 @@ public class RestController extends AbstractComponent implements HttpServerTrans
requestHandled = true;
}
} else {
if (request.method() == RestRequest.Method.OPTIONS) {
// when we have OPTIONS request, simply send OK by default (with the Access Control Origin header which gets automatically added)
channel.sendResponse(new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
// Get the map of matching handlers for a request, for the full set of HTTP methods.
final Set<RestRequest.Method> validMethodSet = getValidHandlerMethodSet(request);
if (validMethodSet.size() > 0
&& validMethodSet.contains(request.method()) == false
&& request.method() != RestRequest.Method.OPTIONS) {
// If an alternative handler for an explicit path is registered to a
// different HTTP method than the one supplied - return a 405 Method
// Not Allowed error.
handleUnsupportedHttpMethod(request, channel, validMethodSet);
requestHandled = true;
} else if (validMethodSet.contains(request.method()) == false
&& (request.method() == RestRequest.Method.OPTIONS)) {
handleOptionsRequest(request, channel, validMethodSet);
requestHandled = true;
} else {
requestHandled = false;
@ -263,9 +272,9 @@ public class RestController extends AbstractComponent implements HttpServerTrans
* If a request contains content, this method will return {@code true} if the {@code Content-Type} header is present, matches an
* {@link XContentType} or the handler supports a content stream and the content type header is for newline delimited JSON,
*/
private boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) {
private static boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) {
if (restRequest.getXContentType() == null) {
if (restHandler != null && restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) {
if (restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) {
final String lowercaseMediaType = restRequest.header("Content-Type").toLowerCase(Locale.ROOT);
// we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/
if (lowercaseMediaType.equals("application/x-ndjson")) {
@ -325,7 +334,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans
Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
final Optional<RestHandler> mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(request.method()));
requestHandled = dispatchRequest(request, channel, client, threadContext, mHandler);
requestHandled = dispatchRequest(request, channel, client, mHandler);
if (requestHandled) {
break;
}
@ -349,6 +358,47 @@ public class RestController extends AbstractComponent implements HttpServerTrans
});
}
/**
* Handle requests to a valid REST endpoint using an unsupported HTTP
* method. A 405 HTTP response code is returned, and the response 'Allow'
* header includes a list of valid HTTP methods for the endpoint (see
* <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
* 10.4.6 - 405 Method Not Allowed</a>).
*/
private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
try {
BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED,
"Incorrect HTTP method for uri [" + request.uri() + "] and method [" + request.method() + "], allowed: " + validMethodSet);
bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
channel.sendResponse(bytesRestResponse);
} catch (final IOException e) {
logger.warn("failed to send bad request response", e);
channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
}
}
/**
* Handle HTTP OPTIONS requests to a valid REST endpoint. A 200 HTTP
* response code is returned, and the response 'Allow' header includes a
* list of valid HTTP methods for the endpoint (see
* <a href="https://tools.ietf.org/html/rfc2616#section-9.2">HTTP/1.1 - 9.2
* - Options</a>).
*/
private void handleOptionsRequest(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() > 0) {
BytesRestResponse bytesRestResponse = new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY);
bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
channel.sendResponse(bytesRestResponse);
} else if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() == 0) {
/*
* When we have an OPTIONS HTTP request and no valid handlers,
* simply send OK by default (with the Access Control Origin header
* which gets automatically added).
*/
channel.sendResponse(new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY));
}
}
/**
* Handle a requests with no candidate handlers (return a 400 Bad Request
* error).

View File

@ -212,8 +212,7 @@ public class RestControllerTests extends ESTestCase {
final RestController restController = new RestController(Settings.EMPTY, Collections.emptySet(), wrapper, null,
circuitBreakerService, usageService);
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(),
null, null, threadContext, Optional.of(handler));
restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(), null, null, Optional.of(handler));
assertTrue(wrapperCalled.get());
assertFalse(handlerCalled.get());
}

View File

@ -0,0 +1,159 @@
/*
* 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.rest;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.rest.FakeRestChannel;
import org.elasticsearch.test.rest.FakeRestRequest;
import org.elasticsearch.usage.UsageService;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.is;
import static org.mockito.Mockito.mock;
public class RestHttpResponseHeadersTests extends ESTestCase {
/**
* For requests to a valid REST endpoint using an unsupported HTTP method,
* verify that a 405 HTTP response code is returned, and that the response
* 'Allow' header includes a list of valid HTTP methods for the endpoint
* (see
* <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 {
/*
* Generate a random set of candidate valid HTTP methods to register
* with the test RestController endpoint. Enums are returned in the
* order they are declared, so the first step is to shuffle the HTTP
* method list, passing in the RandomizedContext's Random instance,
* before picking out a candidate sublist.
*/
List<RestRequest.Method> validHttpMethodArray = new ArrayList<RestRequest.Method>(Arrays.asList(RestRequest.Method.values()));
validHttpMethodArray.remove(RestRequest.Method.OPTIONS);
Collections.shuffle(validHttpMethodArray, random());
/*
* The upper bound of the potential sublist is one less than the size of
* the array, so we are guaranteed at least one invalid method to test.
*/
validHttpMethodArray = validHttpMethodArray.subList(0, randomIntBetween(1, validHttpMethodArray.size() - 1));
assert(validHttpMethodArray.size() > 0);
assert(validHttpMethodArray.size() < RestRequest.Method.values().length);
/*
* Generate an inverse list of one or more candidate invalid HTTP
* methods, so we have a candidate method to fire at the test endpoint.
*/
List<RestRequest.Method> invalidHttpMethodArray = new ArrayList<RestRequest.Method>(Arrays.asList(RestRequest.Method.values()));
invalidHttpMethodArray.removeAll(validHttpMethodArray);
// Remove OPTIONS, or else we'll get a 200 instead of 405
invalidHttpMethodArray.remove(RestRequest.Method.OPTIONS);
assert(invalidHttpMethodArray.size() > 0);
// Initialize test candidate RestController
CircuitBreakerService circuitBreakerService = new HierarchyCircuitBreakerService(Settings.EMPTY,
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS));
final Settings settings = Settings.EMPTY;
UsageService usageService = new UsageService(settings);
RestController restController = new RestController(settings, Collections.emptySet(),
null, null, circuitBreakerService, usageService);
// A basic RestHandler handles requests to the endpoint
RestHandler restHandler = new RestHandler() {
@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
channel.sendResponse(new TestResponse());
}
};
// Register valid test handlers with test RestController
for (RestRequest.Method method : validHttpMethodArray) {
restController.registerHandler(method, "/", restHandler);
}
// Generate a test request with an invalid HTTP method
FakeRestRequest.Builder fakeRestRequestBuilder = new FakeRestRequest.Builder(xContentRegistry());
fakeRestRequestBuilder.withMethod(invalidHttpMethodArray.get(0));
RestRequest restRequest = fakeRestRequestBuilder.build();
// Send the request and verify the response status code
FakeRestChannel restChannel = new FakeRestChannel(restRequest, false, 1);
NodeClient client = mock(NodeClient.class);
restController.dispatchRequest(restRequest, restChannel, new ThreadContext(Settings.EMPTY));
assertThat(restChannel.capturedResponse().status().getStatus(), is(405));
/*
* Verify the response allow header contains the valid methods for the
* test endpoint
*/
assertThat(restChannel.capturedResponse().getHeaders().get("Allow"), notNullValue());
String responseAllowHeader = restChannel.capturedResponse().getHeaders().get("Allow").get(0);
List<String> responseAllowHeaderArray = Arrays.asList(responseAllowHeader.split(","));
assertThat(responseAllowHeaderArray.size(), is(validHttpMethodArray.size()));
assertThat(responseAllowHeaderArray, containsInAnyOrder(getMethodNameStringArray(validHttpMethodArray).toArray()));
}
private static class TestResponse extends RestResponse {
@Override
public String contentType() {
return null;
}
@Override
public BytesReference content() {
return null;
}
@Override
public RestStatus status() {
return RestStatus.OK;
}
}
/**
* Convert an RestRequest.Method array to a String array, so it can be
* compared with the expected 'Allow' header String array.
*/
private List<String> getMethodNameStringArray(List<RestRequest.Method> methodArray) {
return methodArray.stream().map(method -> method.toString()).collect(Collectors.toList());
}
}

View File

@ -61,7 +61,7 @@ In previous versions of Elasticsearch, delete by query requests without an expli
were accepted, match_all was used as the default query and all documents were deleted
as a result. From version 6.0.0, delete by query requests require an explicit query.
=== DELETE document calls now implicitly create the type
==== DELETE document calls now implicitly create the type
Running `DELETE index/type/id` now implicitly creates `type` with a default
mapping if it did not exist yet.
@ -76,8 +76,39 @@ removed.. `GET /_all` can be used to retrieve all aliases, settings, and
mappings for all indices. In order to retrieve only the mappings for an index,
`GET /myindex/_mappings` (or `_aliases`, or `_settings`).
==== Requests to existing endpoints with incorrect HTTP verb now return 405 responses
Issuing a request to an endpoint that exists, but with an incorrect HTTP verb
(such as a `POST` request to `/myindex/_settings`) now returns an HTTP 405
response instead of a 404. An `Allow` header is added to the 405 responses
containing the allowed verbs. For example:
[source,text]
-------------------------------------------
$ curl -v -XPOST 'localhost:9200/my_index/_settings'
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9200 (#0)
> POST /my_index/_settings HTTP/1.1
> Host: localhost:9200
> User-Agent: curl/7.51.0
> Accept: */*
>
< HTTP/1.1 405 Method Not Allowed
< Allow: PUT,GET
< content-type: application/json; charset=UTF-8
< content-length: 134
<
{
"error" : "Incorrect HTTP method for uri [/my_index/_settings] and method [POST], allowed: [PUT, GET]",
"status" : 405
}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact
--------------------------------------------
==== Dissallow using `_cache` and `_cache_key`
The `_cache` and `_cache_key` options in queries have been deprecated since version 2.0.0 and
have been ignored since then, issuing a deprecation warning. These options have now been completely
removed, so using them now will throw an error.
removed, so using them now will throw an error.

View File

@ -0,0 +1,105 @@
/*
* 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.http;
import org.apache.http.util.EntityUtils;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import java.util.Arrays;
import java.util.List;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
/**
* Refer to
* <a href="https://github.com/elastic/elasticsearch/issues/15335">Unsupported
* methods on REST endpoints should respond with status code 405</a> for more
* information.
*/
public class RestHttpResponseHeadersIT extends HttpSmokeTestCase {
/**
* For an OPTIONS request to a valid REST endpoint, verify that a 200 HTTP
* response code is returned, and that the response 'Allow' header includes
* a list of valid HTTP methods for the endpoint (see
* <a href="https://tools.ietf.org/html/rfc2616#section-9.2">HTTP/1.1 - 9.2
* - Options</a>).
*/
public void testValidEndpointOptionsResponseHttpHeader() throws Exception {
Response response = getRestClient().performRequest("OPTIONS", "/_tasks");
assertThat(response.getStatusLine().getStatusCode(), is(200));
assertThat(response.getHeader("Allow"), notNullValue());
List<String> responseAllowHeaderStringArray =
Arrays.asList(response.getHeader("Allow").split(","));
assertThat(responseAllowHeaderStringArray, containsInAnyOrder("GET"));
}
/**
* For requests to a valid REST endpoint using an unsupported HTTP method,
* verify that a 405 HTTP response code is returned, and that the response
* 'Allow' header includes a list of valid HTTP methods for the endpoint
* (see
* <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 {
try {
getRestClient().performRequest("DELETE", "/_tasks");
fail("Request should have failed with 405 error");
} catch (ResponseException e) {
Response response = e.getResponse();
assertThat(response.getStatusLine().getStatusCode(), is(405));
assertThat(response.getHeader("Allow"), notNullValue());
List<String> responseAllowHeaderStringArray =
Arrays.asList(response.getHeader("Allow").split(","));
assertThat(responseAllowHeaderStringArray, containsInAnyOrder("GET"));
assertThat(EntityUtils.toString(response.getEntity()),
containsString("Incorrect HTTP method for uri [/_tasks] and method [DELETE], allowed: [GET]"));
}
}
/**
* Test if a POST request to /{index}/_settings matches the update settings
* handler for /{index}/_settings, and returns a 405 error (see
* <a href="https://github.com/elastic/elasticsearch/issues/17853">Issue
* 17853</a> for more information).
*/
public void testIndexSettingsPostRequest() throws Exception {
try {
createIndex("testindex");
getRestClient().performRequest("POST", "/testindex/_settings");
fail("Request should have failed with 405 error");
} catch (ResponseException e) {
Response response = e.getResponse();
assertThat(response.getStatusLine().getStatusCode(), is(405));
assertThat(response.getHeader("Allow"), notNullValue());
List<String> responseAllowHeaderStringArray =
Arrays.asList(response.getHeader("Allow").split(","));
assertThat(responseAllowHeaderStringArray, containsInAnyOrder("PUT", "GET"));
assertThat(EntityUtils.toString(response.getEntity()),
containsString("Incorrect HTTP method for uri [/testindex/_settings] and method [POST], allowed:"));
assertThat(EntityUtils.toString(response.getEntity()), containsString("GET"));
assertThat(EntityUtils.toString(response.getEntity()), containsString("PUT"));
}
}
}

View File

@ -36,6 +36,7 @@ public final class FakeRestChannel extends AbstractRestChannel {
private final CountDownLatch latch;
private final AtomicInteger responses = new AtomicInteger();
private final AtomicInteger errors = new AtomicInteger();
private RestResponse capturedRestResponse;
public FakeRestChannel(RestRequest request, boolean detailedErrorsEnabled, int responseCount) {
super(request, detailedErrorsEnabled);
@ -69,6 +70,7 @@ public final class FakeRestChannel extends AbstractRestChannel {
@Override
public void sendResponse(RestResponse response) {
this.capturedRestResponse = response;
if (response.status() == RestStatus.OK) {
responses.incrementAndGet();
} else {
@ -76,6 +78,10 @@ public final class FakeRestChannel extends AbstractRestChannel {
}
latch.countDown();
}
public RestResponse capturedResponse() {
return capturedRestResponse;
}
public boolean await() throws InterruptedException {
return latch.await(10, TimeUnit.SECONDS);