feat: Add support for HTTP keep-alive between the proxy and upstream

[rhansen]

Fixes #609

Note: This depends on PR #2127, and will be marked as draft until that PR is merged.

[rhansen]

I'm not super satisfied with the tests I added, but I couldn't think of a good way to thoroughly test the feature (e.g., grepping the generated config file seems too fragile).

[rhansen]

Friendly ping @buchdag

[rhansen]

Marking as draft until #2127 is merged.

[buchdag]

@rhansen I've got mixed feeling about the use of labels. While I too do prefer labels to environment variables (as they inherently avoid stuff like #2139), I think we should stick to environment variables unless we're prepared to migrate everything to labels, as we don't use labels for anything yet. I also believe that people are more familiar with environment variables than they are with labels.

Other than that, LGTM ✅

[rhansen]

I think we should stick to environment variables unless we're prepared to migrate everything to labels, as we don't use labels for anything yet. I also believe that people are more familiar with environment variables than they are with labels.

Good points. However, assuming we will eventually migrate the existing environment variables to labels (I think we should; they are designed for this purpose), using an environment variable for this feature makes more work for us now and later. I'd rather live with the short-term inconsistency.

[buchdag]

@rhansen the way I see it, in the end we should keep environment variables for configuration of the nginx-proxy (or docker-gen) container, and switch to labels on the proxied containers. Do you agree with this ?

[rhansen]

@buchdag I'm not sure. There are pros and cons either way. I opened #2148 so we can discuss it further.

[buchdag]

@rhansen I'll review this again tonight or tomorrow but it should be okay to merge before next release.

[buchdag]

@rhansen I did not notice the experimental status of the feature. What would be needed to confirm that it work as intended ?

[rhansen]

What would be needed to confirm that it work as intended ?

Time, or discussions/bug reports. We don't have a way of collecting usage statistics, so there's not much we can do other than eventually think to ourselves, "this feature has been around for a while and nobody has complained, so it's probably OK."

Mostly I'm concerned with the user interface, not the concept or implementation. I don't want to be stuck preserving backwards compatibility for a fundamentally flawed interface.

[SchoNie]

keepalive is not experimental and is even recommended to have enabled. I do like the way of this implementation by @rhansen (also I like the docker label concept)
Currently I manually overwrite the template file to have the keepalive directions in there. So having this merged would help me to run this container clean without any overrides.

[rhansen]

keepalive is not experimental

Agreed; the experimental thing here is the ability to enable keepalive in nginx-proxy, not keepalive itself.

I do like the way of this implementation by @rhansen (also I like the docker label concept)

Thanks for the feedback!

[SchoNie]

I suspect there is a problem setting the proxy_set_header Connection $proxy_connection_noclose; in the location block.

According to the manpage: Allows redefining or appending fields to the request header passed to the proxied server. The value can contain text, variables, and their combinations. These directives are inherited from the previous configuration level if and only if there are no proxy_set_header directives defined on the current level.

So setting this single header in the current (location) level will cancel the inherited directives inherited from the global level:

proxy_set_header Host $http_host;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection $proxy_connection;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Host $proxy_x_forwarded_host;
proxy_set_header X-Forwarded-Proto $proxy_x_forwarded_proto;
proxy_set_header X-Forwarded-Ssl $proxy_x_forwarded_ssl;
proxy_set_header X-Forwarded-Port $proxy_x_forwarded_port;
proxy_set_header X-Original-URI $request_uri;

# Mitigate httpoxy attack (see README for details)
proxy_set_header Proxy "";

I tried to test this PR but as side effect it closed all my websocket upgrades. And also the x-real-ip was missing.
Maybe a better way would be to wrap the map in an if, and overwrite the first $proxy_connection (it will stay upgrade if received, else it clears the close with an empty string).

{{- if $keepalive }}			
map $http_upgrade $proxy_connection {
    default upgrade;
    # By default, nginx sets "Connection: close". Cancel that to allow keepalive
    # connections between nginx and the upstream server.
    '' '';
}
{{- end }}

[rhansen]

I suspect there is a problem setting the proxy_set_header Connection $proxy_connection_noclose; in the location block.

Good catch, thank you @SchoNie!

Maybe a better way would be to wrap the map in an if, and overwrite the first $proxy_connection (it will stay upgrade if received, else it clears the close with an empty string).

Thanks for the suggestion. I went with a different approach that preserves per-upstream configurability (it can be turned on for some containers and left off for others). I also updated the tests to check for the regression you found. If you have the time, I would appreciate it if you looked at the new revision.

[SchoNie]

Tested, LGTM.
Good you added a X-Real-IP test!

[buchdag]

@rhansen could you just wait a few days before merging this ? I'd like to release a patch version of docker-gen and then of nginx-proxy before merging this feature and the load balancing one from @SchoNie