-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
refactor: Allow superset to be deployed under a prefixed URL #30134
base: master
Are you sure you want to change the base?
Conversation
Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere. |
Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment? |
); | ||
this.baseUrl = url.href.replace(/\/+$/, ''); // always strip trailing slash | ||
this.basePath = basePath.replace(/\/+$/, ''); // always strip trailing slash |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
library input
This
regular expression
library input
I think this may be quite an undertaking and might have to be tackled in phases. Would love to see this done :) I remember giving it a shot early on in the project at least once, and never wrapped up the work. My main advice would be to avoid taking in side mission or bigger refactor while tackling this. |
docker/docker-frontend.sh
Outdated
@@ -17,19 +17,23 @@ | |||
# | |||
set -e | |||
|
|||
# zstd exe is required by simple-zstd package | |||
apt-get update | |||
apt-get install -y zstd |
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.
there's a recent fix in master for this
Thanks for these changes. |
For what it's worth, we've been running this fork to serve superset on a path prefix in a Kubernetes(K3S) + traefik context without any problems so far. Though testing has been limited. Thanks for the work so far |
When ENABLE_PROXY_FIX is configured url_for will ensure any base prefix is correctly dealt with.
Allows an application prefix to be defined separately to the URL prefix used for the static assets. Webpack looks for a BASE_PATH environment variable and injects this into the built assets.
93d66c7
to
09a05b3
Compare
Docker version of superset_config sets ENABLE_PROXY_FIX=True if BASE_PATH not empty.
09a05b3
to
fe3b01a
Compare
Sorry for the delay in replying to this. I've had zero time to spend on this lately. The latest commits rebase on the current master and I have also added some documentation to the configuring superset pages that describe the new variables with an example |
If you think it's ready for review, I'd say you can go ahead and move it from Draft to "Ready for Review" - though it looks like CI might need a little more love still. As for the SIP, since @sfirke reopened the GitHub issue, I set the SIP itself back to "pre-discussion" state for its official consensus. Did you want to re-kindle that pre-existing SIP, or would it be easier to open a new one? I'm happy to help you carry that SIP through to fruition if you'd like. Feel free to DM me on slack if it's easier. |
SUMMARY
Introduces changes in the front and backend to allow Superset to be deployed under a prefixed-URL path rather than the root of the domain. A new environment variable
BASE_PATH
has been introduced into webpack, alongside the existingASSET_BASE_URL
, to define the prefix.The frontend has been updated to include the prefix in any resource links along with two new helper utilities,
assetUrl
&pathUtils
, to help construct the paths. TheSupersetClient
has been update to remove the unusedbaseUrl
field and rename it asbasePath
such that the object can be initialized with the base path and calling any endpoints through this class can use the same relative link as before. QUESTION: Have I broken an API here?The backend has been refactored to avoid using hardcoded paths in redirects and instead uses
Flask.url_for
to construct reference. This requires that the proxy server set theX-Forwarded-Prefix
header along with settingENABLE_PROXY_FIX=True
insuperset_config.py
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION