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

PD < v4.0 shouldn't expect a client cert when server TLS is enabled #2385

Closed
mightyguava opened this issue May 6, 2020 · 6 comments · Fixed by #2389
Closed

PD < v4.0 shouldn't expect a client cert when server TLS is enabled #2385

mightyguava opened this issue May 6, 2020 · 6 comments · Fixed by #2389
Assignees
Milestone

Comments

@mightyguava
Copy link
Contributor

Feature Request

Is your feature request related to a problem? Please describe:
Right now, if .spec.tidb.tlsClient.enabled is set to true, PD tried to mount the secret name ${cluster}-tidb-client-secret. This seems to enable the dashboard for TiDB > v4.0. It doesn't exist for 3.1.

Describe the feature you'd like:
Option 1: automatically detect that the version doesn't support a dashboard, and not try to mount the cert
Option 2: add a setting to the PD spec to disable this volume mount

@DanielZhangQD DanielZhangQD added this to the v1.1.0 milestone May 7, 2020
@DanielZhangQD
Copy link
Contributor

@weekface Please help check this issue, Thanks!

@weekface
Copy link
Contributor

weekface commented May 7, 2020

@mightyguava There is a workaround solution, add this annotation to TidbCluster CR can disable the secret mount when creating tidb cluster.

annotations:
  tidb.tidb.pingcap.com/skip-tls-when-connect-tidb: ""

@mightyguava
Copy link
Contributor Author

Yeah I saw that, but in context of #2384, we don't want to disable TLS for BR, so we can't use this annotation.

@weekface
Copy link
Contributor

weekface commented May 7, 2020

We use TidbCluster CR to create PD(Dashboard)/TiKV/TiDB, and PD Dashboard can use this annotation to skip TLS when connecting to TiDB Server. This is the PR: #2143

This doesn't affect the BR component, because they use Backup and Resore CR not TidbCluster CR.

@mightyguava
Copy link
Contributor Author

Maybe I'm understanding the PR, but doesn't this change disable TLS for BR when that annotation is set? https://github.com/pingcap/tidb-operator/pull/2143/files#diff-f533d169102c76e345b10744a0e5c377

@weekface
Copy link
Contributor

weekface commented May 7, 2020

Good catch, I missed the backup and restore code in the pr #2143. I will open a PR to let PD skip TiDB Server client certificate mount when its version is less than v4.0

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 a pull request may close this issue.

3 participants