feat: Use container names for upstream names to avoid repeats

nginx-proxy · Jan 13, 2023 · 2664d2a · 2664d2a
1 parent 77c743b
commit 2664d2a
Show file tree

Hide file tree

Showing 5 changed files with 199 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -482,11 +482,70 @@ 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
+### Upstream name style
 
-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.
+The generated NGINX config will have one or more `upstream` blocks, each of which defines a [server group](https://nginx.org/en/docs/http/ngx_http_upstream_module.html) for the applicable containers. The name of the upstream is computed in one of three different ways (or "styles"), described below. The desired style is selected by setting the `UPSTREAM_NAME_STYLE` environment variable 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.
+#### `UPSTREAM_NAME_STYLE: container-name`
+
+This is the recommended style, but it is not the default for legacy compatibility reasons.
+
+When this style is selected, the upstream name is computed by joining with a tilde character (`~`) the sorted names of the applicable containers. For example, if containers named `foo` and `bar` are used for the upstream, the upstream name is `bar~foo`.
+
+#### `UPSTREAM_NAME_STYLE: virtual-host`
+
+This style is deprecated; you are encouraged to use `container-name` instead. This is the default style for legacy compatibility reasons.
+
+When this style is selected, the upstream name is the name of the virtual host. If a container is used in multiple virtual hosts (e.g., `VIRTUAL_HOST: foo.example,bar.example`) then multiple identical upstreams are defined for the same container.
+
+If `VIRTUAL_PATH` is used for the virtual host, the virtual path in effect is hashed via SHA1 and appended to the upstream name.
+
+If the virtual host name is a regular expression (begins with `~`), the style is automatically switched to `virtual-host-sha1`.
+
+#### `UPSTREAM_NAME_STYLE: virtual-host-sha1`
+
+This style is deprecated; you are encouraged to use `container-name` instead.
+
+This style can also be selected by leaving `UPSTREAM_NAME_STYLE` unset and setting `SHA1_UPSTREAM_NAME` to `true`.
+
+This style is identical to `virtual-host`, except the hostname is first hashed via SHA1.
+
+#### Upstream name style example
+
+```yaml
+version: "3.8"
+services:
+  nginx-proxy:
+    image: nginxproxy/nginx-proxy
+    volumes:
+      - /var/run/docker.sock:/tmp/docker.sock:ro
+    environment:
+      UPSTREAM_NAME_STYLE: container-name
+  whoami1:
+    container_name: whoami1
+    image: jwilder/whoami
+    environment:
+      VIRTUAL_HOST: whoami.example.net,whoami.example.com
+      VIRTUAL_PATH: /whoami
+      VIRTUAL_DEST: /
+  whoami2:
+    container_name: whoami2
+    image: jwilder/whoami
+    environment:
+      VIRTUAL_HOST: whoami.example.net,whoami.example.com
+      VIRTUAL_PATH: /whoami
+      VIRTUAL_DEST: /
+```
+
+With `UPSTREAM_NAME_STYLE` set to `container-name` as shown in the example above, one upstream is defined named `whoami1~whoami2` for serving both `whoami.example.net/whoami` and `whoami.example.com/whoami`.
+
+With `UPSTREAM_NAME_STYLE` set to `virtual-host`, two upstreams are defined:
+  * An upstream named `whoami.example.net-e36977100088be78b338862f2a84ebc1ce959fef` that is used for serving `whoami.example.net/whoami`.
+  * An upstream named `whoami.example.com-e36977100088be78b338862f2a84ebc1ce959fef` that is used for serving `whoami.example.com/whoami`. (This upstream is identical to the other upstream.)
+
+With `UPSTREAM_NAME_STYLE` set to `virtual-host-sha1` two upstreams are defined:
+  * An upstream named `41d3ec6bf8437c9bb49fd0a1fe07275a6d1d714c-e36977100088be78b338862f2a84ebc1ce959fef` that is used for serving `whoami.example.net/whoami`.
+  * An upstream named `19cc2f29cfd8c0c5fda00d8312f24aad3c729c61-e36977100088be78b338862f2a84ebc1ce959fef` that is used for serving `whoami.example.com/whoami`. (This upstream is identical to the other upstream.)
 
 ### Troubleshooting
 

diff --git a/app/docker-entrypoint.sh b/app/docker-entrypoint.sh
@@ -116,6 +116,11 @@ if [[ $* == 'forego start -r' ]]; then
 			Warning: The default value of TRUST_DOWNSTREAM_PROXY might change to "false" in a future version of nginx-proxy. If you require TRUST_DOWNSTREAM_PROXY to be enabled, explicitly set it to "true".
 		EOT
 	fi
+	if [ "${UPSTREAM_NAME_STYLE}" != "container-name" ]; then
+		cat >&2 <<-EOT
+			Warning: UPSTREAM_NAME_STYLE values other than container-name are deprecated.
+		EOT
+	fi
 fi
 
 exec "$@"
diff --git a/nginx.tmpl b/nginx.tmpl
@@ -4,9 +4,66 @@
 {{- $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" }}
 {{- $trust_downstream_proxy := parseBool (coalesce $.Env.TRUST_DOWNSTREAM_PROXY "true") }}
+{{- $upstream_name_style := coalesce $.Env.UPSTREAM_NAME_STYLE (when (parseBool (coalesce $.Env.SHA1_UPSTREAM_NAME "false")) "virtual-host-sha1" "virtual-host") }}
+
+{{- /*
+     * Template used as a function to compute an upstream name. This template
+     * does not output anything -- it is only used for its side effect: the
+     * computed upstream name is "returned" by storing the value in the provided
+     * dot dict.
+     *
+     * The dot dict is expected to have the following entries:
+     *   - "containers": An array or slice of RuntimeContainer structs.
+     *   - "host": The virtual hostname the upstream will be served from.
+     *   - "path": The virtual path the upstream will be served from, or unset
+     *     if the VIRTUAL_PATH environment variable is not used in any of the
+     *     containers.
+     *   - "style": The method used to compute the upstream name.
+     *
+     * The return value will be added to the dot dict with key "upstream_name".
+     */}}
+{{- define "upstream_name" }}
+    {{- $args := . }}
+    {{- $style := $args.style }}
+    {{- if and (eq $style "virtual-host") (hasPrefix "~" $args.host) }}
+        {{- $style = "virtual-host-sha1" }}
+    {{- end }}
+    {{- $name := "" }}
+    {{- if eq $style "container-name" }}
+        {{- /*
+             * 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.
+             */}}
+        {{- $name = join "~" (sortStringsAsc (groupByKeys $args.containers "Name")) }}
+    {{- else }}
+        {{- if eq $style "virtual-host" }}
+            {{- $name = $args.host }}
+        {{- else if eq $style "virtual-host-sha1" }}
+            {{- $name = sha1 $args.host }}
+        {{- else }}
+            {{- fail (printf "unknown or unsupported UPSTREAM_NAME_STYLE: %s" $style) }}
+        {{- end }}
+        {{- if contains $args "path" }}
+            {{- $name = printf "%s-%s" $name (sha1 $args.path) }}
+        {{- end }}
+    {{- end }}
+    {{- $_ := set $args "upstream_name" $name }}
+{{- end }}
 
 {{- define "ssl_policy" }}
     {{- if eq .ssl_policy "Mozilla-Modern" }}
@@ -269,11 +326,11 @@ server {
 {{- end }}
 }
 
-{{- range $host, $containers := groupByMulti $ "Env.VIRTUAL_HOST" "," }}
+{{- /* Remember 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 }}
@@ -282,13 +339,14 @@ server {
     {{- end }}
 
     {{- range $path, $containers := $paths }}
-        {{- $upstream := $upstream_name }}
-        {{- if gt $nPaths 0 }}
-            {{- $sum := sha1 $path }}
-            {{- $upstream = printf "%s-%s" $upstream $sum }}
+        {{- $args := dict "containers" $containers "host" $host "style" $upstream_name_style }}
+        {{- if gt $nPaths 0 }}{{- $_ := set $args "path" $path }}{{- end }}
+        {{- template "upstream_name" $args }}
+        {{- $upstream_name := $args.upstream_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 }}
-# {{ $host }}{{ $path }}
-{{ template "upstream" (dict "Upstream" $upstream "Containers" $containers "Networks" $CurrentContainer.Networks "Debug" $debug_all) }}
     {{- end }}
 
     {{- $default_host := or ($.Env.DEFAULT_HOST) "" }}
@@ -440,14 +498,17 @@ server {
              * falling back to "external".
              */}}
         {{- $network_tag := or (first (groupByKeys $containers "Env.NETWORK_ACCESS")) "external" }}
-        {{- $upstream := $upstream_name }}
+
+        {{- $args := dict "containers" $containers "host" $host "style" $upstream_name_style }}
+        {{- if gt $nPaths 0 }}{{- $_ := set $args "path" $path }}{{- end }}
+        {{- template "upstream_name" $args }}
+        {{- $upstream_name := $args.upstream_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 / {

diff --git a/test/test_upstream-name/test_container-name.py b/test/test_upstream-name/test_container-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
diff --git a/test/test_upstream-name/test_container-name.yml b/test/test_upstream-name/test_container-name.yml
@@ -0,0 +1,42 @@
+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
+    environment:
+      UPSTREAM_NAME_STYLE: container-name