Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support proxying multiple paths to different backends #254

Closed
wants to merge 3 commits into from

Conversation

md5
Copy link
Contributor

@md5 md5 commented Oct 9, 2015

I'm finally opening a PR to review my branch for supporting multiple paths with VIRTUAL_PATH. I think that some of the duplicated code could probably be made a bit more DRY, but this code seems like a good starting point for a discussion to me.

A couple things are happening in this patch.

  1. Detect if VIRTUAL_PATH is being used within the containers with a shared VIRTUAL_HOST
  2. If so, take the SHA-1 hash of the path and use it as a suffix for both the upstream name and the proxy_pass statement

In the event that VIRTUAL_PATH is not set for / within the context of a particular VIRTUAL_HOST, a 404 is returned. This should possibly be a 503 instead, but I didn't want to hold up the discussion of this patch any longer (it's been over a year since I first created this branch).

Known issues that might need to be addressed before merging:

  1. Further consolidate duplicate code into the shared "location" template
  2. Figure out if 503 should be returned for a missing / instead of 404
  3. Handle the case where some containers within a VIRTUAL_HOST have VIRTUAL_PATH set but others do not

Fixes #33

@md5
Copy link
Contributor Author

md5 commented Oct 9, 2015

Another thing that would need to happen if there is a desire to merge this PR is an update to README.md to document the feature.

@md5
Copy link
Contributor Author

md5 commented Oct 9, 2015

And I suppose some tests now that #246 has been merged. 👍

@md5
Copy link
Contributor Author

md5 commented Oct 9, 2015

Looks like groupBy will leave out containers without the grouping key. I think a call to whereNotExist to find the containers without VIRTUAL_PATH may be needed, using / as the .Path for those containers.

@md5
Copy link
Contributor Author

md5 commented Oct 14, 2015

@jwilder I'd love to get your thoughts on this PR when you have a moment. Looks like the merge of #262 introduced a conflict, but I can fix that if you're interested.

@jwilder
Copy link
Collaborator

jwilder commented Oct 15, 2015

@md5 What do you think about combining this functionality with #259 so that you can do all of this by just setting the VIRTUAL_HOST appropriately.

Something like:

VIRTUAL_HOST=example.com                  # works as is today
VIRTUAL_HOST=example.com/foo            # proxies example.com/foo to this container
VIRTUAL_HOST=example.com/foo:9090   # proxies example.com/foo to this container's port 9090
VIRTUAL_HOST=example.com,example.com/admin:8080   # proxies example.com to this container port 80 and example.com/admin to port 8080 on the container

There have been a few PRs for implementing parts of this functionality separately, but I think it would be easier for users if the syntax was combined.

Thoughts?

@md5
Copy link
Contributor Author

md5 commented Oct 15, 2015

I think the idea of combining VIRTUAL_PATH and VIRTUAL_HOST is a good one. I'm less keen on combining the port in that way since I think it could be confusing whether the port refers to the backend container port or the nginx-proxy port. There's also the fact that #259 includes setting a different protocol for different backends, which would further complicate the syntax for VIRTUAL_HOST.

At this point, I'd probably start moving toward a JSON config format, similar to what ehazlett/interlock does with INTERLOCK_DATA.

@jwilder
Copy link
Collaborator

jwilder commented Oct 15, 2015

Yeah, originally I didn't like the idea of having the port part in there. Starting to change my mind because the port for nginx-proxy is always what you define when you start the container. I would be fine leaving that out initially though and still requiring VIRTUAL_PORT to change it.

I forgot about the protocol change piece to #259 as well. I'm fine with keeping that out for now as well too.

@md5
Copy link
Contributor Author

md5 commented Oct 16, 2015

It looks like some work would need to be done in docker-gen to support the VIRTUAL_HOST=host.example.com/some/path syntax.

Namely, docker-gen would need to expose the strings.SplitN function to allow something like this:

{{ parts := splitN $host "/" 2 }}
{{ $host := $parts[0] }}
{{ $path := printf "/%s" $parts[1] }}

I'm not sure whether that's the actual code that would be needed; it seems like there could need to be a check to see if the splitN returned 1 or 2 parts and to act accordingly, but it may be that text.template allows indexing into undefined elements of a slice (unlikely).

@md5
Copy link
Contributor Author

md5 commented Oct 17, 2015

It just opened nginx-proxy/docker-gen#126 to add splitN. It appears that a len check will be necessary since text.template is strict about indexing into slices that are too short.

@RheaUN
Copy link

RheaUN commented Oct 17, 2015

Hello. I am not familiar with the netiquette within github, so I am not unsure if "+1" is welcome here. But I'll give it a try. :)

The feature would be really good in the way you propose. And it appears to be consistent/logical in the way the reverse-proxy is configured so far.

The type of setup with a VIRTUAL_PATH can be very common in an environment with a DynDNS-Service in combination with a temporary IP-address. Usually you have only one subdomain referring to your current IP-address/service. A VIRTUAL_PATH option would really help in this environment to reach different containers.

P.S.: THX for engagement in the reverse proxy!

