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

feat(config): Add asgi-root-path option #2493

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

amol-
Copy link

@amol- amol- commented Oct 24, 2024

Summary

Rephrases the --root-path help message to make more clear that the option will prefix all paths with its value and adding the --asgi-root-path option which does set root_path without altering the path itself.

The naming of the new option was chosen to avoid introducing backward incompatibilities with users relying on the old option.

This is necessary when uvicorn is serving an application that is being proxied behind a proxy server to avoid having to rewrite urls in the proxy server which is more complex and error prone.

For example, we could use nginx with following configuration

        location /proxy/ {
            proxy_pass http://127.0.0.1:8000;
            proxy_set_header   Host $host;
            proxy_set_header   X-Real-IP $remote_addr;
            proxy_set_header   X-Forwarded-For $proxy_add_x_forwarded_for;
            proxy_set_header   X-Forwarded-Proto $scheme;
        }

and start unicorn with

$ uvicorn testapp:app --proxy-headers --asgi-root-path=/proxy

the fact that the application is served in a subpath would be totally transparent to the web application as far as the web framework correctly handles root_path.

This could allow a misconfiguration if the nginx location and uvicorn asgi-root-path don't match, but I think it's out of scope for uvicorn to detect deployment misconfigurations.

This was discussed in #2490

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@amol-
Copy link
Author

amol- commented Oct 24, 2024

the test failure seems to be unrelated.

test_close_connection_with_multiple_requests

fails on master too for me

@amol-
Copy link
Author

amol- commented Nov 1, 2024

@Kludex when you have time I would really appreciate your feedback on this one 🙏
It would allow me to remove a bunch of hacks in some proxied applications (as I can't modifty the applications themselves and thus I can't set root_path in the FastAPI app).

I guess the option could also be renamed to --proxy-root-path to align more to --proxy-headers and better communicate that is specific to proxy use case.

@alistairmaclean
Copy link

We are in the same predicament where we cannot use proxy rewrite rules or configure the root_path directly in FastAPI code, and therefore have been unable to leverage Uvicorn/FastAPI technologies in our stack since version 0.26.0 of Uvicorn which changed the behaviour of root_path.

+1 for this feature

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

Successfully merging this pull request may close these issues.

2 participants