client: fix scheme when tls config not match #7901

CabinfeverB · 2024-03-08T09:40:57Z

What problem does this PR solve?

Issue Number: Close #7900 close #7902

What is changed and how does it work?

replace httpAddr and addr with url field which includes scheme info.
modify related fields and function name which mix url and address.
pick the URL which matches TLS config from member resp.

Check List

Tests

Unit test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot · 2024-03-08T09:41:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
rleungx

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.

Signed-off-by: Cabinfever_B <[email protected]>

client/pd_service_discovery.go

HuSharp · 2024-03-08T10:02:59Z

client/pd_service_discovery.go

@@ -124,10 +124,8 @@ type ServiceDiscovery interface {

 // ServiceClient is an interface that defines a set of operations for a raw PD gRPC client to specific PD server.
 type ServiceClient interface {
-	// GetAddress returns the address information of the PD server.
+	// GetAddress returns the address with HTTP scheme of the PD server.


tests seems like will return https scheme?

Signed-off-by: Cabinfever_B <[email protected]>

client/pd_service_discovery.go

CabinfeverB · 2024-03-08T10:25:50Z

client/pd_service_discovery.go

+			addr = fmt.Sprintf("%s%s", httpScheme, addr)
+		}
+	} else {
+		if strings.HasPrefix(addr, httpScheme) {


This is only used for url.Parse. If we don't add scheme, it will return error when creating gRPC connection.
If etcd member URL is not the same as this, the connection map will add a new KV pair(holds both https://127.0.0.1 and http://127.0.0.1)

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB · 2024-03-11T03:18:19Z

/hold

Signed-off-by: Cabinfever_B <[email protected]>

rleungx · 2024-03-11T07:32:47Z

client/pd_service_discovery.go

 }

-func addrsToUrls(addrs []string) []string {
+func addrsToUrls(addrs []string, tlsCfg *tls.Config) []string {


Suggested change

func addrsToUrls(addrs []string, tlsCfg *tls.Config) []string {

func addrsToURLs(addrs []string, tlsCfg *tls.Config) []string {

Signed-off-by: Cabinfever_B <[email protected]>

rleungx · 2024-03-11T07:56:39Z

client/pd_service_discovery.go

+		}
+	}
+	log.Warn("[pd] no matched url found", zap.Strings("urls", urls), zap.Bool("tls-enabled", tlsCfg != nil))
+	return urls[0]


Do we need to change the URL according to the tlscfg instead of return [0]?

Good catch.

Signed-off-by: Cabinfever_B <[email protected]>

rleungx · 2024-03-11T09:03:46Z

@iosmanthus PTAL

ti-chi-bot · 2024-03-11T09:03:49Z

@rleungx: GitHub didn't allow me to request PR reviews from the following users: iosmanthus.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @iosmanthus

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Cabinfever_B <[email protected]>

codecov · 2024-03-12T05:11:49Z

Codecov Report

Merging #7901 (5f21733) into master (efffcc5) will decrease coverage by 0.13%.
The diff coverage is 78.24%.

❗ Current head 5f21733 differs from pull request most recent head 26de3db. Consider uploading reports for the commit 26de3db to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7901      +/-   ##
==========================================
- Coverage   73.46%   73.33%   -0.13%     
==========================================
  Files         435      435              
  Lines       48197    48195       -2     
==========================================
- Hits        35406    35346      -60     
- Misses       9739     9780      +41     
- Partials     3052     3069      +17

Flag	Coverage Δ
unittests	`73.33% <78.24%> (-0.13%)`	⬇️

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

rleungx · 2024-03-12T06:09:23Z

/hold cancel

Signed-off-by: Cabinfever_B <[email protected]>

iosmanthus · 2024-03-12T08:03:57Z

client/meta_storage_client.go

@@ -162,7 +162,7 @@ func (c *client) Get(ctx context.Context, key []byte, opts ...OpOption) (*meta_s
 		Limit:    options.limit,
 		Revision: options.revision,
 	}
-	ctx = grpcutil.BuildForwardContext(ctx, c.GetLeaderAddr())
+	ctx = grpcutil.BuildForwardContext(ctx, c.GetLeaderURL())


How about changing the definition of func BuildForwardContext to BuildForwardContext(..., url string).

iosmanthus · 2024-03-12T08:14:57Z

client/pd_service_discovery.go

+func modifyURLScheme(url string, tlsCfg *tls.Config) string {
+	if tlsCfg == nil {
+		if strings.HasPrefix(url, httpsScheme) {
+			url = fmt.Sprintf("%s%s", httpScheme, strings.TrimPrefix(url, httpsScheme))
+		} else if !strings.HasPrefix(url, httpScheme) {
+			url = fmt.Sprintf("%s%s", httpScheme, url)
+		}
+	} else {
+		if strings.HasPrefix(url, httpScheme) {
+			url = fmt.Sprintf("%s%s", httpsScheme, strings.TrimPrefix(url, httpScheme))
+		} else if !strings.HasPrefix(url, httpsScheme) {
+			url = fmt.Sprintf("%s%s", httpsScheme, url)
+		}
+	}
+	return url
+}


suggestion:

u, err := url.Parse(url) if err != nil { return url } if tls != nil { u.Scheme = "https" } return u.String()

Has addressed this, PTAL

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB · 2024-03-12T09:42:44Z

/merge

ti-chi-bot · 2024-03-12T09:42:46Z

@CabinfeverB: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2024-03-12T09:42:48Z

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

Commit hash: 5f21733

ti-chi-bot · 2024-03-12T09:42:56Z

@CabinfeverB: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

CabinfeverB added 2 commits March 8, 2024 17:32

fix scheme

6438a65

Signed-off-by: Cabinfever_B <[email protected]>

fix scheme

55b6c9a

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 8, 2024

ti-chi-bot bot requested review from JmPotato and lhy1024 March 8, 2024 09:41

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 8, 2024

CabinfeverB added 2 commits March 8, 2024 17:45

Merge branch 'master' into client/fix_scheme

8115700

fix scheme

0d69a07

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-triage-completed labels Mar 8, 2024

HuSharp reviewed Mar 8, 2024

View reviewed changes

client/pd_service_discovery.go Show resolved Hide resolved

CabinfeverB force-pushed the client/fix_scheme branch from 504486e to 0d69a07 Compare March 8, 2024 10:01

HuSharp reviewed Mar 8, 2024

View reviewed changes

fix scheme

52ad50d

Signed-off-by: Cabinfever_B <[email protected]>

HuSharp reviewed Mar 8, 2024

View reviewed changes

client/pd_service_discovery.go Outdated Show resolved Hide resolved

CabinfeverB commented Mar 8, 2024

View reviewed changes

address comment

0c5fec7

Signed-off-by: Cabinfever_B <[email protected]>

HuSharp approved these changes Mar 8, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 8, 2024

rleungx approved these changes Mar 11, 2024

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 11, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2024

address comment

24ce773

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2024

address comment

a6b7ca9

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB requested a review from rleungx March 11, 2024 07:34

CabinfeverB requested a review from HuSharp March 11, 2024 07:34

rleungx reviewed Mar 11, 2024

View reviewed changes

CabinfeverB added 3 commits March 11, 2024 15:38

address comment

727d5d2

Signed-off-by: Cabinfever_B <[email protected]>

Merge branch 'master' into client/fix_scheme

588bc61

address comment

74349ef

Signed-off-by: Cabinfever_B <[email protected]>

rleungx reviewed Mar 11, 2024

View reviewed changes

address comment

af92378

Signed-off-by: Cabinfever_B <[email protected]>

fix test

3fde8de

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2024

fix test

6cf0b07

Signed-off-by: Cabinfever_B <[email protected]>

iosmanthus reviewed Mar 12, 2024

View reviewed changes

CabinfeverB added 4 commits March 12, 2024 16:49

address comment

7429fbd

Signed-off-by: Cabinfever_B <[email protected]>

address comment

f3888ca

Signed-off-by: Cabinfever_B <[email protected]>

address comment

34eedb0

Signed-off-by: Cabinfever_B <[email protected]>

address comment

5f21733

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2024

Merge branch 'master' into client/fix_scheme

26de3db

ti-chi-bot bot merged commit c1eabda into tikv:master Mar 12, 2024
22 checks passed

CabinfeverB deleted the client/fix_scheme branch March 14, 2024 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: fix scheme when tls config not match #7901

client: fix scheme when tls config not match #7901

CabinfeverB commented Mar 8, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 8, 2024 •

edited

Loading

HuSharp Mar 8, 2024 •

edited

Loading

CabinfeverB Mar 8, 2024 •

edited

Loading

CabinfeverB commented Mar 11, 2024

rleungx Mar 11, 2024

rleungx Mar 11, 2024

CabinfeverB Mar 11, 2024

rleungx commented Mar 11, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 11, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

rleungx commented Mar 12, 2024

iosmanthus Mar 12, 2024

iosmanthus Mar 12, 2024

CabinfeverB Mar 12, 2024

CabinfeverB commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

	func addrsToUrls(addrs []string, tlsCfg *tls.Config) []string {
	func addrsToURLs(addrs []string, tlsCfg *tls.Config) []string {

client: fix scheme when tls config not match #7901

client: fix scheme when tls config not match #7901

Conversation

CabinfeverB commented Mar 8, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 8, 2024 • edited Loading

HuSharp Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

CabinfeverB Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

CabinfeverB commented Mar 11, 2024

rleungx Mar 11, 2024

Choose a reason for hiding this comment

rleungx Mar 11, 2024

Choose a reason for hiding this comment

CabinfeverB Mar 11, 2024

Choose a reason for hiding this comment

rleungx commented Mar 11, 2024 • edited Loading

ti-chi-bot bot commented Mar 11, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

rleungx commented Mar 12, 2024

iosmanthus Mar 12, 2024

Choose a reason for hiding this comment

iosmanthus Mar 12, 2024

Choose a reason for hiding this comment

CabinfeverB Mar 12, 2024

Choose a reason for hiding this comment

CabinfeverB commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

ti-chi-bot bot commented Mar 12, 2024

CabinfeverB commented Mar 8, 2024 •

edited

Loading

ti-chi-bot bot commented Mar 8, 2024 •

edited

Loading

HuSharp Mar 8, 2024 •

edited

Loading

CabinfeverB Mar 8, 2024 •

edited

Loading

rleungx commented Mar 11, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading