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

Add basic TLS support for TiDB cluster #750

Merged
merged 26 commits into from
Sep 4, 2019

Conversation

AstroProfundis
Copy link
Contributor

What problem does this PR solve?

Add basic TLS support for TiDB cluster.

When creating new TiDB cluster, set enableTLSServer to true will enable the TLS encryption feature in PD/TiDB/TiKV.

Setting enableTLSClient to true will enable support of TLS encrypted connection from MySQL client.

What is changed and how does it work?

Server certs are placed in Secrets with the same namespace as the cluster, and named <release-name>-{pd, tidb, tikv}, client certs are placed in Secrets with the same namespace as the cluster, and named client-tls, another client cert is placed in TiDB Operator's namespace and named client-tls.

All certs are signed by the CA of the Kubernetes cluster.

All client requests (from operator) are modified to support TLS encrypted connections.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Manually validated on internal testing k8s cluster.

Known Issues

  • Does not support of switching enableTLSServer on/off after creation of cluster
  • gRPC uses unencrypted connections between multiple PDs, this might be an issue of cert generation and is still under investigation

Code changes

  • Has Helm charts change
  • Has Go code change

Related changes

  • Need to update the documentation after all works are done

Does this PR introduce a user-facing change?:

Add basic support of TLS encrypted connections in TiDB cluster

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

weekface commented Aug 9, 2019

/run-e2e-in-kind

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@gregwebs gregwebs left a comment

Choose a reason for hiding this comment

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

Where are the certs generated?

charts/tidb-cluster/templates/scripts/_start_pd.sh.tpl Outdated Show resolved Hide resolved
@gregwebs gregwebs mentioned this pull request Aug 9, 2019
@AstroProfundis
Copy link
Contributor Author

AstroProfundis commented Aug 12, 2019

@gregwebs As of this PR, certs are generated manually, I'm working on an auto-generating feature for necessary certs

@cofyc It doesn't matter, PD accepts any valid certs that signed by the same CA as server (the k8s CA in this case), so controller can use one client cert for all TiDB clusters

@gregwebs
Copy link
Contributor

@AstroProfundis cert-manager is widely used for certificate generation and management and is working well for me now: https://docs.cert-manager.io/en/latest/tasks/issuers/index.html

@AstroProfundis AstroProfundis changed the title [WIP] add basic TLS support for TiDB cluster Add basic TLS support for TiDB cluster Aug 14, 2019
@AstroProfundis
Copy link
Contributor Author

@tennix @cofyc @weekface PTAL

The basic process is working with this PR with some limitations:

  • Secrets that store the certs need to be manually added (and certs are manually issued with k8s' certificate management facility as well)
  • Only 1 PD replica is working at present, no limitations for TiDB and TiKV

Automatically management of certs should be added in another PR (still WIP), and the known multi-PD connectivity issue is still under investigation, not likely to be fixed in this PR.

tennix
tennix previously approved these changes Aug 23, 2019
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 Aug 23, 2019

/run-e2e-in-kind

secret:
defaultMode: 420
secretName: client-tls
{{- end -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, have you changed the way the certs was injected, by injecting certs in the program instead of mount certs into pod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's been changed in #782

- name: tls
secret:
defaultMode: 420
secretName: client-tls
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to create the client-tls secret manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of this PR, Yes, and the cert need to be signed by the k8s CA by kubectl certificate approve, as described in the PR OP.

PR #782 implements the automatic process of generating certs.

Copy link
Contributor

Choose a reason for hiding this comment

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

make this optional, otherwise, it will fail when users don't need to enable TLS

if tlsEnabled {
rootCAs, cert, err := httputil.ReadCerts()
if err != nil {
glog.Errorf("fail to load certs, fallback to plain connection, err: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Some problem with func NewDefaultTiDBControl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic of NewPDClient is changed in #782 and does not have this issue anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

But func NewDefaultTiDBControl still has this problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewDefaultTiDBControl has been changed in the latest commit 7d9d03b

@cofyc
Copy link
Contributor

cofyc commented Sep 2, 2019

/run-e2e-test

cofyc
cofyc previously approved these changes Sep 2, 2019
@cofyc
Copy link
Contributor

cofyc commented Sep 2, 2019

/run-e2e-test

@weekface
Copy link
Contributor

weekface commented Sep 3, 2019

/run-e2e-in-kind

@tennix tennix merged commit c314e94 into pingcap:master Sep 4, 2019
@cofyc cofyc mentioned this pull request Feb 25, 2020
yahonda pushed a commit that referenced this pull request Dec 27, 2021
Signed-off-by: Ran <huangran@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants