Closed jhpedemonte closed 7 years ago
@danielfrg, I started reviewing your PR, but ran into some issues. Then I ended up creating my own patch, based on yours but with some changes/fixes.
Main diffs from that PR:
prefixUrl
, since baseUrl
can have different connotation, depending on the context.urljoin
instead of string concatenation when dealing with paths/urls.urlJoin
do the prefix magic. Instead, I pass in the prefixUrl
variable for use in the correct places.I haven't tested everything yet (hence the "WIP" in the title). Will continue next week.
I did notice that I'm getting these errors when loading the INTRO notebook:
dashboard.js:59067 GET http://127.0.0.1:3000/components/b80c321cdb09746d881e6b9227fa4491.png 404 (Not Found)
This is great. I had not a lot of love for my implementation and I am sure yours is better.
I am out this week but I'll try to find time to test if needed.
Actually, I'm not thinking about this right. #260 is about the dashboard server residing behind a reverse proxy, where the client facing URL will have a prefix (i.e. http://something.com/dashboard-server/...). In that scenario, we really don't need to change how the node/express app serves its routes -- most of that stays the same. Instead, we would need to fix templates (already done), plus maybe some of the websocket proxying, etc.
In order to test this, we'll need to stand up a reverse-proxy in front of the dashboard server. Shouldn't be too hard using Docker.
That makes sense for a reverse proxy but it won't be enough for a the configurable-http-proxy
that JupyterHub uses. My final objective with my PR (and now yours) is to register the jupyter-dashboard-server
as a service on JupyterHub that can be used with JupyterHub authentication.
I'm aware of JupyterHub, just never set it up or played with the source. And configurable-http-proxy
is new to me -- although it's just a wrapper to node-http-proxy
, which we use.
Looks like configurable-http-proxy
can be set up as a simple reverse proxy also, similar to what I described above. Through some combination of these arguments:
--no-prepend-path Avoid prepending target paths to proxied requests
--no-include-prefix Don't include the routing prefix in proxied requests
That way, you could add a route to configurable-http-proxy
like so:
POST /api/routes/foo/bar -> http://somehost:1234
Then, calls to http://config-http-proxy.com/foo/bar
would get routed to http://somehost:1234
-- "prefix" route removed.
Is this sufficient for your needs? I think it would be best to keep the code as simple as possible and not add the ability to run from a prefixed route if we don't need to.
That way, you could add a route to configurable-http-proxy like so:
POST /api/routes/foo/bar -> http://somehost:1234
I think the issue is that it's not possible to tell JupyterHub to use the configurable-http-proxy this way. I may be wrong, but I think that JupyterHub relies on setting the base_url
option for notebook servers so that it can write routes like:
/user/parente/ -> http://somehost:1234/user/parente/
Yes, thats correct the Hub does rely in that notebook setting for the single user notebook (and JupyterLab) servers. I was trying to get the same functionality in here.
Looked through the proposal and changes, sounds exactly like the solution for the problem I had.
How can I gain access to the result? Doesn't look like I can clone the incubator repository in a way that it has this specific branch in it.
@parente Well, that's unfortunate. So sounds like we'll need to have two config options:
PROXIED
- Set to true to let the server know that it is behind a proxy. Express must be told it is behind a proxy. This value should also allow any of the values described in that page.BASE_PATH
- (or something like that) Defines the base prefix path that the server operates with.For a classical reverse proxy situation, an admin would only have to specify PROXIED
(I think; almost got that working). In the JupyterHub situation, admin would define both. Does that seem legit?
@laurikoobas This PR is still a work in progress. While it is mostly working, it is not fully working yet. That said, if you want to test the PR as it currently stands you can either clone my repo and branch (jhpedemonte/dashboards_server@260-prefix-url) or apply this patch to your clone of this repo.
@jhpedemonte The two options sound right to me, assuming running the dashboard server behind the jupyterhub proxy as a managed service makes sense. Maybe @minrk can chime in on whether that fits the model correctly.
Updated patch with support for both passing and not passing the prefix value. Seems to be working all around. Looking at test issues now.
Tests run fine on my machine, but failing on Travis. Yay!
@parente or @dalogsdon, would appreciate a review. @danielfrg, please test this patch when you get a chance. I did not test on JupyterHub, just using Nginx.
I am a little busy this week but I will try to test it as soon as I can. Thanks!
@jhpedemonte tried the patch and it's working. One addition I'd like is just the startup message to mention it as well:
Jupyter dashboard server listening on 127.0.0.1:3000
should be with the path added.
Those options make sense for working JupyterHub. A Hub-managed service will get the JUPYTERHUB_SERVICE_PREFIX environment variable, which will be /services/service-name
in most cases. It also receives JUPYTERHUB_BASE_URL, which is the base URL of the whole application (typically /
, and always the part before /services
in SERVICE_PREFIX).
@laurikoobas Done.
@minrk Great, thanks for the info.
I'll try to kick the tires on this by the end of the week.
I just tested this in a couple of settings and it looks like everything its working.
I tested multiple prefixUrl
values including more "complex" ones like: /hub/service/dashboards/server1
.
I also tested this behind the configurable-http-proxy
, all good as far as I can see.
I also tested the deployment from the notebook server using the Dashboards Bundlers plugin by settings the DASHBOARD_SERVER_URL
env to the full path including the prefixUrl and all looks good.
Taking a look now
Looks good to me. The only issue I saw is the lack of calling out the example nginx config file in the README. I had to read the Makefile to know where the example config is.
Code looks fine to me.
Maybe I missed it, but is there a unit/integration test running with the base URL set to something other than the default? Do we want to do that for one test at least? Not with the proxy, just with a /test-root
base URL or something.
Just tried to run proxy-container by "make proxy-container". Containers started normally. Visited http://localhost but received 404 error message. I noted that nginx container port 8080 is mapped to host port 80 in Makefile, but nginx is listening on port 80 as in conf file. After changing port 8080 to 80 and rebuilding all 3 container, http://localhost/dashboards_server/ can be reached, but styling was not applied due to missing http://localhost/css/style.css (Failed to load resource). I was on the latest commit (# 5cac00) by today. What was I missing? Please help! Thanks very much.
I responded to @bowenli37 in #307.
Ref #260
Based on PR #291 by @danielfrg