-
Notifications
You must be signed in to change notification settings - Fork 495
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 tls support between drainer and downstream database server #2993
Changes from 5 commits
0084f0e
f405536
e60701a
70957c5
287dc29
3ff7023
132a00d
4147bcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -48,6 +48,22 @@ tlsCluster: | |||||||||
certAllowedCN: [] | ||||||||||
# - TiDB | ||||||||||
|
||||||||||
# The tls config between drainer and the downstream database server (MySQL/TiDB) | ||||||||||
tlsSyncer: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
tlsClientSecretName: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add comments to explain what's this field is for and what keys should be included in the secret or add an example command as in L41-L43. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest commenting out the new configurations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
# certAllowedCN is the Common Name that allowed | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between this one and the one in L64, please make it clear. |
||||||||||
certAllowedCN: [] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# - TiDB | ||||||||||
|
||||||||||
# checkpoint is the tls config for the database we save binlog checkpoint | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# Omit this part if you just want to save checkpoint in downstream database | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this mean? |
||||||||||
checkpoint: | ||||||||||
tlsClientSecretName: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an explanation, and if the DB of saving checkpoint is the same as the downstream DB, this can use the same secret as in L53, right? If yes, make it clear in the comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, empty configuration is okay. binlog will fill the security options automatically if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# certAllowedCN is the Common Name that allowed | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make it clear for the difference with L56. |
||||||||||
certAllowedCN: [] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# - TiDB | ||||||||||
|
||||||||||
# Refer to https://github.com/pingcap/tidb-binlog/blob/master/cmd/drainer/drainer.toml | ||||||||||
# [security] section will be generated automatically if tlsCluster.enabled is set to true so users do not need to configure it. | ||||||||||
config: | | ||||||||||
|
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.