Add http stream content size handler by mhl-b · Pull Request #120246 · elastic/elasticsearch

mhl-b · 2025-01-15T23:57:54Z

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 · 2025-01-16T05:59:34Z

...ternalClusterTest/java/org/elasticsearch/http/netty4/Netty4IncrementalRequestHandlingIT.java

-    // ensures that oversized chunked encoded request has no limits at http layer
-    // rest handler is responsible for oversized requests
-    public void testOversizedChunkedEncodingNoLimits() throws Exception {
+    // ensures that oversized chunked encoded request has maxContentLength limit and returns 413
+    public void testOversizedChunkedEncoding() throws Exception {


With stream introduction we dropped limits for chunked content. Bringing them back.

elasticsearchmachine · 2025-01-16T06:13:11Z

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

DaveCTurner

Looks good, just a couple of small points around testing.

DaveCTurner · 2025-01-16T11:52:51Z

...ansport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpContentSizeHandler.java

+            } else {
+                decoder.reset();
+                ctx.writeAndFlush(EXPECTATION_FAILED.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE_ON_FAILURE);
+                return;


I don't think we cover this branch in the unit tests - could you add a test? Also just resetting the decoder seems quite a weak response, I would expect that in this situation we just don't know how to proceed with subsequent data so should give up and close the connection.

Will add a test. Subsequent data will result in decoding failure and trip HttpPipeliningHandler and very likely connection close. If there is no data connection can be reused. I kind of agree that closing makes sense, since the 'Expect' header supports "100-Continue" only, while other values seem suspicious.

DaveCTurner · 2025-01-16T11:54:35Z

...ternalClusterTest/java/org/elasticsearch/http/netty4/Netty4IncrementalRequestHandlingIT.java

 @ESIntegTestCase.ClusterScope(numDataNodes = 1)
 public class Netty4IncrementalRequestHandlingIT extends ESNetty4IntegTestCase {

+    private static final int maxContentLength = 50 * 1024 * 1024;


nit: suggest

Suggested change

private static final int maxContentLength = 50 * 1024 * 1024;

private static final int TEST_MAX_CONTENT_LENGTH_BYTES = ByteSizeUnit.MB.toIntBytes(50);

SHOUTY_SNAKE_CASE for a constant, plus avoiding * 1024 * 1024.

DaveCTurner · 2025-01-16T12:02:34Z

...rt-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpContentSizeHandlerTests.java

+        channel.writeInbound(encode(part1));
+        ((HttpContent) channel.readInbound()).release();
+
+        var part2 = httpContent(MAX_CONTENT_LENGTH);


nit: this should fail at 1 byte, no need to send double the limit here. But also, could we send a wider variety of chunk sizes rather than just one max-sized chunk? Something like this?

int contentBytesSent = 0; do { var thisPartSize = between(1, MAX_CONTENT_LENGTH * 2); channel.writeInbound(encode(httpContent(thisPartSize))); contentBytesSent += thisPartSize; if (contentBytesSent <= MAX_CONTENT_LENGTH) { ((HttpContent) channel.readInbound()).release(); } else { break; } } while (true);

Test covers both invariants of content tracking before reaching limit and after, also at precise mark of MAX_CONTENT_LENGTH to cover one-off case. The chunking itself has no meaning here. I'm fine to change thou.

…andler

DaveCTurner

LGTM

DaveCTurner · 2025-01-16T19:33:51Z

...rt-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpContentSizeHandlerTests.java

+     */
+    public void testExpectationFailed() {
+        var sendRequest = httpRequest();
+        sendRequest.headers().set(HttpHeaderNames.EXPECT, "Potato");


:) maybe randomValueOtherThan("100-Continue", ESTestCase::randomIdentifier); would be a more descriptive way to say what we're trying to do here

mhl-b · 2025-01-16T21:04:48Z

@DaveCTurner, CI catch an interesting exception in testEntityTooLargeWithContent.
In this test client sends oversized request with "expect" and content without waiting for continue response.
So I reset decoder and expect following content to have decoding failure. But in some cases content can look like a new request and confuses decoder.

I'm wondering should we always close connection when request is oversized, including expect-continue?
I would say yes, client should not poke ES with different content sizes, it's not a proxy. Also I wouldn't rely on client to behave and wait for a 100-Continue reply.

DaveCTurner · 2025-01-16T21:18:56Z

In this test client sends oversized request with "expect" and content without waiting for continue response.

That sounds like invalid client behaviour to me. The whole point of Expect: 100-Continue is that the client wants to be told to continue before it starts to send the body, or else to move on to the next request without sending any body on an error. If it's just going to send the body regardless of the response then why send Expect: 100-Continue?

I think the test should instead simulate correct client behaviour: send the headers but no body, check it gets a 413 back, and then ideally try and send another (valid) request to check the decoder gets left in the right state to handle it.

mhl-b · 2025-01-16T22:22:48Z

I think it's correct behaviour based on rfc. The expect header is not very strict.

https://www.rfc-editor.org/rfc/rfc9110.html#section-10.1.1-11.3

A client that sends a 100-continue expectation is not required to wait for any specific length of time; such a client MAY proceed to send the content even if it has not yet received a response. Furthermore, since 100 (Continue) responses cannot be sent through an HTTP/1.0 intermediary, such a client SHOULD NOT wait for an indefinite period before sending the content.

A server MAY omit sending a 100 (Continue) response if it has already received some or all of the content for the corresponding request, or if the framing indicates that there is no content.

mhl-b · 2025-01-16T22:34:15Z

I think the test should instead simulate correct client behaviour: send the headers but no body, check it gets a 413 back, and then ideally try and send another (valid) request to check the decoder gets left in the right state to handle it.

I will add this one.

nicktindall

LGTM

mhl-b · 2025-01-17T04:17:38Z

Added mixed content test. 6080e50. Assuming a well behaving client that patiently awaits Expect response and does not send content if receive rejection.

DaveCTurner · 2025-01-17T08:33:03Z

I think it's correct behaviour based on rfc.

Interesting, I missed that. In which case I think we should reinstate the test behaviour that sends the content straight away too (maybe in a follow-up). The spec doesn't say how to proceed after sending a response other than 100 Continue tho: is the client supposed to skip the body or not? If so how does the server detect the end of the body and thus the start of the next request? It seems the only safe thing to do in this case is indeed to close the connection.

mhl-b · 2025-01-17T19:00:48Z

If so how does the server detect the end of the body and thus the start of the next request?

I'm not liking spec much. The implementation seems not trivial and decoder has to be very smart about it. I think keeping current netty's behaviour - reset decoder and dont close connection - should be ok. And dont test this use case.

elasticsearchmachine · 2025-01-17T19:02:46Z

💔 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 120246

DaveCTurner · 2025-01-17T21:27:42Z

I'm not liking spec much. The implementation seems not trivial and decoder has to be very smart about it. I think keeping current netty's behaviour - reset decoder and dont close connection - should be ok. And dont test this use case.

I'm ok with that too.

This reverts commit 43e3e24.

…ic#120934) This reverts commit 43e3e24.

add http stream content size handler

eaf5d32

elasticsearchmachine added the v9.0.0 label Jan 15, 2025

fix leak

89cd5e5

mhl-b commented Jan 16, 2025

View reviewed changes

mhl-b requested review from DaveCTurner and nicktindall January 16, 2025 06:11

mhl-b marked this pull request as ready for review January 16, 2025 06:11

mhl-b requested a review from ywangd January 16, 2025 06:11

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jan 16, 2025

mhl-b added >non-issue :Distributed/Network Http and internode communication implementations Team:Distributed Coordination (obsolete) Meta label for Distributed Coordination team. Obsolete. Please do not use. and removed needs:triage Requires assignment of a team area label labels Jan 16, 2025

DaveCTurner reviewed Jan 16, 2025

View reviewed changes

mhl-b added 2 commits January 16, 2025 11:07

feedback

1cdfe46

Merge remote-tracking branch 'upstream/main' into http-content-size-h…

06037be

…andler

DaveCTurner approved these changes Jan 16, 2025

View reviewed changes

mhl-b added 2 commits January 16, 2025 12:03

update test

26fd11a

merge main

c560536

mhl-b added the v8.18.0 label Jan 16, 2025

nicktindall approved these changes Jan 17, 2025

View reviewed changes

add mixed test

6080e50

mhl-b added the auto-backport Automatically create backport pull requests when merged label Jan 17, 2025

mhl-b merged commit 43e3e24 into elastic:main Jan 17, 2025

elasticsearchmachine added the backport pending label Jan 17, 2025

mhl-b mentioned this pull request Jan 17, 2025

Backport http stream content size #120246 #120417

Merged

mhl-b added backport and removed backport pending backport labels Jan 21, 2025

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Jan 27, 2025

Revert "Add http stream content size handler (elastic#120246)"

f62fc52

This reverts commit 43e3e24.

ldematte mentioned this pull request Jan 27, 2025

Revert "Add http stream content size handler (#120246)" #120934

Merged

Tim-Brooks pushed a commit that referenced this pull request Jan 27, 2025

Revert "Add http stream content size handler (#120246)" (#120934)

570efa2

This reverts commit 43e3e24.

Tim-Brooks pushed a commit to Tim-Brooks/elasticsearch that referenced this pull request Jan 27, 2025

Revert "Add http stream content size handler (elastic#120246)" (elast…

0ab3677

…ic#120934) This reverts commit 43e3e24.

mhl-b mentioned this pull request Jan 28, 2025

Add http stream content size handler (fixed #120246) #121095

Merged

	private static final int maxContentLength = 50 * 1024 * 1024;
	private static final int TEST_MAX_CONTENT_LENGTH_BYTES = ByteSizeUnit.MB.toIntBytes(50);

Conversation

mhl-b commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhl-b Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 16, 2025

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhl-b Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Jan 16, 2025

Uh oh!

mhl-b commented Jan 16, 2025

Uh oh!

mhl-b commented Jan 16, 2025

Uh oh!

nicktindall left a comment

Choose a reason for hiding this comment

Uh oh!

mhl-b commented Jan 17, 2025

Uh oh!

DaveCTurner commented Jan 17, 2025

Uh oh!

mhl-b commented Jan 17, 2025

Uh oh!

elasticsearchmachine commented Jan 17, 2025

💔 Backport failed

Uh oh!

DaveCTurner commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mhl-b commented Jan 15, 2025 •

edited

Loading

DaveCTurner Jan 16, 2025 •

edited

Loading

mhl-b commented Jan 16, 2025 •

edited

Loading