*: put source/task tls config into etcd #1910

Ehco1996 · 2021-07-25T07:31:47Z

What problem does this PR solve?

close #1909
close #1932
close #1956
#1982

What is changed and how it works?

update /tidb/dumpling/tidb-tools dependencies to the latest version
when the user has configured the SSL-related configuration, the contents of the certificate file are read and saved to the ETCD
In order to simulate this scenario in CI, the PR for the mysql SSL supported by CI must be merged before this PR

I've manually re-palyed the CI changes, link is here

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update the dm/dm-ansible
Need to be included in the release note

ti-chi-bot · 2021-07-25T07:31:49Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Ehco1996

Remove tidb_enable_change_column_type as tidb's defaultSysVars do not contain this key and dm-worker will panic if we pass in a variable that tidb does not RegisterSysVar.

lance6716

rest lgtm

dm/config/security_test.go

tests/tls/run.sh

dm/config/testdata/ca.pem

lichunzhu · 2021-08-13T03:20:29Z

dm/config/security_test.go

+	testdataPath = "./testdata"
+
+	caFile        = "./testdata/ca.pem"


How about using c.Mkdir() to save them in temporary files.

fixed in c5fb6a3

lichunzhu · 2021-08-16T06:14:13Z

dm/ctl/master/start_task.go

