From e97bf606c8c45e708f5617f1f9a6e1dd9e99a243 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 25 Jan 2023 17:49:23 -0500 Subject: [PATCH 1/6] chore: Move version comment to the top of the template to ensure that the version is always the first output line. Also, always output `# nginx-proxy`, even if the version isn't known. This makes it easier to find the start of the generated config in the output of `nginx -T`. --- nginx.tmpl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/nginx.tmpl b/nginx.tmpl index 54872e4..b44f4d5 100644 --- a/nginx.tmpl +++ b/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")) }} @@ -164,10 +165,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 { From 2760ead49041fa6dafaab6fd55120e41c3c1ec28 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 26 Jan 2023 16:39:13 -0500 Subject: [PATCH 2/6] chore: Remove warning comment when port is not exposed Exposing ports is largely deprecated because it doesn't actually do anything in Docker. --- nginx.tmpl | 3 --- 1 file changed, 3 deletions(-) diff --git a/nginx.tmpl b/nginx.tmpl index b44f4d5..0982aaa 100644 --- a/nginx.tmpl +++ b/nginx.tmpl @@ -116,9 +116,6 @@ upstream {{ .Upstream }} { # Exposed ports: {{ $container.Addresses }} # Default virtual port: {{ $defaultPort }} # VIRTUAL_PORT: {{ $container.Env.VIRTUAL_PORT }} - {{- if not $address }} - # /!\ Virtual port not exposed - {{- 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"))) }} From 5a8a6ceae2cee36b3f60168bc496fa9ec568f3de Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 26 Jan 2023 16:53:52 -0500 Subject: [PATCH 3/6] chore: Improve debug comments in `upstream` template --- nginx.tmpl | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/nginx.tmpl b/nginx.tmpl index 0982aaa..9251948 100644 --- a/nginx.tmpl +++ b/nginx.tmpl @@ -109,17 +109,26 @@ 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 }} {{- $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 }} + # Exposed ports:{{ range $container.Addresses }} {{ .Port }}/{{ .Proto }}{{ else }} (none){{ end }} + # Default virtual port: {{ $defaultPort }} + # VIRTUAL_PORT: {{ $container.Env.VIRTUAL_PORT }} + {{- if $container.Node.ID }} + # Swarm node name: {{ $container.Node.Name }} + {{- end }} {{- range $knownNetwork := $networks }} + # Container network reachability from {{ $knownNetwork.Name }}: {{- 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 eq $containerNetwork.Name "ingress" }} + # {{ $containerNetwork.Name }} (ignored) + {{- else if and (ne $knownNetwork.Name $containerNetwork.Name) (ne $knownNetwork.Name "host") }} + # {{ $containerNetwork.Name }} (unreachable) + {{- else }} + # {{ $containerNetwork.Name }} (reachable) {{- if $address }} {{- /* * If we got the containers from swarm and this @@ -128,7 +137,6 @@ upstream {{ .Upstream }} { */}} {{- 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 @@ -136,21 +144,19 @@ upstream {{ .Upstream }} { */}} {{- 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! + # /!\ No IP for this network! {{- end }} {{- end }} - {{- else }} - # Cannot connect to network '{{ $containerNetwork.Name }}' of this container {{- end }} + {{- else }} + # (none) {{- end }} {{- end }} {{- end }} From daeed502cb639e0ffdeca25522af1538e125ccb5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 26 Jan 2023 18:53:50 -0500 Subject: [PATCH 4/6] feat: Add a warning comment if the container port is published --- nginx.tmpl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nginx.tmpl b/nginx.tmpl index 9251948..f236000 100644 --- a/nginx.tmpl +++ b/nginx.tmpl @@ -117,6 +117,11 @@ upstream {{ .Upstream }} { # Exposed ports:{{ range $container.Addresses }} {{ .Port }}/{{ .Proto }}{{ else }} (none){{ end }} # Default virtual port: {{ $defaultPort }} # VIRTUAL_PORT: {{ $container.Env.VIRTUAL_PORT }} + {{- if and $address $address.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 }} From bcec2d9075c053b9930de52a32a712d69961a213 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 26 Jan 2023 19:13:36 -0500 Subject: [PATCH 5/6] chore: Refactor `upstream` template for readability In particular, reduce the nesting depth to make it easier to understand what the code is doing by: * converting an $O(nm)$ nested loop into two serial $O(n)+O(m)$ loops, and * consolidating similar nested `if` cases. --- nginx.tmpl | 72 +++++++++++++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/nginx.tmpl b/nginx.tmpl index f236000..310b60e 100644 --- a/nginx.tmpl +++ b/nginx.tmpl @@ -19,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" }} @@ -113,11 +119,11 @@ upstream {{ .Upstream }} { {{- /* 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 }} {{- $port := (coalesce $container.Env.VIRTUAL_PORT $defaultPort) }} - {{- $address := where $container.Addresses "Port" $port | first }} + {{- $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 $address $address.HostPort }} + {{- 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. @@ -125,44 +131,32 @@ upstream {{ .Upstream }} { {{- if $container.Node.ID }} # Swarm node name: {{ $container.Node.Name }} {{- end }} - {{- range $knownNetwork := $networks }} - # Container network reachability from {{ $knownNetwork.Name }}: - {{- range $containerNetwork := sortObjectsByKeysAsc $container.Networks "Name" }} - {{- if eq $containerNetwork.Name "ingress" }} + # Container networks: + {{- range $containerNetwork := sortObjectsByKeysAsc $container.Networks "Name" }} + {{- if eq $containerNetwork.Name "ingress" }} # {{ $containerNetwork.Name }} (ignored) - {{- else if and (ne $knownNetwork.Name $containerNetwork.Name) (ne $knownNetwork.Name "host") }} - # {{ $containerNetwork.Name }} (unreachable) - {{- else }} - # {{ $containerNetwork.Name }} (reachable) - {{- 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 }} - 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 }} - server {{ $containerNetwork.IP }}:{{ $address.Port }}; - {{- end }} - {{- else if $containerNetwork }} - {{- if $containerNetwork.IP }} - {{- $server_found = true }} - server {{ $containerNetwork.IP }}:{{ $port }}; - {{- else }} - # /!\ No IP for this network! - {{- end }} - {{- end }} - {{- end }} - {{- else }} - # (none) + {{- continue }} {{- end }} + {{- if and (not (index $networks $containerNetwork.Name)) (not $networks.host) }} + # {{ $containerNetwork.Name }} (unreachable) + {{- 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 }} + {{- $server_found = true }} + server {{ $container.Node.Address.IP }}:{{ $addr_obj.HostPort }}; + {{- else if and $containerNetwork $containerNetwork.IP }} + {{- $server_found = true }} + server {{ $containerNetwork.IP }}:{{ $port }}; + {{- else }} + # /!\ No IP for this network! + {{- end }} + {{- else }} + # (none) {{- end }} {{- end }} {{- /* nginx-proxy/nginx-proxy#1105 */}} @@ -293,7 +287,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) "" }} From 6162427c4533a7c48881a5e666e206ba9b87084c Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 28 Jan 2023 00:19:21 -0500 Subject: [PATCH 6/6] fix: Generate at most one `server` directive per container --- nginx.tmpl | 23 +++++++++++++++++++---- test/test_multiple-networks.py | 19 ++++++++++++++++--- test/test_multiple-networks.yml | 15 +++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/nginx.tmpl b/nginx.tmpl index 310b60e..83f9222 100644 --- a/nginx.tmpl +++ b/nginx.tmpl @@ -118,6 +118,7 @@ upstream {{ .Upstream }} { # 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) }} {{- $addr_obj := where $container.Addresses "Port" $port | first }} # Exposed ports:{{ range $container.Addresses }} {{ .Port }}/{{ .Proto }}{{ else }} (none){{ end }} @@ -141,23 +142,37 @@ upstream {{ .Upstream }} { # {{ $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 }} - {{- $server_found = true }} - server {{ $container.Node.Address.IP }}:{{ $addr_obj.HostPort }}; + {{- $ip = $container.Node.Address.IP }} + {{- $port = $addr_obj.HostPort }} {{- else if and $containerNetwork $containerNetwork.IP }} - {{- $server_found = true }} - server {{ $containerNetwork.IP }}:{{ $port }}; + {{- $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 */}} {{- if not $server_found }} diff --git a/test/test_multiple-networks.py b/test/test_multiple-networks.py index b9fa4c5..550d0a3 100644 --- a/test/test_multiple-networks.py +++ b/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" \ No newline at end of file + 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 diff --git a/test/test_multiple-networks.yml b/test/test_multiple-networks.yml index e4548b5..7e79174 100644 --- a/test/test_multiple-networks.yml +++ b/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