-
Notifications
You must be signed in to change notification settings - Fork 502
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
Conversation
/run-e2e-in-kind |
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.
Where are the certs generated?
@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 |
The basic process is working with this PR with some limitations:
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. |
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.
LGTM
/run-e2e-in-kind |
secret: | ||
defaultMode: 420 | ||
secretName: client-tls | ||
{{- 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.
IIRC, have you changed the way the certs was injected, by injecting certs in the program instead of mount certs into pod?
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.
Yes, it's been changed in #782
- name: tls | ||
secret: | ||
defaultMode: 420 | ||
secretName: client-tls |
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.
Do we need to create the client-tls
secret manually?
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.
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.
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.
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) |
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.
Some problem with func NewDefaultTiDBControl
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.
The logic of NewPDClient
is changed in #782 and does not have this issue anymore
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.
But func NewDefaultTiDBControl
still has this problem
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.
NewDefaultTiDBControl
has been changed in the latest commit 7d9d03b
/run-e2e-test |
/run-e2e-test |
/run-e2e-in-kind |
Signed-off-by: Ran <[email protected]>
What problem does this PR solve?
Add basic TLS support for TiDB cluster.
When creating new TiDB cluster, set
enableTLSServer
totrue
will enable the TLS encryption feature in PD/TiDB/TiKV.Setting
enableTLSClient
totrue
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 namedclient-tls
, another client cert is placed in TiDB Operator's namespace and namedclient-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
Manually validated on internal testing k8s cluster.
Known Issues
enableTLSServer
on/off after creation of clusterCode changes
Related changes
Does this PR introduce a user-facing change?: