Skip to content

Conversation

@mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Jan 28, 2025

Reapplying #120246 with fix after it's being reverted #120934.

Fix. In original PR requests without Expect: 100-Continue and oversized are rejected with subsequent channel closure. This is not what netty does and introduced breakage on elasticsearch-js client. The expected behaviour is that ES should reject request and discard body without closing connection.

Description from original PR.

Netty's HttpObjectAggregator handles for Expect: 100-Continue and chunked oversized requests besides aggregating parts. But HTTP stream handling does not use aggregator and we plan to remove it completely. That means we need to handle these cases by ourself.

This PR introduces Netty4HttpContentSizeHandler that handles expect-continue and oversized requests in the same way as HttpObjectAggregator. Some parts are copied from netty's code and simplified for our usage. Follow up on #117787 split into smaller pieces.

Once we completely switch to HTTP stream this handler will replace HttpObjectAggregator. For now there is conditional logic between stream and aggregation.

There is an interesting interaction between HttpRequestDecoder and HttpObjectAggregator . When aggregator responds with 413-too-large to expect-100-continue it resets HttpRequestDecoder through userEvent in pipeline.
Aggregator sends event and Decoder resets state.
This reset is required to avoid treating subsequent request as content of rejected request. But this is private code. Public interface is HttpRequestDecoder#reset(), so Netty4HttpContentSizeHandler requires explicit access to decoder and reset.

@mhl-b mhl-b added >non-issue :Distributed Coordination/Network Http and internode communication implementations v9.0.0 Team:Distributed Coordination Meta label for Distributed Coordination team v8.18.0 labels Jan 28, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Comment on lines +131 to +138
if (isOversized) {
if (isContinueExpected) {
// Client is allowed to send content without waiting for Continue.
// See https://www.rfc-editor.org/rfc/rfc9110.html#section-10.1.1-11.3
// this content will result in HttpRequestDecoder failure and send downstream
decoder.reset();
}
ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE_ON_FAILURE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix. No longer close connection. Previous version:

        if (isOversized) {
            if (isContinueExpected) {
                // Client is allowed to send content without waiting for Continue.
                // See https://www.rfc-editor.org/rfc/rfc9110.html#section-10.1.1-11.3
                // this content will result in HttpRequestDecoder failure and send downstream
                decoder.reset();
                ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE_ON_FAILURE);
            } else {
                // Client is sending oversized content, we cannot safely take it. Closing channel.
                ctx.writeAndFlush(TOO_LARGE_CLOSE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE);
            }

@mhl-b mhl-b changed the title Add http stream content size handler (fixed) Add http stream content size handler (fixed #120246) Jan 28, 2025
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhl-b mhl-b added the auto-backport Automatically create backport pull requests when merged label Jan 29, 2025
@mhl-b mhl-b merged commit 1901c71 into elastic:main Jan 29, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 121095

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Distributed Coordination/Network Http and internode communication implementations >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants