Path based routing (VIRTUAL_PATH) needs tests and feedbacks. #1733

buchdag · 2021-08-17T09:38:24Z

buchdag
Aug 17, 2021
Maintainer

The long requested path based routing feature is now available for tests in the ninxproxy/nginx-proxy:dev and ninxproxy/nginx-proxy:dev-alpine Docker images.

The documentation forr the feature is available on the dev branch.

Tests and remarks are welcome and required before we can consider this feature ready to be merged to main.

This feature is a combination of #599 #1011 #1083 #1863 (in #1607), plus #1699 for some additions.

AlexanderLieret · 2021-08-17T10:38:31Z

AlexanderLieret
Aug 17, 2021

I have written two test cases. https://github.com/AlexanderLieret/nginx-proxy/tree/dev_more_tests

Test the SSL backend.
Testcase for ef9cd4c

0 replies

junderw · 2021-08-17T12:01:56Z

junderw
Aug 17, 2021

There should be a test for 16e0eed by making sure that:

default_location is used when alone
VHOST_location is used when both exist
VHOST_PATHSHA1_location is used when all 3 exist.

I don't think combinations of 1 3 and 2 3 pairs, or 2,3 alone need to be tested.

1 reply

AlexanderLieret Aug 18, 2021

The new test case in ce20ff2 should cover this.

OliverRhyme · 2021-08-18T09:25:41Z

OliverRhyme
Aug 18, 2021

http to https redirect does not work properly. Calling post request to http endpoint will instead call get method on the reverse proxy

See logs

nginx-proxy          | nginx.1     | <REDACTED> - - [18/Aug/2021:09:23:31  0000] "POST /api/v1/register HTTP/1.1" 301 169 "-" "PostmanRuntime/7.28.2" "-"
asi-server           | 2021-08-18 09:23:32.081 [DefaultDispatcher-worker-1] TRACE ktor.application - 404 Not Found: GET - /register
nginx-proxy          | nginx.1     | <REDACTED> - - [18/Aug/2021:09:23:32  0000] "GET /api/v1/register HTTP/1.1" 404 0 "http://<REDACTED>/api/v1/register" "PostmanRuntime/7.28.2" "172.19.0.5:8080"

But calling https directly works just fine

asi-server           | 2021-08-18 09:27:17.602 [DefaultDispatcher-worker-3] TRACE ktor.application - 400 Bad Request: POST - /register
nginx-proxy          | nginx.1     | <REDACTED> - - [18/Aug/2021:09:27:17  0000] "POST /api/v1/register HTTP/1.1" 400 62 "-" "PostmanRuntime/7.28.2" "<REDACTED>"

My docker-compose env variables is

- VIRTUAL_HOST=REDACTED
- VIRTUAL_PORT=8080
- VIRTUAL_PATH=/api/v1/
- VIRTUAL_DEST=/

20 replies

junderw Aug 18, 2021

It looks like all browsers support 308...

Maybe we should just use 308 and drop 301 altogether?

junderw Aug 18, 2021

IE 11, of course, does not support it however. (and is still used in some internal networks in corporate-land)
kubernetes/ingress-nginx#1825

junderw Aug 18, 2021

Google Drive uses a non-standard 308, "Resume Incomplete"...

Yeah, maybe the solution is: "don't use http to POST stuff"

(Edit: ... maybe the internet was a big mistake... existential crisis)

OliverRhyme Aug 18, 2021

Google Drive uses a non-standard 308, "Resume Incomplete"...

Yeah, maybe the solution is: "don't use http to POST stuff"

(Edit: ... maybe the internet was a big mistake... existential crisis)

what do you think we should do or workarounds?

buchdag Aug 18, 2021
Maintainer Author

@junderw @OliverRhyme maybe we should keep the discussion about 301 vs 308 on #1737 ? 😅

warren-ru · 2021-08-19T04:34:03Z

warren-ru
Aug 19, 2021

I use all the functionality (two VIRTUAL_PATH services, one of them use VIRTUAL_DEST, a "default" file for configuring nginx location) in the project I mentioned in #1699
Everything is working properly, HTTP to HTTPS redirection either.

0 replies

AlexanderLieret · 2021-08-24T12:41:34Z

AlexanderLieret
Aug 24, 2021

I found another bug.The function NETWORK_ACCESS=internal is applied to the server block instead of the location block.
This sets this behavior for all locations.

0 replies

Rolly992 · 2021-09-15T10:19:22Z

Rolly992
Sep 15, 2021

Btw, are multiple paths supported?
For example

VIRTUAL_PATH=/api/v1/,/assets

UPD: From the code I see, it is

0 replies

JMLX42 · 2021-09-21T11:38:22Z

JMLX42
Sep 21, 2021

I think I have a bug regarding VIRTUAL_PROTO: I have two services, one with VIRTUAL_PROTO=https and another with VIRTUAL_PROTO=http.

services:
    gateway:
        image: nginxproxy/nginx-proxy:dev
        ports:
            - "80:80"
            - "443:443"
        volumes:
            - /var/run/docker.sock:/tmp/docker.sock:ro
            - ../nginx-proxy/certs:/etc/nginx/certs
        networks:
            - reverse-proxy
    doc:
        image: smartshape-doc:10.7.0
        environment:
            - 'VIRTUAL_HOST=smartshape.io.test'
            - 'VIRTUAL_PATH=/documentation/'
            - 'VIRTUAL_PORT=80'
            - 'VIRTUAL_PROTO=http'
        networks:
            - reverse-proxy
    app:
        image: smartshape-app:next
        environment:
            - 'VIRTUAL_HOST=smartshape.io.test'
            - 'VIRTUAL_PATH=/app/'
            - 'VIRTUAL_PORT=80'
            - 'VIRTUAL_PROTO=http'
        networks:
            - reverse-proxy
    server:
        image: smartshape-server:next
        environment:
            - 'VIRTUAL_HOST=smartshape.io.test'
            - 'VIRTUAL_PATH=~^/(account|annotation|file|scene|streaming|update|webhook|live)/'
            - 'VIRTUAL_PORT=443'
            - 'VIRTUAL_PROTO=https'

For some reason, nginx always uses http and I get this error in my Web browser:

Bad Request

Your browser sent a request that this server could not understand.
Reason: You're speaking plain HTTP to an SSL-enabled server port.
Instead use the HTTPS scheme to access this URL, please.
Apache/2.4.41 (Ubuntu) Server at localhost Port 80

If I start only the gateway and server services though, the requests goes through.
We will remove the HTTPS from the server service backend, but IMHO that it still a bug.

8 replies

JMLX42 Sep 21, 2021

Confirmed, identified and fixed the bug. 1bcc556

Yay! I can also confirm that speaking HTTP for all services (I removed the HTTPS from the server service) worked as intended.

You need to build the image yourself

Will do and report back.

JMLX42 Sep 21, 2021

You need to build the image yourself

I just did.

Fix confirmed: VIRTUAL_PROTO=https is now honored even when other services are using VIRTUAL_PROTO=http.

JMLX42 Sep 27, 2021

@AlexanderLieret isn't there supposed to be a nightly build/push to the dev tag on Docker Hub?

JMLX42 Sep 27, 2021

Hmmm it's weird: GitHub says 1bcc556 is not on any branch... any reason why it is not on dev?

AlexanderLieret Sep 27, 2021

@JMLX42 1bcc556 is part of my recent pull request #1747.

buchdag · 2021-10-09T19:15:44Z

buchdag
Oct 9, 2021
Maintainer Author

#1747 merged, should I rebase dev on top of main ?

0 replies

buchdag · 2021-10-18T14:17:49Z

buchdag Oct 18, 2021
Maintainer Author

I'm not certain this is related to this feature, are your experiencing the same issue with the main branch ?

acme-companion does need write access to /etc/nginx/vhost.d/default, there is currently no way around it.

samyak-jain · 2021-10-24T12:06:39Z

samyak-jain Oct 24, 2021

I was able to make it work by just creating an empty default file on my own and mount it.

tristan-zellner · 2022-01-03T16:10:54Z

tristan-zellner
Jan 3, 2022

I tried the multiple paths approach and noticed the following difference in behavior compared to single path:

Single Path behavior:

VIRTUAL_PATH=/admin/
results in the following location in the nginx config

location /admin/ {
		proxy_pass http://6eb91dde2913014bb43a33e5936e112296e55a12-07354ca2969df78aa67794847e5e59bed611673a;
}

https://example.com/admin/* and https://example.com/admin are both caught by the location

Multiple Path behavior:

VIRTUAL_PATH=~^/(admin|assets)/
results in the following location in the nginx config

location ~^/(admin|assets)/ {
		proxy_pass http://6eb91dde2913014bb43a33e5936e112296e55a12-07354ca2969df78aa67794847e5e59bed611673a;
}

https://example.com/admin/* is caught by the location
https://example.com/admin is not caught by the location

I know that this is not a problem with this feature, but an inconsistent behavior of the nginx config. I think it is worth to add it to the documentation here, to save other people the time that I have spent. What do you think?

My workaround is VIRTUAL_PATH=~^/(admin/?|assets/). This leads to working https://example.com/admin/* and https://example.com/admin paths.

2 replies

JMLX42 Jan 29, 2022

I know that this is not a problem with this feature, but an inconsistent behavior of the nginx config. I think it is worth to add it to the documentation here, to save other people the time that I have spent. What do you think?

The ^/(admin|assets)/ regex does not match /admin so there is no inconsistency here.

My workaround is VIRTUAL_PATH=~^/(admin/?|assets/).

IMHO that is not a "workaround". It is how it is supposed to be to match /admin/).

So AFAIK this is not related to the feature at hand here.

tristan-zellner Jan 29, 2022

location /admin/ works for https://example.com/admin/* and https://example.com/admin

location ^/(admin|assets)/ matches /admin/ (just like in the non-regex way) and in this case https://example.com/admin does not work

Doesn't feel like a consistent behavior to me.

And yes, like I wrote in the initial post, it is not really related to the feature. But at least related to the use of this feature when using multiple paths

rriemann · 2022-02-05T00:49:35Z

rriemann
Feb 5, 2022

#1747 merged, should I rebase dev on top of main ?

Hi @buchdag , what stops nginx-proxy from merging?

2 replies

AlexanderLieret Feb 10, 2022

@buchdag I squashed insignificant commits and rebased dev on my working copy

buchdag Feb 14, 2022
Maintainer Author

@rriemann I guess I still feel that not a lot of people have tested and reported on this feature despite seemingly high demand, but that's probably me. I'll take a look at @AlexanderLieret copy ASAP and go forward with this.

buchdag · 2022-02-16T15:58:52Z

buchdag
Feb 16, 2022
Maintainer Author

I fixed the rebase issue I encountered, I still have to do some verification to make sure we don't accidentally break some less used and untested feature but I think this feature could realistically be merged to main by the end of the month.

0 replies

buchdag · 2022-02-24T15:16:27Z

buchdag
Feb 24, 2022
Maintainer Author

The VIRTUAL_PATH feature has been merged to main.

2 replies

buchdag Feb 25, 2022
Maintainer Author

I think I'm going to release 0.10.1 just prior to this feature then release 1.0.0 with the VIRTUAL_PATH feature merged in just so we don't stay on 0.x.y forever. Seems like a good landmark.

junderw Feb 25, 2022

congrats!

Path based routing (VIRTUAL_PATH) needs tests and feedbacks. #1733

buchdag Aug 17, 2021 Maintainer

Replies: 13 comments · 37 replies

buchdag Aug 18, 2021 Maintainer Author

buchdag Oct 9, 2021 Maintainer Author

This comment was marked as off-topic.

buchdag Oct 18, 2021 Maintainer Author

buchdag Feb 14, 2022 Maintainer Author

buchdag Feb 16, 2022 Maintainer Author

buchdag Feb 24, 2022 Maintainer Author

buchdag Feb 25, 2022 Maintainer Author

buchdag
Aug 17, 2021
Maintainer

Replies: 13 comments 37 replies

buchdag Aug 18, 2021
Maintainer Author

buchdag
Oct 9, 2021
Maintainer Author

buchdag Oct 18, 2021
Maintainer Author

buchdag Feb 14, 2022
Maintainer Author

buchdag
Feb 16, 2022
Maintainer Author

buchdag
Feb 24, 2022
Maintainer Author

buchdag Feb 25, 2022
Maintainer Author