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

Jump to bottom

add tls support between drainer and downstream database server #2993

Merged

lichunzhu merged 8 commits into pingcap:master from lichunzhu:addTlsForDrainerSyncer

Jul 22, 2020

Contributor

lichunzhu commented Jul 20, 2020 •

edited by DanielZhangQD

Loading

What problem does this PR solve?

resolve #2969
add tls support between drainer and downstream database server.

What is changed and how does it work?

add tls syncer part for drainer.

Check List

Tests

Unit test
E2E test
Stability test
Manual test (add detailed scripts or steps below)
- Write to downstream database without tls.
- Write to downstream database with only downstream tls specified.
- Write to downstream but checkpoint to another database with/without tls enabled.

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Does this PR introduce a user-facing change?:

Add TLS support between drainer and downstream database server


          add tls support between drainer and downstream database server

0084f0e

lichunzhu requested review from cofyc and DanielZhangQD

July 20, 2020 11:30

lichunzhu added status/PTAL needs-cherry-pick-1.1 labels


          Merge branch 'master' into addTlsForDrainerSyncer

f405536

codecov-commenter commented Jul 20, 2020 •

edited

Loading

Codecov Report

Merging #2993 into master will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2993      +/-   ##
==========================================
+ Coverage   41.41%   41.64%   +0.22%     
==========================================
  Files         155      156       +1     
  Lines       16744    16801      +57     
==========================================
+ Hits         6934     6996      +62     
+ Misses       9250     9241       -9     
- Partials      560      564       +4

Flag	Coverage Δ
#unittest	`41.64% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

lichunzhu added 3 commits

July 21, 2020 12:07


          add tls for checkpoint db

e60701a


          fix bug

70957c5


          Merge branch 'master' into addTlsForDrainerSyncer

287dc29

lichunzhu requested a review from weekface

July 21, 2020 06:18

Contributor Author

lichunzhu commented Jul 21, 2020

/test pull-e2e-kind

Contributor Author

lichunzhu commented Jul 21, 2020

/test pull-e2e-kind-serial

DanielZhangQD reviewed

View reviewed changes

charts/tidb-drainer/values.yaml Outdated

@@ @@ -48,6 +48,22 @@ tlsCluster: @@
                 certAllowedCN: []
                 #  - TiDB
+              # The tls config between drainer and the downstream database server (MySQL/TiDB)

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
            # The tls config between drainer and the downstream database server (MySQL/TiDB)
          
            # The TLS config between drainer and the downstream database server (MySQL/TiDB)

charts/tidb-drainer/values.yaml Outdated

@@ @@ -48,6 +48,22 @@ tlsCluster: @@
                 certAllowedCN: []
                 #  - TiDB
+              # The tls config between drainer and the downstream database server (MySQL/TiDB)
+              tlsSyncer:
+                tlsClientSecretName:

Contributor

DanielZhangQD Jul 22, 2020

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.

Contributor

DanielZhangQD Jul 22, 2020

Suggest commenting out the new configurations.

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
              tlsClientSecretName:
          
              # tlsClientSecretName: ""

charts/tidb-drainer/values.yaml

+              tlsSyncer:
+                tlsClientSecretName:
+                # certAllowedCN is the Common Name that allowed

Contributor

DanielZhangQD Jul 22, 2020

What's the difference between this one and the one in L64, please make it clear.

charts/tidb-drainer/values.yaml Outdated

+                certAllowedCN: []
+                #  - TiDB
+                # checkpoint is the tls config for the database we save binlog checkpoint

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
              # checkpoint is the tls config for the database we save binlog checkpoint
          
              # checkpoint is the TLS config for the database we save binlog checkpoint

charts/tidb-drainer/values.yaml Outdated

+                #  - TiDB
+                # checkpoint is the tls config for the database we save binlog checkpoint
+                # Omit this part if you just want to save checkpoint in downstream database

Contributor

DanielZhangQD Jul 22, 2020

What does this mean?

charts/tidb-drainer/values.yaml

+                # Omit this part if you just want to save checkpoint in downstream database
+                checkpoint:
+                  tlsClientSecretName:
+                # certAllowedCN is the Common Name that allowed

Contributor

DanielZhangQD Jul 22, 2020

Make it clear for the difference with L56.

charts/tidb-drainer/values.yaml Outdated

@@ @@ -48,6 +48,22 @@ tlsCluster: @@
                 certAllowedCN: []
                 #  - TiDB
+              # The tls config between drainer and the downstream database server (MySQL/TiDB)
+              tlsSyncer:

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
            tlsSyncer:
          
            tlsSyncer: {}

charts/tidb-drainer/values.yaml Outdated

+                tlsClientSecretName:
+                # certAllowedCN is the Common Name that allowed
+                certAllowedCN: []

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
              certAllowedCN: []
          
              # certAllowedCN: []

charts/tidb-drainer/values.yaml Outdated

Comment on lines 61 to 62

		checkpoint:
		tlsClientSecretName:

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
              checkpoint:
          
                tlsClientSecretName:
          
              # checkpoint:
          
                # tlsClientSecretName: ""

charts/tidb-drainer/values.yaml Outdated

+                checkpoint:
+                  tlsClientSecretName:
+                # certAllowedCN is the Common Name that allowed
+                  certAllowedCN: []

Contributor

DanielZhangQD Jul 22, 2020

Suggested change

      
                certAllowedCN: []
          
                # certAllowedCN: []

weekface previously approved these changes

View reviewed changes

Contributor

weekface left a comment

LGTM

Please also update the TLS document.

ti-srebot added the status/LGT1 label

lichunzhu added 2 commits

July 22, 2020 12:04


          address comments

3ff7023


          address comments again

132a00d

lichunzhu dismissed weekface’s stale review via

132a00d

July 22, 2020 04:24


          Merge branch 'master' into addTlsForDrainerSyncer

4147bcd

Contributor

DanielZhangQD commented Jul 22, 2020

/test pull-e2e-kind

DanielZhangQD approved these changes

View reviewed changes

Contributor

DanielZhangQD left a comment

LGTM

ti-srebot removed the status/LGT1 label

ti-srebot approved these changes

View reviewed changes

ti-srebot added the status/LGT2 label

Contributor Author

lichunzhu commented Jul 22, 2020

@weekface PTAL again

weekface approved these changes

View reviewed changes

lichunzhu merged commit 741b18e into pingcap:master

lichunzhu deleted the addTlsForDrainerSyncer branch

July 22, 2020 06:26

ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request


          cherry pick pingcap#2993 to release-1.1

cdb3de1

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request

add tls support between drainer and downstream database server (#2993) #3000

Merged

Contributor

ti-srebot commented Jul 22, 2020

cherry pick to release-1.1 in PR #3000

DanielZhangQD added a commit that referenced this pull request


          cherry pick #2993 to release-1.1 (#3000)

15a7619

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-cherry-pick-1.1 status/LGT2 status/PTAL