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

Set password for TiDB when first deployed #171

Merged
merged 6 commits into from
Nov 8, 2018
Merged

Conversation

tennix
Copy link
Member

@tennix tennix commented Nov 7, 2018

This PR closes #167. It uses a BatchJob to set TiDB password when first deployed. When the job is completed, users can use the password to access TiDB.

type: Opaque
data:
{{- if .Values.tidb.password }}
password: {{ printf "SET PASSWORD FOR 'root'@'%%' = '%s' ; FLUSH PRIVILEGES;" .Values.tidb.password | b64enc }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would much prefer to have just the password as the secret.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see the reasons why I store the sql command here instead of only password.
https://github.com/pingcap/tidb-operator/pull/171/files#diff-7b1a419604c0fcc0219f5b547c56eb40R27

Copy link
Contributor

Choose a reason for hiding this comment

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

The non-secret part could be a ConfigMap then. Encrypting the SQL means that editing this SQL requires accessing the password.

Copy link
Member Author

Choose a reason for hiding this comment

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

For other SQL initialization setups, I think we can store them in another ConfigMap instead of storing them in this Secret.

@gregwebs
Copy link
Contributor

gregwebs commented Nov 7, 2018

If we take the existing password as input then this can also function as a general password reset (particularly if the user that the password is set for is also a variable)

@tennix
Copy link
Member Author

tennix commented Nov 7, 2018

If users remember the current password, then sure it works. But sometimes they just forget the current password, so this method won't work.

@gregwebs
Copy link
Contributor

gregwebs commented Nov 7, 2018

If users remember the current password, then sure it works. But sometimes they just forget the current password, so this method won't work.

Okay, this does seem like a different workflow. There are other considerations also: ideally we would have the network (service) turned off until the password is set. In that line of thought I am actually wondering if this should be ran as an init container (that will wait indefinitely for TiDB to be ready) to ensure that the password change job is scheduled.

@gregwebs
Copy link
Contributor

gregwebs commented Nov 7, 2018

sorry, I guess initContainer doesn't make sense. But probably the operator should ensure that this runs.

@tennix
Copy link
Member Author

tennix commented Nov 8, 2018

This is a one-time job, the chart will serve us well for this task. We don't have to embed this into tidb-operator Go code.

@tennix tennix changed the title [DNM] set password for TiDB when first deployed Set password for TiDB when first deployed Nov 8, 2018
@gregwebs
Copy link
Contributor

gregwebs commented Nov 8, 2018

One more thought on doing this the secure way: add the concept of a "ready" label that the service uses as a label selector. That label is added when this job is complete so that TiDB would not be available as an endpoint until after

@tennix
Copy link
Member Author

tennix commented Nov 8, 2018

The password setter itself uses TiDB service, so we can't handle it this way unless we use TiDB pod IP, but this is unstable.

@tennix
Copy link
Member Author

tennix commented Nov 8, 2018

/run-e2e-tests

@tennix
Copy link
Member Author

tennix commented Nov 8, 2018

/run-e2e-tests

@tennix
Copy link
Member Author

tennix commented Nov 8, 2018

/run-e2e-tests

@onlymellb
Copy link
Contributor

LGTM

@tennix tennix merged commit 918a13e into pingcap:master Nov 8, 2018
@tennix tennix deleted the password branch November 8, 2018 12:57
queenliuxx pushed a commit to queenliuxx/tidb-operator that referenced this pull request Dec 19, 2018
* set password for TiDB when first deployed

* Fix tidb password setting and update docs

* use admin as e2e test password

* decrease a e2e test cluster due to CI machine resource limits
yahonda pushed a commit that referenced this pull request Dec 27, 2021
* update webhook tls

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: DanielZhangQD <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: DanielZhangQD <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: DanielZhangQD <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* address the comment

* address the comment

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: DanielZhangQD <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: DanielZhangQD <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* address the comment

* revise the code

* add conditions

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

* Update zh/enable-admission-webhook.md

Co-Authored-By: Ran <[email protected]>

Co-authored-by: Ran <[email protected]>
Co-authored-by: DanielZhangQD <[email protected]>
fgksgf added a commit to fgksgf/tidb-operator that referenced this pull request Dec 23, 2024
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.

Set TiDB password when creating TiDB cluster
4 participants