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

feat: Custom superset_config.py + secret envs #13096

Merged
merged 1 commit into from
Feb 13, 2021
Merged

feat: Custom superset_config.py + secret envs #13096

merged 1 commit into from
Feb 13, 2021

Conversation

Yann-J
Copy link
Contributor

@Yann-J Yann-J commented Feb 12, 2021

SUMMARY

This update to the Helm chart provides support for:

  • Specifying custom variables to be passed to the pods as secrets
  • Specifying extra conf snippets to append to superset_config.py

TEST PLAN

  • Deploy updated chart passing custom values for:
    • extraSecretEnv
    • configOverrides
helm upgrade --install --values my-values.yaml superset helm/superset

Sample values.yaml:

additionalRequirements:
  - Authlib

extraSecretEnv:
  MAPBOX_API_KEY: ...
  GOOGLE_KEY: ...
  GOOGLE_SECRET: ...

configOverrides:
  # requires Authlib
  enable_oauth: |
    from flask_appbuilder.security.manager import AUTH_OAUTH
    AUTH_TYPE = AUTH_OAUTH

    OAUTH_PROVIDERS = [
        {
            "name": "google",
            "icon": "fa-google",
            "token_key": "access_token",
            "remote_app": {
                "client_id": os.environ.get("GOOGLE_KEY"),
                "client_secret": os.environ.get("GOOGLE_SECRET"),
                "api_base_url": "https://www.googleapis.com/oauth2/v2/",
                "client_kwargs": {"scope": "email profile"},
                "request_token_url": None,
                "access_token_url": "https://accounts.google.com/o/oauth2/token",
                "authorize_url": "https://accounts.google.com/o/oauth2/auth",
            },
        }
    ]

    # Will allow user self registration, allowing to create Flask users from Authorized User
    AUTH_USER_REGISTRATION = True

    # The default user self registration role
    AUTH_USER_REGISTRATION_ROLE = "Public"

### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a major upgrade to the flexibility of managing multiple supersets at scale
left one comment

Comment on lines +68 to +85
# from flask_appbuilder.security.manager import AUTH_DB
# AUTH_TYPE = AUTH_OAUTH

# OAUTH_PROVIDERS = [
# {
# "name": "google",
# "icon": "fa-google",
# "token_key": "access_token",
# "remote_app": {
# "client_id": os.environ.get("GOOGLE_KEY"),
# "client_secret": os.environ.get("GOOGLE_SECRET"),
# "api_base_url": "https://www.googleapis.com/oauth2/v2/",
# "client_kwargs": {"scope": "email profile"},
# "request_token_url": None,
# "access_token_url": "https://accounts.google.com/o/oauth2/token",
# "authorize_url": "https://accounts.google.com/o/oauth2/auth",
# },
# }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be injected from a separate file called enable_oauth_config.py?
by that, we can upgrade configOverrides solution with multiple config modules that can be independent separately and can be mixed with other modules seamlessly

we could have a config_modules folder from which all different config will be loaded from

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is indeed probably a better approach... I went for appending to the existing file because that was the easiest for me, as I'm not 100% confident with my python / FAB skills and didn't want to break things...

I see that /app/pythonpath already had other files:

superset_bootstrap.sh
superset_config.py
superset_init.sh

But I suppose it's safe to simply create those separate config modules in the same directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just realize you meant loading from a separate file and not into a separate file... :)

I'm not sure if there's a really convenient way to do that in the chart definition itself... it IS possible to read files but I seem to remember this comes with a few limitations...

At install time however, I think something like helm upgrade --set-file configOverrides.enable_oauth_config=oauth.py would work - but AFAIK not with a full directory...

@craig-rueda craig-rueda merged commit 6b615c4 into apache:master Feb 13, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 14, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 14, 2021
* master: (30 commits)
  refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021)
  fix(native-filters): set currentValue null when empty (apache#13000)
  Custom superset_config.py + secret envs (apache#13096)
  Update http error code from 400 to 403 (apache#13061)
  feat(native-filters): add storybook entry for select filter (apache#13005)
  feat(native-filters): Time native filter (apache#12992)
  Force pod restart on config changes (apache#13056)
  feat(cross-filters): add cross filters (apache#12662)
  fix(explore): Enable selecting an option not included in suggestions (apache#13029)
  Improves RTL configuration (apache#13079)
  Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083)
  chore: lock down npm to v6 (apache#13069)
  fix: API tests, make them possible to run independently again (apache#13076)
  fix: add config to disable dataset ownership on the old api (apache#13051)
  add required * indicator to message content/notif method (apache#12931)
  fix: Retroactively add granularity param to charts (apache#12960)
  fix(ci): multiline regex in change detection (apache#13075)
  feat(style): hide dashboard header by url parameter (apache#12918)
  fix(explore): pie chart label bugs (apache#13052)
  fix: Disabled state button transition time (apache#13008)
  ...
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants