feat: Use container names for upstream names to avoid repeats

README.md

@@ -460,12 +460,6 @@ If you want most of your virtual hosts to use a default single `location` block
 #### Per-VIRTUAL_HOST `server_tokens` configuration
 Per virtual-host `servers_tokens` directive can be configured by passing appropriate value to the `SERVER_TOKENS` environment variable. Please see the [nginx http_core module configuration](https://nginx.org/en/docs/http/ngx_http_core_module.html#server_tokens) for more details.
 
-### Unhashed vs SHA1 upstream names
-
-By default the nginx configuration `upstream` blocks will use this block's corresponding hostname as a predictable name. However, this can cause issues in some setups (see [this issue](https://github.com/nginx-proxy/nginx-proxy/issues/1162)). In those cases you might want to switch to SHA1 names for the `upstream` blocks by setting the `SHA1_UPSTREAM_NAME` environment variable to `true` on the nginx-proxy container.
-
-Please note that using regular expressions in `VIRTUAL_HOST` will always result in a corresponding `upstream` block with an SHA1 name.
-
 ### Troubleshooting
 
 In case you can't access your VIRTUAL_HOST, set `DEBUG=true` in the client container's environment and have a look at the generated nginx configuration file `/etc/nginx/conf.d/default.conf`:

nginx.tmpl

@@ -4,7 +4,6 @@
 {{ $external_http_port := coalesce $.Env.HTTP_PORT "80" }}
 {{ $external_https_port := coalesce $.Env.HTTPS_PORT "443" }}
 {{ $debug_all := $.Env.DEBUG }}
-{{ $sha1_upstream_name := parseBool (coalesce $.Env.SHA1_UPSTREAM_NAME "false") }}
 {{ $default_root_response := coalesce $.Env.DEFAULT_ROOT "404" }}
 
 {{ define "ssl_policy" }}
@@ -252,27 +251,38 @@ server {
 }
 {{ end }}
 
-{{ range $host, $containers := groupByMulti $ "Env.VIRTUAL_HOST" "," }}
+{{- /* Keep track of created upstreams to avoid creating redundant upstreams. */}}
+{{- $upstream_names := dict }}
 
+{{ range $host, $containers := groupByMulti $ "Env.VIRTUAL_HOST" "," }}
 {{ $host := trim $host }}
-{{ $is_regexp := hasPrefix "~" $host }}
-{{ $upstream_name := when (or $is_regexp $sha1_upstream_name) (sha1 $host) $host }}
 
 {{ $paths := groupBy $containers "Env.VIRTUAL_PATH" }}
 {{ $nPaths := len $paths }}
 {{ if eq (len $paths) 0 }}
 	{{ $paths = dict "/" $containers }}
 {{ end }}
 
-{{ range $path, $containers := $paths }}
-	{{ $upstream := $upstream_name }}
-	{{ if gt $nPaths 0 }}
-		{{ $sum := sha1 $path }}
-		{{ $upstream = printf "%s-%s" $upstream $sum }}
-	{{ end }}
-# {{ $host }}{{ $path }}
-	{{ template "upstream" (dict "Upstream" $upstream "Containers" $containers "Networks" $CurrentContainer.Networks "Debug" $debug_all) }}
-{{ end }}
+{{- range $path, $containers := $paths }}
+  {{- /*
+       * Create the upstream name by joining the sorted container names. nginx-proxy does not
+       * support more than one upstream server per container, so the collection of containers
+       * sufficiently characterizes an upstream.
+       *
+       * The containers are sorted by name before being joined to avoid redundant upstreams.
+       *
+       * Tilde is used to separate the names because it is in the URI unreserved character set (no
+       * percent escaping required), and it is not permitted in Docker container names (according to
+       * https://github.com/moby/moby/blob/v20.10.14/daemon/names/names.go). Thus, joining multiple
+       * container names with "~" should yield a name that is both a valid NGINX upstream name and a
+       * name that can never collide with a Docker container name.
+       */}}
+  {{- $upstream_name := join "~" (sortStringsAsc (groupByKeys $containers "Name")) }}
+  {{- if not (index $upstream_names $upstream_name) }}
+    {{- template "upstream" (dict "Upstream" $upstream_name "Containers" $containers "Networks" $CurrentContainer.Networks "Debug" $debug_all) }}
+    {{- $_ := set $upstream_names $upstream_name true }}
+  {{- end }}
+{{- end }}
 
 {{ $default_host := or ($.Env.DEFAULT_HOST) "" }}
 {{ $default_server := index (dict $host "" $default_host "default_server") $host }}
@@ -389,14 +399,12 @@ server {
 
 		{{/* Get the NETWORK_ACCESS defined by containers w/ the same vhost, falling back to "external" */}}
 		{{ $network_tag := or (first (groupByKeys $containers "Env.NETWORK_ACCESS")) "external" }}
-		{{ $upstream := $upstream_name }}
+		{{ $upstream_name := join "~" (sortStringsAsc (groupByKeys $containers "Name")) }}
 		{{ $dest := "" }}
 		{{ if gt $nPaths 0 }}
-			{{ $sum := sha1 $path }}
-			{{ $upstream = printf "%s-%s" $upstream $sum }}
 			{{ $dest = (or (first (groupByKeys $containers "Env.VIRTUAL_DEST")) "") }}
 		{{ end }}
-		{{ template "location" (dict "Path" $path "Proto" $proto "Upstream" $upstream "Host" $host "VhostRoot" $vhost_root "Dest" $dest "NetworkTag" $network_tag) }}
+		{{ template "location" (dict "Path" $path "Proto" $proto "Upstream" $upstream_name "Host" $host "VhostRoot" $vhost_root "Dest" $dest "NetworkTag" $network_tag) }}
 	{{ end }}
 	{{ if (not (contains $paths "/")) }}
 	location / {
@@ -432,14 +440,12 @@ server {
 
 		{{/* Get the NETWORK_ACCESS defined by containers w/ the same vhost, falling back to "external" */}}
 		{{ $network_tag := or (first (groupByKeys $containers "Env.NETWORK_ACCESS")) "external" }}
-		{{ $upstream := $upstream_name }}
+		{{ $upstream_name := join "~" (sortStringsAsc (groupByKeys $containers "Name")) }}
 		{{ $dest := "" }}
 		{{ if gt $nPaths 0 }}
-			{{ $sum := sha1 $path }}
-			{{ $upstream = printf "%s-%s" $upstream $sum }}
 			{{ $dest = (or (first (groupByKeys $containers "Env.VIRTUAL_DEST")) "") }}
 		{{ end }}
-		{{ template "location" (dict "Path" $path "Proto" $proto "Upstream" $upstream "Host" $host "VhostRoot" $vhost_root "Dest" $dest "NetworkTag" $network_tag) }}
+		{{ template "location" (dict "Path" $path "Proto" $proto "Upstream" $upstream_name "Host" $host "VhostRoot" $vhost_root "Dest" $dest "NetworkTag" $network_tag) }}
 	{{ end }}
 	{{ if (not (contains $paths "/")) }}
 	location / {

test/test_upstream-name.py

@@ -0,0 +1,15 @@
+import pytest
+import re
+
+
+def test_single_container_in_upstream(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode('ASCII')
+    assert re.search(r"upstream web1 \{", conf)
+
+def test_multiple_containers_in_upstream(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode('ASCII')
+    assert re.search(r"upstream web2~web3 \{", conf)
+
+def test_no_redundant_upstreams(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode('ASCII')
+    assert len(re.findall(r"upstream web4 \{", conf)) == 1

test/test_upstream-name.yml

@@ -0,0 +1,40 @@
+version: '2'
+
+services:
+  web1:
+    container_name: web1
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web1.nginx-proxy.tld
+  web2:
+    container_name: web2
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web23.nginx-proxy.tld
+  web3:
+    container_name: web3
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web23.nginx-proxy.tld
+  web4:
+    container_name: web4
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web4a.nginx-proxy.tld,web4b.nginx-proxy.tld
+
+  sut:
+    image: nginxproxy/nginx-proxy:test
+    volumes:
+      - /var/run/docker.sock:/tmp/docker.sock:ro