Merge pull request #2153 from rhansen/upstream-cleanup

fix: Generate at most one `server` directive per container

nginx.tmpl

@@ -1,3 +1,5 @@
+# nginx-proxy{{ if $.Env.NGINX_PROXY_VERSION }} version : {{ $.Env.NGINX_PROXY_VERSION }}{{ end }}
+
 {{- /*
      * Global values.  Values are stored in this map rather than in individual
      * global variables so that the values can be easily passed to embedded
@@ -9,7 +11,6 @@
 {{- $_ := set $globals "Env" $.Env }}
 {{- $_ := set $globals "Docker" $.Docker }}
 {{- $_ := set $globals "CurrentContainer" (where $globals.containers "ID" $globals.Docker.CurrentContainerID | first) }}
-{{- $_ := set $globals "nginx_proxy_version" (coalesce $globals.Env.NGINX_PROXY_VERSION "") }}
 {{- $_ := set $globals "external_http_port" (coalesce $globals.Env.HTTP_PORT "80") }}
 {{- $_ := set $globals "external_https_port" (coalesce $globals.Env.HTTPS_PORT "443") }}
 {{- $_ := set $globals "sha1_upstream_name" (parseBool (coalesce $globals.Env.SHA1_UPSTREAM_NAME "false")) }}
@@ -18,6 +19,12 @@
 {{- $_ := set $globals "access_log" (or (and (not $globals.Env.DISABLE_ACCESS_LOGS) "access_log /var/log/nginx/access.log vhost;") "") }}
 {{- $_ := set $globals "enable_ipv6" (parseBool (coalesce $globals.Env.ENABLE_IPV6 "false")) }}
 {{- $_ := set $globals "ssl_policy" (or ($globals.Env.SSL_POLICY) "Mozilla-Intermediate") }}
+{{- $_ := set $globals "networks" (dict) }}
+# networks available to nginx-proxy:
+{{- range $globals.CurrentContainer.Networks }}
+    {{- $_ := set $globals.networks .Name . }}
+#     {{ .Name }}
+{{- end }}
 
 {{- define "ssl_policy" }}
     {{- if eq .ssl_policy "Mozilla-Modern" }}
@@ -108,52 +115,63 @@
 upstream {{ .Upstream }} {
     {{- $server_found := false }}
     {{- range $container := .Containers }}
+    # Container: {{ $container.Name }}
         {{- /* If only 1 port exposed, use that as a default, else 80 */}}
         {{- $defaultPort := (when (eq (len $container.Addresses) 1) (first $container.Addresses) (dict "Port" "80")).Port }}
+        {{- $ip := "" }}
         {{- $port := (coalesce $container.Env.VIRTUAL_PORT $defaultPort) }}
-        {{- $address := where $container.Addresses "Port" $port | first }}
-    # Exposed ports: {{ $container.Addresses }}
-    # Default virtual port: {{ $defaultPort }}
-    # VIRTUAL_PORT: {{ $container.Env.VIRTUAL_PORT }}
-        {{- if not $address }}
-    # /!\ Virtual port not exposed
+        {{- $addr_obj := where $container.Addresses "Port" $port | first }}
+    #     Exposed ports:{{ range $container.Addresses }} {{ .Port }}/{{ .Proto }}{{ else }} (none){{ end }}
+    #     Default virtual port: {{ $defaultPort }}
+    #     VIRTUAL_PORT: {{ $container.Env.VIRTUAL_PORT }}
+        {{- if and $addr_obj $addr_obj.HostPort }}
+    #     /!\ WARNING: Virtual port published on host.  Clients might be able to
+    #                  bypass nginx-proxy and access the container's server
+    #                  directly.
+        {{- end }}
+        {{- if $container.Node.ID }}
+    #     Swarm node name: {{ $container.Node.Name }}
         {{- end }}
-        {{- range $knownNetwork := $networks }}
-            {{- range $containerNetwork := sortObjectsByKeysAsc $container.Networks "Name" }}
-                {{- if (and (ne $containerNetwork.Name "ingress") (or (eq $knownNetwork.Name $containerNetwork.Name) (eq $knownNetwork.Name "host"))) }}
-    ## Can be connected with "{{ $containerNetwork.Name }}" network
-                    {{- if $address }}
-                        {{- /*
-                             * If we got the containers from swarm and this
-                             * container's port is published to host, use host
-                             * IP:PORT.
-                             */}}
-                        {{- if and $container.Node.ID $address.HostPort }}
-                            {{- $server_found = true }}
-    # {{ $container.Node.Name }}/{{ $container.Name }}
-    server {{ $container.Node.Address.IP }}:{{ $address.HostPort }};
-                            {{- /*
-                                 * If there is no swarm node or the port is not
-                                 * published on host, use container's IP:PORT.
-                                 */}}
-                        {{- else if $containerNetwork }}
-                            {{- $server_found = true }}
-    # {{ $container.Name }}
-    server {{ $containerNetwork.IP }}:{{ $address.Port }};
-                        {{- end }}
-                    {{- else if $containerNetwork }}
-    # {{ $container.Name }}
-                        {{- if $containerNetwork.IP }}
-                            {{- $server_found = true }}
-    server {{ $containerNetwork.IP }}:{{ $port }};
-                        {{- else }}
-    # /!\ No IP for this network!
-                        {{- end }}
-                    {{- end }}
-                {{- else }}
-    # Cannot connect to network '{{ $containerNetwork.Name }}' of this container
-                {{- end }}
+    #     Container networks:
+        {{- range $containerNetwork := sortObjectsByKeysAsc $container.Networks "Name" }}
+            {{- if eq $containerNetwork.Name "ingress" }}
+    #         {{ $containerNetwork.Name }} (ignored)
+                {{- continue }}
+            {{- end }}
+            {{- if and (not (index $networks $containerNetwork.Name)) (not $networks.host) }}
+    #         {{ $containerNetwork.Name }} (unreachable)
+                {{- continue }}
+            {{- end }}
+            {{- /*
+                 * Do not emit multiple `server` directives for this container
+                 * if it is reachable over multiple networks.  This avoids
+                 * accidentally inflating the effective round-robin weight of
+                 * this container due to the redundant upstreams that nginx sees
+                 * as belonging to distinct servers.
+                 */}}
+            {{- if $ip }}
+    #         {{ $containerNetwork.Name }} (ignored; reachable but redundant)
+                {{- continue }}
             {{- end }}
