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.

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

[close #232] Add https for tikv sink #233

Merged
merged 12 commits into from
Sep 22, 2022
Merged

[close #232] Add https for tikv sink #233

merged 12 commits into from
Sep 22, 2022

Conversation

zeminzhou
Copy link
Contributor

Signed-off-by: zeminzhou [email protected]

What problem does this PR solve?

Support https for tikv sink

Issue Number: close #232

Problem Description: TBD

What is changed and how does it work?

Create tikv client with security.

Code changes

  • Has exported function/method change

Check List for Tests

This PR has been tested by at least one of the following methods:

  • Unit test
  • Integration test

Signed-off-by: zeminzhou <[email protected]>
Signed-off-by: zeminzhou <[email protected]>
pdAddrPrefix := "http://"

if sinkURI.Query().Get("ca-path") != "" {
config.Security = tikvconfig.NewSecurity(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could CA CERT and KEY be set by config file?

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #233 (4419205) into main (b3bd7f2) will increase coverage by 0.0255%.
The diff coverage is 90.9090%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #233        +/-   ##
================================================
+ Coverage   61.1273%   61.1528%   +0.0255%     
================================================
  Files           239        239                
  Lines         20207      20210         +3     
================================================
+ Hits          12352      12359         +7     
+ Misses         6739       6709        -30     
- Partials       1116       1142        +26     
Flag Coverage Δ *Carryforward flag
br 59.9088% <ø> (-0.3675%) ⬇️ Carriedforward from 34df9ab
cdc 61.7245% <90.9090%> (+0.2055%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
cdc/cdc/owner/feed_state_manager.go 74.8743% <ø> (ø)
cdc/cdc/sink/tikv.go 81.1245% <90.9090%> (+0.7078%) ⬆️
cdc/pkg/pdtime/acquirer.go 58.1395% <0.0000%> (-6.9768%) ⬇️
cdc/pkg/retry/retry_with_opt.go 90.9090% <0.0000%> (-4.5455%) ⬇️
br/pkg/task/common.go 67.3913% <0.0000%> (-2.3890%) ⬇️
cdc/cdc/kv/client.go 84.0057% <0.0000%> (-2.3055%) ⬇️
cdc/cdc/sink/buffer_sink.go 80.0000% <0.0000%> (-1.8182%) ⬇️
br/pkg/conn/conn.go 52.5423% <0.0000%> (-1.6950%) ⬇️
br/pkg/restore/client.go 58.0808% <0.0000%> (-1.0102%) ⬇️
br/pkg/checksum/executor.go 76.5957% <0.0000%> (-0.9553%) ⬇️
... and 11 more

require := require.New(t)

uri := "tikv://127.0.0.1:1001,127.0.0.2:1002/?concurrency=10"
uri := "tikv://127.0.0.1:1001,127.0.0.2:1002/?concurrency=10&ca-path=./ca-cert.pem&cert-path=./client-cert.pem&key-path=./client-key"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to keep test case with no tls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it back. PTAL~

}

func (cfg *Config) ParseFromFlags(flags *pflag.FlagSet) error {
func (cfg *Config) ParseFromFlags(flags *pflag.FlagSet, useDstPD bool) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: useDstPD -> requireDstPD

opts := make(map[string]string)
config, pdAddr, err := parseTiKVUri(sinkURI, opts)
require.NoError(err)
require.Len(pdAddr, expected[i].pdAddrCount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems not necessary to verify Len. pdAddr with different length must not be equal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, PTAL~

Signed-off-by: zeminzhou <[email protected]>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

Copy link
Contributor

@haojinming haojinming left a comment

Choose a reason for hiding this comment

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

LGTM

@pingyu pingyu merged commit 1b6667f into tikv:main Sep 22, 2022
@zeminzhou zeminzhou deleted the sink-https branch September 23, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdc: Add https for tikv sink
3 participants