+	if task.TargetDB != nil && task.TargetDB.Security != nil {
+		loadErr := task.TargetDB.Security.LoadTLSContent()
+		if loadErr != nil {
+			return loadErr
+		}


It seems not compatible with the former version. If we still put TLS files on the dm-worker server we will fail to start a task now.

Maybe I could add a hint to the error message to help the user locate the problem?

Can we ignore this error here and let the dm-worker take a try?
If it still fails to read tls content, we can report an error. If users have several sources/tasks using tls before, it might be hard for them to change their way to use DM.

Besides, I find that if we directly rolling upgrade DM from former version using MySQL tls, it seems that the former tasks will directly fail because they didn't have SSLCABytes in source config. Am I right?

To ensure that tasks already using tls connections are not interrupted after this upgrade
Before the database connection is established, an attempt is made to read the configuration from the local disk

6d62706

cc @lance6716

pkg/log/log_test.go

lichunzhu · 2021-08-16T06:36:30Z

tests/tls/run.sh

+	echo "check data"
+	check_sync_diff $WORK_DIR $cur/conf/diff_config.toml
+
+	echo "pause task before kill and restart dm-worker"


I think we should delete tls files first to make sure we have ha.

I don't think so, the worker doesn't read the certificate from the file in the code now,
we always read it from etcd, because we use this method to load the certificate (ToTLSConfigWithVerifyByRawbytes)

If dm-worker reads tls files from the local disk, we can also pass this test. Our test should make sure the former way fails this test.

also added in 6d62706

lichunzhu

LGTM

Ehco1996 · 2021-08-16T22:59:56Z

/merge

ti-chi-bot · 2021-08-16T22:59:59Z

This pull request has been accepted and is ready to merge.

Commit hash: 39268c5

ti-chi-bot · 2021-08-16T23:11:46Z

In response to a cherrypick label: new pull request created: #1998.

Ehco1996 added 2 commits July 23, 2021 17:47

add ssl content in cfg

04e1b80

save work

fd5df76

ti-chi-bot added the do-not-merge/work-in-progress label Jul 25, 2021

ti-chi-bot requested review from lance6716 and lichunzhu July 25, 2021 07:31

ti-chi-bot added the size/L label Jul 25, 2021

Ehco1996 added 5 commits July 26, 2021 14:24

enable tls test

639486c

save work

7c3b521

Merge branch 'master' into source-ssl-cfg

c379e6f

update tidb-tools

4486db9

save work

32342ea

ti-chi-bot added size/XXL and removed size/L labels Jul 28, 2021

add tls ha test

2069c7d

GMHDBJD added this to the v2.0.6 milestone Jul 29, 2021

Ehco1996 changed the title *: put source tls config into etcd *: put source/task tls config into etcd Jul 29, 2021

Ehco1996 added 11 commits July 30, 2021 11:04

remove replace

bf15be8

Merge branch 'master' into source-ssl-cfg

cf943d0

hide ssl field

6f0b29e

print sync_diff_inspector version

3b0c126

fix mod

6a433b4

add dump print

2ac025e

wip

c0b1253

save work

812bf8f

fix mod

5b6466e

Merge branch 'master' into source-ssl-cfg

5bcea38

use EmbedUnistore

768946d

Ehco1996 commented Jul 31, 2021

View reviewed changes

fix setup_mysql_tls

9e904e9

GMHDBJD added this to the v2.1.0 milestone Aug 13, 2021

Merge branch 'master' into source-ssl-cfg

d2eeb03

lance6716 reviewed Aug 16, 2021

View reviewed changes

dm/config/security_test.go Outdated Show resolved Hide resolved

dm/config/security_test.go Show resolved Hide resolved

dm/config/security_test.go Outdated Show resolved Hide resolved

tests/tls/run.sh Outdated Show resolved Hide resolved

Ehco1996 added 2 commits August 16, 2021 13:47

address comment

28799e8

address comment

fff653c

lance6716 approved these changes Aug 16, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Aug 16, 2021

lichunzhu reviewed Aug 16, 2021

View reviewed changes

Ehco1996 added 2 commits August 16, 2021 15:08

address comment

c5fb6a3

try load tis data before setup conn

6d62706

lichunzhu approved these changes Aug 16, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 16, 2021

Ehco1996 added 2 commits August 16, 2021 16:32

fix a UT

8374493

Merge branch 'master' into source-ssl-cfg

7733ccc

Ehco1996 removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Aug 16, 2021

Merge branch 'master' into source-ssl-cfg

39268c5

ti-chi-bot added the status/can-merge label Aug 16, 2021

ti-chi-bot merged commit 6d30e10 into pingcap:master Aug 16, 2021

Ehco1996 added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label Aug 16, 2021

ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Aug 16, 2021

*: put source/task tls config into etcd (pingcap#1910)

37f3289

ti-chi-bot mentioned this pull request Aug 16, 2021

*: put source/task tls config into etcd (#1910) #1998

Merged

ti-chi-bot added a commit that referenced this pull request Aug 17, 2021

*: put source/task tls config into etcd (#1910) (#1998)

843b4a8

lichunzhu added a commit to lichunzhu/dm that referenced this pull request Sep 17, 2021

cherry pick part of pingcap#1910 again

2445291

lichunzhu added a commit to lichunzhu/dm that referenced this pull request Sep 17, 2021

cherry pick part of pingcap#1910 again

7a3727a

lance6716 mentioned this pull request May 6, 2022

regression: Support TLS connections when only ssl-ca is set pingcap/tiflow#5335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: put source/task tls config into etcd #1910

*: put source/task tls config into etcd #1910

Ehco1996 commented Jul 25, 2021 •

edited

Loading

ti-chi-bot commented Jul 25, 2021 •

edited

Loading

Ehco1996 left a comment •

edited

Loading

lance6716 left a comment

lichunzhu Aug 13, 2021

Ehco1996 Aug 16, 2021

lichunzhu Aug 16, 2021

Ehco1996 Aug 16, 2021

lichunzhu Aug 16, 2021

lichunzhu Aug 16, 2021

Ehco1996 Aug 16, 2021

lichunzhu Aug 16, 2021

Ehco1996 Aug 16, 2021

lichunzhu Aug 16, 2021

Ehco1996 Aug 16, 2021

lichunzhu left a comment

Ehco1996 commented Aug 16, 2021

ti-chi-bot commented Aug 16, 2021

ti-chi-bot commented Aug 16, 2021

*: put source/task tls config into etcd #1910

*: put source/task tls config into etcd #1910

Conversation

Ehco1996 commented Jul 25, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Jul 25, 2021 • edited Loading

Ehco1996 left a comment • edited Loading

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu left a comment

Choose a reason for hiding this comment

Ehco1996 commented Aug 16, 2021

ti-chi-bot commented Aug 16, 2021

ti-chi-bot commented Aug 16, 2021

Ehco1996 commented Jul 25, 2021 •

edited

Loading

ti-chi-bot commented Jul 25, 2021 •

edited

Loading

Ehco1996 left a comment •

edited

Loading