Support for path-based routing

[buchdag]

This is a work in progress combination of #599 + #1011 + #1083 + #1594 with proper authoring credits.

• VIRTUAL_PATH: route using this path
• VIRTUAL_DEST: rewrite the query path (optional)
• Support for custom config snippets files
• Add test cases

NOTE: This requires another pull request to function nginx-proxy/docker-gen#343 to function.

[buchdag]

@AlexanderLieret okay this looks promising, we've got only two failing tests. I've reset my local branch to your last commit and fixed an error I did on a merge conflict resolution.

Next step is having nginx-proxy/docker-gen#343 merged then taking a look at the two failing tests.

Given the radical changes this PR would make to nginx.tmpl, merging it will probably cause non trivial merge conflict on most if not all other feature PR. I have to think a bit about the best strategy for this, for now I came up with:

1. doing triage on the PR and merging as much low hanging fruits as possible before merging this one to main.
2. merging this PR to a new edge branch and release an edge version of the container, then merge it to main later, after it's been sufficiently field tested.

I'm leaning more toward 2.

[AlexanderLieret]

@buchdag I had a look at those failed tests. The first was the recent change of the CI test pipeline.
The other does need a bit more troubleshooting.

Option 2 will push the merge conflicts to a later point. Being able to field test should be an advantage.

[buchdag]

So the sole failing test is:

=================================== FAILURES ===================================
_ test_non_matching_host_is_503[web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond] _

docker_compose = <docker.client.DockerClient object at 0x7f8c5a603130>
nginxproxy = <conftest.requests_for_docker object at 0x7f8c5a4399d0>
host = 'web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond'

    @pytest.mark.parametrize("host", [
        "unexpected.nginx-proxy.tld",
        "web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond"
    ])
    def test_non_matching_host_is_503(docker_compose, nginxproxy, host):
        r = nginxproxy.get(f"http://{host}/port")
>       assert r.status_code == 503, r.text
E       AssertionError: answer from port 84
E         
E       assert 200 == 503
E         +200
E         -503

test_wildcard_host.py:32: AssertionError
----------------------------- Captured stderr call -----------------------------
DEBUG:DNS:resolving domain name ('web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond', 80, <AddressFamily.AF_UNSPEC: 0>, <SocketKind.SOCK_STREAM: 1>)
DEBUG:DNS:nginx_proxy_dns_resolver('web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond')
INFO:DNS:resolving domain name 'web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond' as IP address 172.17.0.2 of nginx-proxy container test_sut_1
------------------------------ Captured log call -------------------------------
DEBUG    DNS:conftest.py:218 resolving domain name ('web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond', 80, <AddressFamily.AF_UNSPEC: 0>, <SocketKind.SOCK_STREAM: 1>)
DEBUG    DNS:conftest.py:169 nginx_proxy_dns_resolver('web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond')
INFO     DNS:conftest.py:177 resolving domain name 'web4.whatever.nginx-proxy.regexp-to-infinity-and-beyond' as IP address 172.17.0.2 of nginx-proxy container test_sut_1
--------------------------- Captured stderr teardown ---------------------------
INFO:root:docker-compose -f /home/runner/work/nginx-proxy/nginx-proxy/test/test_wildcard_host.yml down
---------------------------- Captured log teardown -----------------------------
INFO     root:conftest.py:274 docker-compose -f /home/runner/work/nginx-proxy/nginx-proxy/test/test_wildcard_host.yml down

Maybe tied to 343cdd0 ?

[buchdag]

Also I'm still not sure how I'll handle the final commit history that'll get merged.

The current one, while keeping proper authoring credit for everyone's specific commits, is super messy and forces to merge the target branch back into the source branch. I'd like to avoid that if at all possible.

I'm thinking about the following:

• a single commit for nginx.tmpl
• a single commit for README.md
• a single commit for 343cdd0
• a single commit for the tests you added @AlexanderLieret

Each commit being authored by all the relevant contributors.

[AlexanderLieret]

So the sole failing test is:
Maybe tied to 343cdd0 ?

This commit does break the test which is now failing. Its purpose is to test forced full regex matches instead of partial matches. The commit message does mention #124 which is superseded. This commit should be removed.

Also I'm still not sure how I'll handle the final commit history that'll get merged.
Each commit being authored by all the relevant contributors.

The main work was done by @gregsymons. The others and I rebased it on the current main branch and added some fixes.

I added some QoL improvements to the "new" feature. Most of them were from the individual PRs and #254.

[buchdag]

I've cleaned up the commit history to get something a bit more legible and removed the code change that made the test fail.

I haven't added back the support for custom location files per-VIRTUAL_HOST VIRTUAL_PATH yet.

I noticed two things in the README history of this feature:

If you wish to have a container serve the root while other containers serve other paths, make give the root container a VIRTUAL_PATH of /. Unmatched paths will be served by the container at / or will return the default nginx error page if no container has been assigned /.

The part in bold does not work.

When you want to forward many paths to the same container you can set the variable using a regular expression. For example VIRTUAL_PATH=~^/(path1|path2) will answer to requests that start with /path1 or /path2.

This doesn't work either.

Both might have been working at the time their respective documentations were written then broken by subsequent commits. The first was directly tested in @gregsymons original tests.

[AlexanderLieret]

I will have another look at both problems. It seems like some features have been removed while rebasing on the current main.

While improving the test cases I should move all virtual_path tests into a single directory.

