[pve-devel] [RFC http-server] fix #5391: proxy request: avoid HTTP 599 Too many redirections

[Friedrich Weber]

Forgot to include the following note here:

    Fabian noted we should do some benchmarks to validate that the more
    conservative/safe settings for connection reuse do not result in a
    huge performance drop. I haven't gotten around to do these benchmarks,
    but wanted to submit the patch as RFC already.

On 17/06/2024 18:03, Friedrich Weber wrote:
> The API server proxies HTTP requests in two cases:
> 
> - between cluster nodes (pveproxy->pveproxy)
> - between daemons on one node for protected API endpoints
>   (pveproxy->pvedaemon)
> 
> The API server uses AnyEvent::HTTP for proxying, with unfortunate
> settings for connection reuse (details below). With these settings,
> long-running synchronous API requests on the proxy destination's side
> can cause unrelated proxied requests to fail with a misleading HTTP
> 599 "Too many redirections" error response. In order to avoid these
> errors, improve the connection reuse settings.
> 
> In more detail:
> 
> Per default, AnyEvent::HTTP reuses previously-opened connections for
> requests with idempotent HTTP verbs, e.g. GET/PUT/DELETE [1]. However,
> when trying to reuse a previously-opened connection, it can happen
> that the destination unexpectedly closes the connection. In case of
> idempotent requests, AnyEvent::HTTP's http_request will retry by
> recursively calling itself. Since the API server disallows recursion
> by passing `recurse => 0` to http_request initially, the recursive
> call fails with "HTTP 599 Too many redirections".
> 
> This can happen both for pveproxy->pveproxy and pveproxy->pvedaemon,
> as connection reuse is enabled in both cases. Connection reuse being
> enabled in the pveproxy->pvedaemon case was likely not intended: A
> comment mentions that "keep alive for localhost is not worth it", but
> only sets `keepalive => 0` and not `persistent => 0`. This setting
> switches from HTTP/1.1 persistent connections to HTTP/1.0-style
> keep-alive connections, but still allows connection reuse.
> 
> The destination unexpectedly closing the connection can be due to
> unfortunate timing, but it becomes much more likely in case of
> long-running synchronous requests. An example sequence:
> 
> 1) A pveproxy worker P1 handles a protected request R1 and proxies it
>    to a pvedaemon worker D1, opening a pveproxy worker->pvedaemon
>    worker connection C1. The pvedaemon worker D1 is relatively fast
>    (<1s) in handling R1. P1 saves connection C1 for later reuse.
> 2) A different pveproxy worker P2 handles a protected request R2 and
>    proxies it to the same pvedaemon worker D1, opening a new pveproxy
>    worker->pvedaemon connection C2. Handling this request takes a long
>    time (>5s), for example because it queries a slow storage. While
>    the request is being handled, the pvedaemon worker D1 cannot do
>    anything else.
> 3) Since pvedaemon worker D1 sets a timeout of 5s when accepting
>    connections and it did not see anything on connection C1 for >5s
>    (because it was busy handling R2), it closes the connection C1.
> 3) pveproxy worker P1 handles a protected idempotent request R3. Since
>    the request is idempotent, it tries to reuse connection C1. But C1
>    was just closed by D1, so P1 fails request R3 with HTTP 599 as
>    described above.
> 
> In addition, AnyEvent::HTTP's default of reusing connections for all
> idempotent HTTP verbs is problematic in our case, as not all PUT
> requests of the PVE API are actually idempotent, e.g. /sendkey [2].
> 
> To fix the issues above, improve the connection reuse settings:
> 
> - Actually disable connection reuse for pveproxy->pvedaemon requests,
>   by passing `persistent => 0`.
> - For pveproxy->pveproxy requests, enable connection reuse for GET
>   requests only, as these should be actually idempotent.
> - If connection reuse is enabled, allow one retry by passing `recurse
>   => 1`, to avoid the HTTP 599 errors.
> 
> [1] https://metacpan.org/pod/AnyEvent::HTTP#persistent-=%3E-$boolean
> [2] https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/qemu/{vmid}/sendkey
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber at proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> index a8d60c1..32eb223 100644
> --- a/src/PVE/APIServer/AnyEvent.pm
> +++ b/src/PVE/APIServer/AnyEvent.pm
> @@ -710,7 +710,12 @@ sub proxy_request {
>  
>      eval {
>  	my $target;
> -	my $keep_alive = 1;
> +
> +	# By default, AnyEvent::HTTP reuses connections for the idempotent
> +	# request methods GET/HEAD/PUT/DELETE. But not all of our PUT requests
> +	# are idempotent, hence, reuse connections for GET requests only, as
> +	# these should in fact be idempotent.
> +	my $persistent = $method eq 'GET';
>  
>  	# stringify URI object and verify it starts with a slash
>  	$uri = "$uri";
> @@ -722,8 +727,8 @@ sub proxy_request {
>  	my $may_stream_file;
>  	if ($host eq 'localhost') {
>  	    $target = "http://$host:85$uri";
> -	    # keep alive for localhost is not worth (connection setup is about 0.2ms)
> -	    $keep_alive = 0;
> +	    # connection reuse for localhost is not worth (connection setup is about 0.2ms)
> +	    $persistent = 0;
>  	    $may_stream_file = 1;
>  	} elsif (Net::IP::ip_is_ipv6($host)) {
>  	    $target = "https://[$host]:8006$uri";
> @@ -798,9 +803,13 @@ sub proxy_request {
>  	    $method => $target,
>  	    headers => $headers,
>  	    timeout => 30,
> -	    recurse => 0,
>  	    proxy => undef, # avoid use of $ENV{HTTP_PROXY}
> -	    keepalive => $keep_alive,
> +	    persistent => $persistent,
> +	    # if connection reuse is enabled ($persistent is 1), allow one retry, to avoid returning
> +	    # HTTP 599 Too many redirections if the server happens to close the connection
> +	    recurse => $persistent ? 1 : 0,
> +	    # when reusing a connection, send keep-alive headers
> +	    keepalive => 1,
>  	    body => $content,
>  	    tls_ctx => AnyEvent::TLS->new(%{$tls}),
>  	    sub {