-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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 hardcoded urls in python and template files. #1866
Conversation
Thanks for the PR, I would have left the tests untouched and added a single test to check that the middleware is working. Otherwise we are not testing that our urls are stable. Please wait for @mistercrunch and @bkyryliuk opinions before updating the PR. |
I'm open ot the idea of doing this. Though what about the hard coded urls on the We should also add tests around the FAB urls. And to @xrmx's point, we need to make sure that all current URLs are unchanged as users have bookmarks. |
@xrmx thought we would do the python stuff first and then tackle the I didn't think about bookmarks, good point. I still think it's necessary to run the tests under a prefix, otherwise hard coded urls will creep back in. How about manually adding the prefix to urls in the tests? That way it should also fail if someone changes an endpoint. The FAB urls seems pretty good. The only one I have found so far is in |
Awesome. I have [recently obtained] merge priviledges on the FAB repo if we need to change anything on that side. I'm assuming your end goal is to add a prefix (subfolder) in the URL for the whole site for reverse-proxying, so where in the configuration would you set this prefix? I was looking for a |
Cool with the FAB merge powers. I might have a more thorough look for hard coded urls there as well then. I'd go for not adding a It's fairly straight forward to setup the proxying, so I think it should be left as an exercise to the reader, with good hints in the documentation of course. |
@mistercrunch I ran into the same issue when trying to host Superset and our company's Django-based apps on the same domain under different paths (the primary purpose being not having to deal with CORS when embedding/interacting with graphs). The django apps required no changes or configuration, I just had to set the SCRIPT_NAME paramater in my nginx location config (see below) and the rest was handled by WSGI (PEP-0333) and Django. I thus agree with @joharohl that all Superset needs to do is not have hard-coded URLs. Especially with #1832, the ReverseProxied middleware @joharohl referenced is easy to add to your local Example nginx location setting the location /d/ {
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Scheme $scheme;
proxy_set_header Host $http_host;
# Django running on host using Gunicorn
proxy_set_header SCRIPT_NAME /d;
proxy_pass http://django_server/d/;
} |
@joharohl I'm not sure what you have in mind for the javascript side of things, but what I'm currently doing for my projects is to make an AJAX request at the top of each of my javascript files and then get the SCRIPT_NAME value from the response header: let request = new XMLHttpRequest();
request.open('GET', '/health');
request.send();
const SCRIPT_NAME = request.getResponseHeader('SCRIPT_NAME'); Then I construct all my javascript urls from the SCRIPT_NAME variable. This approach can be improved by storing the SCRIPT_NAME value in a cookie (or other form of local storage) and only making the ajax request if cookie value is not set. I have no idea if this is a solution, a workaround or a dirty hack, but it allows the client-side code to be independent from the server-side and have both honor the "SCRIPT_NAME" header set by the proxy server. *Got the idea from here |
@jr-minnaar I was lazy and just templated js constants in const serverRoot = "{{ request.script_root }}";
const staticRoot = "{{ url_for('static', filename='') }}"; I then just used these for all the urls in I'm all for either approach, but I have the nagging feeling that there is a better way to do it. |
Another option would be to add the needed prefix at build time through webpack. |
@joharohl Thats not lazy, that's efficient! I think when there is no need to separate client-side from server-side (meaning you can use Jinja templates to pass variables to javascript), then your solution is far more robust. @xrmx, I'd prefer a solution like @joharohl described where the |
We also need a dynamic prefix/subdir for our usecase. We want to do multi-tenancy on the path so we need one instance of superset to be able to serve many different paths. It's still possible if the prefix is added on build time, but a lot more painful. To not derail this much further, I think I should compile my JS changes and do another PR and we can discuss it there. On topic: is it fine to change the tests to hardcode |
What is the best way to prove that URLs aren't changing (insuring that bookmarks point to the right place when running the default configuration)? Maybe deriving the |
It's probably fairly easy to run the tests twice, once with a prefix and once without. If you feel doubling the number of tests is fine, I can go ahead and make that change |
@joharohl it's not that it's difficult it's that it'll make tests longer to run :) unless made parallel too. |
Which is why I asked if you really want to run the tests twice ;). I don't see any good way of making sure that the original urls are not changed. I personally think always running tests under a prefix is fine, but I got the impression that's not enough. |
@xrmx, @mistercrunch It's less robust perhaps than running the tests with a prefix on all different variations, but it feels like it should be fine, |
@joharoh l was thinking on adding just a test case, we want to check that the this work in every platform and not just in the one with the flag set no? Somethings like this in tests/url_tests.py:
|
Ah, I see. I was obviously a bit confused what you were trying to get at earlier. I think I'm not convinced that the approach in To catch hard coded urls in those cases, there has to be tests in place that asserts that generated urls have the proper prefix. In addition, all redirects have to be tested. By having the current tests run under a prefix it is possible to at least test a lot of the redirects, but I have no idea how the coverage looks like. I don't think there are many tests that look at hrefs in the templates. So, I'm seeing two main routes here:
I'm fine with 1. It will mean there will be the occasional hard coded url slipping in, but it should be easy to patch when it happens. The few of us that need to be able to run superset under a prefix just has to be a bit careful with the upgrades :) |
I'm fine with just 1 either too but please add a testcase checking that loading the flask app under a different path works. |
…refixed superset instances.
I have updated the PR now to follow point 1 in my previous comment. I added a few urls that felt like it made sense, but you probably have more of an idea how superset is used and which urls are important to test. So please let me know which additional urls to add. I feel we don't need to test the FAB urls because they work and should be the responsibility of FAB anyway. If you still want to include some of them (like the Dashboard model view e.g.) just let me know. |
Can you solve these conflicts? or When will you solve these conflicts? |
There wasn't any conflicts when I updated this PR in January. I never got any feedback after that, so assumed it wasn't very highly prioritized. We have since started maintaining or own fork of superset so we are no longer in need for this to be merged. Updating this PR right now will most likely involve going through the code again and look for more hardcoded urls. Since it's been a while (both time and commit wise) I'm expecting this to take some time. If you think this type of PR is useful I'd be happy to do it. I'm wondering though if it's better to close this and #985 and accept that superset isn't supported running under a prefix? It's fairly complicated to maintain, and the number of users that need it is most likely small. |
I'm also maintaining a (fragile) fork based on this pull request. I still hope this will eventually get merged as that would save me a lot of rebasing and cherry picking effort. But I agree with @joharohl, resolving conflicts now without merging will be effort better spent elsewhere. |
Things aren't looking good as far as merge conflicts go... |
@xrmx Hardly surprising as it was almost 3 months and 277 commits to master since I last updated the PR. I just need to know if this is a type of contribution that you are interested in. If yes, as stated above I'm happy to do the necessary adjustments to get things in line with master. If not, please close this PR and #985 as won't fix. |
These changes would also be helpful for me. |
@joharohl I ran into the same issue with prefix. Is this a lot of work to get it merged and maybe resolve new hardcoded urls? It will be very useful. |
@gajes03 It's not that much work to fix all the urls. The problem is more on the testing side where it would be good to be able to still use hardcoded urls to make sure the relative ones don't break existing links. I have some ideas I wan't to try, but need some time to sit down and try them out. I'm a bit pressed for time right now, but I will try and revisit this in a few weeks time |
Closing old PRs with broken build or merge conflict, feel free to re-open once merge conflicts are sorted out. |
Any alternative solutions until you guys fix it? |
If someone wants to start this over, I'd recommend writing a small PR with a proof of concep with a single URL on the client and server side so that it doesn't merge conflict with anything. Once this lands we can go section by section and get some cadence. The alternative is some good old proxying. |
Wanted to confirm - since in my org we wanted to deploy superset behind nginx reverse proxy under different prefix - and now that does only works on some links - so basically the effort is killed? Any other solutions? examples, hints ? Some fancy url rewrite ? |
@mrpapini Does |
Hi - thanks for response - no it does not - I actually tweaked the code from werkzeug.contrib.fixers.ProxyFix to parse HTTP_X_SCRIPT_NAME in addition what they have there - some links work - some still not - for instance sqllab links will not work - of course this is with conjuction with nginx reverse proxy config - of course if location is just "/" all works fine but if location is say "/apps/" it does not - just looking for proper combination of nginx parameters - but judging from this thread and others seems this is not possible unless some heavy patching of entire code base is done |
Hey @mrpapini A solution that will avoid patching the code might be to do rewrites in the html in nginx. The sub_filter module should be able to do the trick. this stack overflow looks promising. Might have a look myself soonish as this would probably be the easiest way of solving this issue. |
Thanks @joharohl - seen this one - didn't try it yet - will do today - first need to compile that module in. As a note, there is lot's of try this - try that but no definitive answer - I need to get to the bottom of this - like you said - even you didn't really try it so all these are guesses now |
Hey @joharohl - so looks like sub_filter will work - I'm in process of figuring out exact pattern but so far I'm getting good results - once I have the nginx setting working I'll post it here |
Hello @mrpapini . Here my nginx configuration to make it work completely :
|
@Webgardener - thanks for the tip - but I have to say I just tried it and that didn't work basically i swtiched ENABLE_PROXY_FIX=True should your location be location /apps/ { ... instead of location /apps { ... (trailing slash) ? |
@mrpapini
You have to replace the uppercase text with your values. Since you work on a mac, I assume Nginx and Superset run on the same "local" machine. Here is the config on my local machine : in /etc/nginx/sites-enabled/superset.conf:
In /etc/hosts: About the trailing slash : it depends on what you need. The behavior is different whether you add a trailing slash or not. On the second location, without trailing slash: with trailing slash : On the first location, since the requests are processed by proxy_pass, the behavior is special:
and go to http://superset.local/analytics, nginx will automatically redirect to http://superset.local/analytics/ |
Hi, I'm triyng to get working superset as a subfolder of a virtual host: http://my.server.com/superset/ Been unable so far. Is the previous config suitable to achieve this? If i replace YOUR_PREFIX for superset I end up in an endless loop:
Thank you. |
Hi. Here is the content of my nginx config file :
Also, don't forget to put the middleware in the superset_config.py file :
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@Webgardener Thanks that works for nginx Is this possible with a haproxy settings ? |
@Webgardener |
Hi all, i have been working on the same issue for a while and i have some suggestions,
as you can see you need to match all the hard codded path that are being redirected to the default root context ('/'). i will say this is ugly. There might be some further modifications in these paths which make the maintenance not very interesting.
this is working well as long as all the paths start with /superset, but because there are many other paths that does you have to match all of them like mentioned above. Even if it will still be hard coded at least it will make the workaround much easy to maintain and reliable. 2- And the second which might be less easier but good for long term is to define a global parameter configuration in the config file like (URL_PREFIX), which should be added as base path to all the routes, then get rid of the hard coded URL. Thanks. |
#1866 (comment) to |
#1866 (comment) My trys ended up in cascades of https://host.xyz/superset/superset/superset/[...]/welcome, in lots of 404s, never-ending redirects or something similar. I even configured nginx to listen to 8443 (since 443 is bounded to a bunch of other applications) to pass everything to the superset server. But even this ended up in an error: "The referrer does not match the host." To be honest, I came in touch with several (>50) semi professional and professional web applications as an administrator of reverse proxies and web application firewalls. Even the worse ones were able to define a url prefix or were able to work behind a reverse proxy without breaking the fingers. This is a must have for web applications nowadays. (To be fair, it seems to be more a flask issue than a Superset issue, maybe flask is not the right choice.) I like the idea of hiding applications behind url prefixes for public access-able services, because the most scanners try to tackle / and a bunch of well-know directories and nothing more. In addition to that it is much, much easier for http based routing, if you have one central entry point. Any hint to solve it is appreciated. |
Hi, I wanted to run 'Superset' in complete serverless environment on AWS. Basically, want to run superset on AWS Lambda. I have tried different methods recommended and all of them resulted in some or other place break of functionality. Eventually what i did was the below to make it work:
After above all are done, I have used AWS Aurora Serverless as my metadata database. This way I don't even need to pay for the database since Aurora is MySQL version. Hope this helps. Once i fix up the remaining this, I need to check how to make this available to all. Thanks |
I found it very frustrating to setup a base url for superset. If you want to save some time, I condensed a couple of comments into a working example here: https://github.com/komoot/superset-reverse-nginx-example |
@christophlingg this is just doing the job for the HTML page, all other stuff (static CSS, ...) is not using URL prefix and should never hit the right backend server (superset). Most entreprise users are using software behind proxy, for infrastructure / security / auth reasons. At X.com, we are running all analytics services behind |
yes @ebuildy , I eventually gave up and went for a dedicated subdomain for superset |
Thanks for this. It helped solve some issues |
This PR is in relation to issue #985.
The first commit basically converts all existing hard coded urls in superset to use the Flask
url_for
method to generate urls.The second commit adds a middleware that runs all tests under the prefix
/test
to make sure that future contributions will not add in hard coded urls again. It also fixes current tests that wouldn't work with the new prefix.I need to clean some of this up a bit, but wanted to create the PR now to get some feedback before I do much more with it.
If you want to go with running tests under a prefix, there are a some complications due to the fact that
url_for
needs the application context in order to resolve the urls. And it needs to be prepared with the right prefix to give the right urls. In addition, any test code that runs app code which generates urls need to wrap this code in an app context as well (exceptions beingself.client.get/post
). I have added a number of help methods to theSupersetTestCase
class that should help people out (self.url_for
,self.get_app_context
. It still requires a bit more from the test code though since app context mishaps give very confusing errors.Anything I missed, should change, etc., Please let me know