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

Fix URL generation to better support serving behind proxy #451

Closed
wants to merge 2 commits into from

Conversation

frankie567
Copy link

@frankie567 frankie567 commented Aug 23, 2022

Objective

Currently, it's very hard to serve a JustPy app behind a reverse proxy.

Typically, if we want to serve the JustPy app under a path prefix. In ASGI apps, this can be handled thanks to the root_path parameter. There is a very good rationale about this in the FastAPI documentation: https://fastapi.tiangolo.com/advanced/behind-a-proxy/

However, for this to work, we need to make sure every URL are generated properly, especially by using the url_for function of Starlette.

Currently, there are many places where the URL is manually built.

Changes

The presented changes try to solve this.

  1. The URL of the WebSocket is now dynamically generated in the main.html script. For this to work, we had to add the name parameter to the WebSocket route.
  2. Every CSS and JS static files in the /templates/local folder now have a dynamically generated URL.

Feel free to make some comments about this; I'll be happy to improve my proposal so it fits well in the project.

@rodja
Copy link
Collaborator

rodja commented Aug 23, 2022

Very interesting. I'll have look at this tomorrow. We have found a slightly different solution but maybe this is nicer.

@falkoschindler
Copy link
Collaborator

Discussion #402 seems related.

@WolfgangFahl WolfgangFahl added core enhancement New feature or request labels Aug 24, 2022
@rodja
Copy link
Collaborator

rodja commented Aug 26, 2022

The Issue #220 is also related.

@rodja
Copy link
Collaborator

rodja commented Aug 26, 2022

And this pull request: #258

@rodja
Copy link
Collaborator

rodja commented Aug 26, 2022

@frankie567 for comparison with our internal solution I created #459. What do you think?

@WolfgangFahl WolfgangFahl added this to the 0.3.0 milestone Aug 26, 2022
@WolfgangFahl WolfgangFahl reopened this Aug 26, 2022
@rodja rodja mentioned this pull request Aug 27, 2022
@rodja
Copy link
Collaborator

rodja commented Aug 27, 2022

@frankie567 I just pushed a reverse_proxy_demo (see #220 (comment)). Unfortunately with your current solution I just see a blank page when accessing it from behind the reverse proxy.

It seems like url_for() from Starlette does not take the reverse proxied path into account (see encode/starlette#604 for example).

@frankie567
Copy link
Author

frankie567 commented Aug 27, 2022

To make it work with this solution, the root_path needs to be set with the wanted prefix. With Uvicorn, this can be done either by:

  • Setting the UVICORN_ROOT_PATH environment variable
  • Setting the --root-path option when running uvicorn

I've it working that way on a project with Docker and Traefik.

@rodja
Copy link
Collaborator

rodja commented Aug 27, 2022

Ah of course. Setting root_path works in about 99% of all scenarios. But we for example use NiceGUI (and hence JustPy as basis) on mobile robots. These are accessable through their local wifi network (eg. http://192.168.42.1/) and through a device gateway hosted on the internet (eg. https://devices.zauberzeug.com/robots/3/). Thereby the root_path is different for different users.

@frankie567
Copy link
Author

I see, that's indeed an unusual use-case 🤔 Well, I'll let you choose the best solution then.

@rodja
Copy link
Collaborator

rodja commented Aug 29, 2022

Ok, I think the X-Forwarded-Prefix solution is more general and flexible than using url_for. I'll close this pull request to further work on #459. We still need to decide whether to use tag or not.

@rodja rodja closed this Aug 29, 2022
@WolfgangFahl
Copy link
Collaborator

#389 is also offering a solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants