Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate response parameters up-front (especially for chunked APIs) #93981

Closed
DaveCTurner opened this issue Feb 21, 2023 · 3 comments · Fixed by #94136
Closed

Validate response parameters up-front (especially for chunked APIs) #93981

DaveCTurner opened this issue Feb 21, 2023 · 3 comments · Fixed by #94136
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team

Comments

@DaveCTurner
Copy link
Contributor

The node stats API accepts a ?level= query parameter which determines the level of detail in the response. This parameter is only used when serialising the response, it is not even validated up front. However, once we've started to serialize a chunked response we don't really have a way to handle an exception caused by a sudden discovery that one of the response parameters is invalid.

This means that if you do GET /_nodes/stats?level=invalid you get an empty response and the following errors in the logs:

[2023-02-21T14:35:49,023][WARN ][i.n.c.AbstractChannelHandlerContext] [node-0] Failed to mark a promise as failure because it has failed already: DefaultChannelPromise@767aeec1(failure: java.nio.channels.ClosedChannelException), unnotified cause: java.nio.channels.ClosedChannelException
	at [email protected]/org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.write(Netty4HttpPipeliningHandler.java:168)
	at [email protected]/io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:879)
	at [email protected]/io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:940)
	at [email protected]/io.netty.channel.AbstractChannelHandlerContext$WriteTask.run(AbstractChannelHandlerContext.java:1247)
	at [email protected]/io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174)
	at [email protected]/io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167)
	at [email protected]/io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
	at [email protected]/io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
	at [email protected]/io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
	at [email protected]/io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at java.base/java.lang.Thread.run(Thread.java:1589)

java.lang.IllegalArgumentException: level parameter must be one of [indices] or [node] or [shards] but was [invalid]
	at org.elasticsearch.indices.NodeIndicesStats.toXContent(NodeIndicesStats.java:224) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.action.admin.cluster.node.stats.NodeStats.toXContent(NodeStats.java:300) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.action.admin.cluster.node.stats.NodesStatsResponse.lambda$xContentChunks$1(NodesStatsResponse.java:51) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.rest.ChunkedRestResponseBody$1.encodeChunk(ChunkedRestResponseBody.java:101) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.writeChunk(Netty4HttpPipeliningHandler.java:312) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.doWrite(Netty4HttpPipeliningHandler.java:221) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.doWrite(Netty4HttpPipeliningHandler.java:197) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.write(Netty4HttpPipeliningHandler.java:160) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:879) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:940) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext$WriteTask.run(AbstractChannelHandlerContext.java:1247) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470) ~[?:?]
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[?:?]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[?:?]
	at java.lang.Thread.run(Thread.java:1589) ~[?:?]
[2023-02-21T14:35:49,025][WARN ][o.e.h.AbstractHttpServerTransport] [node-0] caught exception while handling client http traffic, closing connection Netty4HttpChannel{localAddress=/127.0.0.1:9200, remoteAddress=/127.0.0.1:55113}
java.lang.IllegalStateException: Failed to close the XContentBuilder
	at org.elasticsearch.xcontent.XContentBuilder.close(XContentBuilder.java:1237) ~[elasticsearch-x-content-8.6.2.jar:?]
	at org.elasticsearch.rest.ChunkedRestResponseBody$1.encodeChunk(ChunkedRestResponseBody.java:107) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.writeChunk(Netty4HttpPipeliningHandler.java:312) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.doFlush(Netty4HttpPipeliningHandler.java:294) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.flush(Netty4HttpPipeliningHandler.java:259) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeFlush0(AbstractChannelHandlerContext.java:923) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:941) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext$WriteTask.run(AbstractChannelHandlerContext.java:1247) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470) ~[?:?]
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[?:?]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[?:?]
	at java.lang.Thread.run(Thread.java:1589) ~[?:?]
Caused by: java.io.IOException: Unclosed object or array found
	at org.elasticsearch.xcontent.provider.json.JsonXContentGenerator.close(JsonXContentGenerator.java:560) ~[?:?]
	at org.elasticsearch.xcontent.XContentBuilder.close(XContentBuilder.java:1235) ~[elasticsearch-x-content-8.6.2.jar:?]
	... 14 more
[2023-02-21T14:35:49,025][ERROR][o.e.h.n.Netty4HttpServerTransport] [node-0] unexpected error while releasing pipelined http responses
java.lang.IllegalStateException: complete already: DefaultChannelPromise@767aeec1(failure: java.nio.channels.ClosedChannelException)
	at io.netty.util.concurrent.DefaultPromise.setFailure(DefaultPromise.java:112) ~[?:?]
	at io.netty.channel.DefaultChannelPromise.setFailure(DefaultChannelPromise.java:89) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.safeFailPromise(Netty4HttpPipeliningHandler.java:351) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.close(Netty4HttpPipeliningHandler.java:335) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeClose(AbstractChannelHandlerContext.java:751) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:727) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:560) ~[?:?]
	at io.netty.channel.DefaultChannelPipeline.close(DefaultChannelPipeline.java:957) ~[?:?]
	at io.netty.channel.AbstractChannel.close(AbstractChannel.java:244) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpChannel.close(Netty4HttpChannel.java:67) ~[?:?]
	at org.elasticsearch.core.IOUtils.close(IOUtils.java:71) ~[elasticsearch-core-8.6.2.jar:?]
	at org.elasticsearch.core.IOUtils.close(IOUtils.java:119) ~[elasticsearch-core-8.6.2.jar:?]
	at org.elasticsearch.common.network.CloseableChannel.closeChannels(CloseableChannel.java:78) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.common.network.CloseableChannel.closeChannel(CloseableChannel.java:67) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.common.network.CloseableChannel.closeChannel(CloseableChannel.java:57) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.http.AbstractHttpServerTransport.onException(AbstractHttpServerTransport.java:322) ~[elasticsearch-8.6.2.jar:?]
	at org.elasticsearch.http.netty4.Netty4HttpServerTransport.onException(Netty4HttpServerTransport.java:287) ~[?:?]
	at org.elasticsearch.http.netty4.Netty4HttpPipeliningHandler.exceptionCaught(Netty4HttpPipeliningHandler.java:383) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeExceptionCaught(AbstractChannelHandlerContext.java:346) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeFlush0(AbstractChannelHandlerContext.java:928) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:941) ~[?:?]
	at io.netty.channel.AbstractChannelHandlerContext$WriteTask.run(AbstractChannelHandlerContext.java:1247) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[?:?]
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470) ~[?:?]
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569) ~[?:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[?:?]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[?:?]
	at java.lang.Thread.run(Thread.java:1589) ~[?:?]
Caused by: java.nio.channels.ClosedChannelException
	... 26 more

In a sense the notion of "response parameter" is incompatible with a chunked API. We need to validate all the parameters up front, we cannot wait until response-serialisation time to do that. But even for non-chunked APIs it's pretty wasteful to do all the work to generate the response and then fail at response-serialisation time.

@DaveCTurner DaveCTurner added >bug :Distributed Coordination/Network Http and internode communication implementations labels Feb 21, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Feb 21, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner DaveCTurner added :Core/Infra/REST API REST infrastructure and utilities and removed :Distributed Coordination/Network Http and internode communication implementations Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. labels Feb 23, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 23, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@howardhuanghua
Copy link
Contributor

I am trying to add level validation in rest layer in #94136.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants