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

Auto generate and sign certificates for TLS enabled cluster #782

Merged
merged 70 commits into from
Nov 15, 2019
Merged

Auto generate and sign certificates for TLS enabled cluster #782

merged 70 commits into from
Nov 15, 2019

Conversation

AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented Aug 16, 2019 •
edited by tennix
Loading

What problem does this PR solve?

Automatically generate and sign certificates for TLS enabled TiDB clusters, if:

  • No Secret matching the name and labels, and
  • No CertificateSigningRequest matching the name that not matching the labels (so that they are not created by us)

After certificates get automatically approved, they are stored as Secrets and mount to Pods.

The procedure is based on the manual for ansible deployment.

The corresponding documents are here:

What is changed and how does it work?

This PR includes and close #750 and added controllers to handle certificate managements.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    Manually validated on internal testing k8s cluster.

Code changes

  • Has Helm charts change
  • Has Go code change

Known Issues

  • pd#1682 (fixed)
    • Only support PD version >= v3.0.5
  • TiKV and TiDB/PD have different behavior on metrics endpoints, TiKV does not require client certs so the monitoring metrics of TiKV are collected via plain HTTP as a workaround
  • The certificates are signed with 1 year expiration time, but due to the limit of servers, there is no clean way (yet) to renew the certs without stopping the TiDB cluster

Related changes

#750

Does this PR introduce a user-facing change?:

Enable auto generate certificates for TiDB cluster

@AstroProfundis AstroProfundis added the enhancement New feature or request label Aug 16, 2019
@AstroProfundis AstroProfundis self-assigned this Aug 16, 2019
@gregwebs
Copy link
Contributor

It can be problematic to automatically create the cert if the secret is not found: the deployer may have forgotten that step. It may be better to more explicitly configure the cert source.

@AstroProfundis
Copy link
Contributor Author

@gregwebs We can check if a csr already exist and try to approve that, but we don't have private key in that situation, I haven't came up with any idea good enough to check if user had forgotten to create secret (or mis-configured the secret), or there is no pre-confiugred key pair at all. Any suggestions?

BTW, I think our current procedure don't support user generated certs, supporting custom cert would need to support external CA as well, this won't work with this PR at present.

@gregwebs
Copy link
Contributor

The approach would be to explicitly configure whether a secret is expected. If it is expected and does not exist, than that produces an error.

@AstroProfundis
Copy link
Contributor Author

/run-e2e-in-kind

@Yisaer Yisaer mentioned this pull request Nov 11, 2019
Closed
@AstroProfundis
Copy link
Contributor Author

/run-e2e-in-kind

@tennix
Copy link
Member

tennix commented Nov 13, 2019

The make tidy fails in GitHub Action CI, please fix it @AstroProfundis

tennix
tennix reviewed Nov 13, 2019
View reviewed changes
charts/tidb-cluster/templates/config/_prometheus-config.tpl Show resolved Hide resolved
charts/tidb-cluster/templates/config/_drainer-config.tpl Outdated
@@ -11,7 +11,7 @@ detect-interval = {{ .Values.binlog.drainer.detectInterval | default 10 }}
data-dir = "/data"

# a comma separated list of PD endpoints
pd-urls = "http://{{ template "cluster.name" . }}-pd:2379"
pd-urls = "{{ if .Values.enableTLSCluster }}https{{ else }}http{{ end }}://{{ template "cluster.name" . }}-pd:2379"
Copy link
Member

Choose a reason for hiding this comment

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

You can define a SCHEME macro in _helpers.tpl to avoid repeating the if-else block again and again.

charts/tidb-cluster/templates/monitor-deployment.yaml Show resolved Hide resolved
tennix
tennix approved these changes Nov 13, 2019
View reviewed changes
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix
Copy link
Member

tennix commented Nov 13, 2019

/run-e2e-in-kind

1 similar comment
@tennix
Copy link
Member

tennix commented Nov 13, 2019

/run-e2e-in-kind

@pingcap pingcap deleted a comment from tennix Nov 14, 2019
@tennix
Copy link
Member

tennix commented Nov 14, 2019

/run-e2e-in-kind

@tennix
Copy link
Member

tennix commented Nov 15, 2019

/run-e2e-in-kind

@tennix tennix changed the title Automatically generate and sign certificates for TLS enabled clusters Auto generate and sign certificates for TLS enabled cluster Nov 15, 2019
@tennix tennix merged commit 7425285 into pingcap:master Nov 15, 2019
@tennix tennix mentioned this pull request Nov 15, 2019
Closed
@AstroProfundis AstroProfundis deleted the tls-auto-sign branch November 27, 2019 08:57
yahonda pushed a commit that referenced this pull request Dec 27, 2021
@Joyinqin
en: Doc update for monitoring multiple TiDB clusters with one TidbMon…
43ab7da 
…itor (#782)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregwebs gregwebs gregwebs left review comments

@onlymellb onlymellb onlymellb left review comments

@weekface weekface weekface approved these changes

@tennix tennix tennix approved these changes

@cofyc cofyc Awaiting requested review from cofyc

@LinuxGit LinuxGit Awaiting requested review from LinuxGit

Assignees

@AstroProfundis AstroProfundis

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AstroProfundis @gregwebs @tennix @weekface @onlymellb @cofyc