+    #         {{ $containerNetwork.Name }} (reachable)
+            {{- /*
+                 * If we got the containers from swarm and this container's
+                 * port is published to host, use host IP:PORT.
+                 */}}
+            {{- if and $container.Node.ID $addr_obj $addr_obj.HostPort }}
+                {{- $ip = $container.Node.Address.IP }}
+                {{- $port = $addr_obj.HostPort }}
+            {{- else if and $containerNetwork $containerNetwork.IP }}
+                {{- $ip = $containerNetwork.IP }}
+            {{- else }}
+    #             /!\ No IP for this network!
+            {{- end }}
+        {{- else }}
+    #         (none)
+        {{- end }}
+        {{- if $ip }}
+            {{- $server_found = true }}
+    server {{ $ip }}:{{ $port }};
         {{- end }}
     {{- end }}
     {{- /* nginx-proxy/nginx-proxy#1105 */}}
@@ -164,10 +182,6 @@ upstream {{ .Upstream }} {
 }
 {{- end }}
 
-{{- if ne $globals.nginx_proxy_version "" }}
-# nginx-proxy version : {{ $globals.nginx_proxy_version }}
-{{- end }}
-
 # If we receive X-Forwarded-Proto, pass it through; otherwise, pass along the
 # scheme used to connect to this server
 map $http_x_forwarded_proto $proxy_x_forwarded_proto {
@@ -288,7 +302,7 @@ server {
             {{- $upstream = printf "%s-%s" $upstream $sum }}
         {{- end }}
 # {{ $host }}{{ $path }}
-{{ template "upstream" (dict "Upstream" $upstream "Containers" $containers "Networks" $globals.CurrentContainer.Networks) }}
+{{ template "upstream" (dict "Upstream" $upstream "Containers" $containers "Networks" $globals.networks) }}
     {{- end }}
 
     {{- $default_host := or ($globals.Env.DEFAULT_HOST) "" }}

test/test_multiple-networks.py

@@ -1,15 +1,28 @@
+import re
+
 import pytest
 
+
 def test_unknown_virtual_host(docker_compose, nginxproxy):
     r = nginxproxy.get("http://nginx-proxy/")
     assert r.status_code == 503
 
 def test_forwards_to_web1(docker_compose, nginxproxy):
     r = nginxproxy.get("http://web1.nginx-proxy.local/port")
-    assert r.status_code == 200   
+    assert r.status_code == 200
     assert r.text == "answer from port 81\n"
 
 def test_forwards_to_web2(docker_compose, nginxproxy):
     r = nginxproxy.get("http://web2.nginx-proxy.local/port")
-    assert r.status_code == 200   
-    assert r.text == "answer from port 82\n"
+    assert r.status_code == 200
+    assert r.text == "answer from port 82\n"
+
+def test_multipath(docker_compose, nginxproxy):
+    r = nginxproxy.get("http://web3.nginx-proxy.test/port")
+    assert r.status_code == 200
+    assert r.text == "answer from port 83\n"
+    cfg = nginxproxy.get_conf().decode()
+    lines = cfg.splitlines()
+    web3_server_lines = [l for l in lines
+                         if re.search(r'(?m)^\s*server\s+[^\s]*:83;\s*$', l)]
+    assert len(web3_server_lines) == 1

test/test_multiple-networks.yml

@@ -3,6 +3,8 @@ version: '2'
 networks:
   net1: {}
   net2: {}
+  net3a: {}
+  net3b: {}
 
 services:
   nginx-proxy:
@@ -12,6 +14,8 @@ services:
     networks:
       - net1
       - net2
+      - net3a
+      - net3b
 
   web1:
     image: web
@@ -32,3 +36,14 @@ services:
       VIRTUAL_HOST: web2.nginx-proxy.local
     networks:
       - net2
+
+  web3:
+    image: web
+    expose:
+      - "83"
+    environment:
+      WEB_PORTS: 83
+      VIRTUAL_HOST: web3.nginx-proxy.test
+    networks:
+      - net3a
+      - net3b