feat: Add support for HTTP load balancing between the proxy and upstream server groups

[SchoNie]

Currently all connections are forwarded upstream with Round Robin algorithm. (default Nginx)
This pull request adds a container labels to specify the load balancing algorithm. Which can be used to have session persistence / affinity.
Some web applications which use the HTTP long-polling transport like websocket require sticky sessions.

Related #871, #299, #221, #886

Note if #1934 is merged: When using load balancing methods other than the default round-robin method, it is necessary to activate them before the keepalive directive.

If you think this PR is usefull I also plan to add the 'weight' directive.

I am trying to write a test. I have no pytest experience and having a little hard time to setup the test suite.

def test_web_loadbalance(docker_compose, nginxproxy):
    conf = nginxproxy.get_conf().decode('ASCII')
    r1 = nginxproxy.get("http://web.nginx-proxy.tld")
    r2 = nginxproxy.get("http://web.nginx-proxy.tld")
    assert r1.status_code == 200
    assert r2.text == r1.text

If I am correct the test webserver should output the container hostname.
But this always passes. In my first test when still using round robin I would expect this to fail.
Could someone point me in the right direction?

[rhansen]

If you think this PR is useful

I do! Thank you for the PR!

If I am correct the test webserver should output the container hostname.

Yes.

But this always passes. In my first test when still using round robin I would expect this to fail.

I too would expect that test to fail. Some questions:

• Could this be caused by Connection: keep-alive from the test client to nginx?
  • Does nginx use the same upstream server to handle all requests transmitted in the same persistent connection?
  • Does the HTTP client in the nginxproxy fixture use persistent connections?
• What does the associated .yml file contain?
• What do you get when you manually query? Like this:

  docker-compose -f test/test_load_balancing.yml up -d
  ip=$(docker container inspect -f '{{.NetworkSettings.Networks.bridge.IPAddress}}' test_sut_1)
  curl --connect-to ::${ip}: http://web.nginx-proxy.tld/

• Does your nginx-proxy container have any extra config files that enable content caching? (run docker exec test_sut_1 nginx -T to see nginx's full config)

[rhansen]

Given that the user provides the full line including the semicolon, maybe it would be better to just add an include directive if a certain file exists. For example, look for /etc/nginx/upstream.d/${upstream_name}.conf first, and if that doesn't exist, look for /etc/nginx/upstream.d/default.conf. If either exists, that file will be included. Then users can put the load balancing directive in the include file.

Two annoying things about my suggestion:

• The user has to be able to determine the upstream name that nginx-proxy generates, which I plan on changing (feat: Use container names for upstream names to avoid repeats #1938).
• The user has to create a separate file and bind mount it into the container, rather than simply define a label.

Maybe a compromise would be to rename the label in this PR from *.loadbalance to something like *.upstream_directives and adjust the documentation to say that any directives suitable in an upstream block can be placed here, such as load balancing directives. One problem with this: what if multiple containers in the same upstream set this label? This PR picks an arbitrary winner; if the label is renamed, maybe it would be better to concatenate the values. There are usability downsides either way.

[SchoNie]

Thank you Richard! Your hints pointed me in the right direction and I had a small error in my compose file.
Added some initial tests. All feedback is appreciated.

Given that the user provides the full line including the semicolon, maybe it would be better to just add an include directive if a certain file exists. For example, look for /etc/nginx/upstream.d/${upstream_name}.conf first, and if that doesn't exist, look for /etc/nginx/upstream.d/default.conf. If either exists, that file will be included. Then users can put the load balancing directive in the include file.

Two annoying things about my suggestion:

• The user has to be able to determine the upstream name that nginx-proxy generates, which I plan on changing (feat: Use container names for upstream names to avoid repeats #1938).
• The user has to create a separate file and bind mount it into the container, rather than simply define a label.

Maybe a compromise would be to rename the label in this PR from *.loadbalance to something like *.upstream_directives and adjust the documentation to say that any directives suitable in an upstream block can be placed here, such as load balancing directives. One problem with this: what if multiple containers in the same upstream set this label? This PR picks an arbitrary winner; if the label is renamed, maybe it would be better to concatenate the values. There are usability downsides either way.

I was at first thinking to make the loadbalancing a true/false option. But then because of all flexible loadbalance options I decided to keep it clean and just insert the directive as-is from the label to prevent from a lot if/else stuff.
And focused more on writing the documentation with good examples.

I do like your suggestions. They gave me new ideas. My thoughts:
Including a upstream.d file is smart, but indeed your predicable name PR needs to be in there first. And maybe not so easy for the user to find out.
More and more apps use websockets and now we can provide a documented and unit-tested approach to handle this.

Renaming to *.upstream_directives is a possibility but for me it sounds a little to global. I don't see much more options in the upstream module an average user would use. (The most interesting are related to the keepalive timeouts) and like you say can be dangerous if set different on multiple containers.

I rather would expand this PR to include the server weight and make a separate PR to include a upstream.d file. Which we can benefit from in more complex scenarios.

[rhansen]

LGTM. What do you think @buchdag?

We should remember to squash the commits when merging (or you can squash them yourself @SchoNie and force push before we merge).

[buchdag]

@rhansen apart from the request change related to the feedback discussion and its inclusion in the docs, this LGTM (and yes we should squash the commits).