Allow complete override of location blocks

[harvdogg]

Under the existing implementation, it would be impossible to change the actual behavior of the default location block, which attempts to proxy_pass onto the container. This is probably acceptable for many use cases but not all.

This change adds some additional template logic that considers the existence of a VIRTUAL_HOST_locations (plural "locations" as opposed to the otherwise considered "location") configuration file. When present, it will bypass generating any nginx location blocks and instead use those provided by the configuration file.

This allows for some really advanced use cases with nginx's location filtering. You can then forward the root location of a monitored domain onto a completely different container or even an entirely external host while still introducing logic that allows certain locations to forward to the container like normal.

The README has been updated to reflect this.

Fixes #1385
Fixes #1286
Fixes #1086
Fixes #1003
Fixes #567

[RazaGR]

This is a must-have, please merge @jwilder

[liudonghua123]

This is a must-have, please merge @jwilder

[liudonghua123]

I like this feature. 😃

[DominMuda]

It would be awesome if we could have this feature in master 🙏 @jwilder

[sl45sms]

+1

[thg303]

any idea why this is not merged yet?? :(

[roysG]

any update?

[gymnae]

Please merge this

[ngoonee]

This would solve the issue I raise in #1385. Testing on my own docker build here works fine (the resulting conf has some indentation issues though).

[cmargroff]

Whats the hold up here?
I really need this feature to be able to replace the default location / {} block.

[r-t-k]

Why hasn't this been merged?

[banagale]

Thanks for this PR. I suspect the hold up is that this just takes time to deal with. :)

That said, I could also use this PR. Django complains bitterly about [Django] ERROR (EXTERNAL IP): Invalid HTTP_HOST if you do not specifically disallow requests from domains outside the ALLOWED_HOST configuration.

The current build of this repo sends requests for the server IP to the first server block which results in 503's bubbling up to admin emails / sentry etc. This is a real problem on a VPS host where IP ranges are constantly probed.

This is a great container. @jwilder, when time allows please consider merging this.

[shoff]

Why hasn't this been merged?

Indeed why has this not been merged?

[buchdag]

@rhansen I'm considering merging this if we can test it.

I'm just not entirely sold on the VIRTUAL_HOST_locations vs existing VIRTUAL_HOST_location file name as I think it lacks explicitness. Maybe VIRTUAL_HOST_root_location, VIRTUAL_HOST_custom_location or VIRTUAL_HOST_bypass_location?

[rhansen]

Thinking about this more, users of this feature are unlikely to use VIRTUAL_PATH, right? So most of the time this would only override a single location block. So maybe pluralized isn't the way to go. How about *_location_override?

Users can already inject server-scoped config via /etc/nginx/vhost.d/VIRTUAL_HOST or /etc/nginx/vhost.d/default, so really all we need is a boolean that disables the location blocks. Right?

If the user does use VIRTUAL_PATH, then the override/boolean would be overriding/disabling multiple location blocks, which might be surprising. We could error out in that case. If requested, we could add per-path overrides, like this:

diff --git a/nginx.tmpl b/nginx.tmpl
index e7d77c9..e994712 100644
--- a/nginx.tmpl
+++ b/nginx.tmpl
@@ -52,6 +52,10 @@
 {{ end }}
 
 {{ define "location" }}
+    {{- $override := printf "/etc/nginx/vhost.d/%s_%s_location_override" .Host (sha1 .Path) }}
+    {{- if (exists $override) }}
+        include {{ $override }}
+    {{- else }}
 	location {{ .Path }} {
 	{{ if eq .NetworkTag "internal" }}
 		# Only allow traffic from internal clients
@@ -84,6 +88,7 @@
 		include /etc/nginx/vhost.d/default_location;
 	{{ end }}
 }
+    {{- end }}
 {{ end }}
 
 {{ define "upstream" }}

[rhansen]

I rebased onto latest main and cleaned up some newlines in README.md. No other changes were made.

[rhansen]

I pushed a couple of fixes (typo fix, move documentation) as well as a commit that changes the approach to be per-location instead of per-host. Please let me know what you think; I'll add tests once we reach consensus.

