Skip to content
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

support TLS in cdc components #347

Merged
merged 26 commits into from
Jul 14, 2020
Merged

support TLS in cdc components #347

merged 26 commits into from
Jul 14, 2020

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented Mar 17, 2020 •
edited by overvenus
Loading

What problem does this PR solve?

Support TLS in TiCDC, fulfill pingcap/ticdc#250

What is changed and how it works?

  • Code changed:

    • upgrade PD dependency to github.com/pingcap/pd/v4
    • Add security config to cdc server and cdc cli
    • gRPC communication with PD/TiKV will be encrypted
    • HTTP status/metric server will be encrypted if security config is enabled
    • Redirect gRPC log to CDC

  • Test changed:

    • Add an integration test case with TLS enabled.

TODO:

  • Encrypt Sink communication with MySQL/Kafka
  • Support common name check

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Support TLS

@amyangfei amyangfei added the WIP label Mar 17, 2020
@amyangfei
Copy link
Contributor Author

/run-integration-tests

@amyangfei amyangfei added status/ptal Could you please take a look? and removed WIP labels Mar 18, 2020
@amyangfei amyangfei changed the title support TLS in all components support TLS in cdc components Mar 18, 2020
shafreeck
shafreeck reviewed Mar 19, 2020
View reviewed changes
tests/_utils/run_curl Outdated
POST_ARGS="--post-data $2"
fi

# FIXME: use `wget` instead of `curl` because the latter rejects ECC certs on our CI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this FIXME still required?

@amyangfei amyangfei added WIP and removed status/ptal Could you please take a look? labels Apr 13, 2020
@CLAassistant
Copy link

CLAassistant commented Jun 19, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sre-bot
Copy link

sre-bot commented Jun 19, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@overvenus
remove unnecessary files
a46d734 
Signed-off-by: Neil Shen <[email protected]>
@overvenus
Copy link
Member

/run-integration-tests

overvenus
overvenus reviewed Jul 13, 2020
View reviewed changes
cdc/kv/client.go Outdated Show resolved Hide resolved
@overvenus overvenus added status/ptal Could you please take a look? and removed status/WIP labels Jul 13, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2020 •
edited
Loading

Codecov Report

Merging #347 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #347   +/-   ##
===========================================
  Coverage   34.3204%   34.3204%           
===========================================
  Files            91         91           
  Lines         10874      10874           
===========================================
  Hits           3732       3732           
  Misses         6810       6810           
  Partials        332        332

@overvenus overvenus added this to the v4.0.3 milestone Jul 13, 2020
@DanielZhangQD DanielZhangQD mentioned this pull request Jul 13, 2020
Closed
6 tasks
@amyangfei amyangfei added the release-blocker This issue blocks a release. Please solve it ASAP. label Jul 13, 2020
zier-one
zier-one reviewed Jul 14, 2020
View reviewed changes
Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

I found that we pass config everywhere, (including pkg/Config and security/Credential), how about making the config as a global variable or setting config into context?

cdc/kv/store_op.go Outdated
@@ -18,6 +18,8 @@ import (

"github.com/pingcap/errors"
"github.com/pingcap/ticdc/pkg/flags"
"github.com/pingcap/ticdc/pkg/security"
"github.com/pingcap/tidb/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid confusion with ticdc/pkg/config

Suggested change
"github.com/pingcap/tidb/config"
tidbconfig "github.com/pingcap/tidb/config"

Copy link
Contributor

Choose a reason for hiding this comment

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

@overvenus

@overvenus
Copy link
Member

how about making the config as a global variable or setting config into context?

Generally speaking, using global variables is not good especially when it relates to security issues. Setting credentials in context is doable but it makes code less explicitly and it is very likely to leak credential to packages we don't want.

@zier-one
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 14, 2020
@amyangfei
Copy link
Contributor Author

/lgtm

@ti-srebot
Copy link
Contributor

@amyangfei Oops. LGTM is restricted to reviewers.

@overvenus
address comments
9fb5ee4 
Signed-off-by: Neil Shen <[email protected]>
@overvenus
Copy link
Member

/lgtm

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 14, 2020
@zier-one
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 14, 2020
@zier-one zier-one removed the status/ptal Could you please take a look? label Jul 14, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@overvenus overvenus merged commit cb5e102 into pingcap:master Jul 14, 2020
@DanielZhangQD DanielZhangQD mentioned this pull request Jul 17, 2020
Closed
@amyangfei amyangfei deleted the support-tls branch August 1, 2020 10:39
amyangfei pushed a commit to amyangfei/tiflow that referenced this pull request May 6, 2022
@lance6716
Test CreateWorker using resource constraint (pingcap#347)
2840ac1 
Signed-off-by: lance6716 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shafreeck shafreeck shafreeck left review comments

@overvenus overvenus overvenus left review comments

@zier-one zier-one zier-one left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
release-blocker This issue blocks a release. Please solve it ASAP. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
v4.0.3
Development

Successfully merging this pull request may close these issues.
8 participants
@amyangfei @CLAassistant @sre-bot @overvenus @codecov-commenter @zier-one @ti-srebot @shafreeck