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

Support user-defined tidb server/client certificate #1714

Merged
merged 11 commits into from
Feb 20, 2020
Merged

Support user-defined tidb server/client certificate #1714

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3715538
support user custom certificate
weekface Feb 17, 2020
e08a42a
refine API
weekface Feb 17, 2020
aea638f
fix typo
weekface Feb 17, 2020
5920eab
fix some bugs
weekface Feb 18, 2020
30a65bd
create service before certificate
weekface Feb 18, 2020
da831be
tiny fix
weekface Feb 18, 2020
cf8dc50
address comments
weekface Feb 18, 2020
6cfaa45
address comment
weekface Feb 18, 2020
20c4743
address comment
weekface Feb 18, 2020
1b03947
Merge branch 'master' into custom-tls
Yisaer Feb 19, 2020
88acf67
Merge branch 'master' into custom-tls
Yisaer Feb 20, 2020
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
8 changes: 6 additions & 2 deletions charts/tidb-cluster/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,20 @@ config-file: |-
{{- if .Values.tidb.config }}
{{ .Values.tidb.config | indent 2 }}
{{- end -}}
{{- if or .Values.enableTLSCluster .Values.tidb.enableTLSClient }}
{{- if or .Values.enableTLSCluster .Values.tidb.tlsClient.enabled }}
[security]
{{- end -}}
{{- if .Values.enableTLSCluster }}
cluster-ssl-ca = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
cluster-ssl-cert = "/var/lib/tidb-tls/cert"
cluster-ssl-key = "/var/lib/tidb-tls/key"
{{- end -}}
{{- if .Values.tidb.enableTLSClient }}
{{- if .Values.tidb.tlsClient.enabled }}
{{- if .Values.tidb.tlsClient.userGenerated.secretName }}
ssl-ca = "/var/lib/tidb-server-tls/ca"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use user-defined ca file.

{{- else }}
ssl-ca = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
{{- end }}
ssl-cert = "/var/lib/tidb-server-tls/cert"
ssl-key = "/var/lib/tidb-server-tls/key"
{{- end -}}
Expand Down
15 changes: 3 additions & 12 deletions charts/tidb-cluster/templates/tidb-cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,9 @@ spec:
{{- end }}
maxFailoverCount: {{ .Values.tikv.maxFailoverCount | default 3 }}
tidb:
enableTLSClient: {{ .Values.tidb.enableTLSClient | default false }}
{{- if .Values.tidb.extraSANIPList }}
extraSANIPList:
{{- range .Values.tidb.extraSANIPList }}
- {{ . }}
{{- end }}
{{- end }}
{{- if .Values.tidb.extraSANDomainList }}
extraSANDomainList:
{{- range .Values.tidb.extraSANDomainList }}
- {{ . }}
{{- end }}
{{- if .Values.tidb.tlsClient }}
tlsClient:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use the same struct with values.yaml

{{ toYaml .Values.tidb.tlsClient | indent 6 }}
{{- end }}
replicas: {{ .Values.tidb.replicas }}
image: {{ .Values.tidb.image }}
Expand Down
29 changes: 18 additions & 11 deletions charts/tidb-cluster/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,18 +428,25 @@ tidb:
list: ["whitelist-1"]

# Whether enable TLS connection between TiDB server and MySQL client.
# When enabled, TiDB will accept TLS encrypted connections from MySQL client, certificates will be generated
# automatically.
# Note: TLS connection is not forced on the server side, plain connections are also accepted after enableing.
enableTLSClient: false
# # extra SAN IP list when you set tidb.enableTLSClient to true
# extraSANIPList:
# - 1.1.1.1
# - 2.2.2.2
# # extra SAN Domain List when you set tidb.enableTLSClient to true
# extraSANDomainList:
# - example1.com
# - example2.com
tlsClient:
# When enabled, TiDB will accept TLS encrypted connections from MySQL client
enabled: false
# Auto-generated certificate
Copy link
Member

Choose a reason for hiding this comment

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

This is generated in k8s, you can add a document link here.

autoGenerated:
# # extra SAN IP list
# extraSANIPList:
# - 1.1.1.1
# - 2.2.2.2
# # extra SAN Domain list
# extraSANDomainList:
# - example1.com
# - example2.com
# User-generated certificate
userGenerated:
# # secretName is the name of the secret which stores user-defined tidb server certificate, key and ca, create it with:
# # kubectl create secret generic <secret-name> --namespace=<namespace> --from-file=cert=<tidb server certificate file path> --from-file=key=<tidb server key file path> --from-file=ca=<ca file path>
# secretName: "demo-tidb-server-secret"
Copy link
Contributor

@cofyc cofyc Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can simplify this a bit, like ingress tls config:

  tls:
  - hosts:
    - sslexample.foo.com
    secretName: testsecret-tls

the difference is we can specify how to issue a certificate automatically (if the secret name is not specified) at the same place.

tlsClient:
    enabled: false
    # secretName is the name of the secret that stores user-defined tidb server certificate, key and ca...
    # if not specified but tls client is enabled, certificated signed by k8s is created automatically
    # ...
    # secretName: "<tidb-tls-secret-name>"
    autoGenerated:
       extraSANIPList: ...
       extraSANDomainList: ...

Copy link
Contributor Author

@weekface weekface Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ingress hosts is the listen address of the http server. A little different from ours.

