Use HTTP status code 413 for exceedingly large payloads

[dimas-b]

The 413 status code seems to be more appropriate than
400 (Bad Request) in this case.

[adutra]

IMHO we do need to take connection closed into account. With Quarkus for sure, maybe even with Dropwizard. From my recollection and manual testing, both frameworks close the connection on this response error. The problem is that sometimes the client receives the response payload before the connection is closed, sometimes after. Again from my recollection, in Quarkus, 1 execution out of 3 or 4 ends up with connection closed.

[adutra]

From RFC 9110 on status 413:

The server MAY terminate the request, if the protocol version in use allows it; otherwise, the server MAY close the connection.

[dimas-b]

RFC 9110 defines the protocol-level behaviour, but in Polaris, the REST endpoint produces a response object with a status code in this case. I would not think Quarkus closing the connection prematurely would be correct. I believe Quarkus (or any other Web App framework) ought to send the response object to the client.

@adutra : are you sure the connection resets happened in the "body" test and and not in the "header" test? The headers are handled by the Web Apop framework, so the connection reset decision lies with the framework itself. Still, the error on large payload objects is handled differently.

[adutra]

are you sure the connection resets happened in the "body" test and and not in the "header" test?

Yes 😄 – those two tests gave me enough headaches that I still remember...

The headers are handled by the Web Apop framework,

In Quarkus, both cases are handled by Vertx.

[adutra]

Here is the code in Quarkus that handles large bodies:

https://github.com/quarkusio/quarkus/blob/0d6a99e1dc7d7394adfde2a836442124848d7e81/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerCommonHandlers.java#L49-L55

As you can see, the connection is closed when the response is done.

[adutra]

on the client side the connection can be closed only after the status line is received

Where is that specified? I find it hard to enforce such a guarantee. If the server closes the connection, I don't think there is a way for the TCP layer to close the connection gracefully only after some TCP message containing the status is received by the client.

[dimas-b]

RFC 9110 Section 3.4 A server responds to a client's request by sending one or more response messages, each including a status code [...]

[adutra]

Sure, but from RFC 9112:

If a server performs an immediate close of a TCP connection, there is a significant risk that the client will not be able to read the last HTTP response. If the server receives additional data from the client on a fully closed connection, such as another request sent by the client before receiving the server's response, the server's TCP stack will send a reset packet to the client; unfortunately, the reset packet might erase the client's unacknowledged input buffers before they can be read and interpreted by the client's HTTP parser.

[dimas-b]

What an interesting :) mess, TIL... In this case, I'd say we should drop both the body and header size tests, because it's way beyond the scope of Polaris to ensure deterministic behaviours of these tests.

I personally do not see much value in a test that either validates the status code or does not validate anything because a connection close exception is inconclusive and does not guarantee that even the request was sent.

[adutra]

Yeah... the more it goes the more I think it makes sense to drop both tests.

[dimas-b]

Rebased and re-implemented per discussion above.

[adutra]

Sorry... I re-checked the Quarkus version that I made a while ago, and I believe it will fail with your current code. The version I had already checks for the status only (no check on the response body). And yet, it fails quite frequently with "connection closed" errors.

The current Quarkus version for reference:

polaris/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/PolarisApplicationIntegrationTest.java

Lines 682 to 704 in e0a6074

	public void testRequestBodyTooLarge() {
	Entity<PrincipalRole> largeRequest =
	Entity.json(new PrincipalRole("r".repeat((int) (maxBodySize + 1))));
	try (Client client = ClientBuilder.newClient()) {
	try (Response response =
	client
	.target(String.format("%s/api/management/v1/principal-roles", testEnv.baseUri()))
	.request("application/json")
	.header("Authorization", "Bearer " + fixture.adminToken)
	.header(REALM_PROPERTY_KEY, fixture.realm)
	.post(largeRequest)) {
	assertThat(response)
	.returns(Response.Status.REQUEST_ENTITY_TOO_LARGE.getStatusCode(), Response::getStatus);
	} catch (ProcessingException e) {
	// In some runtime environments the request above will return a 431 but in others it'll
	// result
	// in a ProcessingException from the socket being closed. The test asserts that one of those
	// things happens.
	assertThat(e).hasMessageContaining("Connection was closed");
	}
	}
	}

[dimas-b]

For the sake of preventing spurious CI failures, I added a catch block for expected connection closures.

[eric-maynard]

We don't need to do this here, but I wonder if we could somehow add a check for the limit to this test.

[eric-maynard]

This catch makes sense to me. It might be worth filing an issue to see if we can reproduce the disconnect elsewhere