[buchdag]

@rhansen I've given that a bit more thought, do you think an environment variable like VIRTUAL_ROOT_LOCATION_BYPASS or VIRTUAL_ROOT_LOCATION_DISABLE would be better ? What we actually need here is a way to disable the default root location, the existing custom location mechanism can handle the root location once the default one has been disabled.

[rhansen]

What we actually need here is a way to disable the default root location

Is it? The problem we are trying to solve here is not clear to me. The VIRTUAL_PATH feature did not exist when this PR was opened, so maybe the original problem has been largely mitigated and we are trying to solve something else?

If we only want to disable the default root location, then I propose extending the existing DEFAULT_ROOT environment variable instead:

diff --git a/README.md b/README.md
index 0ca08d7..a656637 100644
--- a/README.md
+++ b/README.md
@@ -152,10 +152,17 @@ The filename of the previous example would be `example.tld_8610f6c344b4096614eab
 
 This environment variable of the nginx proxy container can be used to customize the return error page if no matching path is found. Furthermore it is possible to use anything which is compatible with the `return` statement of nginx.
 
-For example `DEFAULT_ROOT=418` will return a 418 error page instead of the normal 404 one.
-Another example is `DEFAULT_ROOT="301 https://github.com/nginx-proxy/nginx-proxy/blob/main/README.md"` which would redirect an invalid request to this documentation.
-Nginx variables such as $scheme, $host, and $request_uri can be used. However, care must be taken to make sure the $ signs are escaped properly.
-If you want to use `301 $scheme://$host/myapp1$request_uri` you should use:
+Exception:  If this is set to the string `none`, no default `location /` directive will be generated.  This makes it possible for you to provide your own `location /` directive in your [`/etc/nginx/vhost.d/VIRTUAL_HOST`](#per-virtual_host) or [`/etc/nginx/vhost.d/default`](#per-virtual_host-default-configuration) files.
+
+If unspecified, `DEFAULT_ROOT` defaults to `404`.
+
+Examples (YAML syntax):
+
+  * `DEFAULT_ROOT: "none"` prevents `nginx-proxy` from generating a default `location /` directive.
+  * `DEFAULT_ROOT: "418"` returns a 418 error page instead of the normal 404 one.
+  * `DEFAULT_ROOT: "301 https://github.com/nginx-proxy/nginx-proxy/blob/main/README.md"` redirects the client to this documentation.
+
+Nginx variables such as `$scheme`, `$host`, and `$request_uri` can be used.  However, care must be taken to make sure the `$` signs are escaped properly.  For example, if you want to use `301 $scheme://$host/myapp1$request_uri` you should use:
 
 * Bash: `DEFAULT_ROOT='301 $scheme://$host/myapp1$request_uri'`
 * Docker Compose yaml: `- DEFAULT_ROOT: 301 $$scheme://$$host/myapp1$$request_uri`
diff --git a/nginx.tmpl b/nginx.tmpl
index fb4f76f..64a3edb 100644
--- a/nginx.tmpl
+++ b/nginx.tmpl
@@ -453,7 +453,7 @@ server {
         {{- end }}
         {{- template "location" (dict "Path" $path "Proto" $proto "Upstream" $upstream "Host" $host "VhostRoot" $vhost_root "Dest" $dest "NetworkTag" $network_tag) }}
     {{- end }}
-    {{- if (not (contains $paths "/")) }}
+    {{- if and (not (contains $paths "/")) (ne $globals.default_root_response "none")}}
     location / {
         return {{ $globals.default_root_response }};
     }

[buchdag]

The problem we are trying to solve here is not clear to me.

From what I understood of the various issues mentioned, the problem is that nginx-proxy generate a location / { [...] } block no matter what you do with the per-location config files. Some setup need to entirely bypass this default root location and provide a fully custom one but that's not possible at the moment because the default always generated one will always bypass whatever the user provide. IMHO the problem should be considered the same wether you use VIRTUAL_PATH without a container handling the root path or not.

If we only want to disable the default root location, then I propose extending the existing DEFAULT_ROOT environment variable instead:

I really like this idea, using DEFAULT_ROOT=none to achieve this seems the best way to implement this.

[rhansen]

From what I understood of the various issues mentioned, the problem is that nginx-proxy generate a location / { [...] } block no matter what you do with the per-location config files. Some setup need to entirely bypass this default root location and provide a fully custom one but that's not possible at the moment because the default always generated one will always bypass whatever the user provide. IMHO the problem should be considered the same whether you use VIRTUAL_PATH without a container handling the root path or not.

I see a mix. Some users seem to want to be able to disable the default root and some want to tweak the location /foo block generated by VIRTUAL_PATH=/foo in a way that is not currently supported by the *_location include file. Specifically:

#1554

I believe this can be fixed by setting VIRTUAL_PATH: '~ \.php$$' (Docker compose YAML syntax) and disabling the default root block, so the DEFAULT_ROOT=none proposal would be an adequate solution here.

#1385

For this case, the DEFAULT_ROOT=none proposal would not be sufficient because the implicit VIRTUAL_PATH=/ suppresses the default block anyway.

I guess this user could do the following:

1. Set VIRTUAL_PATH: '~ ^\b$' (where ^\b$ is a regular expression that will never match anything) to move the proxy_pass from location / to a pointless location block. This prevents the generation of a location / block for the proxy_pass, which would trigger the creation of the default root. (A regular expression that never matches ensures that users can't craft special URLs to match the location block and thus bypass the custom location / config.)
2. Disable the default root by setting DEFAULT_ROOT=none.
3. Define a suitable location / block in their vhost-specific include file.

However, that's a very kludgy solution.

#1286

The OP doesn't say whether the *_location include file meets their needs, so it's unclear whether any change is necessary at all. However, a follow-up comment says, "I only want to change the proxy_pass value in location /", which implies that they have (implicit) VIRTUAL_PATH=/. Thus, the DEFAULT_ROOT=none approach would not work for that commenter (unless they used the above kludge).

#1086
#1003

Not enough information to make a determination.

#567

The user wants to override a VIRTUAL_PATH-generated location block, so DEFAULT_ROOT does not apply here either (except for the kludge).

using DEFAULT_ROOT=none to achieve this seems the best way to implement this.

I don't think there's any reason we couldn't do both options (both per-path location overrides and a way to disable the default root). I'll open a separate PR for DEFAULT_ROOT=none, and keep this PR about per-path location overrides.

[rhansen]

I'll open a separate PR for DEFAULT_ROOT=none, and keep this PR about per-path location overrides.

Done: #2146

[buchdag]

I'll open a separate PR for DEFAULT_ROOT=none, and keep this PR about per-path location overrides.

Suits me, I approved #2146

Regarding the per-path location overrides, I'm on board with the *_location_override files that simply disables the location block on inclusion, as you suggested a bit earlier. This feels more coherent with the existing custom location mechanisms than adding yet another environment variables as I suggested last week.

[buchdag]

@rhansen looks good to me so far, with added tests this can be merged.

[rhansen]

@rhansen looks good to me so far, with added tests this can be merged.

Done.

[buchdag]

Do you think you could add a test container that does not use VIRTUAL_PATH at all ?

Even if this is functionally the same as VIRTUAL_PATH: /, I'd prefer to have a dedicated test for the baseline use of the feature (no VIRTUAL_PATH and only VIRTUAL-ROOT: root-nohash.nginx-proxy.test).

Other than that, LGTM.

[rhansen]

Do you think you could add a test container that does not use VIRTUAL_PATH at all ?

Done. Turns out we have a bug unrelated to this PR: If one container has VIRTUAL_PATH=/foo and another doesn't have VIRTUAL_PATH at all (but the same VIRTUAL_HOST), then nginx-proxy ignores the container without VIRTUAL_PATH. (Maybe this is working as intended?) I avoided this issue in the expanded test cases.

[buchdag]

That might be convenient or okay but I'm pretty sure we never explicitly made it work that way 🤔