cluster: support enable or disable TLS for an exiting cluster #1657

srstack · 2021-12-07T09:29:36Z

What problem does this PR solve?

Only supports single PD node cluster

What is changed and how it works?

add a new tiup-cluster sub cmd

tiup-cluster tls -h
Enable/Disable TLS between TiDB components

Usage:
  tiup-cluster tls <cluster-name> <enable/disable> [flags]

Flags:
      --clean-certificate    Cleanup the certificate file if it already exists when tls disable
      --force                Force enable/disable tls regardless of the current state
  -h, --help                 help for tls
      --reload-certificate   Load the certificate file whether it exists or not when tls enable

First check whether the number of PD nodes is 1
Generate/Delete certificate key and push it to the TiDB components
Refresh components configuration file
Restart cluster
Update PD-Etcd memberList

Check List

Tests

Manual test (add detailed scripts or steps below)
It is best to run tpcc during the manual test

enable

 ./tiup-cluster tls cluster-name enable --reload-certificate 
 ./tiup-cluster display cluster-name
./tiup-cluster scale-out cluster-name scale-out.yaml

disable

 ./tiup-cluster tls cluster-name disable --clean-certificate 
 ./tiup-cluster display cluster-name
./tiup-cluster scale-out cluster-name scale-out.yaml

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods 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

Release notes:

NONE

…ue-1598

ti-chi-bot · 2021-12-07T09:29:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AstroProfundis

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.

codecov-commenter · 2021-12-07T09:43:11Z

Codecov Report

Merging #1657 (770dd3b) into master (c3ecda7) will decrease coverage by 0.91%.
The diff coverage is 64.40%.

❗ Current head 770dd3b differs from pull request most recent head 1ec1562. Consider uploading reports for the commit 1ec1562 to get more accurate results

@@            Coverage Diff             @@
##           master    #1657      +/-   ##
==========================================
- Coverage   48.22%   47.31%   -0.91%     
==========================================
  Files         298      298              
  Lines       33496    34137     +641     
==========================================
  Hits        16151    16151              
- Misses      15258    15899     +641     
  Partials     2087     2087

Flag	Coverage Δ
tiup	`14.66% <0.00%> (-0.14%)`	⬇️
unittest	`?`

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

Impacted Files	Coverage Δ
pkg/cluster/manager/cacert.go	`45.00% <ø> (ø)`
pkg/cluster/operation/operation.go	`80.65% <ø> (ø)`
pkg/cluster/spec/profile.go	`55.26% <ø> (-10.36%)`	⬇️
pkg/cluster/spec/alertmanager.go	`58.59% <16.67%> (-9.65%)`	⬇️
pkg/cluster/spec/instance.go	`50.00% <21.43%> (-16.37%)`	⬇️
pkg/cluster/spec/spec.go	`57.74% <33.33%> (-18.27%)`	⬇️
pkg/cluster/spec/grafana.go	`54.46% <40.00%> (-11.08%)`	⬇️
pkg/cluster/spec/drainer.go	`47.97% <41.18%> (-9.75%)`	⬇️
pkg/cluster/spec/pump.go	`55.24% <45.16%> (-12.28%)`	⬇️
pkg/cluster/spec/tikv.go	`48.29% <47.06%> (-10.81%)`	⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3ecda7...1ec1562. Read the comment docs.

AstroProfundis

I did some manual tests and seems all functions are working.

AstroProfundis · 2021-12-23T08:41:39Z

pkg/cluster/manager/tls.go

