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 wip role for superset #140

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

add wip role for superset #140

wants to merge 15 commits into from

Conversation

landryb
Copy link
Member

@landryb landryb commented Dec 27, 2024

cf georchestra/improvement-proposals#10, still wip/untested

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

@jeanpommier moving the technical aspects of the integration here, instead of the GIP..

so far, to get the feature from the PR, my idea is to:

@jeanpommier
Copy link
Member

You mean you want to make ansible do the build ?

Alternatively, we could build the frontend with a designated path prefix and serve it (the built files) from a git repo for instance.

I don't know which is prefereable, to build it locally or to centralize the build (which means we would all agree on the path prefix)

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

You mean you want to make ansible do the build ?

Alternatively, we could build the frontend with a designated path prefix and serve it (the built files) from a git repo for instance.

I don't know which is prefereable, to build it locally or to centralize the build (which means we would all agree on the path prefix)

yeah, was pondering this option too..i'm not sure we want to make the prefix user-configurable, so we could settle on /superset or /analytics, build the assets somewhere (from the matching version of the superset source) and download/unpack them on top of the ones shipped by upstream. I'm not a fan of dragging npm to the party, especially given the amount of bloat that hell is.

@jeanpommier
Copy link
Member

Yup, I agree. /analytics is probably a bit too specific, since Superset will probably not be used only for that purpose (at least at geo2france). If we want to use a functional name, rather than an app-related path, I'd rather suggest /dataviz or /dashboards

We'll probably have to make a poll.

It's really a pity they're hardcoding the path in Superset

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

notes for the ones who want to play at home, apparently the requirements to build the frontend are node 20/npm 10:

[30/12 11:06] [email protected]:/data/src/georchestra $git clone https://github.com/apache/superset
[30/12 11:07] [email protected]:/data/src/georchestra $cd superset/
[30/12 11:13] [email protected]:/data/src/georchestra/superset $git remote add martyngigg https://github.com/martyngigg/superset
[30/12 11:14] [email protected]:/data/src/georchestra/superset $git fetch martyngigg
[30/12 11:23] [email protected]:/data/src/georchestra/superset $git checkout allow_prefixed_paths
branch 'allow_prefixed_paths' set up to track 'martyngigg/allow_prefixed_paths'.
Switched to a new branch 'allow_prefixed_paths'
[30/12 11:25] [email protected]:/data/src/georchestra/superset $cd superset-frontend/
[30/12 11:25] [email protected]:/data/src/georchestra/superset/superset-frontend $npm i
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '^20.16.0', npm: '^10.8.1' },
npm WARN EBADENGINE   current: { node: 'v20.11.1', npm: '10.2.4' }
...
[30/12 11:28] [email protected]:/data/src/georchestra/superset/superset-frontend $BASE_PATH=/dashboards npm run build

and after a while the generated assets in ../superset/static/assets/ have a recent timestamp, so i guess those are the result of the process. I'll tar them up and put them somewhere on https://packages.georchestra.org/bot/war/...

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

https://packages.georchestra.org/bot/wars/superset/ has a 40mb archive of assets/ built from the above, untested yet.

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

since we dont have the auth bits from the headers yet, created a local admin this way:

[30/12 13:00] [email protected]:/srv/apps/superset $
    env FLASK_APP=superset SUPERSET_CONFIG_PATH=superset_config.py \
        /srv/apps/superset/venv/bin/superset  fab create-admin
Loaded your LOCAL configuration at [superset_config.py]
2024-12-30 13:01:05,527:INFO:superset.initialization:Setting database isolation level to READ COMMITTED
2024-12-30 13:01:05,637:INFO:superset.utils.screenshots:No PIL installation found
2024-12-30 13:01:06,125:INFO:superset.utils.pdf:No PIL installation found
Username [admin]: 
User first name [admin]: 
User last name [user]: 
Email [[email protected]]: [email protected]
Password: 
Repeat for confirmation: 
Recognized Database Authentications.
Admin User admin created.

so far im still fighting a bit with the redirections/which page should be where, but curl http://localhost:9080/login/ sends me the login page. I still have to figure out the right nginx magic.

from my understanding of the returned html, the /dashboards url prefix is correctly used, eg the login page contains script src="/dashboards/static/assets/1301.a41427b36747cc29b293.entry.js"

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

so far i have something that works (eg renders fine, i can login as admin), but only if superset is right behind nginx, not yet with superset behind the s-p (yes, im using the s-p). probably me messing up smth in the s-p config.

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

i have something that works here but is a bit gross (like what was done for mviewerstudio in #132), eg a specific nginx vhost to put behind the s-p, and in front of gunicorn.

@jeanpommier
Copy link
Member

Wow, I can see you've been active.

a specific nginx vhost to put behind the s-p, and in front of gunicorn.

Yes, that's the only way as of now (if using the above-mentioned PR). PR 30134 is actually not enabling the support for path prefix inside superset, which is still only able to run on the root of the domain, it is merely fixing Superset so that it is possible to properly proxy-pass on a path prefix behind a reverse proxy like for instance nginx.

As of now, this is the only possible option.

To summarize (things as I undertstand them):

  • Superset frontend is able to support a path prefix. But requires a dedicated rebuild for that
  • Superset backend does not support a path prefix, even though it should (Superset is built on top of flask_appbuilder, which does support configurable blueprints/base paths)
  • It is usually advised to use a reverse-proxy for that. But AFAIK, most people were not able to configure it properly, not sure it is even possible if using upstream Superset
  • The PR 30134 fixes Superset so that the reverse-proxy solution works (but you still need a dedicated build of the frontend)

@landryb landryb marked this pull request as ready for review December 30, 2024 14:15
@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

what i have is i think in a testable state. mostly missing content/dashboards specific to the IDS (having something that actually replaces analytics from geoserver ootb would be.. definitely nice), geor-header integration, roles/auth from sec-proxy headers :) @jeanpommier let me know when you have something to test...

@jeanpommier
Copy link
Member

Still a bit early stage ;o)

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

looking a bit, i guess we'll need integrate <geor-header> in the react/typescript hell that's the frontend.. maybe in https://github.com/apache/superset/blob/master/superset-frontend/src/features/home/Menu.tsx . But i'll leave that to frontend experts (poke @f-necas ;)

so in all cases, the frontend needs to be forked/rebuilt for proper geor-header integration.

@landryb
Copy link
Member Author

landryb commented Dec 30, 2024

what's left in terms of integration on my side:

@jeanpommier
Copy link
Member

  • and maybe the ldap integration for roles ? or that'll be superseded by the custom python code for header auth ?

It is. That's the point

@landryb
Copy link
Member Author

landryb commented Dec 31, 2024

i've just deployed this branch on https://demo.georchestra.org/dashboards/superset/welcome/ - same thing, manually created a testadmin user.

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