Skip to content

cluster: add support of TLS encryption for TiDB cluster#673

Merged
lucklove merged 13 commits intopingcap:masterfrom
AstroProfundis:cluster-tls
Sep 3, 2020
Merged

cluster: add support of TLS encryption for TiDB cluster#673
lucklove merged 13 commits intopingcap:masterfrom
AstroProfundis:cluster-tls

Conversation

@AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented Aug 12, 2020

What problem does this PR solve?

Add support for TLS encryption for TiDB cluster. Close #529

What is changed and how it works?

  • Specification checks for TLS enabled cluster
  • Generate CA and certificate for cluster
  • Deploy certificates to server
  • Start/Stop/Restart TLS enabled cluster
  • Scale in/out TLS enabled cluster
  • Upgrade TLS enabled cluster
  • Update monitoring configs for TLS enabled cluster
  • Adjust display output for TLS enabled cluster
  • Testing

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

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 update the documentation

Release notes:

Add support of TLS encryption for TiDB cluster

@AstroProfundis AstroProfundis added type/new-feature Categorizes pr as related to a new feature. status/WIP category/security Categorizes issue or PR as a security enhancement. labels Aug 12, 2020
@AstroProfundis AstroProfundis self-assigned this Aug 12, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
+ Coverage   58.07%   58.77%   +0.70%     
==========================================
  Files         255      257       +2     
  Lines       18904    19439     +535     
==========================================
+ Hits        10978    11425     +447     
- Misses       6473     6499      +26     
- Partials     1453     1515      +62     
Flag Coverage Δ
#coverage 58.77% <ø> (+0.70%) ⬆️

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

Impacted Files Coverage Δ
...ingcap/tiup/components/cluster/command/scale_in.go 75.00% <0.00%> (-7.36%) ⬇️
...pingcap/tiup/components/cluster/command/display.go 35.78% <0.00%> (-7.07%) ⬇️
...m/pingcap/tiup/pkg/cluster/task/update_topology.go 78.57% <0.00%> (-1.83%) ⬇️
...ngcap/tiup/components/cluster/command/scale_out.go 67.92% <0.00%> (-1.47%) ⬇️
...src/github.com/pingcap/tiup/pkg/cluster/manager.go 68.19% <0.00%> (-1.31%) ⬇️
...ithub.com/pingcap/tiup/pkg/cluster/spec/tispark.go 69.52% <0.00%> (-1.07%) ⬇️
....com/pingcap/tiup/pkg/cluster/operation/destroy.go 61.73% <0.00%> (-0.64%) ⬇️
...github.com/pingcap/tiup/pkg/cluster/task/action.go 17.07% <0.00%> (ø)
...ithub.com/pingcap/tiup/components/dm/spec/logic.go 77.65% <0.00%> (ø)
...ithub.com/pingcap/tiup/pkg/cluster/spec/profile.go 77.77% <0.00%> (ø)
... and 41 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 6f08328...1fac178. Read the comment docs.

@lonng lonng added this to the v1.1.0 milestone Aug 16, 2020
@lonng lonng requested review from july2993 and lucklove August 18, 2020 09:27
@AstroProfundis AstroProfundis force-pushed the cluster-tls branch 6 times, most recently from c0b4fa0 to 87d43bd Compare August 19, 2020 08:56
@AstroProfundis AstroProfundis removed this from the v1.1.0 milestone Aug 19, 2020
@AstroProfundis AstroProfundis force-pushed the cluster-tls branch 2 times, most recently from 0b90913 to ecfa558 Compare August 19, 2020 09:55
@AstroProfundis AstroProfundis marked this pull request as ready for review August 19, 2020 10:32
@AstroProfundis AstroProfundis added category/testing Categorizes issue or PR as a testing enhancement. and removed status/WIP labels Aug 19, 2020
@AstroProfundis AstroProfundis force-pushed the cluster-tls branch 5 times, most recently from 7faa9fc to fc3ccf0 Compare August 24, 2020 03:04
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error: failed to transfer CA cert to server: Process exited with status 1

pls fix ci

// String implements the fmt.Stringer interface
func (c *TLSCert) String() string {
return fmt.Sprintf("TLSCert: host=%s role=%s cn=%s",
c.inst.GetHost(), c.inst.Role(), c.inst.ComponentName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Role alway = ComponentName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's for example tispark-master as role and tispark as component name, identical for most components but not all.

if !ok {
return ErrNoExecutor
}
if err := e.Transfer(caFile,
Copy link
Contributor

@july2993 july2993 Aug 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this API should be designed as Transfer and Download at first. (not block this pr)

@AstroProfundis AstroProfundis force-pushed the cluster-tls branch 4 times, most recently from ac1f874 to cb1382b Compare September 2, 2020 03:10
Copy link
Member

@lucklove lucklove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 2, 2020
@lonng
Copy link
Contributor

lonng commented Sep 3, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 3, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 745

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@AstroProfundis merge failed.

@lonng
Copy link
Contributor

lonng commented Sep 3, 2020

@AstroProfundis Please resolve the conflicts.

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

Labels

category/security Categorizes issue or PR as a security enhancement. category/testing Categorizes issue or PR as a testing enhancement. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/new-feature Categorizes pr as related to a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support deploying and managing TLS encryption enabled TiDB cluster

6 participants