varnish-cache icon indicating copy to clipboard operation
varnish-cache copied to clipboard

Return HTTP response to RX_OVERFLOW

Open cartoush opened this issue 1 year ago • 1 comments

Resolves https://github.com/varnishcache/varnish-cache/issues/2735

Before this PR, Varnish would silently close the connection to a client that triggered an RX_OVERFLOW (too long URI), this PR allows configuring Varnish to respond to RX_OVERFLOW with a minimal HTTP response that will contain a chosen HTTP code instead.

Its done by adding a new feature flag in order to enable the feature and a new parameter in order to set the HTTP code that will be sent in the response.

cartoush avatar Oct 03 '24 13:10 cartoush

bugwash: we would appreciate some more research regarding which scenarios would benefit from a similar mechanism. Also, the argument has been made that, once we send a response at all, we might as well do so through vcl_synth, but then the question arises what to present to VCL as the request (url, proto, headers).

@cartoush volunteered to spend some time on researching these questions, thank you!

nigoroll avatar Oct 07 '24 13:10 nigoroll

bugwash: we would appreciate some more research regarding which scenarios would benefit from a similar mechanism. Also, the argument has been made that, once we send a response at all, we might as well do so through vcl_synth, but then the question arises what to present to VCL as the request (url, proto, headers).

@cartoush volunteered to spend some time on researching these questions, thank you!

Sorry for the delay, I did some thinking on that and I think one could argue all cases where an error causes a connection to close silently could benefit from that. But generalizing that using vcl_synth might not be appropriate, as you can do restart for example which be quite dangerous, something better could be creating a vcl_invalid subroutine which only gives access to what's relevant to handle the error. In this case the default vcl behavior would be:

sub vcl_invalid {
        return (close);
}

and in order to handle the case here we would have something like:

sub vcl_invalid {
        if (req.error.reason == REQ_HTTP_OVERFLOW) {
                resp.status == 413;
                return (deliver);
        }
        return (close);
}

I think the the whole req should not be usable in vcl_invalid but req.error should be populated with all necessary information to do the processing. But then this would have to take place in another PR as this requires quite a bit of planning in order to decide what can be done in vcl_invalid.

cartoush avatar Nov 21 '24 15:11 cartoush

bugwash: rework to parameter only, then OK

nigoroll avatar Jan 22 '25 14:01 nigoroll

This is not as trivial as I thought to support the status range 400..499 or 0.

nigoroll avatar Jan 22 '25 19:01 nigoroll

has been discussed again, but the decision remains the same...

nigoroll avatar Jan 29 '25 14:01 nigoroll

I renamed the parameter to http_req_overflow_status for consisteny with http_req_size.

nigoroll avatar Feb 05 '25 08:02 nigoroll