+	// if force is true, skip this check
+	if globalOptions.TLSEnabled == enable && !gOpt.Force {
+		if enable {
+			m.logger.Infof("cluster `%s` TLS status is already enable\n", name)


Suggested change

m.logger.Infof("cluster `%s` TLS status is already enable\n", name)

m.logger.Infof("cluster `%s` TLS status is already enabled\n", name)

AstroProfundis · 2021-12-23T08:41:51Z

pkg/cluster/manager/tls.go

+		if enable {
+			m.logger.Infof("cluster `%s` TLS status is already enable\n", name)
+		} else {
+			m.logger.Infof("cluster `%s` TLS status is already disable\n", name)


Suggested change

m.logger.Infof("cluster `%s` TLS status is already disable\n", name)

m.logger.Infof("cluster `%s` TLS status is already disabled\n", name)

AstroProfundis · 2021-12-23T08:42:27Z

pkg/cluster/manager/tls.go

+	}
+
+	if enable {
+		m.logger.Infof("Enable cluster `%s` TLS between TiDB components successfully", name)


Suggested change

m.logger.Infof("Enable cluster `%s` TLS between TiDB components successfully", name)

m.logger.Infof("Enabled TLS between TiDB components for cluster `%s` successfully", name)

AstroProfundis · 2021-12-23T08:42:51Z

pkg/cluster/manager/tls.go

+	if enable {
+		m.logger.Infof("Enable cluster `%s` TLS between TiDB components successfully", name)
+	} else {
+		m.logger.Infof("Disable cluster `%s` TLS between TiDB components successfully", name)


Suggested change

m.logger.Infof("Disable cluster `%s` TLS between TiDB components successfully", name)

m.logger.Infof("Disabled TLS between TiDB components for cluster `%s` successfully", name)

AstroProfundis · 2021-12-23T09:10:24Z

pkg/cluster/manager/tls.go

+		}
+
+		if len(clusterSpec.PDServers) != 1 {
+			return errorx.EnsureStackTrace(fmt.Errorf("Multiple PD nodes is not supported enable/disable TLS")).


Suggested change

return errorx.EnsureStackTrace(fmt.Errorf("Multiple PD nodes is not supported enable/disable TLS")).

return errorx.EnsureStackTrace(fmt.Errorf("Having multiple PD nodes is not supported when enable/disable TLS")).

AstroProfundis · 2021-12-23T09:12:30Z

pkg/cluster/manager/tls.go

+	if !skipConfirm {
+		return tui.PromptForConfirmOrAbortError(
+			fmt.Sprintf("Enable/Disable TLS will %s the cluster `%s`\nDo you want to continue? [y/N]:",
+				color.HiYellowString("restart"),


Suggested change

color.HiYellowString("restart"),

color.HiYellowString("stop and restart"),

It would be better if we make it clear that the cluster will be unavailable during the process.

AstroProfundis · 2021-12-23T09:42:55Z

/merge

ti-chi-bot · 2021-12-23T09:42:57Z

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

Commit hash: 1ec1562

srstack and others added 30 commits November 26, 2021 11:46

add tls comand

890bb20

Merge branch 'issue-1598' of https://github.com/srstack/tiup into iss…

758813c

…ue-1598

add tls func

042a1e7

add tls command

ca4303c

Complete the enable tls operation

c727d52

finish enable tls, but maybe pd is error

31408fa

finish disable tls

8c7520b

add cleanup remote host tls file

e4aba12

add clean tls file tips

2969e37

add some tips

2ff46b9

fix scale-out operation not create tls files

620bb7c

fix deploy operation not create tls files

a92b58a

fix cannot cleanup monitor data/log/tls files

3a3dbe4

fix

3eb4528

Merge branch 'pingcap:master' into issue-1598

b3c5018

Merge branch 'master'

a14d0bb

Merge branch 'pingcap-master' into issue-1598

d61a9d8

delete skip-restart flag

b539985

Merge branch 'pingcap:master' into issue-1598

53f6a7a

fix skip confirm

0d39bc0

skip confirm

934f431

Merge branch 'pingcap:master' into issue-1598

fbe568e

skip confirm

9e74753

perfect tips

251111e

fix

023eee6

finish 1 pd cluster enable/disable tls

5760ac8

prevent duplicate set tls

d36ca86

perfect tips

93e5ffa

perfect tips

d2b9b6a

Merge branch 'pingcap:master' into issue-1598

f096dd9

srstack and others added 8 commits December 5, 2021 21:16

add force flag

5b88b7f

Merge branch 'issue-1598' of https://github.com/srstack/tiup into iss…

3086473

…ue-1598

add tls path with clean op

9828b3e

Merge branch 'pingcap:master' into issue-1598

aeee4f8

m

3ddac95

m

90f3d35

Merge branch 'master' of https://github.com/pingcap/tiup into issue-1598

2b846b5

perfect tips

3f9d140

srstack requested review from AstroProfundis and nexustar December 7, 2021 09:29

srstack self-assigned this Dec 7, 2021

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 7, 2021

AstroProfundis added this to the v1.9.0 milestone Dec 15, 2021

srstack added 2 commits December 15, 2021 15:00

Merge branch 'master' into issue-1598

a5f8da9

Merge branch 'master' into issue-1598

1ec1562

AstroProfundis approved these changes Dec 23, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 23, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 23, 2021

ti-chi-bot merged commit 934f837 into pingcap:master Dec 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: support enable or disable TLS for an exiting cluster #1657

cluster: support enable or disable TLS for an exiting cluster #1657

srstack commented Dec 7, 2021

ti-chi-bot commented Dec 7, 2021 •

edited

Loading

codecov-commenter commented Dec 7, 2021 •

edited

Loading

AstroProfundis left a comment

AstroProfundis Dec 23, 2021

AstroProfundis Dec 23, 2021

AstroProfundis Dec 23, 2021

AstroProfundis Dec 23, 2021

AstroProfundis Dec 23, 2021

AstroProfundis Dec 23, 2021

AstroProfundis commented Dec 23, 2021

ti-chi-bot commented Dec 23, 2021

	m.logger.Infof("cluster `%s` TLS status is already enable\n", name)
	m.logger.Infof("cluster `%s` TLS status is already enabled\n", name)

	m.logger.Infof("Enable cluster `%s` TLS between TiDB components successfully", name)
	m.logger.Infof("Enabled TLS between TiDB components for cluster `%s` successfully", name)

	m.logger.Infof("Disable cluster `%s` TLS between TiDB components successfully", name)
	m.logger.Infof("Disabled TLS between TiDB components for cluster `%s` successfully", name)

	return errorx.EnsureStackTrace(fmt.Errorf("Multiple PD nodes is not supported enable/disable TLS")).
	return errorx.EnsureStackTrace(fmt.Errorf("Having multiple PD nodes is not supported when enable/disable TLS")).

	color.HiYellowString("restart"),
	color.HiYellowString("stop and restart"),

cluster: support enable or disable TLS for an exiting cluster #1657

cluster: support enable or disable TLS for an exiting cluster #1657

Conversation

srstack commented Dec 7, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Dec 7, 2021 • edited Loading

codecov-commenter commented Dec 7, 2021 • edited Loading

Codecov Report

AstroProfundis left a comment

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis Dec 23, 2021

Choose a reason for hiding this comment

AstroProfundis commented Dec 23, 2021

ti-chi-bot commented Dec 23, 2021

ti-chi-bot commented Dec 7, 2021 •

edited

Loading

codecov-commenter commented Dec 7, 2021 •

edited

Loading