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.

Sign up for GitHub

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

Jump to bottom

feat: helm deploys with a password #9113

Merged
merged 2 commits into from
Apr 8, 2024
Merged

feat: helm deploys with a password #9113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9034665
fix: helm prefers initialUserPassword over defaultPassword, never mak…
jesse-amano-hpe Apr 5, 2024
bbc5034
docs: use PR suggestions
jesse-amano-hpe Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/reference/deploy/helm-config-reference.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,12 @@
automatically terminated. A TensorBoard instance is considered to be idle if it does not receive
any HTTP traffic. The default timeout is 300 (5 minutes).

- ``defaultPassword``: Specifies a string containing the default password for the admin and
- ``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. Use ``initialUserPassword`` instead.

- ``logging``: Configures where trial logs are stored. This section takes the same shape as the
logging configuration in the :ref:`cluster configuration <cluster-configuration>`, except that
names are changed to camel case to match Helm conventions (e.g., ``skip_verify`` would be
Expand Down
9 changes: 9 additions & 0 deletions docs/release-notes/helm-requires-user-password.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
:orphan:

**Security Fixes**

- 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 either
initialUserPassword (preferred) or defaultPassword (deprecated). For reference, visit
:ref:helm-config-reference.
107 changes: 0 additions & 107 deletions helm/charts/determined/scripts/k8s-password-change.py
View file
Open in desktop

This file was deleted.

44 changes: 0 additions & 44 deletions helm/charts/determined/templates/change-password.yaml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion helm/charts/determined/templates/master-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Contributor

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

Copy link
Contributor Author

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:

  1. Launch a cluster with empty admin password
  2. Wait a bit for the helm postInstall hook to fire
  3. 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:

  1. Launch a cluster; if .Values.initialUserPassword is set, use that for admin/determined users, otherwise allow empty passwords
  2. Wait a bit for the helm postInstall hook to fire
  3. 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:

  1. 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. 😄

{{- if .Values.tlsSecret }}
tls:
Expand Down
Loading