Skip to content
This repository was archived by the owner on Nov 5, 2022. It is now read-only.

Virtual_PATH, Added trailing '/' to proxy_pass #1

Open
wants to merge 5 commits into
base: multiple-paths
Choose a base branch
from

Conversation

jswann
Copy link

@jswann jswann commented Mar 26, 2015

For me to successfully get Virtual_Path's to work, I had to add a trailing slash (/) to the proxy_pass url. Otherwise the proxy would append the path when proxying requests.

md5 and others added 5 commits February 25, 2015 22:56
* Require `dict` and `sha1` function support in docker-gen
Currently uses the same htpasswd for all services behind a hostname, but it
could be desirable to allow different htpasswd for different paths
@md5
Copy link
Member

md5 commented Mar 26, 2015

Does this work with subpaths of VIRTUAL_PATH? For example, if I put VIRTUAL_PATH=/admin and then request /admin/foo, will the backend see /foo or /?

Assuming that subpaths work correctly, I'm still not sure this is the right default behavior, since I could see this being a matter of user preference. Do you mind bringing up the issue over at nginx-proxy#47 where there can be more visibility? Only you and I are likely to ever see this PR.

@md5
Copy link
Member

md5 commented Mar 26, 2015

BTW, this branch is being occasionally rebased against the master branch of jwilder/nginx-proxy, so I may need to pull in your commit in a manual merge (or you could rebase in the event that I rebase).

@jswann
Copy link
Author

jswann commented Mar 26, 2015

i didn't try it against subpaths of VIRTUAL_PATH, i only needed to route
/admin to /. I'll test it out before I bring up the issue on nginx-proxy#47.

I placed the PR against your branch because I wasn't sure how to generate
one against jwilder/nginx-proxy as a PR.

On Thu, Mar 26, 2015 at 2:01 PM, Mike Dillon [email protected]
wrote:

BTW, this branch is being occasionally rebased against the master branch
of jwilder/nginx-proxy, so I may need to pull in your commit in a manual
merge (or you could rebase in the event that I rebase).


Reply to this email directly or view it on GitHub
#1 (comment).

@md5
Copy link
Member

md5 commented Mar 26, 2015

Placing a PR against my branch is appropriate. I just wanted to give you a heads-up since I saw that your PR contains commits from the upstream master (which makes me think I need to rebase).

@jswann
Copy link
Author

jswann commented Mar 26, 2015

Just verified that it'll work for request paths like /admin/foo -- which
will proxy to the backend server on /.

I'll update nginx-proxy#47 with the information.

Thanks.

On Thu, Mar 26, 2015 at 2:10 PM, Mike Dillon [email protected]
wrote:

Placing a PR against my branch is appropriate. I just wanted to give you a
heads-up since I saw that your PR contains commits from the upstream
master (which makes me think I need to rebase).


Reply to this email directly or view it on GitHub
#1 (comment).

@md5
Copy link
Member

md5 commented Mar 26, 2015

I'm guessing that's not the behavior most people would want. I can imagine two behaviors that I would expect users to want:

  1. Pass through the request URI unchanged
  2. Pass through the request URI with the VIRTUAL_PATH prefix stripped

I don't think that rewriting all requests from VIRTUAL_PATH to / will be a common configuration.

The other thing I wonder about is whether this has any effect on query strings.

@jswann
Copy link
Author

jswann commented Mar 26, 2015

From my research, which lead me to finding the missing slash, it appears
the only way to achieve the functionality that you're talking about is to
use rewrite rules as well. They'll keep query strings, etc. I was able to
get that working but decided against it after seeing it wasn't really a
best practice in nginx.

But, reading over the comments in the original Issue, I felt that most just
wanted to use a virtual host at times when they weren't able to have the
DNS configured with *.hostname. That's what i'm dealing with in my
organization, and just need to use a servername/context_root as a way to
run multiple services on port 80 on the same docker host.

On Thu, Mar 26, 2015 at 4:10 PM, Mike Dillon [email protected]
wrote:

I'm guessing that's not the behavior most people would want. I see two
behaviors that I would expect users to want:

  1. Pass through the request URI unchanged
  2. Pass through the request URI with the VIRTUAL_PATH prefix stripped

I don't think that rewriting all requests from VIRTUAL_PATH to / will be
a common configuration.

The other thing I wonder about is whether this has any effect on query
strings.


Reply to this email directly or view it on GitHub
#1 (comment).

@jswann
Copy link
Author

jswann commented Mar 26, 2015

also - just to clarify.

If i set VIRTUAL_PATH=/admin:
/admin will proxy to my service on /
/admin/users will proxy to my service on /users