in addition, if user-defined certificate is used, they can't set extraSAN... here, they should set other SANs when creating certificate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean we need to add the hosts field. I just copied it from the ingress page I linked. I mean we can move secretName field to the up level, like secretName in ingress tls config. The below example is what I suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

secretName can have high precedence over autoGenerated, like userGenerated over autoGenerated. but because if a pre-created certificate is provided, I guess no extra configurations can be add in userGenerated, so we can replace userGenerated with secretName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user may use their custom Cluster TLS which used between pd, tikv and tidb peers. so there may be extra configrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may other userDefined attributes will be added in the future that we don't know yet

Copy link
Contributor

Choose a reason for hiding this comment

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

this tlsClient field is under tidb field and used to configure tidb only. if we configure tls secret for other components, we can add secretName in the configuration of other components.

Copy link
Contributor Author

@weekface weekface Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I mean the TLS between TiDB components (supported already): https://pingcap.com/docs/stable/how-to/secure/enable-tls-between-components/

But i am fine with only secretName.


# mysqlClient is used to set password for TiDB
# it must has Python MySQL client installed
Expand Down
16 changes: 1 addition & 15 deletions manifests/crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3779,21 +3779,6 @@ spec:
cluster-level updateStrategy if present Optional: Defaults to
cluster-level setting'
type: string
enableTLSClient:
description: 'Whether enable the TLS connection between the SQL
client and TiDB server Optional: Defaults to false'
type: boolean
extraSANDomainList:
description: extra SAN Domain list when setting EnableTLSClient
to true
items:
type: string
type: array
extraSANIPList:
description: extra SAN IP list when setting EnableTLSClient to true
items:
type: string
type: array
hostNetwork:
description: 'Whether Hostnetwork of the component is enabled. Override
the cluster-level setting if present Optional: Defaults to cluster-level
Expand Down Expand Up @@ -3988,6 +3973,7 @@ spec:
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/'
type: object
type: object
tlsClient: {}
tolerations:
description: 'Tolerations of the component. Override the cluster-level
tolerations if non-empty Optional: Defaults to cluster-level setting'
Expand Down
37 changes: 4 additions & 33 deletions pkg/apis/pingcap/v1alpha1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions pkg/apis/pingcap/v1alpha1/tidbcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,15 @@ func (tc *TidbCluster) IsTiDBBinlogEnabled() bool {
}

func (tidb *TiDBSpec) IsTLSClientEnabled() bool {
enableTLSClient := tidb.EnableTLSClient
if enableTLSClient == nil {
return defaultEnableTLSClient
return tidb.TLSClient != nil && tidb.TLSClient.Enabled
}

func (tidb *TiDBSpec) IsUserGeneratedCertificate() bool {
if !tidb.IsTLSClientEnabled() {
return false
}
return *enableTLSClient

return tidb.TLSClient.UserGenerated != nil && tidb.TLSClient.UserGenerated.SecretName != ""
}

func (tidb *TiDBSpec) ShouldSeparateSlowLog() bool {
Expand Down
43 changes: 35 additions & 8 deletions pkg/apis/pingcap/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,9 @@ type TiDBSpec struct {
SeparateSlowLog *bool `json:"separateSlowLog,omitempty"`

// Whether enable the TLS connection between the SQL client and TiDB server
// Optional: Defaults to false
// Optional: Defaults to nil
// +optional
EnableTLSClient *bool `json:"enableTLSClient,omitempty"`

// extra SAN IP list when setting EnableTLSClient to true
ExtraSANIPList []string `json:"extraSANIPList,omitempty"`

// extra SAN Domain list when setting EnableTLSClient to true
ExtraSANDomainList []string `json:"extraSANDomainList,omitempty"`
TLSClient *TiDBTLSClient `json:"tlsClient,omitempty"`

// The spec of the slow log tailer sidecar
// +optional
Expand Down Expand Up @@ -602,6 +596,39 @@ type PumpStatus struct {
StatefulSet *apps.StatefulSetStatus `json:"statefulSet,omitempty"`
}

// TiDBTLSClient can enable TLS connection between TiDB server and MySQL client
type TiDBTLSClient struct {
// When enabled, TiDB will accept TLS encrypted connections from MySQL client
// +optional
Enabled bool `json:"enabled,omitempty"`

// Auto-generated certificate
// +optional
AutoGenerated *TiDBAutoGeneratedCertificate `json:"autoGenerated,omitempty"`

// User-generated certificate
// +optional
UserGenerated *TiDBUserGeneratedCertificate `json:"userGenerated,omitempty"`
}

// TiDBAutoGeneratedCertificate is TiDB auto-generated certificate
type TiDBAutoGeneratedCertificate struct {
// Extra SAN IP list
// +optional
ExtraSANIPList []string `json:"extraSANIPList,omitempty"`

// Extra SAN Domain list
// +optional
ExtraSANDomainList []string `json:"extraSANDomainList,omitempty"`
}

// TiDBUserGeneratedCertificate is TiDB user-generated certificate
type TiDBUserGeneratedCertificate struct {
// Secret name which stores user custom TiDB Server certificate, key and ca
// +optional
SecretName string `json:"secretName,omitempty"`
}

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

Expand Down
86 changes: 72 additions & 14 deletions pkg/apis/pingcap/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading