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

refactor: Allow superset to be deployed under a prefixed URL #30134

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

martyngigg
Copy link
Contributor

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 existing ASSET_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. The SupersetClient has been update to remove the unused baseUrl field and rename it as basePath 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 the X-Forwarded-Prefix header along with setting ENABLE_PROXY_FIX=True in superset_config.py

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas
Copy link
Member

rusackas commented Sep 5, 2024

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.

@martyngigg
Copy link
Contributor Author

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

This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
@mistercrunch
Copy link
Member

mistercrunch commented Sep 19, 2024

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.

@@ -17,19 +17,23 @@
#
set -e

# zstd exe is required by simple-zstd package
apt-get update
apt-get install -y zstd
Copy link
Member

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

@ravikantkml
Copy link

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?

Thanks for these changes.
Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.

@kasper-rutten
Copy link

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.
@github-actions github-actions bot added the doc Namespace | Anything related to documentation label Oct 15, 2024
@martyngigg
Copy link
Contributor Author

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?

Thanks for these changes. Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.

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 nginx.conf. Does that help?

@martyngigg
Copy link
Contributor Author

@rusackas I think this is ready for review but in terms of it being related to SIP-112 is that the right thing to do in terms of the process?

@rusackas
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API doc Namespace | Anything related to documentation packages review:draft size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants