Alternator's health-check request doesn't work properly with HTTPS

[nyh]

When Alternator is configured to listen to HTTPS, the health-check service should work with HTTPS as well. This works in Amazon DynamoDB:

$ curl https://dynamodb.us-east-1.amazonaws.com
healthy: dynamodb.us-east-1.amazonaws.com

We have a unit test for this, but to run the test in HTTPS mode we need to do it explicitly, and we don't do it in Jenkins, so this appears to be a regression: test/alternator/run --https test_health.py now fails. When running all tests on https (test/alternator/run --https), most pass, but we have all test_health.py tests fail, and also test_cors.py tests fail and test_scylla.py. We need to fix all of those!

Also noticed by @bhalevy in dtests: https://github.com/scylladb/scylla-dtest/pull/2138.

[nyh]

This is most likely a recent regression - the test test/alternator/run --https test_health.py passes on scylla-4.4.1.
I'll do a git bisect now.

[nyh]

Bisection shows that the offending commit is f41dac2 (by me):

commit f41dac2a3ae0b631b7381072dfc7d7fd03b3f1fb
Author: Nadav Har'El <nyh@scylladb.com>
Date:   Wed Mar 10 00:25:25 2021 +0200

    alternator: avoid large contiguous allocation for request body

This is a new commit, which did not reach 4.4 or earlier, but the fix will need to be backported to 4.5.

[nyh]

It turns out that two Seastar bugs conspire to create this problem (@wmitros who's the expert in that new Seastar code):

1. Seastar's new "request body streaming" feature (scylladb/seastar@4fa0e4b) recognizes when the application's request handler did not fully read its request's body, and marks the connection for closing as it can no longer be reused. However, in this case, we're talking about the health check which is a GET request with an empty body. Because our handler expects no body, it doesn't even try to read it. For some reason with normal HTTP, not reading the zero-length body was fine and the EOF was recognized, but with the HTTPS layer, the server doesn't know the unread body is already at the EOF. The handler would need to do a read to get that EOF on the content-length stream.
2. Although Scylla can easily skip_entire_stream() in the health handler and fix this problem (I verified), and perhaps should, things should have worked even without it (except the unfortunate disconnection) - our response should have still be sent. I saw that we are setting up a response _content, but it is simply not sent to the client before the disconnection. This appears to be a Seastar bug.

Note that although we can circumvent both Seastar bugs in Scylla by simply using skip_entire_stream() in all Scylla's requests that don't care about the request body (including health check and error messages), I think it is still important to fix these two bugs in Seastar. The second is a straight bug, and the first is a performance problem - it causes an HTTPS server which only cares about GET and no bodies and never has anything in its body to not reuse connections - when it could (and would only need to close the connection if the client unexpectedly sends a body and the server doesn't read it).

[nyh]

I analyzed the two issues listed above and opened two Seastar issues scylladb/seastar#907 and scylladb/seastar#906 respectively.

Note that although fixing scylladb/seastar#906 will solve this issue, fixing that issue alone is not enough. The test will start working, but we won't notice that the connection got closed without reason. To fix that remaining problem we must either fix scylladb/seastar#907 or do a fix in Scylla:

Scylla can do a skip_entire_stream() when it doesn't care about the body. This includes healthcheck, as well as error messages. If we do this, it will also solve another longstanding issue - #8195 (however, the problem there is that the body length limit is done in Seastar, so it can't be fixed in Scylla).

If we do this workaround in Scylla, fixing the two Seastar issues will be good, but not necessary for solving this bug. HOWEVER, this is one extra complication - one of the tests, test_health_only_works_for_root_path, sends a bad GET request ("/abc") and expects a 404. This 404 is sent by Seastar, not Scylla, so we can't fix the not-reading-to-the-end-of-the-input in Scylla. To solve this without changes to Seastar we can change the route we use (server::set_routes) to use any URL, not just "/", and have the Scylla code check whether we got "/" or "/localnodes" (fine) or anything else (404).

[nyh]

I sent a patch to the Seastar mailing list, titled "httpd: allow handler to not read an empty content", fixing Seastar issue 907 and making all the broken HTTPS tests involving empty request bodies to work again.

[nyh]

I confirmed that the Alternator tests with HTTPS work on master now. Thanks @avikivity for the commits.
As I mentioned above, we still need to backport the Seastar patch to 4.5, but not to earlier branches (since this is a recent regression).

[avikivity]

Backported to 4.5.

[nyh]

Thanks @avikivity !