feat: Use container names for upstream names to avoid repeats

nginx-proxy · Jan 29, 2023 · cddce7f · cddce7f
1 parent d6d5389
commit cddce7f
Show file tree

Hide file tree

Showing 5 changed files with 225 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -494,11 +494,73 @@ 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`
+
+> **Warning**
+> This feature is experimental.  The behavior may change (or the feature may be removed) without warning in a future release, even if the release is not a new major version.  If you use this feature, please provide feedback so we know whether it is working properly and can be promoted to officially supported.
+
+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 sorting the names of the applicable Docker containers, appending each one with a tilde character (`~`), and concatenating the results. 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
@@ -12,12 +12,89 @@
 {{- $_ := 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")) }}
 {{- $_ := set $globals "default_root_response" (coalesce $globals.Env.DEFAULT_ROOT "404") }}
 {{- $_ := set $globals "trust_downstream_proxy" (parseBool (coalesce $globals.Env.TRUST_DOWNSTREAM_PROXY "true")) }}
 {{- $_ := 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 "upstream_name_style" (coalesce $globals.Env.UPSTREAM_NAME_STYLE (when (parseBool (coalesce $globals.Env.SHA1_UPSTREAM_NAME "false")) "virtual-host-sha1" "virtual-host")) }}
+{{- $_ := set $globals "upstream_names" (dict) }}
+
+{{- /*
+     * 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.
+             *
+             * The container names are sorted to avoid redundant upstreams.
+             *
+             * A tilde ("~") character is inserted after each container name to
+             * avoid name collisions.  Apparently nginx supports a wide range of
+             * characters in an upstream name (see
+             * <https://stackoverflow.com/q/36485834>), but as far as I can
+             * tell, the exact set of allowed characters is not officially
+             * specified in any of nginx's documentation.  To be safe, the tilde
+             * character is used because it is in the URI unreserved character
+             * set (percent-encoding is never 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 another upstream name.  Futhermore, the name
+             * is unlikely to collide with a DNS hostname.  (Characters in the
+             * <sub-delims> character set of RFC 3986 (see
+             * <https://www.rfc-editor.org/rfc/rfc3986.html#section-2.2>) would
+             * probably also work without any problems.  Those characters are
+             * permitted in the <userinfo> and <host> parts of a URL without
+             * percent-encoding, and they are not permitted to be in Docker
+             * container names.)
+             *
+             * nginx-proxy currently does not support more than one upstream
+             * server per container, so the set of container names are
+             * sufficient to completely identify an upstream.  If support for
+             * multiple proxied servers in a single container is added,
+             * additional disambiguating information such as protocol name and
+             * port number will need to be included in the generated upstream
+             * name.  (To remain backwards compatible with the current scheme,
+             * the additional information must not be included if nginx-proxy
+             * is only proxying one of the container's servers.)
+             */}}
+        {{- $name = printf "%s~" (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" }}
@@ -266,14 +343,11 @@ server {
 }
 
 {{- range $host, $containers := groupByMulti $globals.containers "Env.VIRTUAL_HOST" "," }}
-
     {{- $host := trim $host }}
     {{- if not $host }}
         {{- /* Ignore containers with VIRTUAL_HOST set to the empty string. */}}
         {{- continue }}
     {{- end }}
-    {{- $is_regexp := hasPrefix "~" $host }}
-    {{- $upstream_name := when (or $is_regexp $globals.sha1_upstream_name) (sha1 $host) $host }}
 
     {{- $paths := groupBy $containers "Env.VIRTUAL_PATH" }}
     {{- $nPaths := len $paths }}
@@ -282,13 +356,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" $globals.upstream_name_style }}
+        {{- if gt $nPaths 0 }}{{- $_ := set $args "path" $path }}{{- end }}
+        {{- template "upstream_name" $args }}
+        {{- $upstream_name := $args.upstream_name }}
+        {{- if not (index $globals.upstream_names $upstream_name) }}
+            {{- template "upstream" (dict "Upstream" $upstream_name "Containers" $containers "Networks" $globals.CurrentContainer.Networks) }}
+            {{- $_ := set $globals.upstream_names $upstream_name true }}
         {{- end }}
-# {{ $host }}{{ $path }}
-{{ template "upstream" (dict "Upstream" $upstream "Containers" $containers "Networks" $globals.CurrentContainer.Networks) }}
     {{- end }}
 
     {{- $default_host := or ($globals.Env.DEFAULT_HOST) "" }}
@@ -440,14 +515,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" $globals.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 and (not (contains $paths "/")) (ne $globals.default_root_response "none")}}
     location / {

diff --git a/test/test_upstream-name/test_container-name.py b/test/test_upstream-name/test_container-name.py
@@ -0,0 +1,21 @@
+import pytest
+import re
+
+
+def test_single_container_in_upstream(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode()
+    assert re.search(r"upstream web1~ \{", conf)
+
+def test_multiple_containers_in_upstream(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode()
+    assert re.search(r"upstream web2~web3~ \{", conf)
+
+def test_no_redundant_upstreams(docker_compose, nginxproxy):
+    conf = nginxproxy.get_conf().decode()
+    assert len(re.findall(r"upstream web4~ \{", conf)) == 1
+
+def test_valid_upstream_name(docker_compose, nginxproxy):
+    """nginx should not choke on the tilde character."""
+    r = nginxproxy.get("http://web1.nginx-proxy.test/port")
+    assert r.status_code == 200
+    assert r.text == "answer from port 80\n"
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.test
+  web2:
+    container_name: web2
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web23.nginx-proxy.test
+  web3:
+    container_name: web3
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web23.nginx-proxy.test
+  web4:
+    container_name: web4
+    image: web
+    expose:
+      - "80"
+    environment:
+      WEB_PORTS: 80
+      VIRTUAL_HOST: web4a.nginx-proxy.test,web4b.nginx-proxy.test
+
+  sut:
+    image: nginxproxy/nginx-proxy:test
+    volumes:
+      - /var/run/docker.sock:/tmp/docker.sock:ro
+    environment:
+      UPSTREAM_NAME_STYLE: container-name