md5 added 2 commits October 17, 2015 16:06
`first` returns interface{}, not string, so it makes strings.Trim choke. The
call to `or` returns a string in both cases, which `trim` can handle.
@md5 md5 force-pushed the multiple-paths branch 3 times, most recently from ea49fae to f7d9156 Compare October 20, 2015 02:02
@md5
Copy link
Contributor Author

md5 commented Oct 20, 2015

I've done some work toward getting this working with splitN.

One thing I realized is that there may be some differences of opinion as to how this feature should work. Specifically, I could see some users expecting the path to be stripped from the URL that is received before proxying it to the backend while others might want it to pass through unchanged.

As it stands now, the code will pass through the path unchanged. Whichever option is chosen, the backend application will need to know that it's running at /path, either to have its root URL be at /path or to know how to properly construct relative links to itself.

@md5
Copy link
Contributor Author

md5 commented Oct 20, 2015

We may want to pass an X-Forwarded-Path header to support informing the application which path it's running under.

@md5 md5 changed the title Support paths with VIRTUAL_PATH Support proxying multiple paths to different backends Oct 20, 2015
@jwilder
Copy link
Collaborator

jwilder commented Oct 27, 2015

I think setting X-Forwarded-Path is a good idea. I also think the /path prefix should be stripped too so that the backend is getting paths as if they were not mounted behind a proxy.

@md5
Copy link
Contributor Author

md5 commented Oct 27, 2015

Don't you think some users would want it one way and some the other way?

@jwilder
Copy link
Collaborator

jwilder commented Oct 28, 2015

@md5 The use cases I've heard so far are to support exposing multiple backends where someone only has one domain name. Making the path based proxying transparent to the backends seems simpler for users to me. I.e they would be able to start there backends the same as if they were not using path based proxying (hopefully).

What do you think?

@md5
Copy link
Contributor Author

md5 commented Oct 28, 2015

I think that having a backend service that responds at / and wanting to expose it at /path is probably the more common use case, so it probably makes sense to do it first and have it be the default.

However, whichever option is chosen is not going to be transparent to the backend. Either way, the backend needs to know it's at /path. It either needs to have that be its root path for routing or it needs to know how to structure responses to be aware of the fact that relative paths are under /path instead of /.

@md5
Copy link
Contributor Author

md5 commented Oct 29, 2015

I'm taking a look again at supporting VIRTUAL_HOST=host/path again and I think this may be getting beyond the capabilities of text.template.

When I was using VIRTUAL_HOST and VIRTUAL_PATH, it was possible to group all the containers by VIRTUAL_HOST, then to group them by VIRTUAL_PATH. This allowed me to properly construct a server block per VIRTUAL_HOST with a nested location block per VIRTUAL_PATH.

In order to get the same structure with a single variable, I need to be able to group containers by the first part of the split VIRTUAL_HOST instead. The code would look something like this in Ruby:

containers.reject { |c| c.Env['VIRTUAL_HOST'].empty? }
          .group_by { |c| c.Env['VIRTUAL_HOST'].split('/', 2)[0].strip }
          .each do |h,h_containers|

  puts "server { server_name #{h};"

  h_containers.group_by { |c| c.Env['VIRTUAL_HOST'].split('/', 2)[0].try(:strip) || '/' }
              .each do |p,_|

    upstream = "#{h}-#{Digest::SHA1.digest(p)}"
    puts "location #{p} { proxy_pass http://#{upstream}/; }"
  end

  puts "}"
end

Doing this with docker-gen would require adding something like a groupBySplit function, since it's not possible to define an anonymous function in text.template or to define a template function that can take another function as an argument.

If we didn't have to deal with the comma-separated list in VIRTUAL_HOST, I could see possibly passing a template name instead of a first class function like this:

{{define "virtualHost"}}{{ index (splitN (trim .Env.VIRTUAL_HOST) "/" 2) 0 }}{{end}}
{{ range $host, $containers := groupByTemplate $ "virtualHost" }}

However, I'm at a loss as to how to support the groupByMulti functionality used to split on "," as well. And now I realize that my example Ruby code above didn't deal with the comma case either...

@md5
Copy link
Contributor Author

md5 commented Oct 30, 2015

I've been thinking that it might be worth considering making docker-gen usable as a library instead of just as a command-line tool. That would allow an nginx-proxy binary to be built that has additional Nginx-specific functions beyond those provided by docker-gen itself. I think that could help clean up the template and make things like multiple path support easier to implement.

@jwilder
Copy link
Collaborator

jwilder commented Oct 30, 2015

@md5 Yeah. I've had the same thoughts recently and have implemented docker-gen like functionality a few times now for various things. A custom nginx-proxy binary would make it easier to extend the project.

@CReimer
Copy link

CReimer commented Nov 13, 2017

Just stumbled upon this PR because I actually need this feature, too.
Could someone please pull this into the master branch. Thanks

@andreycizov
Copy link

Any particular reason this is still not merged?

@md5
Copy link
Contributor Author

md5 commented Nov 22, 2017

@andreycizov The reasons are exhaustively covered in the previous comments.

@m4teh
Copy link

m4teh commented Nov 25, 2017

Very much would like to see this also.

@adrianilie9
Copy link

adrianilie9 commented Feb 16, 2018

Giving the fact that this is a very much requested feature that has been in stand-by for 2 years now because @jwilder considers the configuration template as not making "most sense" can we please get a pass and merge this?

It will only affect people whom are interested in this feature, which currently is non-existent. I think writing a couple more ENV variables trumps the lack of existence for this feature.

@md5
Copy link
Contributor Author

md5 commented Feb 16, 2018

@adrianilie9 I haven't used this image in almost 3 years, so I'm not going to be working on this PR again. The only reason I haven't closed it is that I know people want the feature.

@rodrigoaguilera
Copy link
Contributor

I took another version of this feature and I rebased it against the current master in case some is interested

#1083

Alexander-Krause-Glau pushed a commit to Alexander-Krause-Glau/rpi-docker-nginx-proxy that referenced this pull request Mar 30, 2018
Incorrect trimming did lead to empty domains being created on space separated domains
or with comma trailed LETSENCRYPT_HOST environment variable. This in turns led to the
container being caught in an endless loop trying to delete /etc/nginx/certs nginx-proxy#254 nginx-proxy#288
@raphaelyancey
Copy link

I took another version of this feature and I rebased it against the current master in case some is interested

#1083

@rodrigoaguilera It works pretty well thanks a lot, but I was wondering why the path is passed to the target container as well? Would it be difficult to strip the virtual path off the requests before redirecting to VIRTUAL_HOST:VIRTUAL_PORT?

Example: I'd like to serve a phpmyadmin/phpmyadmin container on /phpmyadmin but I can't since it requests /phpmyadmin to the target container, whereas it's served on /.

Or is there a nginx override I might create to make it working without additional code?

@danieldaeschle
Copy link

any updates to this?

1 similar comment
@seompark
Copy link

seompark commented Dec 8, 2019

any updates to this?

@SerdarNur
Copy link

Any updates here? @md5 i really need this feature.

@iedmrc
Copy link

iedmrc commented Mar 18, 2020

Unbelievable! It's been 5 years since this pr is created.

@adrianilie9
Copy link

I have successfully ported this feature into a private fork. If anyone is still interested, i can make a public one. Just let me know.

@iedmrc
Copy link

iedmrc commented Mar 18, 2020

Why it hasn't been merge onto the master branch yet?

@KrzysztofMadejski
Copy link

KrzysztofMadejski commented Mar 31, 2020

@adrianilie9, yes, please share it publicly and send as another PR. I'd love to use it as well.

BTW: Did you resolve issues that md5 mentioned?

The remaining open issues as I recall are:

Sort the available VIRTUAL_PATH values for a VIRTUAL_HOST to ensure that dispatch will work in cases of prefix overlap (e.g. a container with VIRTUAL_PATH=/api alongside a container with VIRTUAL_PATH=/api/v1).

Ensure that a 503 status code is still returned if VIRTUAL_PATH is being used and there is no container mapped to VIRTUAL_PATH=/.

@kumarunster
Copy link

@adrianilie9 could you share your fork please?

@camold
Copy link

camold commented Apr 20, 2020

+1 vote for obtaining a feature to specify different virtual paths/locations on the same host/server that are forwarded to different containers.
Would it not be possible to mount in a configuration file (as is done in section "Per-VIRTUAL_HOST location configuration" of the readme) for each virtual host and location combination? This may resolve the issue of configs (i.e. if the request path should also be forwarded to the container or not).
Concerning the ordering, would it be possible to use regular expressions? For instance "location = /" in the template? Then, the ordering should be irrelevant.

@canhtran
Copy link

canhtran commented Jun 8, 2020

Hi Guys, how is thing PR going ? It really super helpful

@lilgallon
Copy link

Any updates on this?

@yoruvo
Copy link

yoruvo commented Mar 5, 2021

Is this project abandoned? How come this PR hasn't been added in 6 years?

@md5
Copy link
Contributor Author

md5 commented Mar 6, 2021

Is this project abandoned? How come this PR hasn't been added in 6 years?

The lack of updates to this PR should be quite clear from the comment history. I'm not sure there's anything more to say on that.

As for whether this project is abandoned, it looks like @jwilder made some updates in late 2020, but this is clearly not a project that is receiving feature updates. It looks like all recent PRs that were reviewed and merged are hotfixes to existing functionality.

@buchdag buchdag added scope/path-based-routing Issue or PR related to path based routing type/feat PR for a new feature labels Jul 16, 2021
@buchdag
Copy link
Member

buchdag commented Jul 19, 2021

This features has been added to the dev branch with #1607

It's available now on the dev images nginxproxy/nginx-proxy:dev and nginxproxy/nginx-proxy:dev-alpine. Testing and feedback are welcome, the docs for the feature is here and there is another PR opened for additional improvements and comments.

@buchdag buchdag closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/path-based-routing Issue or PR related to path based routing type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Another template for VIRTUAL_PATHs instead of VIRTUAL_HOSTs