Support for Configurable Timeout for Downstream Connections in HTTP/1.1

[ermakov-oleg]

What is the problem your feature solves, or the need it fulfills?

I have several applications that, for various reasons, may open numerous connections and fail to close them properly. As a result, memory usage gradually increases in both the applications and in the Pingora-based proxy due to these open connections. While the main issue originates in the applications, it would be helpful to have protective measures at the proxy level to prevent excessive memory consumption caused by such open connections.

Describe the solution you’d like

It would be helpful to have a downstream setting that configures the keepalive status for connections, so as to limit the time that a connection remains active.

Additional context

Currently, each time a downstream connection is reused, a new ServerSession is created via ServerSession::new_http1, where keepalive_timeout is set to KeepaliveStatus:Off. Then, in HttpSession::read_request in pingora-core/src/protocols/http/v1/server.rs, there is an indefinite wait for data from the socket.

[matthewbjones]

Thanks @ermakov-oleg for creating this issue. I've seen some similar issues with our production deployment, where we have over 4,500+ idle TCP/HTTP connections after just a few days, assuming these are "bad actors" that are constantly out there on the Internet.

In our case, I think there are two separate issues going on:

Issue 1 - Memory bloat / "leaks"

By default, for each new HTTP1 connection, the default is KeepaliveStatus::Off. If, during the lifecycle of your proxy implementation, you do not adjust anything, then there is a Connection: keepalive that is being returned back to the client, and Pingora will keep the connection live, for as long as the connecting client does not disconnect. The problem here, as previously described, is if the connecting client never disconnects, Pingora leaks memory, as it allocates a buffer for each connection. Since the connection does not terminate, over time, more and more memory is consumed.

Over approximately 7 days, the amount of memory bloated/leaked is +389 megabytes. Using a command like netstat -tnp | grep ESTABLISHED | wc -l shows that there are over 4,500 active connections, even though this is a relatively low traffic server. Likely, many of these connections have been idle for hours or days.

Issue 2 - Denial of service potential

Using a simple nc 127.0.0.1 80 command against Pingora without sending any bytes, a connection is established and never closes, even if the client never sends anything. Therefore, it is possible to DoS a Pingora instance by exhausting the # of connections and/or exhausting the available memory (since buffers are allocated per-connection).

As pointed out, a key issue is here: https://github.com/cloudflare/pingora/blob/main/pingora-core/src/protocols/http/v1/server.rs#L140 Beginning here, Pingora attempts to determine if self.keepalive_timeout is set to KeepaliveStatus::Timeout(...), and if so, does properly implement a timeout. However, for a brand-new TCP/HTTP connection, the self.keepalive_timeout is always set to KeepaliveStatus::Off (see https://github.com/cloudflare/pingora/blob/main/pingora-core/src/protocols/http/v1/server.rs#L103C32-L103C52)

There isn't a point in the connection lifecycle where you can get a reference to the HttpSession and change this value prior to Pingora/Tokio "waiting forever". The earliest available lifecycle event, early_request_filter, happens after this infinitely-blocking read.

Given this, the TCP/HTTP connection sits idle indefinitely.

I can confirm that if you change the hardcoded value at https://github.com/cloudflare/pingora/blob/main/pingora-core/src/protocols/http/v1/server.rs#L103C32-L103C52 from KeepaliveStatus::Off to something like KeepaliveStatus::Timeout(Duration::from_secs(5)), for example, that using nc 127.0.0.1 80 and not sending any bytes, that Pingora does successfully close the idle connection after 5 seconds.

Workarounds

I'm still reviewing the code, but the only available workaround I could come up with is to fork the code and change the hardcoded value, as I still am not able to determine any point where one could "hook" into the request to change the HttpSession before this blocking read.

If someone like @andrewhavck could point me in the right direction, I have no problem working on submitting a PR to help address this.

[matthewbjones]

After spending some more time looking at the code (specifically, the 0.4.0 tag), I've determined that some small tweaks can be made to make an improvement.

Here's a commit that addresses the infinitely idling connection problem: https://github.com/matthewbjones/pingora/commit/56163dfd456d092a500f0419e4e4d5cb22dcdf30

This does two things:

1. Updates the request header reading function to use the read_timeout, if there is no applicable keepalive_timeout, which would always be the case as I outlined earlier
2. Sets a sensible default of 60 seconds for both the read_timeout and write_timeout, which matches NGINX's defaults for client_header_timeout (https://nginx.org/en/docs/http/ngx_http_core_module.html#client_header_timeout)

Note: This technically does not match the NGINX client_header_timeout of 60 seconds, in that, this will just block for up to 60 seconds to "read in some data", but if a bad actor client were to "trickle in" a few bytes every now and then, then Pingora could read and read and read well beyond the desired 60 second limit, but I wanted to keep this proof-of-concept change as minimal as possible. Whereas, NGINX uses client_header_timeout as the maximum amount of time a client is provided to send the entire request header, otherwise the connection is dropped.

Now, if a client connects and does nothing, Pingora closes the connection after 60 seconds:

time nc -v 127.0.0.1 80
Connection to 127.0.0.1 port 80 [tcp/*] succeeded!
nc -v 127.0.0.1 80  0.01s user 0.00s system 0% cpu 1:00.02 total

[matthewbjones]

@ermakov-oleg I have a first-pass implementation of a fix over here #541