[PATCH 0 of 1] Upstream: add propagate_connection_close directive

[Shawn J. Goff]

On 01/04/2015 07:47 PM, Maxim Dounin wrote:
> Hello!

Hi, thanks for taking the time to review the patch!

> On Fri, Jan 02, 2015 at 04:03:58PM -0800, Shawn J. Goff wrote:
>
>> This patch adds a new directive to the upstream module:
>> propagate_connection_close. It puts a "Connection: close" header in
>> the downstream response if the upstream server sent one. This is
>> useful when Nginx is used as HTTP/HTTPS endpoints with load blancers
>> in front of them. It allows the upstream server to close connections
>> in order to shed load.
> You may try to better elaborate on the problem you are trying to
> solve and why existing mechanisms do not work for you.

We have HTTP servers that sit behind TCP load balancers. The servers 
currently have a protocol for making sure long-lived connections are 
balanced among them. This involves closing specific connections at 
appropriate times; this causes the client to open a new connection, 
which will most-likely be handled by another host. We now want those 
hosts to accept HTTPS connections in a well-understood, reliable way 
that has acceptable performance characteristics. We are hoping to use 
Nginx. We found that it breaks our load-balancing because connections 
are not being closed.

> As of now, nginx can:
> but
> - Disable keepalive completely ("keepalive_timeout 0") on a
>    per-location basis (http://nginx.org/r/keepalive_timeout).

This will turn off keep-live for every connection at the location, which 
is not what we want - we want to close specific existing connections.

> - Disable keepalive based on a number of requests served in a
>    connection (http://nginx.org/r/keepalive_requests).

This is not enough; we already have a way of choosing connections to 
close, and it's not only based on the number of requests (or on length 
of idle time, keepalive_timeout).

> - Disable keepalive in some specific conditions
>    (http://nginx.org/r/keepalive_disable).

This disables keep-alive per browser, which is very much a different 
thing, and not our problem.

> - Disable chunked transfer encoding on a per-location basis
>    (http://nginx.org/r/chunked_transfer_encoding).

This is also not going to help us.

> I think this addresses most, if not all, problems in the links you
> provided.  In particular, low enough keepalive_requests can be
> used to distribute load if needed.

It can, at the expense of higher latency. We are extremely 
latency-sensitive, so we've put a lot of work into ensuring our p99.9+ 
latencies are consistent and as low as possible. To that end, we have 
our own method of distributing the load that should really not be part 
of Nginx, as it's highly specific to our service.

>
>> I can submit a documentation patch if this patch is accepted.
> The approach taken in the patch looks wrong, for multiple reasons,
> in particular:
>
> - The upstream module isn't expected to contain it's own
>    directives, expect ones used to define upstream{} blocks.
>    Instead, there should be directives in modules implementing
>    protocols, like "proxy_foo_bar...".

I had considered putting it in upstream, but thought the having it in 
location{} would give more flexibility. I'd be fine putting it in 
upstream{} instead.

As far as putting it in a proxy_foo_bar module, I took a look through 
the modules here: 
http://nginx.org/en/docs/http/ngx_http_proxy_module.html . The only one 
I see that might be appropriate is proxy_pass; are there any others you 
were referring to?

I chose to put this in the upstream module because that is what strips 
out the Connection header and sets the connection_close field in the 
headers_in struct that is specific to the upstream module.

>
> - The "Connection: close" header is a hop-by-hop http header, and
>    "propogating" it looks like a bad idea.  It mixes control of the
>    nginx-to-backend connection with control of the client-to-nginx
>    connection.  Instead, there should be a way to control these
>    connections separately.  It may be an option to add X-Accel-...
>    header instead, similart to X-Accel-Limit-Rate.  Though this
>    approach has it's own problems too, see below.

It is hop-by-hop, but we're not really wanting Nginx as a separate hop; 
that is just a byproduct. Nginx on the same host as the upstream server; 
it's just there to take care of TLS for us.

>
> - It is not possible to control connections that aren't proxied
>    to backends but are handled locally - e.g., when using embedded
>    perl or even just serving static files.
>
> If there is a need to allow dynamic control of keepalive, I think
> that proper way would be to extend the "keepalive_disable"
> directive with variables support.
>

How would this work? Should I set a variable depending on whether some 
X-Accel- header is present, then set keepalive_disable per request 
depending on that variable?