-
Notifications
You must be signed in to change notification settings - Fork 356
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: helm deploys with a password #9113
Conversation
✅ Deploy Preview for determined-ui canceled.
|
0098fd0
to
9034665
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9113 +/- ##
=======================================
Coverage 46.63% 46.64%
=======================================
Files 1172 1172
Lines 143619 143619
Branches 2410 2410
=======================================
+ Hits 66983 66986 +3
+ Misses 76431 76428 -3
Partials 205 205
Flags with carried forward coverage won't be shown. Click here to find out more. |
- ``defaultPassword``: Specifies a string containing the default password for the admin and | ||
determined user accounts. | ||
determined user accounts. (Deprecated; prefer ``initialUserPassword``) |
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.
Does this mean the "defaultPassword" setting should not be used? i.e.,
-
initialUserPassword
: Specifies a string containing the default password for the admin and
determined user accounts. -
defaultPassword
: (Deprecated) Specifies a string containing the default password for the admin and
determined user accounts. UseinitialUserPassword
instead.
- Helm: When deploying a new cluster with Helm, an initial password for the ``admin`` and | ||
``determined`` users should be specified using either ``initialUserPassword`` or | ||
``defaultPassword`` (see helm/charts/determined/values.yaml). Configuring a password is no | ||
longer done as a separate step. |
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.
:orphan:
Security
- Helm: When deploying a new cluster with Helm, configuring an initial password for the "admin" and "determined" users is required and is no longer a separate step. To specify an initial password for these users, visit the
helm/charts/determined/values.yaml
file and use eitherinitialUserPassword
(preferred) or
defaultPassword
(deprecated). For reference, visit :ref:helm-config-reference
.
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.
Thanks!
@@ -57,7 +57,7 @@ stringData: | |||
|
|||
security: | |||
{{- if .Values.initialUserPassword }} | |||
initial_user_password: {{ .Values.initialUserPassword | quote }} | |||
initial_user_password: {{ coalesce .Values.initialUserPassword .Values.defaultPassword | quote }} | |||
{{- end }} |
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.
please check if anything has changed here:
https://docs.determined.ai/latest/reference/deploy/master-config-reference.html#initial-user-password
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.
I don't think anything should change there -- that's from #8851 which describes the structure that this configuration file is using. In Helm's case, there was previous support for a .Values.defaultPassword
that would launch a sidecar. Before that, the flow for configuring the admin password was something like:
- Launch a cluster with empty admin password
- Wait a bit for the helm
postInstall
hook to fire - If
defaultPassword
is set,
a. authorize as admin using empty password
b. change the admin and determined passwords to.defaultPassword
After #8851 and #9074 , the flow is:
- Launch a cluster; if
.Values.initialUserPassword
is set, use that for admin/determined users, otherwise allow empty passwords - Wait a bit for the helm
postInstall
hook to fire - If
defaultPassword
is set,
a. authorize as admin using no password (fails if.Values.initialUserPassword
was also set, since admin has a non-empty password!)
b. try to change the admin and determined passwords to.defaultPassword
By changing this line and deleting the sidecar, the intention (which I've manually tested, but probably not exhaustively!) is that the new flow is:
- Launch a cluster; if
.Values.initialUserPassword
is set, use that for admin/determined users; otherwise use.Values.defaultPassword
; if both are blank, an attempt is made with a blank password (which will eventually fail because of all the other changes we're making around this).
I hope this is helpful/informative and not just over-explaining things you already knew! If there's something about this that needs to be communicated and isn't already handled in your #9101 I could use some help writing copy in the style most consistent with Determined docs. 😄
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.
left comments and suggestions
Description
DET-10208 Helm deploys with the
initialUserPassword
ordefaultPassword
Test Plan
Helm deployment succeeds with
initialUserPassword
uncommented and set in values.yamlCommentary
#9074 ended up taking care of a lot of the work needed for this ticket, this mostly documents the behavior in helm and makes it so we never launch a sidecar with
defaultPassword
(which, ifinitialUserPassword
is also set, would error out anyway because its assumption that it can use an empty admin password would be incorrect).Checklist
docs/release-notes/
.See Release Note for details.
Ticket
DET-10208