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

Add X-Original-URI support #826

Closed
wants to merge 1 commit into from

Conversation

Jyhess
Copy link
Contributor

@Jyhess Jyhess commented Dec 17, 2018

Fixes #527 .

Changes proposed in this pull request:

  • Dynamically update base_base using X-Original-URI header to support connexion behind a proxy

@dtkav
Copy link
Collaborator

dtkav commented Dec 17, 2018

hey @Jyhess did you happen to see #823 ?
AFAICT flask's url_for provides a higher level abstraction around this concept.
If possible, we should support the concept of being behind a reverse proxy in general, not just a specific header implementation.
In addition, it would be wise to update the docs so folks know how to take advantage of this feature.
This will be really useful - thanks for the PR!

@Jyhess
Copy link
Contributor Author

Jyhess commented Dec 18, 2018

@dtkav I had a look on your PR, but didn't understand how you succeed to get path part modified by the reverse proxy. Can you give me a hint ?

Using an header make a solution generic, dynamic and configurable for all modern reverse proxy. But I agree with you the header name should be configurable and documented.
For a more static solution, we can use an application option like root_uri. It can fix this issue, and is easier to understand. But it is less dynamic, and will not work if same application can be accessed through 2 different reverse proxy. Even if its not common and maybe not a good practice, I had this case in the past with 2 reverse proxies (for admins and users) using 2 different paths.

@Jyhess
Copy link
Contributor Author

Jyhess commented Jan 8, 2019

@dtkav Looking example you added in your last commit, I understand we are not trying to fix the same issue.
If I well understand, you are fixing the case when application is mounted under a subpath in a WSGI server. Like it is well explained in #392.
On my side, I'm trying to solve the same issue, but when a reverse proxy is used as a completely separated service with rewrite rules:
http://mydomain.com/api/myservice/v1/index.html -> reverse proxy service -> http://myservice/v1/index.html -> my service
In this case, both services are running in different process, (different docker on my case). So there is no way for myservice to know where it is mounted. To fix this case, there is mainly 2 solutions:

  • A static configuration for my service used to define proxied base path. Easy to implement, but it introduce a configuration coupling between 2 services, and not allow to have same service mounted under 2 different path (I add this case in a past where the same service was used under /api/ and /admin/)
  • Configure the reverse proxy to forward original hostname and path via HTTP header. This is the solution I'm proposing.

By the way, this will not solve #527 as I guessed originally. Do you think I should open a different issue with my use case explained ?

@Jyhess
Copy link
Contributor Author

Jyhess commented Jan 8, 2019

I replaced cookbook minimal example with a more complete one in examples/openapi3/external_reverse_proxy

@dtkav
Copy link
Collaborator

dtkav commented Jan 8, 2019

@Jyhess thanks for laying out the issue so clearly.

Please have another look at the example I posted - I forgot to add the file where all the magic was happening :)

I believe it supports your use case by passing the header X-Script-Name

Let me know if I'm still missing something!

@Jyhess
Copy link
Contributor Author

Jyhess commented Jan 9, 2019

@dtkav Review your example with the magic part. It works nicely for Flask using X-Script-Name header, but not for aiohttp, which is my use case.
I had a look on how to implement same patten for AioHttp, but didn't find an easy an proper way to do it. There is an url_for method for resource, but no environ nor script_name concept to dynamically change its behaviour.

What I can do, is to port header based solution for AioHttp in your branch, with header name alignment, and keep your solution for Flask. Do you agree with this plan ?

Note your ReverseProxied class is not usable with AioHttp. WSGI native support has been removed and is now part of an external third party. So if you introduce a WSGI module in connexion and want to keep Flask and AioHttp compatibility, you will need more works. Maybe it can be easier to move your wsgi.py in your example folder instead of main source code.

@dtkav
Copy link
Collaborator

dtkav commented Jan 10, 2019

@Jyhess my mistake - I didn't realize your use case was with aiohttp (which I'm not very familiar with).
I will read more about url_for for aiohttp. There must be a way to do this with the framework - it's such a common use case to generate fully qualified urls within a web framework.

What I can do, is to port header based solution for AioHttp in your branch, with header name alignment, and keep your solution for Flask. Do you agree with this plan ?

I don't think connexion/apis/* should be proxy-aware. The reason I'm going down the url_for path, is that I think that is the right layer of abstraction to fix it.

Many users will be using reverse proxies, and they will often need to rely on this type of functionality (access to incoming path, scheme, host). If it is implemented with the primitives available from the underlying web framework, then it's available in the handler functions. If the user understands the framework well, they won't have to learn anything new.

Note your ReverseProxied class is not usable with AioHttp. WSGI native support has been removed and is now part of an external third party. So if you introduce a WSGI module in connexion and want to keep Flask and AioHttp compatibility, you will need more works. Maybe it can be easier to move your wsgi.py in your example folder instead of main source code.

Ok, that makes sense. I didn't even realise that aiohttp had a wsgi compatibility layer at one point.
I agree that it probably makes sense to move wsgi.py into the example folder, or potentially built into connexion/apps/flask_app.py.

For aiohttp, I think the best way to do this is to set the original uri/script_name in the middleware along with the scheme and host. I've just started reading about this, but here's what I found so far:
https://github.com/aio-libs/aiohttp-remotes/blob/master/aiohttp_remotes/x_forwarded.py
http://docs.aiohttp.org/en/stable/web_advanced.html#deploying-behind-a-proxy

So, to sum up:

  1. I don't think reverse-proxy logic belongs in connexion/apis/*
  2. I think we should rely on the primitives of the underlying framework
  3. I will learn more about aiohttp, and try to come up with a solution using aiohttp middlewear

I appreciate your clear communication style. Hopefully I'm making sense too 😁

Let me know what you think!

@dtkav
Copy link
Collaborator

dtkav commented Jan 10, 2019

I think I got it working with aiohttp in #823 today.
I'd love your feedback now that they are more concretely comparable.

@Jyhess
Copy link
Contributor Author

Jyhess commented Jan 10, 2019

Good catch for aiohttp-remotes. I missed it during my own research.
This solution is better than mine. I will do some minor comments on your PR.
Thanks for your work.

@Jyhess Jyhess closed this Jan 10, 2019
@dtkav
Copy link
Collaborator

dtkav commented Jan 13, 2019

Thanks for the collaboration @Jyhess !

@Jyhess
Copy link
Contributor Author

Jyhess commented Jan 14, 2019 via email

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.

None yet

2 participants