feat(helm): allow init admin credentials from existing Kubernetes secret - #42753
feat(helm): allow init admin credentials from existing Kubernetes secret#42753dominicl-proxora wants to merge 4 commits into
Conversation
dosubot
Bot
added
deploy:helm
install:config
Code Review Agent Run #824eddActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
34efc13 to
2b24185
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| {{- if and .Values.init.createAdmin (not (or $adminPasswordSet $adminSecretSet)) }} | ||
| {{- fail "SECURITY ERROR: init.createAdmin is true but neither init.adminUser.password nor init.adminUser.existingSecret is set. You must set a secure password using --set init.adminUser.password='your-password' or provide an existing Kubernetes secret via init.adminUser.existingSecret." }} | ||
| {{- end }} |
There was a problem hiding this comment.
Suggestion: The validation runs whenever superset.config is included, including deployments and the config Secret, but it does not check init.enabled. Consequently, configurations that disable the init Job while leaving createAdmin enabled and clearing the unused password will fail Helm rendering even though no admin creation will run. Gate this validation on init.enabled as well. [api mismatch]
Severity Level: Major ⚠️
- ❌ Disabled init deployments fail Helm rendering unnecessarily.
- ⚠️ Config and deployment manifests cannot be generated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 213:215
**Comment:**
*Api Mismatch: The validation runs whenever `superset.config` is included, including deployments and the config Secret, but it does not check `init.enabled`. Consequently, configurations that disable the init Job while leaving `createAdmin` enabled and clearing the unused password will fail Helm rendering even though no admin creation will run. Gate this validation on `init.enabled` as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ADMIN_USERNAME={{ .Values.init.adminUser.username | quote }} | ||
| ADMIN_FIRSTNAME={{ .Values.init.adminUser.firstname | quote }} | ||
| ADMIN_LASTNAME={{ .Values.init.adminUser.lastname | quote }} | ||
| ADMIN_EMAIL={{ .Values.init.adminUser.email | quote }} | ||
| ADMIN_PASSWORD={{ .Values.init.adminUser.password | quote }} |
There was a problem hiding this comment.
Suggestion: The literal values are rendered inside shell double quotes using Helm's quote, which is not shell escaping. Values containing $ or backticks can be expanded or command-substituted when the init script runs, and embedded shell-sensitive content can alter or break the assignments. Render these values with shell-safe single-quote escaping or pass them through environment variables instead. [security]
Severity Level: Critical 🚨
- ❌ Malicious credential values execute commands in init containers.
- ⚠️ Init pod service-account and mounted-secret access may be exposed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 713:717
**Comment:**
*Security: The literal values are rendered inside shell double quotes using Helm's `quote`, which is not shell escaping. Values containing `$` or backticks can be expanded or command-substituted when the init script runs, and embedded shell-sensitive content can alter or break the assignments. Render these values with shell-safe single-quote escaping or pass them through environment variables instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if superset fab list-users 2>/dev/null | grep -qF 'username:'"${ADMIN_USERNAME}"; then | ||
| echo "Admin user already exists, skipping." | ||
| else | ||
| superset fab create-admin \ | ||
| --username {{ .Values.init.adminUser.username | squote }} \ | ||
| --firstname {{ .Values.init.adminUser.firstname | squote }} \ | ||
| --lastname {{ .Values.init.adminUser.lastname | squote }} \ | ||
| --email {{ .Values.init.adminUser.email | squote }} \ | ||
| --password {{ .Values.init.adminUser.password | squote }} | ||
| --username "${ADMIN_USERNAME}" \ | ||
| --firstname "${ADMIN_FIRSTNAME}" \ | ||
| --lastname "${ADMIN_LASTNAME}" \ | ||
| --email "${ADMIN_EMAIL}" \ | ||
| --password "${ADMIN_PASSWORD}" | ||
| fi |
There was a problem hiding this comment.
Suggestion: With /bin/sh, the pipeline status is the status of grep, not superset fab list-users. A database or CLI failure from list-users is therefore treated as a non-match and the script proceeds to create-admin, while the original error is suppressed by 2>/dev/null. This can produce a misleading creation error or create an account despite an unavailable metadata database. Preserve and fail on the list-users error before attempting creation. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Database/listing failures are misreported during initialization.
- ❌ Init Jobs may attempt account creation after failed discovery.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 719:728
**Comment:**
*Possible Bug: With `/bin/sh`, the pipeline status is the status of `grep`, not `superset fab list-users`. A database or CLI failure from `list-users` is therefore treated as a non-match and the script proceeds to `create-admin`, while the original error is suppressed by `2>/dev/null`. This can produce a misleading creation error or create an account despite an unavailable metadata database. Preserve and fail on the `list-users` error before attempting creation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
list-users | grep existed before
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
- fix init.enabled check - use squote
1f99a74 to
656eebb
Compare
Code Review Agent Run #bc27b3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| # instead of from the values above. The secret keys are configurable via `secretKeys`. | ||
| existingSecret: "" | ||
| # -- Keys inside the existing secret that hold admin user fields. | ||
| secretKeys: |
There was a problem hiding this comment.
IMO, the existingSecret above can be nested into secretKeys for better org. existingSecret sounds generic enough to conflict with other future possible changes.
There was a problem hiding this comment.
Also needs to fix the failed pre-commit CI by running helm docs on the helm/superset dir
2 participants
SUMMARY
This change adds support for sourcing Superset init admin credentials from an existing Kubernetes Secret.
Previously, admin credentials for
init.createAdminwere expected from literalinit.adminUser.*values (especiallyinit.adminUser.password).With this change, users can set
init.adminUser.existingSecretand map key names viainit.adminUser.secretKeysto provide:Key implementation details:
init-job.yamlnow injects admin env vars fromsecretKeyRefwhenexistingSecretis set.superset_init.shgeneration in_helpers.tplnow uses env-based admin values when secret mode is enabled, otherwise keeps literal values behavior.init.createAdmin=true, eitherinit.adminUser.passwordorinit.adminUser.existingSecretmust be set.README.md,UPGRADING.md,values.yaml,values.schema.json).This enables setups where admin credentials are automatically provisioned (for example with randomly generated passwords) without storing them directly in
values.yaml.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (Helm chart/template behavior only).
TESTING INSTRUCTIONS
helm template ...superset_init.shcontainsADMIN_USERNAME="admin"(or configured value).and verify render succeeds.
example for corresponding secret:
enventries withsecretKeyReffor:SUPERSET_ADMIN_USERNAMESUPERSET_ADMIN_FIRSTNAMESUPERSET_ADMIN_LASTNAMESUPERSET_ADMIN_EMAILSUPERSET_ADMIN_PASSWORDhelm unittest helm/supersethelm/superset/tests/config_test.yamlhelm/superset/tests/initscript_test.yamlADDITIONAL INFORMATION