[buchdag]

@AlexanderLieret let me know if you want me to merge this to the dev branch and push the image to Dockerhub under the dev tag.

I'm not sold on the fact that the custom location files per-VIRTUAL_HOST VIRTUAL_PATH needs a sha1 of the virtual path in their name, but I don't know if there is another way to do it.

[AlexanderLieret]

@buchdag I have finished rebasing @gregsymons original commit to see if other stuff had been changed in the rebase chain. The new attempt is in a new branch. An updated documentation is needed.

I have found a bug which prevents dockergen from updating the generated config file when another container is added to or removed from a domain. This could explain your failed attempts at manual testing. The currently failing test case is caused by this bug.

When you want to forward many paths to the same container you can set the variable using a regular expression. For example VIRTUAL_PATH=~^/(path1|path2) will answer to requests that start with /path1 or /path2.

This sentence was added by @rodrigoaguilera and only works if the routable-path is not to be overwritten by the proxypass (VIRTUAL_DEST=""). It is possible to overwrite the matched path with a regex. This is also one test case.

I'm not sold on the fact that the custom location files per-VIRTUAL_HOST VIRTUAL_PATH needs a sha1 of the virtual path in their name, but I don't know if there is another way to do it.

The hash is used to sanitize the VIRTUAL_PATH to be used as part of the file name. This is similar to how this works with regex domains.

A suitable default value for VIRTUAL_DEST is a tough decision and will need to be covered in the upcoming documentation. In short this depends on the handling of subpaths by the proxied application.

[buchdag]

@KunalSolanke just to be sure: are you using the specific code from this PR ? It's based on but not the same as #599

[KunalSolanke]

@buchdag yeah ,initially I landed on that PR and tried that,but later found this one is the latest one so I tried this code from this one.Sorry for that.I later realized that the way I wanted this to work wouldn't be helpful rather the default behaviour is better .So you can safely ignore previous comment.

[KunalSolanke]

Sorry for bothering again but I am stuck with another things again .This may require a bit of understanding of django.
I am trying VIRTUAL_PATH=/test,VIRTUAL_PROTO=http

These are my django urls configuration.So the thing is

Now the issue:
When i navigate to "/test/swagger/" with ending slash it works fine but when I do "/test/swagger"(without end slash) nginx redirects to "/swagger/",request doesn't even reach container.It like it is not even recognizing the url.
This is my issue .But now the weird part,

when I do /test/debug(without end slash) nginx somehow redirects to /test/debug/ and I am sure this redirect is done by nginx and not by django.
No idea how that is happening.

A basic sol for my problem would be to redirect everything from nginx to /uri to /uri/ ,but I have no idea how to do it here.

This is autogenrated nginx.conf file
``nginx

# localhost/test
upstream localhost-upstream-f133a4599372cf531bcdbfeb1116b9afe8d09b4f {
					## Can be connected with "nginx-proxy" network
		# evy_web_1
		server 172.19.0.3:8000;
}
    server {
        listen 80 ;
            server_name localhost;
        access_log /var/log/nginx/access.log vhost;
        include /etc/nginx/vhost.d/default;
                location /test {
	                    proxy_pass http://localhost-upstream-f133a4599372cf531bcdbfeb1116b9afe8d09b4f;
                }
            location / {
	            return 404;
            }
    }

``
again sorry for bothering here,but as this isn't merged yet I can't ask it asnywhere else.

[AlexanderLieret]

@KunalSolanke all of these redirect are done by your django application.

With VIRTUAL_PATH=/test all request which begin with /test are proxied to your application. Nginx does not do any redirects in this situation. You need to test your application first that it is able to work on the subpath first.

The only part I did see where Nginx propably was redirecting was something like VIRTUAL_PATH=/foo/ VIRTUAL_DEST=/ and then only redirecting /foo to /foo/. It should only add a slash when the resulting proxy request has a slash in the end.

[KunalSolanke]

@AlexanderLieret The reason why I said I am sure nginx is doing those redirects so requests don't even reach the target container in those cases. Ok now I know this might be naive observation, so there is middleware called commonMiddleware in django which handles these redirects, I just copied that middleware and made my own with debug statements and yeah the requests didn't reach there if I don't append slash.It might be so that my django application is indeed redirecting,I will try to debug more in my django app. Thanks for your reply😥😥.

[AlexanderLieret]

@buchdag I have finished redoing the commits. The new test cases are covering more parts of the code. Furthermore the documentation should be easier to read and contain more information. This is still in the new branch.

The failing test case covers one previously not covered scenario. After removing one container from the configuration file is generated but the reload is not triggered by docker-gen. This only happens if at least one path for this subdomain is remaining. A workaround is to manually restart the nginx container or triggering the reload.
I did forget a sleep to wait for docker-gen to fninsh

[AlexanderLieret]

@buchdag I did split the commits into smaller ones. I am not sure what went wrong in your attempt.

My branch virtual-path has the original code rebased/merged with the current main and split into 3 commits (implementation, tests and documentation). My other branch virtual-path-additions contains my additions in separate commits.

[buchdag]

I am not sure what went wrong in your attempt.

@AlexanderLieret me neither, I'll try to clean this up based on your branch tomorrow.

[buchdag]

@AlexanderLieret PR merged to dev, I'll start to work on publishing the dev image to DockerHub immediately.

Thanks for the excellent work.

[buchdag]

Further tests and remarks / discussions in #1733