if I set VIRTUAL_PATH=/admin/v1
/admin/v1 will proxy to my service on /
/admin/v1/users will proxy to my service on /users

On Thu, Mar 26, 2015 at 4:16 PM, Josh Swann [email protected] wrote:

From my research, which lead me to finding the missing slash, it appears
the only way to achieve the functionality that you're talking about is to
use rewrite rules as well. They'll keep query strings, etc. I was able to
get that working but decided against it after seeing it wasn't really a
best practice in nginx.

But, reading over the comments in the original Issue, I felt that most
just wanted to use a virtual host at times when they weren't able to have
the DNS configured with *.hostname. That's what i'm dealing with in my
organization, and just need to use a servername/context_root as a way to
run multiple services on port 80 on the same docker host.

On Thu, Mar 26, 2015 at 4:10 PM, Mike Dillon [email protected]
wrote:

I'm guessing that's not the behavior most people would want. I see two
behaviors that I would expect users to want:

  1. Pass through the request URI unchanged
  2. Pass through the request URI with the VIRTUAL_PATH prefix stripped

I don't think that rewriting all requests from VIRTUAL_PATH to / will be
a common configuration.

The other thing I wonder about is whether this has any effect on query
strings.


Reply to this email directly or view it on GitHub
#1 (comment).

@md5
Copy link
Member

md5 commented Mar 26, 2015

Cool. So that sounds like what I described in the second option. 👍

I'm still not sure what other users would expect the default behavior to be for VIRTUAL_PATH.

@md5 md5 force-pushed the multiple-paths branch from 6f66d0b to b3729bd Compare May 4, 2015 04:02
@WillBeebe
Copy link

I would really like to use this feature! anything I can do to help get it merged?

@jswann
Copy link
Author

jswann commented Jun 29, 2015

Not sure. I've just been using the branch in my repo. I think @md5 has been waiting on the jwilder project before rebasing and merging all changes in

@WillBeebe
Copy link

Thanks for the update @jswann !

@md5
Copy link
Member

md5 commented Jul 1, 2015

@WillBeebe Are you specifically looking for this trailing slash fix (which I'm still not convinced is a good idea as default behavior), or are you talking about getting a new PR for the multiple-paths branch?

In terms of further discussion, I'd like to see that over at nginx-proxy/docker-gen#47 (though I did just comment here too: nginx-proxy#190 (comment))

@WillBeebe
Copy link

Hi @md5 . I was mostly looking for VIRTUAL_PATH support. Here is a modified nginx.tmpl that I used to do this. Most notably I had to add $host to this line:

https://gist.github.com/WillBeebe/0186bae3ac20f6354819#file-gistfile1-txt-L160

@WillBeebe
Copy link

My goal is to have an nginx container auto-detect and route to separate rails containers. Here's my docker-compose.yml. Running docker-compose up on this gets me basically what I want. I can add multiple rails containers and nginx will round-robin to those.

https://gist.github.com/WillBeebe/9143ffe79c5dc3882662

md5 pushed a commit that referenced this pull request Sep 14, 2015
Adjust entrypoint to always warn on missing socket
@md5 md5 force-pushed the multiple-paths branch 4 times, most recently from 6b2e0cf to f2ec4b8 Compare October 9, 2015 03:40
@md5 md5 force-pushed the multiple-paths branch 4 times, most recently from f7d9156 to edfedca Compare October 20, 2015 03:34
@pascalandy
Copy link

@WillBeebe, Can you confirm that your modified nginx.tmpl is stable? Issues since 9 months?

Thanks in advance!!
Pascal

@jagandecapri
Copy link

Hei guys,

I am looking to do something like this.

Nginx proxy -> multiple apps as the following:

  1. Hosted on the same server on different ports
  2. location in Nginx is based on subdirectories

My Nginx config will look something like this.

...
upstream app1{

}
...
location /jenkins {
 ...
 proxy_pass http://app1
 ...
}

I am not really sure whether the fix in this PR is for this use case. If yes, that will be great. Let me know if this is a separate issue that needs to be addressed on its own.

Thanks!!!

@md5
Copy link
Member

md5 commented Aug 7, 2016

@jagandecapri The main discussion around supporting your use case is over at nginx-proxy#47. The most recent PR I created to support multiple paths is at nginx-proxy#254, although it is not in a state to be merged. There is also related discussion in nginx-proxy#54 and nginx-proxy#190.

My feeling is still that a custom nginx-proxy binary will be needed to do this properly. The supporting work of making docker-gen stalled a long time ago, but that discussion is at nginx-proxy/docker-gen#131

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants